You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(minter): finalize withdrawals from their nonce accounts - #260
Finalizes durable-nonce withdrawals from their nonce accounts instead of from getSignatureStatuses. Measurements of the RPC providers' history retention showed that, with the 3-out-of-4 consensus, the status of a withdrawal older than about 16 hours can no longer be agreed on, so a landed withdrawal checked late would stay pending forever. A nonce account's current state does not depend on any provider's retention: an unchanged nonce means the transaction has not landed and it is re-broadcast, a nonce value the minter never bound proves that it landed, and getTransaction then tells whether it succeeded or failed. The nonce account stays bound until that outcome is known. Deposit sweeps are still finalized from their signature statuses.
The withdrawal side of the finalization timer is organized in independent stages, similar to the ckETH minter: the nonce accounts of in-flight withdrawals are read once, then the withdrawals that have not landed are re-broadcast and the landed ones are finalized. The outcome of a landed withdrawal is fetched with the same getTransaction call as for sweeps, which attributes the response by verifying its signature. When a finalization round leaves work for the next one, the timer is rescheduled after a short delay instead of immediately.
Status queries cannot decide older withdrawals, since some providers keep
only hours of history and others answer inconsistently. The finalization
timer now reads the nonce account of each in-flight withdrawal instead: an
unchanged nonce triggers the re-broadcast, a stale one no decision, and a
never-seen one fetches the landed transaction with getTransaction to
record its outcome, keeping the account bound until it is known. Sweeps
are still decided with getSignatureStatuses.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Read the nonce accounts of in-flight withdrawals once, then re-broadcast
the transactions that have not landed and finalize the ones that did,
as independent stages of the finalization timer.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The changes affect financially sensitive transaction finalization and nonce-account lifecycle behavior, warranting final human review despite no identified defects.
0 open findings
What changed in this PR
Moves withdrawal finalization from retention-sensitive signature statuses to durable nonce account state and finalized transaction outcomes.
Changes:
Classifies withdrawal nonces as unchanged, stale, or advanced.
Re-broadcasts unchanged withdrawals and resolves advanced withdrawals through getTransaction.
Adds tests, metrics, integration mocks, and updated design documentation.
File
Description
minter/src/withdraw/tests.rs
Updates withdrawal flow tests.
minter/src/withdraw/nonce/mod.rs
Classifies nonce-read failures.
minter/src/test_fixtures/mod.rs
Adds withdrawal outcome fixtures.
minter/src/storage/mod.rs
Stores the unresolved-outcome metric.
minter/src/state/nonce_pool/mod.rs
Classifies nonce values.
minter/src/state/mod.rs
Exports nonce classification.
minter/src/rpc/mod.rs
Fetches and validates withdrawal outcomes.
minter/src/monitor/withdrawals.rs
Implements nonce-based monitoring.
minter/src/monitor/tests.rs
Tests finalization and re-broadcast behavior.
minter/src/monitor/mod.rs
Separates sweep and withdrawal monitoring.
minter/src/metrics.rs
Exposes unresolved withdrawals.
integration_tests/tests/tests.rs
Updates end-to-end finalization coverage.
integration_tests/src/fixtures.rs
Adds nonce and transaction RPC mocks.
docs/design.md
Documents the revised finalization model.
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Fast sweep retries can repeatedly rebroadcast old withdrawals every 10 seconds, and the critical transaction-mismatch path lacks coverage.
0 open findings
Previously missed (2)
In code that hasn't changed since last review
Fast retries repeatedly rebroadcast withdrawals
minter/src/monitor/mod.rs:65
Fast sweep retries also rerun this withdrawal path. Once a withdrawal is older than 90 seconds, submitted_at never changes, so any sweep/credit backlog that schedules finalize_transactions every 10 seconds causes the same transaction to be re-broadcast every 10 seconds rather than the documented maximum of once per 2-minute round. This can multiply RPC cost during recovery; gate fast retries away from withdrawal re-broadcasting or track the last attempt time.
Add test for rejecting mismatched transaction responses
minter/src/rpc/mod.rs:84
The exact-transaction comparison is the safety check that prevents a bogus nonce advance from finalizing the wrong withdrawal, but the new rejection branch has no test: current tests only pass responses built from the submitted transaction. Add a case returning a different message/signature and assert UnexpectedTransaction (and that monitor state remains submitted).
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Fetch the outcome of a landed withdrawal with the same getTransaction
call as sweeps, which attributes the response by verifying its
signature, instead of a dedicated wrapper comparing it with the
submitted message. Tests that finalize withdrawals now sign them with
the real test key already used for sweeps.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Short sweep-backlog retries can repeatedly re-broadcast old withdrawals every ten seconds instead of the documented two-minute cadence.
0 open findings
Previously missed (1)
In code that hasn't changed since last review
Backlog retries re-broadcast withdrawals every 10 seconds
minter/src/monitor/mod.rs:60
A sweep/credit backlog schedules this whole function again after 10 seconds, so it also reruns withdrawal nonce reads and re-broadcasts. Because withdrawal eligibility is based only on the original submitted_at, every old withdrawal with an unchanged nonce is then re-broadcast on each backlog retry rather than at most once per two-minute interval as documented in docs/design.md:603. Please either make the short retry process only sweep backlog work or track/throttle each withdrawal's last re-broadcast time.
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Short sweep-retry timers currently cause withdrawal polling and rebroadcasts every 10 seconds instead of the documented two-minute cadence.
0 open findings
Previously missed (1)
In code that hasn't changed since last review
10-second retries rebroadcast withdrawals and multiply RPC traffic
minter/src/monitor/mod.rs:66
The 10-second retry path re-runs withdrawal handling as well as the remaining sweep work. While a sweep backlog keeps more_transactions_to_check or more_sweeps_to_credit true, every old withdrawal with an unchanged nonce is therefore re-broadcast every 10 seconds, and every withdrawal nonce/outcome is queried on every retry. This contradicts the documented two-minute cadence and can multiply RPC traffic and cycle usage. Please make the short retry process only the remaining sweep work, or track/gate withdrawal polling and re-broadcasts independently.
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
► docs/design.md Update SOL nonce and finalization behavior references ► integration_tests/src/fixtures.rs Add nonce seed constants and mock behaviors for finalized withdrawals ► integration_tests/tests/tests.rs Adjust test to read nonce advance during finalization ► minter/src/metrics.rs Expose undecided_withdrawal_transactions metrics by reason ► minter/src/monitor/mod.rs Refactor finalize logic and introduce withdrawal-specific monitoring hooks ► minter/src/monitor/tests.rs Update tests for new finalization/withdrawal semantics ► minter/src/monitor/tests.rs Add tests for finalized withdrawal handling and related mocks ► minter/src/monitor/tests.rs Update finalization tests to account for sweep withdrawal scenarios
🤖 With this PR, getTransaction becomes the only way a withdrawal finalizes. On main it still reserves a 2 MB response and attaches 50B cycles, while the SOL RPC canister quotes about 220B, so every call is rejected before reaching a provider. Without #263, every landed withdrawal would stay TxSent with its nonce account bound, and withdrawals would halt once the pool is exhausted. Today they still finalize through getSignatureStatuses, which attaches 1T. #263 fixes this by falling back to the default estimates, and the two only conflict in withdraw/tests.rs, so this PR should not be deployed without it.
Related: the per-withdrawal cost added to §3.3.2 (13.9B) prices getTransaction at 7.5B, i.e. the design's 50 KB estimate. With #263's default it is about 3.2B, so the figure overstates the cost rather than understating it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mbjorkqvist
left a comment
Finalizes durable-nonce withdrawals from their nonce accounts instead of from
getSignatureStatuses. Measurements of the RPC providers' history retention showed that, with the 3-out-of-4 consensus, the status of a withdrawal older than about 16 hours can no longer be agreed on, so a landed withdrawal checked late would stay pending forever. A nonce account's current state does not depend on any provider's retention: an unchanged nonce means the transaction has not landed and it is re-broadcast, a nonce value the minter never bound proves that it landed, andgetTransactionthen tells whether it succeeded or failed. The nonce account stays bound until that outcome is known. Deposit sweeps are still finalized from their signature statuses.The withdrawal side of the finalization timer is organized in independent stages, similar to the ckETH minter: the nonce accounts of in-flight withdrawals are read once, then the withdrawals that have not landed are re-broadcast and the landed ones are finalized. The outcome of a landed withdrawal is fetched with the same
getTransactioncall as for sweeps, which attributes the response by verifying its signature. When a finalization round leaves work for the next one, the timer is rescheduled after a short delay instead of immediately.🤖 Generated with Claude Code