staticaddr: persist multi-address ownership foundation - #1210
Conversation
Reserve separate key families for static receive and change addresses. This keeps derived keys out of the legacy static-address and HTLC key streams.
Associate every deposit with the static address parameters that created it. This lets restored deposits recover the correct script and signing keys instead of assuming the legacy root address.
|
/gateway review |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gateway review — 7 findings
🔴 0 Blocker · 🟠 3 Major · 🟡 4 Minor · 🔵 0 Nit
Summary
This PR reserves two new static-address key families and threads a static_address_id column through the deposit schema, the sqlc queries and the deposit store, so each deposit can carry the address parameters that produced its pkScript. The schema change, generated bindings and query rewrites are internally consistent, and the key-family split is documented and pinned by a test.
Three things stop the foundation from actually holding data. The write path never populates the new column — CreateDeposit accepts a nil AddressParams and nothing in this diff sets it — so deposits created after the migration are ownerless. The loop-in read path drops the column it was just handed, so deposits reached through a swap come back with no parameters at all. And the migration attributes every pre-existing deposit to the lowest-id static address without checking that only one exists, a mis-assignment the batch-repair query shipped here cannot undo because it only touches NULL rows.
The rest is maintainability: three hand-copied row converters that must stay in sync, an exported-signature change filed under Maintenance rather than Breaking Changes, and no test on the new parameter-loading branch.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
|
|
||
| UPDATE deposits | ||
| SET static_address_id = ( | ||
| SELECT id FROM static_addresses ORDER BY id ASC LIMIT 1 |
There was a problem hiding this comment.
🟠 F1 (Major) — Backfill assigns every old deposit to one address · loopdb/sqlc/migrations/000022_deposit_static_address_id.up.sql:5
The migration assigns every pre-existing deposit to the lowest-id row in static_addresses. If a client ever created more than one static address before this migration runs, deposits belonging to the second address are permanently recorded as owned by the first, and every subsequent read hands spending code the wrong client/server pubkeys and expiry through the new AddressParams.
Why this matters
The subselect SELECT id FROM static_addresses ORDER BY id ASC LIMIT 1 has no way to tell which address produced a given deposit — deposits stores only timeout_sweep_pk_script, not the address pkScript — and the EXISTS guard only checks that at least one address row exists, not that exactly one does. The mis-assignment is also unrecoverable through the repair path this PR adds: SetAllNullDepositsStaticAddressID (loopdb/sqlc/queries/static_address_deposits.sql) and BatchSetStaticAddressID (staticaddr/deposit/sql_store.go) both filter on static_address_id IS NULL, so a row that was wrongly filled is never revisited. Gating the backfill on (SELECT COUNT(*) FROM static_addresses) = 1 and leaving the column NULL otherwise keeps the ambiguous case in the hands of application code, which can derive scripts and match them against each deposit. I cannot tell from the loaded context whether the client can currently create more than one static address; if it provably cannot, this reduces to a hardening request.
| TimeoutSweepPkScript: deposit.TimeOutSweepPkScript, | ||
| StaticAddressID: sql.NullInt32{}, | ||
| } | ||
| if deposit.AddressParams != nil { |
There was a problem hiding this comment.
🟠 F2 (Major) — Deposits are created with NULL address ownership · staticaddr/deposit/sql_store.go:59
CreateDeposit writes static_address_id = NULL whenever deposit.AddressParams is nil, and nothing in this diff sets AddressParams on the creation path. Every deposit created after the migration therefore lands without the ownership metadata the PR exists to persist, and GetStaticAddressScript will fail for all of them.
Why this matters
The AddressParams.ID <= 0 guard only fires once a caller has already supplied parameters; the nil case falls through to the zero-valued sql.NullInt32{} initialized two lines above. The only writer of AddressParams in the loaded context is toDeposit, which populates it from rows that already have a non-NULL static_address_id — the field is read-path-only today. staticaddr/deposit/manager.go is not in the provided context, so a caller there may already set it; if it does not, the new GetStaticAddressID query is exactly the lookup needed at creation time, and the nil case should return an error rather than silently persisting NULL. Note that BatchSetStaticAddressID — the mechanism that would repair such rows — has no caller in this diff either.
| } | ||
|
|
||
| sqlcDeposit := sqlc.Deposit{ | ||
| sqlcDeposit := sqlc.AllDepositsRow{ |
There was a problem hiding this comment.
🟠 F3 (Major) — Loop-in deposits lose their address parameters · staticaddr/loopin/sql_store.go:604
Deposits reconstructed for a static address loop-in always come back with AddressParams == nil, even when the database row has a valid static_address_id. Spending code that follows this PR's own contract — "use these per-deposit parameters rather than assuming all deposits belong to one address" — sees an ownerless deposit and either errors out of GetStaticAddressScript or silently falls back to global address parameters, which is the behavior the change is meant to eliminate.
Why this matters
toStaticAddressLoopIn builds a sqlc.AllDepositsRow field by field from a DepositsForSwapHashRow and never copies StaticAddressID, despite this PR adding that exact field to DepositsForSwapHashRow (loopdb/sqlc/static_address_loopin.sql.go:76). Copying it alone would not be enough: DepositsForSwapHash was not given the LEFT JOIN static_addresses that GetDeposit, DepositForOutpoint and AllDeposits received, so ClientPubkey/ServerPubkey are absent and toDeposit would reach btcec.ParsePubKey(nil) and fail. The fix is to add the same join to DepositsForSwapHash and carry all of the joined address columns through this conversion.
| } | ||
|
|
||
| return script.NewStaticAddress( | ||
| input.MuSig2Version100RC2, int64(d.AddressParams.Expiry), |
There was a problem hiding this comment.
🟡 F4 (Minor) — Static address script ignores stored protocol version · staticaddr/deposit/deposit.go:170
GetStaticAddressScript hardcodes input.MuSig2Version100RC2 while discarding AddressParams.ProtocolVersion, which the same struct persists and loads, so a deposit created under a future address protocol version would be reconstructed with the wrong MuSig2 variant. Since AddressParams.PkScript is loaded alongside, the cheapest guard is to compare the derived script's pkScript against it and return an error on mismatch.
| func ToDeposit(row sqlc.Deposit, lastUpdate sqlc.DepositUpdate) (*Deposit, | ||
| // ToDeposit converts an sql deposit row with joined static address metadata to | ||
| // a deposit. | ||
| func ToDeposit(row sqlc.AllDepositsRow, lastUpdate sqlc.DepositUpdate) (*Deposit, |
There was a problem hiding this comment.
🟡 F5 (Minor) — Exported ToDeposit signature change is unannounced · staticaddr/deposit/sql_store.go:270
ToDeposit changes its exported parameter type from sqlc.Deposit to sqlc.AllDepositsRow, and the exported sqlc.Querier interface changes three method return types, which breaks external importers and any hand-written Querier mock. The release-notes entry files this under Maintenance while the Breaking Changes section is left untouched.
| InitiationHeight sql.NullInt32 | ||
| } | ||
|
|
||
| func depositRowFromAll(row sqlc.AllDepositsRow) depositRow { |
There was a problem hiding this comment.
🟡 F6 (Minor) — Three hand-copied row converters must stay in sync · staticaddr/deposit/sql_store.go:297
depositRowFromAll, depositRowFromGet and depositRowFromOutpoint copy the same eighteen fields verbatim, so every future column has to be added in three places — the dropped StaticAddressID in toStaticAddressLoopIn is the same class of mistake one layer up. The three sqlc row structs are field-identical, so having the queries emit one shared row type (or asserting field parity in a test) removes the hazard instead of tripling it.
| { | ||
| name: "fully valid data", | ||
| row: sqlc.Deposit{ | ||
| row: sqlc.AllDepositsRow{ |
There was a problem hiding this comment.
🟡 F7 (Minor) — New AddressParams read path is untested · staticaddr/deposit/sql_store_test.go:45
Both TestToDeposit cases leave StaticAddressID invalid, so the entire new branch in toDeposit — pubkey parsing, key-locator construction, protocol-version and expiry mapping — has no coverage despite being the point of the PR. Add a case with a valid StaticAddressID and real serialized pubkeys, and one with malformed pubkey bytes to pin the error path.
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
1 participant
Prerequisite for #1139.
This isolates the non-user-facing storage and key plumbing needed by the multi-address feature:
This PR does not activate multi-address issuance. The user-facing feature remains in #1139.
Once this lands, #1139 can be rebased onto master without changing its final source tree. Its remaining patch is approximately 283k characters, below the Gateway review ceiling.
Validation:
Release notes: