staticaddr: surface mempool deposits immediately#1104
staticaddr: surface mempool deposits immediately#1104hieblmi wants to merge 10 commits intolightninglabs:masterfrom
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 significantly improves the user experience for static-address deposits by making them available for certain operations, specifically loop-ins, as soon as they are detected in the mempool. This change reduces the waiting time for users by removing the previous requirement for a minimum number of confirmations before a deposit could be utilized. The underlying logic has been updated to correctly track and manage the state and expiry of these newly visible unconfirmed deposits, while ensuring that more sensitive operations like withdrawals and channel opens still adhere to confirmation requirements. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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 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 counter productive. 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 refactors the static address deposit management by decoupling the 'Deposited' state from a minimum confirmation count. Deposits are now considered 'Deposited' as soon as they are detected, even if unconfirmed, and their actual confirmation height is tracked. This change impacts how unspent deposits are listed, how expiry is calculated (introducing depositBlocksUntilExpiry and confirmationHeightForUtxo helpers), and how IsExpired and IsSwappable functions behave for unconfirmed deposits. While unconfirmed deposits are now available for loop-ins, explicit checks have been added to openchannel and withdraw managers to ensure only confirmed deposits are used for these operations. The deposit.Manager now updates currentHeight and reconciles deposits on every new block, making the system more responsive. A review comment suggests that the pollDeposits function might be redundant given that reconcileDeposits is now called on every new block, proposing its removal for efficiency and logic simplification.
| case height := <-newBlockChan: | ||
| m.currentHeight.Store(uint32(height)) | ||
|
|
||
| err := m.reconcileDeposits(ctx) |
There was a problem hiding this comment.
Now that reconcileDeposits is called on every new block, the periodic polling started by pollDeposits seems redundant. Reconciling on new blocks is more efficient and responsive than polling on a fixed interval.
Consider removing the pollDeposits function and its call on line 127 to avoid unnecessary work and simplify the logic.
hieblmi
force-pushed
the
dyn-conf-tracker
branch
2 times, most recently
from
43803c4 to
77cfd21
Compare
hieblmi
force-pushed
the
dyn-conf-tracker
branch
3 times, most recently
from
38c90ee to
f61d02f
Compare
|
@hieblmi, remember to re-request review from reviewers when ready |
hieblmi
force-pushed
the
dyn-conf-tracker
branch
3 times, most recently
from
b4fe625 to
5bba57d
Compare
hieblmi
force-pushed
the
dyn-conf-tracker
branch
from
5bba57d to
88fbde3
Compare
hieblmi
force-pushed
the
dyn-conf-tracker
branch
2 times, most recently
from
14d52be to
6babc2d
Compare
hieblmi
force-pushed
the
dyn-conf-tracker
branch
7 times, most recently
from
a69f04c to
7c9ab14
Compare
Surface static-address deposits as soon as they appear in the wallet, without waiting for six confirmations. 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 deposits confirm, guard unconfirmed deposits against expiry, and mark vanished unconfirmed outpoints as Replaced so RBFed-away deposits stop showing up in RPCs.
Switch ListUnspentDeposits to derive availability from stored deposit state instead of a MinConfs split. Treat wallet UTXOs that are unknown to the store as new deposits, and only keep known deposits that are still in the Deposited state. Also update the static-address RPC responses to report sensible expiry data for unconfirmed deposits, derive summary totals from stored deposit state, and hide Replaced records from normal deposit listings and totals.
Treat deposits with ConfirmationHeight=0 as swappable because their CSV timeout has not started yet. When auto-selecting deposits, still prefer confirmed outputs first so the server can accept them immediately, and only use blocks-until-expiry as a tie-breaker inside the confirmed and unconfirmed groups.
Once Deposited includes mempool outputs, the channel-open and withdrawal flows must explicitly require confirmed inputs. Filter unconfirmed deposits out of automatic selection and fail manual requests that reference mempool deposits so we do not build PSBTs or withdrawal attempts with unusable inputs.
Remove the old "no confirmed deposits available" error now that mempool deposits are listed immediately. Add a conservative warning when the selected deposits have fewer than six confirmations so the user knows the swap payment may wait for the server's dynamic 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.
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, so withdrawal completion does not block while deposit locks are held.
Do not silently drop unconfirmed deposited outputs when the caller requests an all-deposit withdrawal. Fail the request instead so the RPC and CLI semantics stay aligned with the documented meaning of all. Add helper coverage for both the rejecting and all-confirmed cases.
hieblmi
force-pushed
the
dyn-conf-tracker
branch
from
7c9ab14 to
1542a84
Compare
When an unconfirmed static-address deposit was replaced, the client reconciler only knew how to mark the original outpoint gone and discover the descendant as a brand-new deposit. That split the logical deposit identity across two records: the old deposit, active FSM and pending loop-in still pointed at the dead outpoint while the replacement surfaced as unrelated new funds. Teach the deposit manager to rebind uniquely matched unconfirmed replacements in place, carrying the deposit ID, state and active FSM forward to the new outpoint. This keeps looping-in deposits attached to the replacement chain instead of leaving them stranded on the vanished outpoint. Keep the existing vanished-deposit invalidation path for cases that cannot be matched safely. Also reload static loop-ins from the current deposit rows rather than the stale outpoint snapshot stored at swap creation time, and add targeted tests for replacement rebinding and recovered outpoint loading.
Reproduce the server's static-address deposit selection order in the CLI using the already-returned deposit metadata. This keeps the low-confirmation warning focused on the deposits that auto-selection would actually choose. Add coverage for the confirmed-preferred and unconfirmed-needed cases.
hieblmi
force-pushed
the
dyn-conf-tracker
branch
from
1542a84 to
aafa6a7
Compare
4 participants
This PR does the following:
settles.