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)
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 3 findings
🔴 0 Blocker · 🟠 2 Major · 🟡 1 Minor · 🔵 0 Nit
Summary
All seven prior findings are addressed at 22c590a0. The migration backfill is now gated on there being exactly one static_addresses row, CreateDeposit and createNewDeposit fail closed instead of persisting a NULL owner, DepositsForSwapHash gained the address join so loop-in recovery carries AddressParams, script reconstruction validates both the protocol version and the persisted pkScript, the exported ToDeposit/Querier signatures are preserved via new *WithAddress variants, and the converter-parity and joined-branch tests exist.
The fail-closed guards introduced by those fixes create two new exposures, both of which depend on staticaddr/address/manager.go — a file that is not in the review context, so neither can be confirmed here. First, a deposit with a NULL owner (the state the new migration gate deliberately leaves behind) now aborts the whole deposit manager at startup rather than degrading. Second, deposit creation now hard-requires a non-zero AddressParams.ID, and the one lookup that would guarantee it — the newly added GetStaticAddressID — has no production caller.
One residual maintainability gap: the fourth hand-copy of the deposit row, in staticaddr/loopin/sql_store.go, is the exact site that produced F3 and is still the one copy no parity test covers.
Status of prior findings
- F1 addressed: Fixed in
loopdb/sqlc/migrations/000022_deposit_static_address_id.up.sql:8— the backfill is now gated on(SELECT COUNT(*) FROM static_addresses) = 1, andTestDepositAddressBackfillcovers the zero-, one- and two-address cases. - F2 addressed: Fixed in
staticaddr/deposit/manager.go:380andstaticaddr/deposit/sql_store.go:50—createNewDepositattaches the current address parameters to the deposit, andCreateDepositrejects nil or unpersisted parameters instead of writingstatic_address_id = NULL. - F3 addressed: Fixed in
loopdb/sqlc/queries/static_address_loopin.sqlandstaticaddr/loopin/sql_store.go:604—DepositsForSwapHashnowLEFT JOINsstatic_addresses, and the conversion threads all eight address columns plusStaticAddressIDthroughToDepositWithAddress.TestGetStaticAddressLoopInSwapsByStatesasserts the recovered deposit'sAddressParams.IDandPkScript. - F4 addressed: Fixed in
staticaddr/deposit/deposit.go(GetStaticAddressScript) — the reconstruction switches onAddressParams.ProtocolVersion, rejects unknown versions, and compares the derived script against the persistedPkScript.TestGetStaticAddressScriptValidatesPersistedScriptcovers the valid, mismatched-script and unsupported-version paths. - F5 addressed: Fixed —
ToDepositkeeps itssqlc.Depositparameter, andAllDeposits,GetDepositandDepositForOutpointkeep their original return types; the joined reads are additive*WithAddressmethods. The exported surface is now additive, so the Maintenance-only release note is accurate. - F6 addressed: Fixed in
staticaddr/deposit/sql_store_test.go—TestDepositRowConvertersStayInSyncreflectively pins field parity for all three converters and asserts the source row has exactlydepositRow.NumField()+1fields, so a new sqlc column that is not threaded through fails the test. - F7 addressed: Fixed in
staticaddr/deposit/sql_store_test.go—TestToDepositWithAddressexercises theStaticAddressID.Validbranch with real serialized pubkeys, asserts every mapped field, and pins the error path on malformed pubkey bytes.
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)
| @@ -181,13 +181,12 @@ func NewFSM(ctx context.Context, deposit *Deposit, cfg *ManagerConfig, | |||
| finalizedDepositChan chan wire.OutPoint, | |||
| recoverStateMachine bool) (*FSM, error) { | |||
|
|
|||
There was a problem hiding this comment.
🟠 F8 (Major) — Ownerless deposit aborts static-address manager startup · staticaddr/deposit/fsm.go:183
A deposit whose static_address_id is NULL now aborts Manager.Run during recovery, taking the entire static-address subsystem down instead of degrading — no expiry sweeps, no withdrawals and no loop-ins for any deposit on that client.
Why this matters
recoverDeposits calls NewFSM for every non-final deposit and returns the first error, and Run propagates it (staticaddr/deposit/manager.go). NewFSM now hard-fails on deposit.AddressParams == nil, and AddressParams is only populated when row.StaticAddressID.Valid (staticaddr/deposit/sql_store.go, toDeposit). Migration 22 deliberately leaves the column NULL when static_addresses holds zero or two-or-more rows — the gate added in response to F1. Before this PR the same deposit recovered fine, because NewFSM fetched parameters from cfg.AddressManager rather than from the deposit.
Reachability depends on whether a client could ever hold more than one static_addresses row before migrating. staticaddr/address/manager.go is not in the review context, so I cannot confirm that from here; if it could, this is a blocker rather than a major. Skipping or quarantining ownerless deposits during recovery — logging them and continuing — keeps one un-backfilled row from bricking the manager, and pairs naturally with the migration's decision to leave the ambiguous case to application code.
| if addressParams == nil { | ||
| return nil, fmt.Errorf("missing static address parameters") | ||
| } | ||
| if addressParams.ID <= 0 { |
There was a problem hiding this comment.
🟠 F9 (Major) — New deposits rejected if address params carry no database ID · staticaddr/deposit/manager.go:389
If AddressManager.GetStaticAddressParameters returns parameters whose ID is still zero, every new deposit is rejected with missing static address ID and never persisted — funds arrive at the static address but the client neither tracks nor sweeps them.
Why this matters
createNewDeposit requires addressParams.ID > 0, and SqlStore.CreateDeposit repeats the check. ID is only ever set by SqlStore.toAddressParameters (staticaddr/address/sql_store.go), which reads it off the persisted row — so parameters constructed in memory at address-creation time and cached without a read-back carry ID == 0, since the primary key is assigned by the insert. That would make deposit creation fail for the remainder of the session in which an address was created, and start working after the next restart.
The suspicious signal is that this PR adds GetStaticAddressID to staticaddr/address/sql_store.go — exactly the lookup that closes this gap — but no production code calls it; the only callers are persistTestAddressParameters in the loop-in and withdraw store tests. staticaddr/address/manager.go is not in file_contents, so I cannot confirm which shape its parameters have. If they are read back from the store the guard is fine; otherwise this needs GetStaticAddressID wired into the address manager, plus a test that drives the real manager through create-address → create-deposit rather than a mock that hands back ID: 1.
| } | ||
|
|
||
| sqlcDeposit := sqlc.Deposit{ | ||
| sqlcDeposit := sqlc.AllDepositsWithAddressRow{ |
There was a problem hiding this comment.
🟡 F10 (Minor) — Fourth deposit-row copy is outside the parity test · staticaddr/loopin/sql_store.go:604
toStaticAddressLoopIn hand-copies all eighteen deposit columns into a synthetic sqlc.AllDepositsWithAddressRow, but TestDepositRowConvertersStayInSync only pins the three converters in staticaddr/deposit/sql_store.go. This is the same site that dropped StaticAddressID and produced F3, and it remains the one copy no test guards, so a future address column will be silently lost here again. Extending the reflective parity check to cover this conversion — or having DepositsForSwapHashRow feed a shared converter instead of a manual struct literal — closes the hole the other three converters no longer have.
|
/gateway re-review |
Quarantine ambiguous ownerless deposits without blocking manager recovery or reconciliation. Hydrate newly persisted address parameters with their database ID in the creation transaction. Share the Loop-In deposit row converter with the deposit store and cover it with the existing parity test.
80ed5e9 to
2273c04
Compare
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 2 findings
🔴 0 Blocker · 🟠 2 Major · 🟡 0 Minor · 🔵 0 Nit
Summary
All ten prior findings are resolved. Migration 22 now backfills only when exactly one static_addresses row exists; createNewDeposit attaches the persisted address parameters and CreateDeposit fails closed without them; DepositsForSwapHash carries the joined address columns through a shared converter that a reflective parity test pins; script reconstruction validates the protocol version and the persisted pkScript; the exported ToDeposit/Querier signatures are preserved; CreateStaticAddress reads the assigned ID back inside its transaction, and TestManager proves a freshly created address is immediately usable as a deposit owner. Ownerless deposits no longer abort Manager.Run.
Two new concerns in the commits since the last review. The quarantine that fixes F8 has no exit: a deposit left ownerless by the migration is dropped from the active set, filtered out of GetVisibleDeposits, rejected by AllOutpointsActiveDeposits, and never gets an expiry-sweep FSM — with no code path that ever assigns it an owner. Separately, FSM.SignDescriptor still fetches the global static address from the AddressManager while the rest of the FSM was moved onto the deposit's own parameters, so it mixes per-deposit and global data inside one sign descriptor. Neither is reachable while a client holds exactly one static address, which is why both survive the test suite.
Status of prior findings
- F1 addressed: Fixed in
loopdb/sqlc/migrations/000022_deposit_static_address_id.up.sql:8— the backfill is gated on(SELECT COUNT(*) FROM static_addresses) = 1, andTestDepositAddressBackfillcovers the 0/1/many cases. - F2 addressed: Fixed in
staticaddr/deposit/manager.go:391andstaticaddr/deposit/sql_store.go:50—createNewDepositattachesGetStaticAddressParametersoutput and rejects a zero ID, andCreateDepositrefuses to persist a deposit whoseAddressParamsis nil or unpersisted. - F3 addressed: Fixed in
loopdb/sqlc/queries/static_address_loopin.sql:149andstaticaddr/loopin/sql_store.go:598—DepositsForSwapHashnowLEFT JOINsstatic_addresses, and the hand-builtsqlc.Depositwas replaced bydeposit.ToDepositForSwapHash.TestGetStaticAddressLoopInSwapsByStatesasserts the recovered deposit keeps its address ID and pkScript. - F4 addressed: Fixed in
staticaddr/deposit/deposit.go:164—GetStaticAddressScriptmapsProtocolVersion_V0toMuSig2Version100RC2, rejects any other version, and compares the reconstructed script againstAddressParams.PkScript. - F5 addressed: Fixed in
staticaddr/deposit/sql_store.go:268—ToDeposit(sqlc.Deposit, ...)is restored, andGetDeposit/DepositForOutpoint/AllDepositskeep their original generated signatures; the joined reads use new*WithAddressnames, so no exported signature changes. - F6 addressed: Fixed in
staticaddr/deposit/sql_store.go:302— all row shapes funnel through the internaldepositRow, andTestDepositRowConvertersStayInSyncreflectively asserts field parity, so a new shared column that misses a converter fails the test. - F7 addressed: Fixed in
staticaddr/deposit/sql_store_test.go:106—TestToDepositWithAddressexercises the joined branch with real serialized keys across every mapped field and pins the malformed-pubkey error path. - F8 addressed: Fixed in
staticaddr/deposit/manager.go:239—recoverDepositslogs and skips deposits with no owner instead of returning,syncActiveDepositsskips them on reactivation, andTestManagerQuarantinesOwnerlessDepositproves recovery and reconciliation both succeed. The residual consequence of the quarantine is filed as F12. - F9 addressed: Fixed in
staticaddr/address/sql_store.go:42—CreateStaticAddressinserts and reads the assigned ID back in one transaction, then writes it onto the caller's parameters.TestManagerdrives the real address manager throughNewAddress→GetStaticAddressParametersand requires a positive ID, which is the end-to-end coverage this finding asked for. - F10 addressed: Fixed in
staticaddr/loopin/sql_store.go:598— the eighteen-field hand-copy is gone; the conversion is nowdeposit.ToDepositForSwapHash, backed bydepositRowFromSwapHash, whichTestDepositRowConvertersStayInSynccovers alongside the other three.
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)
| params := deposit.AddressParams | ||
|
|
||
| address, err := cfg.AddressManager.GetStaticAddress(ctx) | ||
| address, err := deposit.GetStaticAddressScript() |
There was a problem hiding this comment.
🟠 F11 (Major) — Expiry sweep signs with the global address, not the deposit's · staticaddr/deposit/fsm.go:189
FSM.SignDescriptor builds the expiry-sweep sign descriptor from the global static address while taking the key and output script from the deposit's own parameters. Once a client owns more than one static address, a deposit belonging to address B is signed with address A's timeout leaf as the witness script, so the taproot script-path spend is invalid and the automated expiry sweep can never confirm.
Why this matters
NewFSM was moved onto per-deposit data — params := deposit.AddressParams and address, err := deposit.GetStaticAddressScript() — and stores the result in f.address. SignDescriptor (same file, further down) ignores f.address and instead calls f.cfg.AddressManager.GetStaticAddress(ctx), then combines address.TimeoutLeaf.Script with f.params.ClientPubkey and f.params.PkScript. That is two sources of truth inside one descriptor, and it directly contradicts the contract this PR documents on Deposit.AddressParams in staticaddr/deposit/deposit.go: "Spending code must use these per-deposit parameters rather than assuming all deposits belong to one address."
This is not reachable today — with a single address the two values coincide, which is why no test catches it — and it becomes a fund-recovery failure exactly when #1139 activates multi-address issuance. f.address already holds the correct value, so the fix is to read it (and drop the now-unnecessary ctx/error return) rather than re-fetching from the manager. staticaddr/deposit/expiry.go-style callers of SignDescriptor are not in the review context, so I cannot confirm how many spend paths consume it; that only widens the impact, it does not narrow it.
| // ambiguous. Keep these deposits in the known-deposit set so they | ||
| // aren't rediscovered, but don't let one ambiguous legacy row stop | ||
| // recovery of every other deposit. | ||
| if d.AddressParams == nil { |
There was a problem hiding this comment.
🟠 F12 (Major) — Quarantined deposits are stranded with no path back · staticaddr/deposit/manager.go:239
A deposit the migration leaves ownerless is quarantined permanently: it gets no FSM, so no expiry sweep ever runs, and no code path in this PR can ever assign it an owner. For a client whose database held two or more static_addresses rows at migration time, migration 22's COUNT(*) = 1 gate leaves every deposit ownerless, so the entire static-address feature silently goes dark on the next start — a regression from the pre-PR behavior, where those same deposits were managed using the global address parameters.
Why this matters
The stranding is total, not partial. recoverDeposits skips the deposit before NewFSM, so it never enters activeDeposits. GetVisibleDeposits drops any Deposited record that is not in activeDeposits, so it disappears from user-facing listings. AllOutpointsActiveDeposits and AllStringOutpointsActiveDeposits return false for it, so withdraw and loop-in cannot select it. syncActiveDeposits skips it on every reconciliation by design. The only signal is one log.Warnf per deposit at startup — this fails the 3 AM test: an operator sees funds vanish from listdeposits with no error and no RPC-visible explanation.
There is also no repair mechanism left. The bulk static_address_id IS NULL backfill was removed in response to F1, and the primitive that would replace it — SqlStore.GetLegacyParameters in staticaddr/address/sql_store.go, whose own comment describes it as "the first static address created for this L402" — has no production caller; its only callers are the test helpers in the loop-in and withdraw stores. Either wire that lookup into recovery (attach it, or attach whichever persisted address's derived pkScript matches the deposit's output, which is decidable and unambiguous), or surface quarantined deposits through GetVisibleDeposits with an explicit state so the funds remain visible and actionable. Whether a client could hold two static_addresses rows before this PR is not decidable from the loaded context — staticaddr/address/manager.go is not in file_contents — and this is a blocker rather than a major if it could.
There was a problem hiding this comment.
Gateway review — 2 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 2 Minor · 🔵 0 Nit
Summary
All ten prior findings are now addressed. The migration gate (COUNT(*) = 1), the fail-closed ownership write in CreateDeposit, the static_addresses join on DepositsForSwapHash, the script/pkScript revalidation in GetStaticAddressScript, the restored ToDeposit(sqlc.Deposit, …) signature, the shared depositRow converters with reflective parity coverage, the quarantine-instead-of-abort recovery path, and the in-transaction addrParams.ID write are all present at HEAD, each with a test that fails without the fix.
Two new minor observations come out of the fixes themselves. FSM.SignDescriptor still reads its witness script from the global address manager while every other field now comes from the deposit's own parameters, which is the exact pattern #1139 will trip on. And the new quarantine path removes ownerless legacy deposits from user-facing views as a side effect, leaving a single startup warning as the only signal. Neither is a defect at today's single-address behavior; both are cheap to close now rather than in the follow-up.
Status of prior findings
- F1 addressed: Fixed in
loopdb/sqlc/migrations/000022_deposit_static_address_id.up.sql:8— the backfill is gated on(SELECT COUNT(*) FROM static_addresses) = 1, andTestDepositAddressBackfillpins the 0/1/many cases. - F2 addressed: Fixed in
staticaddr/deposit/manager.go:391—createNewDepositnow fetches and attachesAddressParams, andSqlStore.CreateDeposit(staticaddr/deposit/sql_store.go:50) rejects nil params or a non-positive ID instead of persisting NULL. - F3 addressed: Fixed in
loopdb/sqlc/queries/static_address_loopin.sql:150—DepositsForSwapHashgained theLEFT JOIN static_addresses, andtoStaticAddressLoopInnow routes throughdeposit.ToDepositForSwapHash, so recovered loop-in deposits carry their address parameters. - F4 addressed: Fixed in
staticaddr/deposit/deposit.go:167— the MuSig2 variant is selected fromProtocolVersionwith an explicit default-reject, and the reconstructed script is compared against the persisted pkScript. - F5 addressed: Fixed in
staticaddr/deposit/sql_store.go:268—ToDeposit(sqlc.Deposit, …)and theAllDeposits/GetDeposit/DepositForOutpointquerier signatures are preserved; the joined reads use new*WithAddressnames, so the Maintenance-only release note is accurate. - F6 addressed: Fixed in
staticaddr/deposit/sql_store.go:303— all row shapes funnel through the internaldepositRow, andTestDepositRowConvertersStayInSyncreflectively asserts field parity, so a dropped column now fails the build's tests. - F7 addressed: Fixed in
staticaddr/deposit/sql_store_test.go:106—TestToDepositWithAddresscovers the populated-ownership branch with real serialized keys and the malformed-pubkey error path. - F8 addressed: Fixed in
staticaddr/deposit/manager.go:234—recoverDepositswarns and skips an ownerless deposit rather than returning,syncActiveDepositsskips it on every later reconciliation, andTestManagerQuarantinesOwnerlessDepositasserts recovery and reconciliation both succeed. - F9 addressed: Fixed in
staticaddr/address/sql_store.go:42—CreateStaticAddressreads the assigned ID back inside the write transaction and stores it on the caller's params.staticaddr/address/manager_test.go:135drives the real manager throughNewAddress→GetStaticAddressParametersand requires a positive ID, so the zero-ID session is closed rather than mocked away. - F10 addressed: Fixed in
staticaddr/loopin/sql_store.go:597— the hand-copiedsqlc.Depositliteral is gone, anddepositRowFromSwapHashis covered by the same parity test as the other three converters.
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)
| params := deposit.AddressParams | ||
|
|
||
| address, err := cfg.AddressManager.GetStaticAddress(ctx) | ||
| address, err := deposit.GetStaticAddressScript() |
There was a problem hiding this comment.
🟡 F11 (Minor) — Expiry-sweep witness script still comes from the global address · staticaddr/deposit/fsm.go:189
FSM.SignDescriptor (staticaddr/deposit/fsm.go:530) still takes its WitnessScript from cfg.AddressManager.GetStaticAddress(ctx) while KeyDesc and Output.PkScript come from the deposit's own f.params, so once more than one static address exists the timeout-leaf script and the pkScript being spent can disagree. It is not a defect today — the manager's address and every deposit's parameters are the same address — but f.address is already the per-deposit script this change computes, so using it here removes the mismatch before #1139 activates issuance.
| // aren't rediscovered, but don't let one ambiguous legacy row stop | ||
| // recovery of every other deposit. | ||
| if d.AddressParams == nil { | ||
| log.Warnf("Quarantining deposit %x: static address "+ |
There was a problem hiding this comment.
🟡 F12 (Minor) — Quarantined deposits disappear from user-facing views · staticaddr/deposit/manager.go:240
A quarantined ownerless deposit stays in m.deposits but never enters activeDeposits, so GetVisibleDeposits filters it out of user-facing listings and the withdraw and loop-in paths (which require an active deposit) cannot touch it — the only signal is one Warnf at startup. Reachability depends on whether a client can hold more than one static_addresses row before migrating, which I cannot confirm because staticaddr/address/manager.go is not in the review context; a log.Errorf plus surfacing these rows in GetVisibleDeposits would keep an operator from silently losing sight of the funds.
|
/gateway re-review |
There was a problem hiding this comment.
Gateway review — 5 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 5 Minor · 🔵 0 Nit
Summary
All ten prior findings from the first pass hold up as addressed at this head. Migration 22 is now gated on there being exactly one static_addresses row, deposits carry their owning address parameters from creation with fail-closed guards in the store, the loop-in recovery path joins static_addresses and funnels through a shared converter, script reconstruction validates both protocol version and persisted pkScript, ToDeposit's exported signature is restored, and the reflective parity test now covers all three joined row converters — including the one that originally dropped StaticAddressID. The address manager test that asserts params.ID is positive immediately after NewAddress is the piece that closes F9 convincingly.
Two prior findings are unchanged. SignDescriptor still mixes the manager's global timeout leaf with per-deposit keys and pkScript, and a quarantined ownerless deposit is still surfaced only as a single startup warning — with the repair query removed in the F1 fix, there is now no code path anywhere that can ever assign it an owner.
Three new minor concerns: the down migration cannot execute on SQLite because the dropped column carries a foreign key; the new parameter validation in createNewDeposit runs after a wallet address has already been derived, so a persistent failure burns one address per 10s poll; and the retained ToDeposit silently discards the StaticAddressID its input row now carries.
Status of prior findings
- F1 addressed: Fixed in
loopdb/sqlc/migrations/000022_deposit_static_address_id.up.sql:8— the backfill is now gated on(SELECT COUNT(*) FROM static_addresses) = 1, leaving ambiguous rows NULL, andTestDepositAddressBackfillcovers 0/1/many addresses. - F2 addressed: Fixed in
staticaddr/deposit/manager.go:412—createNewDepositattaches the persistedAddressParams, andstaticaddr/deposit/sql_store.go:50rejects a nil-parameter or unpersisted-ID deposit rather than writing NULL. - F3 addressed: Fixed in
staticaddr/loopin/sql_store.go:598—DepositsForSwapHashgained theLEFT JOIN static_addresses, and the hand-builtsqlc.Depositliteral is replaced bydeposit.ToDepositForSwapHash, which carries every joined address column. - F4 addressed: Fixed in
staticaddr/deposit/deposit.go:193— reconstruction now mapsProtocolVersion_V0toMuSig2Version100RC2, rejects anything else, and compares the derived script againstAddressParams.PkScript. - F5 addressed: Fixed in
staticaddr/deposit/sql_store.go:271—ToDeposit(row sqlc.Deposit, ...)is restored and theAllDeposits/GetDeposit/DepositForOutpointgenerated signatures are unchanged; the joined reads use newWithAddressnames, so the Maintenance-only release note is correct. - F6 addressed: Fixed in
staticaddr/deposit/sql_store_test.go:171— all conversions funnel through the internaldepositRow, andTestDepositRowConvertersStayInSyncreflectively asserts field-for-field parity, so adding a column without updating a converter fails the build. - F7 addressed: Fixed in
staticaddr/deposit/sql_store_test.go:106—TestToDepositWithAddressdrives the new branch with real serialized keys, asserts every mapped field, and pins the malformed-pubkey error path. - F8 addressed: Fixed in
staticaddr/deposit/manager.go:239—recoverDepositsrecords the deposit inm.depositsand then skips FSM creation for ownerless rows instead of abortingRun;TestManagerQuarantinesOwnerlessDepositcovers it. - F9 addressed: Fixed in
staticaddr/address/sql_store.go:50—CreateStaticAddressreads the assigned ID back inside the insert transaction and writes it onto the caller's parameters, andstaticaddr/address/manager_test.goassertsparams.IDis positive afterNewAddressthrough the real manager. - F10 addressed: Fixed in
staticaddr/deposit/sql_store_test.go:184— the unguarded hand-copy intoStaticAddressLoopInno longer exists, anddepositRowFromSwapHashis covered by the same reflective parity check as the other converters.
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)
| @@ -0,0 +1 @@ | |||
| ALTER TABLE deposits DROP COLUMN static_address_id; | |||
There was a problem hiding this comment.
🟡 F13 (Minor) — Down migration 22 cannot run on SQLite · loopdb/sqlc/migrations/000022_deposit_static_address_id.down.sql:1
ALTER TABLE deposits DROP COLUMN static_address_id fails on SQLite because SQLite refuses to drop a column that is used in a foreign key constraint, and the up migration adds this column with REFERENCES static_addresses(id). This is what distinguishes migration 22's rollback from earlier column-adding migrations in this directory, whose columns carry no FK.
The forward path is unaffected, so this only surfaces if something actually runs the down migration. I cannot see from the loaded context whether loopd or its test suite ever does — no migration-down harness is in file_contents — so severity is scaled to that uncertainty. Fixing it requires either dropping the REFERENCES clause from the up migration (SQLite does not enforce it unless foreign_keys is on, and the application already guards ownership in CreateDeposit) or rebuilding the table in the down migration.
| return nil, err | ||
| } | ||
|
|
||
| addressParams, err := m.cfg.AddressManager. |
There was a problem hiding this comment.
🟡 F14 (Minor) — Failed parameter check burns a wallet address every poll · staticaddr/deposit/manager.go:392
createNewDeposit derives a fresh taproot sweep address via WalletKit.NextAddr before validating the static address parameters, so each of the three new failure paths (err, addressParams == nil, addressParams.ID <= 0) consumes a wallet address and then aborts. Because reconcileDeposits errors are only logged in Run's block loop and pollDeposits retries every PollInterval (10s), a persistent parameter failure advances the wallet's derivation index indefinitely while never persisting the deposit.
Before this change the only post-NextAddr failure was a transient Store.CreateDeposit error; these new checks are deterministic, which turns a bounded leak into an unbounded one. Moving the parameter fetch and the two guards above the NextAddr call removes the waste without changing behavior.
| InitiationHeight sql.NullInt32 | ||
| } | ||
|
|
||
| func depositRowFromDeposit(row sqlc.Deposit) depositRow { |
There was a problem hiding this comment.
🟡 F15 (Minor) — ToDeposit drops the StaticAddressID its row now carries · staticaddr/deposit/sql_store.go:320
depositRowFromDeposit copies only nine of the eighteen depositRow fields and omits StaticAddressID, even though this PR adds that field to sqlc.Deposit (loopdb/sqlc/models.go:23). Any caller of the exported ToDeposit therefore gets AddressParams == nil from a row that does have an owner — which then routes the deposit into the quarantine path or fails GetStaticAddressScript.
No production caller remains today (the store uses the WithAddress variants, and ToDeposit was retained for source compatibility per F5), so this is latent rather than live. Copying StaticAddressID alone would make it worse, not better: the address columns are absent from sqlc.Deposit, so toDeposit would reach btcec.ParsePubKey(nil) and error. The honest fix is to document ToDeposit as address-less — or return an explicit error when row.StaticAddressID.Valid — so a future caller cannot mistake nil parameters for "this deposit has no owner".
| params := deposit.AddressParams | ||
|
|
||
| address, err := cfg.AddressManager.GetStaticAddress(ctx) | ||
| address, err := deposit.GetStaticAddressScript() |
There was a problem hiding this comment.
🟡 F11 (Minor) · staticaddr/deposit/fsm.go:189 · unresolved
FSM.SignDescriptor (staticaddr/deposit/fsm.go:530) still takes WitnessScript from f.cfg.AddressManager.GetStaticAddress(ctx) while KeyDesc and Output.PkScript come from the deposit's own f.params, so once more than one static address exists the timeout leaf and the output being spent can disagree. Not a defect today, but f.address — computed here at line 189 from the deposit's own parameters — is already the correct per-deposit script.
| // aren't rediscovered, but don't let one ambiguous legacy row stop | ||
| // recovery of every other deposit. | ||
| if d.AddressParams == nil { | ||
| log.Warnf("Quarantining deposit %x: static address "+ |
There was a problem hiding this comment.
🟡 F12 (Minor) · staticaddr/deposit/manager.go:240 · unresolved
A quarantined ownerless deposit stays in m.deposits but never enters activeDeposits, so GetVisibleDeposits filters it out and the withdraw and loop-in paths cannot reach it; the only signal is this one Warnf at startup. The F1 fix also removed BatchSetStaticAddressID, so there is now no code path that can ever assign such a row an owner — quarantine is permanent and silent. Reachability still depends on whether a client can hold more than one static_addresses row before migrating, which I cannot confirm because staticaddr/address/manager.go is not in the review context; a log.Errorf plus surfacing these rows in GetVisibleDeposits would at least keep an operator from losing sight of the funds.
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: