offers: add a SQL-backed store for BOLT 12 offers - #11278
bitromortac wants to merge 17 commits into
Conversation
The invoice reader skipped the must-understand check for types 240 to 1000, so an invoice carrying unknown even type 242 was accepted while the same type on an invoice_request was rejected. The spec exempts that range from the out-of-range rule only, and BOLT 1 still makes an unknown even type fatal. Core Lightning grants the range no exception either, so this was a divergence rather than a choice. Finding F8. lightningnetwork#10941 (comment)
The offer writer checked the allowed range but not the must-understand rule, while the offer reader checks both. The state is only reachable for an offer decoded and then mutated, since the typed field set cannot express an unknown type, but the asymmetry made a reader of the code work out why one of the two rules was missing. Finding F5. lightningnetwork#10789 (comment)
Decode capped the raw and the cleaned string length, which no other implementation does and which the spec does not ask for. The cap protected nothing: a decode allocates on the order of its input, each record is already bounded by tlv.MaxRecordSize and every subtype decoder bounds itself against the bytes present, and the input exists in the caller's memory before Decode runs. What it could do is reject a spec-valid message once unknown odd fields push a string past a limit chosen today. Encode keeps its payload bound, since there we choose what to emit, and the constant now says it is a writer policy. The caller bounds its own medium: the onion-message envelope for an invoice_request and an invoice, the RPC or CLI for a pasted or scanned offer string. Finding F41. lightningnetwork#11001 (comment)
The receiver identifies the offer an invoice request answers by hashing the request's offer fields and looking the result up in its store, so that lookup is the exact-match check the reader requirements ask for. The rule belongs in the codec, which owns the encoding and the range predicate, rather than in each caller. The hash covers the offer ranges of any pure-TLV message, so one function serves the offer, the invoice_request and the invoice. For an offer it equals the hash of the whole encoding, since every offer TLV already sits in those ranges. Finding F40. lightningnetwork#10941 (comment)
The invoice_request and invoice validators quote each spec bullet above the check it authorises and run in the spec's order, so they can be read beside the spec block. The two offer validators, which landed before that convention, had none: 29 and 37 stanza lines now, against zero before. The writer moves two checks to reach spec order, the chains rule ahead of the amount rules and the nil-key guard down to the issuer stanza it belongs to. Every check is otherwise the same one, and no test expectation changed, so no input flipped to a different first error. Both functions now also state the rules the codec cannot enforce and why, which is how the gaps stay visible. Finding F38.
DecodeInvoiceString folds the reader gates into the decode, so a caller that only wants to display an invoice it already validated when it stored it has no entry point. Such a caller had to reach for the raw bech32 decode and parse the bytes itself, which accepts any of the three prefixes. DecodeInvoiceStringUnvalidated pins the lni prefix and skips the gates. It is the one legitimate raw-decode caller, so the bech32 primitives can leave the API next. DecodeInvoiceString now delegates to it and adds the gates on top, so the prefix check and the TLV decode have one home rather than two copies. Finding F22.
Decode and Encode become decodeBech32 and encodeBech32. The bare names read like the message codec they sit next to, and no caller outside the package needs them: the string entry points fold bech32 into a validated call, and a caller that wants a raw decode has DecodeInvoiceStringUnvalidated. The rename is mechanical. Four comments in bech32.go named the old symbols, so they name the new ones now. The encoder docstring also stops claiming its payload bound mirrors a reader limit, because the reader no longer has one.
The verifiers go, because the readers call them and nothing verifies a signature on its own. The writer validators go, because Encode is the gate and already runs them. The reader validators that an exported entry point folds in go too: DecodeOfferString for the offer, and ValidateInvoiceForPayment for the invoice against its request. What stays exported is what a caller that reaches the codec over an onion message needs, because an invoice_request and an invoice arrive as raw TLV rather than as strings. Finding F22.
Three symbols are un-exported rather than deleted: the two invoice_request string wrappers and UsableFallbackAddresses. Nothing transports that message as a string today, and nothing dispatches an on-chain fallback yet, so none of the three has a caller. They stay because the wrappers carry the lnr prefix the spec defines, the helper encodes the reader's MUST-ignore rules for fallback addresses, and their tests exercise both. Finding F22.
An invoice_request and an invoice reach a peer as raw TLV inside an onion message, not as a bech32 string. Encode is permissive about the signature by design, because a caller must encode before it can derive the Merkle root it signs, so nothing enforced the writer-side MUST on the path the bytes actually take. A caller that forgot to sign would find out only when the remote peer rejected the message. EncodeSigned is that entry point for both messages. It requires the signature and verifies it against invreq_payer_id or invoice_node_id, and EncodeInvoiceString now delegates to it, so the raw and string forms carry one rule.
A signature over a message that breaks the writer requirements is worthless, because the peer rejects the message on read. Running the write validator before the Merkle root is derived keeps a key from signing bytes no correct reader accepts, and it moves the check off the caller, which previously had to encode once purely to trigger it.
EncodeSigned is the entry point for an invoice_request and an invoice that leave the node, and the bech32 wrappers cover the string forms, so no caller outside this package needs a serialisation that skips the signature check. Un-exporting encode makes the writer-side requirement a property of the API rather than a rule a caller has to remember. Offer follows for consistency, since EncodeOfferString covers the only form it travels in. InvoiceError keeps its exported Encode. It is unsigned, so there is no check to skip, and raw TLV is the only form a receiver can send it in.
Nothing transports an invoice request as a string. It reaches a peer as raw TLV inside an onion message, so the bech32 encoder had no caller once EncodeSigned took over the signature rule, leaving a tested function that no code path could reach. The decoder stays: another implementation may hand us an lnr string, and the fuzz harness exercises that path. The three tests that pinned the encoder's refusal to emit an invalid, unsigned or badly signed request move to EncodeSigned, which is where that rule now lives.
Add the offers table migration and its generated sqlc queries. Offers are long-lived reusable templates, a lifecycle with no BOLT 11 analog, so they get their own table rather than extending invoices. The table keeps the encoded offer and the local state only. The lno1 string is the content, and it never changes after creation. The local state is the data that no offer or invoice request carries: the disabled flag and the creation time. No column holds a typed copy of an offer field. This is enough because the invoice request carries the offer terms. The payer copies every offer field into the request, and the offer hash covers the whole offer range. A lookup by that hash therefore proves an exact match, so a spec extension needs no migration. The hash is UNIQUE, which also indexes it for the lookup. The queries are the insert and the lookup by hash, the two the offer store calls. A table with one row per TLV record was also considered. It needs a query over many rows per offer, and it loses the column types.
Introduce the offers package with a SQL-backed store for BOLT 12 offers. The store keeps the encoded offer and the local state that the offer string does not carry. It has an insert and a lookup by offer hash. CreateOffer validates the parameters, encodes the offer and inserts it.
Add the offer store to the BOLT 12 section of the 0.22.0 release notes. The PR link holds a placeholder until the PR number is known.
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (7 files)
🟡 Medium (12 files)
🟢 Low (15 files)
AnalysisThis PR adds a new SQL-backed To override, add a |
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Adding a few comments from my initial review. In general maybe we should consider merging this PR into a side branch instead of master until more code which uses this table exists, so that we have a clear definition of how it'll be used?
Other than that, this PR in general looks good 🔥
| INSERT INTO offers ( | ||
| hash, encoded, is_disabled, created_at | ||
| ) VALUES ( | ||
| $1, $2, $3, $4 | ||
| ) RETURNING id; |
There was a problem hiding this comment.
nit: Probably no need to insert is_disabled when creating an offer, as it defaults to FALSE & there shouldn't be any use case where we insert disabled offers unless I'm wrong?
Maybe we should also have a query to update that flag, but I assume we'll add that when that's implemented in the rest of the code base.
|
|
||
| -- The full bech32-encoded offer string (lno1...). This is the | ||
| -- authoritative source for all offer fields. | ||
| encoded TEXT NOT NULL, |
There was a problem hiding this comment.
Hmmm I'm not sure if just having the encoded field for all other offer fields is the best choice here, as say for example we'd want to implement a use case where we'd want to filter all offers that expire before a certain timestamp. That'd then require that we decode all offers in the database before we'd be able to filter them.
Maybe we should not merge this PR to master yet before we've decided what exactly how we'd want the table definition to look like, so that it becomes much more flexible to change without doing a lot migrations?
| identityErr = ErrMissingIssuerKey | ||
| } | ||
| }) | ||
| params.Identity.WhenRight(func(paths []lnwire.BlindedPath) { |
There was a problem hiding this comment.
The offer_paths identity branch has no test at all: neither a blinded-path offer round-tripping through the store nor the empty-paths rejection. Same for the Chains branch. Every row in TestCreateOffer uses the issuer key.
| var ( | ||
| // ErrMissingDescription is returned when the offer has an amount but no | ||
| // description. | ||
| ErrMissingDescription = errors.New("description required when amount " + |
There was a problem hiding this comment.
This re-implements a rule validateOfferWrite already enforces through EncodeOfferString, and bolt12 has its own ErrMissingDescription for it. So the same rule yields two different sentinels depending on which layer catches it first. The empty-paths check below has the same shape with an ad hoc fmt.Errorf. Suggest dropping the pre-checks and wrapping the bolt12 errors, keeping only the checks the codec cannot make, like the expiry against the clock.
|
Since the offers database design is still evolving and we expect additional schema and store work, could we keep this migration development-only for now? We already have the established mechanism for this in //go:build test_db_postgres || test_db_sqlite || test_native_sqlI suggest moving the Besides preventing normal This would let us continue developing and testing the complete offers persistence model under the SQL test build tags. Once its schema, queries, lifecycle operations, and integration are ready, we can promote the finalized migrations into The SQL-backed offer tests should also be gated with To be precise, the migration SQL and generated |
|
Good idea, ACK on the idea of keeping the migration as a |
5 participants
This PR adds the
offerspackage and theofferstable. It is the storage base for the BOLT 12 receive flow. It depends on #11191, so only the last three commits are new here.Offers get their own table and do not extend invoices. An offer is a long-lived template that can pay many invoices, and BOLT 11 has no such lifecycle. The table keeps only the encoded offer and the local state that no offer or invoice request carries: the disabled flag and the creation time. No column holds a typed copy of an offer field.
This is sufficient because the payer copies every offer field into the invoice request, and the offer hash covers the whole offer range. A lookup by that hash therefore proves an exact match, and a spec extension needs no migration. For this reason the store has only an insert and a lookup by offer hash.
The store maps a duplicate offer to
ErrOfferExistsand a missing offer toErrOfferNotFound. It sets the creation time from its own clock.