staticaddr: multi-address support#1139
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 introduces multi-address support for static-address deposits, significantly increasing the flexibility of loop-in and withdrawal operations. By persisting per-deposit address parameters and implementing a generation-based recovery flow, the system ensures that funds remain accessible even after local data loss. Additionally, the PR includes robust CLI updates for address funding and improved deposit reconciliation logic to handle mempool-level changes and chain reorgs. 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 implements a robust recovery system for Loop's static addresses and L402 tokens, utilizing encrypted immutable backups. Key additions include a new recover CLI command, a dedicated recovery package for backup orchestration, and significant updates to the address and deposit managers to support multi-address derivation and mempool-aware reconciliation. The loop-in FSM is also enhanced to handle fractional swaps with static change outputs and server-side risk notifications. Feedback focuses on improving the clarity of change amount logic, optimizing the performance of multiset matching during change verification, and making the retry limit for stable block height lookups configurable.
| changeAmt) | ||
|
|
||
| expectedChange += changeAmt | ||
| changeAmt := loopIn.ExpectedChangeAmount() |
There was a problem hiding this comment.
The ExpectedChangeAmount function returns 0 if changeAmount >= totalDepositAmount. This logic seems to imply that if the change amount is equal to the total deposit amount (i.e., selected amount is 0), it returns 0. This is correct, but the condition changeAmount >= totalDepositAmount is slightly unusual. It might be clearer to explicitly check if l.SelectedAmount == 0.
| // Match expected change outputs as a multiset. This rejects batched | ||
| // transactions that collapse two equal client change outputs into one | ||
| // output unless the protocol explicitly negotiates such aggregation. | ||
| matchedOutputs := make([]bool, len(sweepTx.TxOut)) | ||
| for _, expected := range expectedChanges { | ||
| var found bool | ||
| for i, out := range sweepTx.TxOut { | ||
| if matchedOutputs[i] { | ||
| continue | ||
| } | ||
|
|
||
| if out.Value == expected.Value && | ||
| bytes.Equal(out.PkScript, expected.PkScript) { | ||
|
|
||
| matchedOutputs[i] = true | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| // We found the expected change output. | ||
| return nil | ||
| if found { | ||
| continue | ||
| } | ||
|
|
||
| return fmt.Errorf("couldn't find expected change of %v "+ | ||
| "satoshis sent to static address", expected.Value) | ||
| } |
There was a problem hiding this comment.
The multiset matching logic in checkChange is
| for range maxAttempts { | ||
| _, beforeHeight, err := m.cfg.ChainKit.GetBestBlock(ctx) | ||
| if err != nil { | ||
| return nil, 0, fmt.Errorf("unable to get best block "+ | ||
| "before listing deposits: %w", err) | ||
| } | ||
|
|
||
| utxos, err = m.cfg.AddressManager.ListUnspent(ctx, 0, MaxConfs) | ||
| if err != nil { | ||
| return nil, 0, fmt.Errorf("unable to list new deposits: %w", | ||
| err) | ||
| } | ||
|
|
||
| _, afterHeight, err := m.cfg.ChainKit.GetBestBlock(ctx) | ||
| if err != nil { | ||
| return nil, 0, fmt.Errorf("unable to get best block "+ | ||
| "after listing deposits: %w", err) | ||
| } | ||
|
|
||
| if beforeHeight == afterHeight { | ||
| m.currentHeight.Store(uint32(afterHeight)) | ||
| return utxos, afterHeight, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The retry loop in listUnspentWithBestHeight uses a hardcoded maxAttempts = 3. This is reasonable, but consider making this a configurable parameter or using a more robust backoff strategy if this becomes a frequent source of flakes.
There was a problem hiding this comment.
Code Review
This pull request introduces static address recovery for Loop, enabling the restoration of L402 and static-address state from encrypted backups. It also adds support for fractional loop-ins by implementing change address derivation and handling, along with improved deposit reconciliation and validation logic. My review identified several critical issues regarding notification replay logic, performance concerns with wallet transaction scanning, and potential address leakage during retries.
| if ntfn, ok := m.staticLoopInRiskAccepted[swapHash]; ok { | ||
| notifChan <- ntfn | ||
| delete(m.staticLoopInRiskAccepted, swapHash) | ||
| } |
There was a problem hiding this comment.
The replay logic for riskAccepted notifications deletes the entry from the map as soon as the first subscriber joins. If there are multiple concurrent subscribers (e.g., the FSM and an RPC handler), only the first one will receive the replayed notification. The same issue exists for riskRejected notifications at line 227.
| context.AfterFunc(ctx, func() { | ||
| m.removeSubscriber(NotificationTypeStaticLoopInRiskAccepted, sub) | ||
| m.Lock() | ||
| delete(m.staticLoopInRiskAccepted, swapHash) |
There was a problem hiding this comment.
Deleting the cached notification when a subscriber's context is cancelled is problematic. If one subscriber (e.g., a transient RPC call) cancels its subscription, the notification is removed from the manager, preventing the primary FSM or future subscribers from ever seeing it. The same issue exists at line 235.
| // We need lnd's wallet transaction view rather than only the funding | ||
| // transaction: a matching previous outpoint tells us the deposit has | ||
| // already been spent by a wallet-known transaction. When mempool spends | ||
| // matter, lnd exposes them through ListTransactions with endHeight=-1. |
There was a problem hiding this comment.
ListTransactions(ctx, 0, endHeight) scans the entire wallet history from genesis. This is an expensive operation in lnd. Since GetTxOut is called in a loop for every deposit input during HTLC signing (see originalDepositOutpointUnavailable in actions.go), this can lead to significant delays and high resource usage for wallets with many transactions. Consider calling ListTransactions once and checking all outpoints against the result, or using a more efficient API like ListUnspent if the goal is just to check for spends.
| f.loopIn.ChangeAddressParams, err = | ||
| f.cfg.AddressManager.NewChangeAddress(ctx) | ||
| if err != nil { | ||
| err = fmt.Errorf("unable to create static address "+ | ||
| "change output: %w", err) | ||
|
|
||
| return returnError(err) | ||
| } |
There was a problem hiding this comment.
NewChangeAddress is called without checking if f.loopIn.ChangeAddressParams is already populated. On retries of InitHtlcAction (e.g., if a failure occurs after address generation but before swap persistence), this will generate and persist a new change address on each attempt. This leaks address space and can bloat the database and recovery scan range. Consider checking if f.loopIn.ChangeAddressParams is already set before deriving a new one.
if f.loopIn.ChangeAddressParams == nil {
f.loopIn.ChangeAddressParams, err =
f.cfg.AddressManager.NewChangeAddress(ctx)
if err != nil {
err = fmt.Errorf("unable to create static address "+
"change output: %w", err)
return returnError(err)
}
}
hieblmi
force-pushed
the
multi-address
branch
8 times, most recently
from
850dd2f to
c4fccb8
Compare
Surface static-address deposits as soon as they appear in the wallet instead of waiting for the old six-confirmation readiness threshold. Reconcile the wallet view on startup, on each block, and on the polling ticker so mempool deposits are created immediately. Backfill the first confirmation height once those outputs confirm, protect unconfirmed deposits from expiry, and mark vanished unconfirmed outpoints as Replaced so RBFed-away deposits stop showing up in RPCs. Expose the new state through static-address RPCs by deriving availability and summary totals from stored deposit state, reporting sensible expiry data for unconfirmed outputs, and hiding Replaced records from normal listings.
Allow static loop-ins to select unconfirmed deposits because their CSV timeout has not started yet, while still preferring confirmed outputs during automatic selection. Keep confirmed-input requirements for channel opens and withdrawals now that Deposited includes mempool outputs. Filter unconfirmed deposits out of automatic selection for those flows and fail manual requests that reference them, so the client does not build PSBTs or withdrawal attempts with unusable inputs. Treat deposit.MinConfs as the legacy readiness threshold rather than the single source of truth for all flows. Loop-in readiness is now governed by server confirmation-risk policy, while withdrawals and channel opens keep their confirmed-input checks.
Remove the old "no confirmed deposits available" error now that mempool deposits are listed immediately and can be selected for static loop-ins. Reproduce the server static-address deposit selection order in the CLI using the already-returned deposit metadata. This keeps the low-confirmation warning focused on the deposits auto-selection would actually choose, so users only see it when the swap payment may wait for the server confirmation-risk policy.
If InitHtlcAction creates the private swap invoice but fails before the loop-in is stored, the retry path otherwise leaves behind a live orphan invoice. Cancel that invoice on the early error path with a detached, timeout-limited context, and reuse the same helper when tearing down the monitor path. This keeps failed initialization attempts from leaving invoices that no local swap can complete.
FinalizeDepositAction only needs to tell the manager to remove the FSM from its active set, but the old synchronous send was still tied to the caller context and could race with request cancellation or a busy manager loop. Send the cleanup notification asynchronously and tie it to the FSM lifetime instead. Withdrawal completion no longer blocks while deposit locks are held just because the original request context was canceled.
Keep replacement UTXOs as fresh deposits while preserving the original deposit record and selected outpoint snapshot for pending swaps. Before signing a static loop-in HTLC, check each original selected outpoint with GetTxOut(..., includeMempool=true). Cancel the pending invoice only when that check reports an original outpoint unavailable; lookup errors fail the action without canceling so transient chain backend errors do not incorrectly abandon the swap. Keep recovered loop-ins using their stored outpoint snapshot and cover replacement discovery and cancellation in tests.
ListUnspentDeposits now reports only wallet UTXOs that have an active Deposited record. That matches the static loop-in admission path and avoids exposing wallet-seen outputs that are not ready for loop-in selection. Make local notification fan-out non-blocking for best-effort categories so a slow subscriber cannot stall the notification manager while it holds the subscriber lock. Static loop-in sweep signing requests remain blocking because they are work requests required for sweepbatcher presigning and must not be dropped.
Wait for the server's static loop-in risk-accepted notification before starting the client payment deadline. The server may intentionally hold the swap at the confirmation-risk gate after HTLC signing, and the client deadline should not run while that server-side wait is still in progress. Cache risk-accepted notifications by swap hash inside the local notification manager and replay them to the per-swap subscriber. This covers both reconnects and the internal race where the global notification stream receives the server event before the static loop-in FSM registers its waiter.
Add client handling for the server's static loop-in risk-rejected notification. If the server aborts confirmation-risk waiting before payment, the client fails the local swap instead of waiting for a payment deadline that will never start. Cache rejected notifications by swap hash using the same replay path as accepted notifications, and clear the opposite cached state when a final risk decision is received. This keeps reconnect and subscription-order races from stranding the client in the risk wait.
GetStaticAddressLoopInSwapsByStates passes a comma-separated state list into a SQL LIKE membership check. The query wraps the input with commas before matching latest update states as comma-delimited tokens. Wrapping that list in braces meant the first and last states were not bounded by commas, so boundary entries in a state set could be missed. In particular, Failed is the last final static-address loop-in state, which made final-state queries skip failed swaps. Drop the braces from the serialized state list and extend the SQL store test with a failed swap so the final-state boundary is covered.
Add SendCoins-backed funding through loop static deposit, keeping loop static new focused on address creation. Wire the nested lnd SendCoins request through the NewStaticAddress RPC and return the SendCoins response. Validate cheap SendCoins errors before address creation and require execute permissions for the funding-capable RPC. Regenerate CLI docs and looprpc artifacts, and add validation coverage for the static address funding request.
Add recoverdeposit CLI/RPC support for verifying one static-address output on-chain, matching it to a derived static address, restoring the address/import, and directly creating or reactivating the deposit row. The recovery request also accepts optional seed metadata so operators can recover deposits before the local static-address seed row has been restored.
2 participants
Summary
This PR adds multi-address support for static-address deposits.
Static-address deposits now retain the concrete address parameters they were sent to, instead of assuming every deposit belongs to a single reusable output script. This allows loop-ins and withdrawals to spend deposits that were received across multiple derived static addresses.
Key Changes
SendCoins.Recovery
The recovery flow now restores multi-address static deposits by deriving receive and change address candidates from the immutable static-address backup, scanning wallet-visible UTXOs, and reconciling matching deposits back into loopd.
Testing
This branch adds coverage for multi-address deposit persistence, recovery, loop-in signing, generated change outputs, server input proofs, and the new static-address funding flow.