staticaddr: harden deposit and loop-in lifecycle handling#1161
staticaddr: harden deposit and loop-in lifecycle handling#1161hieblmi wants to merge 22 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request strengthens the reliability of the static address deposit and loop-in lifecycle management. It introduces critical hardening measures, including improved lock ordering, safer state transitions, and more robust recovery mechanisms for loop-in swaps. These changes ensure that deposit and loop-in states remain consistent during concurrent operations and provide better handling for edge cases like duplicate outpoints or invoice updates. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements and safety checks to the static address deposit and loop-in management. Key changes include adding duplicate outpoint checks across various managers, introducing a TxOutChecker interface with an LND-backed implementation, refactoring FSM handling to support graceful shutdown and HTLC timeout sweeps, and adding comprehensive unit tests. Feedback on the changes highlights two main concerns: first, a potential data race in staticaddr/deposit/sql_store.go where GetConfirmationHeightNoLock() is called without ensuring the deposit lock is held; second, an issue in staticaddr/loopin/actions.go where silently setting invoiceUpdateChan = nil upon channel closure stops invoice monitoring, which could lead to false timeouts and protocol inconsistencies.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@claude please review |
Review: staticaddr deposit/loop-in lifecycle hardeningOverall this is a well-scoped, well-tested PR — each commit isolates one behavioral fix, and nearly every fix ships with a regression test that fails without the change (e.g. Findings1. Same applies to 2. Possible gap between the new "preserve outpoints" behavior and
var depositLookup = make(map[string]*deposit.Deposit)
for i, d := range allDeposits {
depositLookup[d.OutPoint.String()] = allDeposits[i]
}
for i, s := range swaps {
var deposits []*deposit.Deposit
for _, outpoint := range s.DepositOutpoints {
if d, ok := depositLookup[outpoint]; ok {
deposits = append(deposits, d)
}
}
swaps[i].Deposits = deposits
}If a deposit's outpoint has changed since the swap was created, this lookup silently misses it and Nit
Things that look solid
Minor/perf
Nice test coverage overall (fsm, manager reconciliation, actions, sql store, txout checker, outpoint duplicates all have new focused tests). |
|
Some commits are not signed. |
| if htlcConfirmed { | ||
| transitionDepositsToHtlcTimeout("payment deadline") | ||
| } else { | ||
| err = f.unlockDeposits(ctx) |
There was a problem hiding this comment.
AI finding.
This fixes the case where the HTLC is already confirmed at the payment deadline, but I think there is still a reuse window when the deadline fires before HTLC confirmation.
In that case we still unlock the deposits here, even though the server already has our HTLC signatures and can still publish/confirm the HTLC before CLTV. If the user reuses the deposit before that late confirmation arrives, transitionDepositsToHtlcTimeout will later try OnSweepingHtlcTimeout from whatever state the new flow put the deposit in.
That works for another loop-in because LoopingIn has the transition, but not for withdraw/open-channel states. A concrete sequence is:
- payment deadline expires,
htlcConfirmed == false, deposits are unlocked; - user starts a withdrawal or channel open;
- server HTLC confirms later;
- this monitor calls
OnSweepingHtlcTimeout, but the deposit is nowWithdrawing/OpeningChannel, where that event is invalid.
I think either the deposits need to remain unavailable until the HTLC can no longer confirm, or all reuse states that are allowed during this window need explicit handling/tests for losing the race to the HTLC.
There was a problem hiding this comment.
Addressed in 7cecfc2c.
The payment deadline now only cancels the invoice and keeps the deposits locked while the HTLC can still confirm. Deposits are only unlocked once the HTLC timeout has opened and no HTLC confirmation was seen; if the HTLC did confirm, the deposits transition to SweepHtlcTimeout instead. Added TestMonitorInvoiceAndHtlcTxKeepsDepositsLockedUntilHtlcTimeout for the race window.
|
|
||
| outpoints[i] = d.OutPoint | ||
| } | ||
| if outpoint.HasDuplicates(outpoints) { |
There was a problem hiding this comment.
This rejects duplicate outpoints within one transition request, but overlapping multi-deposit requests can still deadlock if they arrive in different orders.
For example, one goroutine can transition [A, B] while another transitions [B, A]. lockDeposits still locks in caller order, so G1 can hold A and wait for B while G2 holds B and waits for A. The documented manager/deposit lock ordering does not cover ordering among multiple deposit locks.
I think lockDeposits should canonicalize by outpoint before acquiring locks, or callers should be required to pass deposits in a canonical order. A reversed-order concurrency test would cover this.
There was a problem hiding this comment.
Addressed in 4fadd74b.
lockDeposits now locks a canonical copy sorted by outpoint and unlocks that same copy in reverse order, so overlapping [A, B] / [B, A] requests cannot acquire deposit locks in conflicting orders. Added TestLockDepositsCanonicalizesOutpoints and TestLockDepositsAllowsReversedConcurrentRequests.
|
|
||
| require.NoError(t, swapStore.CreateLoopIn(ctxb, &swap)) | ||
|
|
||
| d.OutPoint = currentOutpoint |
There was a problem hiding this comment.
Can we clarify the intended invariant this test is documenting?
This updates the deposit row to a different current outpoint while expecting the loop-in's stored DepositOutpoints to remain the original selected outpoint. That makes sense if future replacement handling treats a deposit as a stable logical record whose current outpoint can change.
If that is the intended model, I think recovery needs a follow-up: GetLoopInByHash can reconstruct deposits through the stable swap_hash/deposit-id mapping, but recoverLoopIns overwrites loopIn.Deposits by looking up loopIn.DepositOutpoints in the active-deposit map. If DepositOutpoints is the original-input snapshot and the deposit row now has a replacement/current outpoint, recovery can miss the deposit and continue with an incomplete deposit set.
If deposits are meant to be immutable by outpoint and replacement should cancel the current swap/create a new deposit, then this test may be stronger than the production invariant. In that case it would help to document that DepositOutpoints remains both the swap input snapshot and a valid recovery lookup key.
There was a problem hiding this comment.
Addressed in 37c5cf85.
The invariant is that DepositOutpoints remains the original swap input snapshot. Recovery/listing now relies on the store swap_hash/deposit-id reconstruction for the current deposit rows; recoverLoopIns only uses current reconstructed outpoints when checking active in-memory deposits. Added coverage in TestActiveDepositsForLoopInUsesCurrentDepositOutpoints and TestGetAllSwapsPreservesStoreDeposits.
Keep deposits locked when the server publishes the loop-in HTLC without paying the invoice. This lets the client sweep through the HTLC timeout path instead of making the same outputs available for another action.
Return early when block notifications reach deposits that already moved into a terminal state. This prevents final deposits from retrying expiry handling after recovery or while their FSM is still draining block updates.
Reject duplicate static-address deposit outpoints before creating withdrawal, loop-in, or channel-open requests. Use the shared outpoint duplicate helper so each flow reports the same input validation failure.
Treat closed invoice update channels as terminal for the monitor loop. This avoids spinning when lnd closes the subscription after invoice cancellation or shutdown.
Add an explicit Stop method for deposit FSM block-notification loops. Call it when the manager removes a finalized active deposit so stale FSM goroutines stop consuming block updates.
Store an independent snapshot of the outpoints selected for a static loop-in. Recovered swaps remain tied to the original funding outputs even if deposit records later change confirmation or replacement metadata. Avoid decoding an empty database outpoint string as a synthetic outpoint.
Add a TxOutChecker interface for checking whether a selected deposit outpoint is still available before signing the HTLC transaction. Back the implementation with lnd wallet transaction data so known confirmed and mempool spends mark the outpoint unavailable.
Document the lock-order invariant between Manager.mu and individual deposit locks. Later changes need both locks in the same path, so make the rule explicit before the locking surface grows.
Move active-deposit block notification fan-out into a helper. This keeps the event loop small and gives later startup replay logic a single path for notifying recovered deposit FSMs.
Guard reconcileDeposits with a dedicated mutex. Polling and block-driven reconciliation can overlap, so serialize the path before it updates confirmation data and active FSM state.
Reject nil deposits and final-state deposits before sending FSM events. This keeps callers from transitioning stale or completed deposits and uses the no-lock state helper while deposits are already locked.
Add a duration helper that falls back to the default payment timeout. Recovered legacy swaps can have a zero persisted timeout, so later deadline logic can use this without treating zero as immediate expiry.
A block notification can queue OnExpiry before a deposit reaches a final state. If the final transition wins that race first, the stale expiry event must not overwrite the terminal outcome. Keep LoopedIn and Withdrawn as self-loops on OnExpiry, matching the other final states. Add a focused FSM test that sends OnExpiry directly to each final state and verifies the state is preserved.
Document deposit lock ownership for mutable confirmation state and route production reads through deposit accessors. Keep store persistence on no-lock helpers while callers hold the deposit lock, preserving the existing transition behavior without leaving direct field reads in user-facing paths.
A shutdown while publishing or monitoring the HTLC timeout sweep should not transition the loop-in to Failed. Return NoOp on context cancellation in those actions so the persisted state remains a recovery point. Add focused tests for shutdown during publication retry and confirmation monitoring.
After the client gives the server HTLC signatures, shutdown must not drive the monitor state through the generic error path. That path cancels the invoice and attempts to unlock deposits even though the server can still publish the HTLC. Return NoOp for monitor-state cancellation races and cover shutdown with a regression test that asserts no invoice cancellation or deposit unlock occurs.
Recovered loop-ins carry two outpoint views. DepositOutpoints is the immutable swap input snapshot sent to the server and used to validate sweep requests. Deposits comes from the store's swap_hash/deposit-id join and reflects the current deposit rows. The active-deposit lookup takes a detour through the reconstructed deposit rows before asking the deposit manager for active deposits. That keeps recovery from depending on the historical input snapshot. A future replacement path can RBF a deposit from its original funding outpoint to a replacement outpoint while the swap still needs to retain the original input list. Looking up active deposits by DepositOutpoints would then fail recovery even though the store still maps the correct deposit IDs to the swap hash. Keep list responses on the store reconstruction too, so they do not re-resolve deposits through historical outpoints.
Before we send HTLC signatures to the server, the server cannot publish the HTLC transaction. After those signatures are handed over, the server can publish an HTLC that spends the selected deposits even if it never pays the swap invoice. Defend against stale local deposit state by checking the wallet's current txout view immediately before signing. A deposit can have been spent by a known withdrawal, channel open, timeout sweep, replacement, or another wallet transaction while the loop-in FSM is recovering or while earlier state still marked it as selected. Failing before signing leaves the server without spend authority over an unavailable input. Include mempool spends in the check so wallet-known unconfirmed spends are treated as unavailable too.
The txout checker needs a conservative answer when lnd's wallet history contains both the original deposit transaction and a later wallet-known spend of that outpoint. Scan the wallet transaction list once, remember a matching live txout, and keep scanning for matching previous outpoints. A known spend must win over the candidate funding output regardless of transaction ordering in the wallet response. This keeps the availability check cheap and preserves the rule that a deposit is unavailable if any wallet-known confirmed or mempool transaction already spends it.
2 participants
This PR prepares the static address deposit and loop-in code for later
confirmation-tracking changes.
It hardens deposit lifecycle handling by keeping deposits locked for the
loop-in HTLC timeout path, ignoring expiry handling in final states, rejecting
duplicate or invalid deposit transitions, stopping removed deposit FSMs, and
documenting/enforcing deposit manager lock ordering.
It also improves loop-in recovery behavior by preserving selected deposit
outpoints, handling closed invoice update channels, adding an lnd-backed txout
checker, and defaulting recovered payment timeout values.
Finally, it factors active deposit block notifications, serializes deposit
reconciliation, adds locked confirmation-height accessors, and increases the
lint timeout for the expanded static-address test suite.