staticaddr: multi-address - #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.
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.
bc06754 to
242a3e1
Compare
5a5c613 to
c1c691e
Compare
82ce8fa to
e481301
Compare
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gateway review — 11 findings
🔴 0 Blocker · 🟠 2 Major · 🟡 9 Minor · 🔵 0 Nit
Summary
The new commits addressed the reviewer feedback on RPC-contract clarity — NewStaticAddress's loss of idempotency and the swap:read → swap:execute permission change are now documented as explicit breaking changes, and the proto/Swagger/gRPC artifacts were regenerated to match. The per-deposit address threading itself is well-executed: ToPrevOuts, CreateMusig2Session, SelectDeposits, and the deposit FSM all now derive scripts from the deposit's own parameters instead of a single global address, and the new count checks in signMusig2Tx plus the multiset change matching in checkChange close real gaps that single-address code papered over.
Two new majors stand out. recordConfirmedHtlc persists the first output whose pkScript matches the HTLC without validating its value against the amounts the client actually signed, so the outpoint the timeout sweep later spends can be steered by any confirmed transaction paying that script. And the new loop static deposit command defines its confirmation-bypass flag as Name: "force, f", which does not match the generated docs and cannot resolve via cmd.Bool("force").
The remaining eight new findings are minor: a confirmation gate that is silently skipped in non-interactive use, three fail-open/fragile guards (lnd error-string matching, silent hydration no-op, positional change-output extraction), a MuSig2 session leak that the sibling function in the same file now handles correctly, dead code added without callers, and one O(n) address lookup that the manager already exposes an O(1) path for. Prior finding F1 is unchanged.
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)
Serialize address issuance with a dedicated mutex while limiting the shared map lock to in-memory updates. This keeps deposit and address lookups responsive during lnd, server, and database calls.
|
/gateway re-review |
|
Consider splitting this PR into smaller pieces, then re-invoke |
|
/gateway review |
|
gateway is already active on this PR. Reply with |
|
/gateway review |
|
Consider splitting this PR into smaller pieces, then re-invoke |
Declare f through the urfave/cli alias field instead of embedding it in the flag name. Cover both the long and short forms with a parsing test.
Require a confirmed HTLC transaction to spend the selected deposits in their signed order before recording its output. This prevents unrelated transactions using the public HTLC script from locking deposits.
Reject non-forced funding when stdin is not interactive and write the prompt to stderr. Create and fund the address in one RPC after confirmation so canceled requests leave no unused address.
Decode the requested address to its pkScript and use the active address index for ownership lookup. This avoids reconstructing and scanning every persisted static address.
Ignore an lnd duplicate-import error only when it names the expected Taproot output key. Propagate unrelated errors that happen to contain the words already exists.
Return an error when a legacy deposit needs ownership parameters but no address manager is configured. This prevents callers from proceeding with incomplete deposit metadata.
Remove the unused runtime backfill method, query, and generated bindings. Existing deposits remain covered by the schema migration that establishes their address ownership.
Track sessions created by the batch helper and clean them up if a later creation fails. Cleanup uses a non-cancelled context and preserves both creation and cleanup errors.
Identify withdrawal change through its active change-family script instead of a fixed output index. Record amounts correctly when confirmed replacements reorder or add outputs.
Stop republishing an obsolete withdrawal when its confirmed spender consumes only part of the deposit group. Finalize spent deposits, release untouched deposits, and clear stale transaction references.
|
/gateway re-review |
|
gateway is not active on this PR. Run |
|
/gateway review |
|
Consider splitting this PR into smaller pieces, then re-invoke |
4 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.