staticaddr/withdraw: prepare deposits before publish#1176
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 improves the reliability of the loop-in process by ensuring that selected deposits are always retrieved from the live, canonical state managed by the deposit manager. By moving away from potentially stale database snapshots, the system avoids incorrect state validation and prevents failures during timeout transitions. 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 updates the refreshSelectedDeposits function in staticaddr/loopin/actions.go to retrieve active deposits using the deposit manager's AllStringOutpointsActiveDeposits method instead of DepositsForOutpoints. This ensures that recovery relies on the deposit manager's active set rather than stale snapshots. The corresponding unit tests and the noopDepositManager mock in actions_test.go have been updated to reflect and verify this change. There are no review comments, and I have no additional feedback to provide.
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.
695f614 to
12cd993
Compare
Neutrino can remove a spent input from its wallet view while transaction publication is still blocked. If the deposit remains Deposited, reconciliation can remove it before the withdrawal manager records the withdrawal intent. Attach the finalized transaction, transition the deposits to Withdrawing, and set up local tracking before publishing. Keep the prepared state on publication errors because the wallet RPC outcome can be ambiguous and recovery or fee bumping must be able to retry.
12cd993 to
9ee5c42
Compare
hieblmi
changed the title
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
Found some potentially fragile parts in this branch. I haven't validated them empirically, so some of them may be false-positives.
Also I propose to add regression coverage in a separate commit. If the main fix commit is then reverted, the new test must build and run, but fail with a symptomatic error, reproducing the failure you are fixing. So it is easy to demo that the fix is sound.
| deposits[0].Lock() | ||
| prevTx := deposits[0].FinalizedWithdrawalTx | ||
| deposits[0].Unlock() | ||
| previousWithdrawalTx := previousWithdrawalTxns[0] |
There was a problem hiding this comment.
This assumption is pre-existing, but could we fix it while touching this logic? The fee-bump path verifies that all Withdrawing deposits reference the same previous transaction, but that invariant is not enforced for Deposited deposits or inconsistent recovery states.
Since we now retain every previous transaction, consider deleting every distinct non-nil previous hash other than finalizedTx, or explicitly validating that all entries match. Otherwise, a stale transaction associated with a deposit other than index 0 could remain in finalizedWithdrawalTxns and continue being republished. A multi-deposit regression test would be useful here.
| withdrawalPkScript, err := txscript.PayToAddrScript(withdrawalAddress) | ||
| // Transition before publishing so wallet reconciliation can't remove a | ||
| // spent deposit from the active set while publication is in progress. | ||
| err = m.cfg.DepositManager.TransitionDeposits( |
There was a problem hiding this comment.
Can we ensure persistence errors are observable here? TransitionDeposits returns success even when the FSM’s Store.UpdateDeposit fails, because that error is only logged. Since the explicit update now only runs for fee bumps, an initial withdrawal can reach publication while the database still says Deposited and has no finalized transaction. That recreates the restart/reconciliation race this change is intended to fix.
| if err != nil { | ||
| return "", "", fmt.Errorf("could not get withdrawal "+ | ||
| "pkscript: %w", err) | ||
| for i, d := range deposits { |
There was a problem hiding this comment.
Is this rollback safe for multiple deposits? TransitionDeposits processes FSMs sequentially, so an earlier deposit may already be Withdrawing and persisted when a later transition fails. Restoring only FinalizedWithdrawalTx would leave that deposit in Withdrawing with its old or nil transaction, while the cluster may have mixed states. We likely need atomic persistence/transition or a rollback that also restores state and database records.
| err = m.handleWithdrawal( | ||
| ctx, deposits, finalizedTx.TxHash(), withdrawalPkScript, | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
At this point the deposits are already Withdrawing. If handleWithdrawal fails, we return before publishing or adding local tracking. A retry is then classified as a fee bump and skips handleWithdrawal, so the replacement can publish without any spend/confirmation watcher and remain Withdrawing until restart. Could this path retry notifier setup or restore the prepared state before returning?
|
|
||
| // Add the new withdrawal tx to the finalized withdrawals to republish | ||
| // it on block arrivals. | ||
| m.mu.Lock() |
There was a problem hiding this comment.
Could we add the replacement to this map only after all deposit updates succeed? If an UpdateDeposit below fails, the method returns but the old transaction has already been removed and the replacement remains eligible for block-triggered republication. That can broadcast a transaction whose database records are old or only partially updated.
| // notifier is run. | ||
| if allDeposited { | ||
| // Persist info about the finalized withdrawal. | ||
| err = m.cfg.Store.CreateWithdrawal(ctx, deposits) |
There was a problem hiding this comment.
Should creating the withdrawal record be part of the durable preparation and return its error? The deposits are already persisted as Withdrawing, so a crash or ignored error here allows recovery to publish the transaction without a corresponding withdrawal record. Confirmation then cannot update that record, leaving the withdrawal permanently absent from history.
2 participants
Neutrino can remove a spent input from its wallet view while transaction
publication is still blocked. If the deposit remains Deposited,
reconciliation can remove it before the withdrawal manager records the
withdrawal intent.
Attach the finalized transaction, transition the deposits to Withdrawing,
and set up local tracking before publishing. Keep the prepared state on
publication errors because the wallet RPC outcome can be ambiguous and
recovery or fee bumping must be able to retry.