Repository navigation
offers: add a SQL-backed store for BOLT 12 offers - #11278
Conversation
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.
There was a problem hiding this comment.
Done, InsertOffer no longer sets is_disabled, and the store docs say that a new offer is always enabled. Yes, the update query for the flag comes with the change that disables offers. (F2)
|
|
||
| -- 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?
There was a problem hiding this comment.
Good point. Because the migration is now a dev migration, I start with the encoded offer only, and later in this series we can see if other use cases emerge. With the encoded offer as the one source, BOLT 12 features that change the offer need no migration. If a use case such as an expiry filter emerges, we can denormalize then and add the typed column with its index. (F1)
| 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.
There was a problem hiding this comment.
Added, a blinded-path offer now stores offer_paths and no issuer id, an empty path list is rejected, and a two-chain offer stores offer_chains. (F6)
| 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.
There was a problem hiding this comment.
Nice! CreateOffer now only checks the expiry against the clock, and it returns the wrapped bolt12 error for everything else. The tests expect bolt12.ErrMissingDescription, bolt12.ErrNilPublicKey and bolt12.ErrEmptyBlindedPaths. (F5)
|
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 |
|
Ok, nice suggestion, will use that. |
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. The migration is a development migration. It is in migrationAdditions, so only builds with the SQL test tags apply it, and a normal build does not change the user's schema. The offers schema can still change before the receive flow is complete. When it is final, the migration moves to the main line with the next free versions. Review: lightningnetwork#11278 (comment)
bd49608 to
757e121
Compare
|
Thanks @ziggie1984 and @ViktorT-11! |
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Nice, thanks for making this a dev migration. Since we can then modify the migration easily if we notice that we need anything extra, this is good to go IMO. LGTM 🔥
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
The SQL schema and store paths look sound overall. I found one writer/reader consistency class to address before the offer creation flow is exposed: CreateOffer can persist offers that the local reader rejects, both through more than 32 chain entries and through an individual TLV record above tlv.MaxRecordSize. I also left three non-blocking nits about documenting the hash/encoding invariant, using consistent clocks for expiry and creation time, and reusing the PostgreSQL fixture across parallel tests.
| // store sets the creation time, and a new offer is always enabled. It | ||
| // returns ErrOfferExists if an offer with the same offer hash is | ||
| // already stored. | ||
| InsertOffer(ctx context.Context, offer *Offer) (int64, error) |
There was a problem hiding this comment.
nit: Could we document that callers must provide an already validated offer and that Hash must be derived from the offer TLVs represented by Encoded? This makes the consistency invariant explicit for future callers of InsertOffer.
There was a problem hiding this comment.
Done, the InsertOffer doc now states that the caller passes a validated offer and that Hash must come from the offer TLVs in Encoded. CreateOffer meets both. (F11)
| // and the offer hash. It rejects an absolute expiry at or before the time of | ||
| // clk. Any other invalid parameter returns the bolt12 validation error, | ||
| // wrapped. | ||
| func CreateOffer(ctx context.Context, store Store, clk clock.Clock, |
There was a problem hiding this comment.
nit: CreateOffer validates expiry with the clk passed here, while SQLStore.InsertOffer assigns CreatedAt from its own independently injected clock. Could we either source both values from the same clock/timestamp or document that the two clocks must agree? Otherwise a caller can persist an offer whose expiry is at or before its stored creation time even though this check passed.
There was a problem hiding this comment.
Thanks! I kept the store clock, because the store owns the creation time, and documented that CreateOffer must get the clock of the store. (F10)
|
|
||
| // newTestBaseDB creates a Postgres database for the offer store tests. | ||
| func newTestBaseDB(t *testing.T) *sqldb.BaseDB { | ||
| fixture := sqldb.NewTestPgFixture( |
There was a problem hiding this comment.
nit: Every call to newTestBaseDB starts a fresh Postgres container. Since the TestCreateOffer subtests and the store tests all call this through newTestSQLStore and run in parallel, this package can launch roughly 16 Postgres containers concurrently. Could we share one fixture and create an isolated database per test, as the invoice SQL tests do, to keep CI resource usage bounded?
There was a problem hiding this comment.
Done. The package now starts one Postgres container per top-level test, and each subtest gets its own database, as in the invoice SQL tests. (F12)
| // fixed amount (the payer must specify invreq_amount). | ||
| AmountMsat uint64 | ||
|
|
||
| // AbsoluteExpiry is seconds since epoch after which the offer expires. |
There was a problem hiding this comment.
Remark: offer_absolute_expiry expires the offer as a whole; lnwire.BlindedPath has no explicit per-path expiry field. When the later path-generation/receive flow is wired, we should make sure any implementation-defined blinded-path lifetime is at least as long as the offer lifetime (or rotate/replace the offer when its paths expire), otherwise an offer can remain formally valid while all of its contact paths are unusable.
There was a problem hiding this comment.
Agreed, thanks. CreateOffer only stores the offer_paths it gets. The receive flow that builds the paths will keep each path alive at least as long as the offer, have taken note of that. (F13)
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
Additional store-design consistency remarks.
| type SQLStore struct { | ||
| db BatchedSQLOfferQueries | ||
| clock clock.Clock | ||
| } |
There was a problem hiding this comment.
nit: Could we add an explicit compile-time assertion here that *SQLStore implements Store? Both the invoice and payment SQL stores use this pattern, and it makes interface drift fail at compile time even when the SQL-tagged tests are not being run.
There was a problem hiding this comment.
Done, SQLStore now has the compile-time Store check. (F14)
| return 0, fmt.Errorf("insert offer: %w", err) | ||
| } | ||
|
|
||
| return id, nil |
There was a problem hiding this comment.
InsertOffer returns the generated ID, but because the store also chooses CreatedAt internally, the caller object still has both ID and CreatedAt unset after a successful insert and differs from the persisted row. For comparison, AddInvoice documents updating AddIndex on the supplied invoice. Could we either update the offer with both persisted values or return the complete stored Offer?
There was a problem hiding this comment.
Done. InsertOffer now sets ID and CreatedAt on the offer, as AddInvoice does with AddIndex. The creation time is cut to microseconds before the insert, so the offer equals the stored row on SQLite and on Postgres, and the test checks that. (F15)
| CreatedAt: row.CreatedAt, | ||
| } | ||
|
|
||
| copy(offer.Hash[:], row.Hash) |
There was a problem hiding this comment.
nit: Could we validate that row.Hash is exactly 32 bytes and return an error instead of silently copying it? The invoice converter uses lntypes.MakeHash for this. copy will otherwise zero-pad or truncate malformed database data. The exact-key lookup makes this difficult to hit today, but rejecting malformed rows would preserve the existing corruption-detection pattern.
There was a problem hiding this comment.
Done, marshalOffer now uses lntypes.MakeHash, so it rejects a stored hash that is not 32 bytes. A unit test covers the lengths around 32. (F16)
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.
757e121 to
9449605
Compare
| if len(params.Chains) > 0 { | ||
| readChain = params.Chains[0] | ||
| } | ||
| _, err = bolt12.DecodeOfferString(encoded, clk.Now(), readChain) |
There was a problem hiding this comment.
Would writer-side validation be cleaner than encoding and then decoding here? Specifically, we could reject more than maxOfferChains in ChainsRecord/validateOfferWrite and reject any Offer.AllRecords entry above tlv.MaxRecordSize during encoding. That would protect every EncodeOfferString caller, avoid selecting a synthetic readChain and reading the clock again, and return writer errors directly. The tradeoff is that future reader limits must also be mirrored, which boundary tests could pin. What do you think: would you prefer that approach here, or keep this guard and add the writer checks separately?
There was a problem hiding this comment.
Agreed that this isn't perfect. I think we should keep it for now and address it later, since reader and writer rules aren't really 100% symmetric and there are other cases as well.
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.