fix(platform-wallet): release UTXO reservation when broadcast fails#3985
Conversation
`CoreWallet::send_to_addresses` builds+signs a transaction, which reserves the selected UTXOs in the funds account's `ReservationSet`, then broadcasts. On broadcast failure the `?` propagated the error without releasing the reservation, so the inputs stayed reserved until the 24-block (~1h) TTL backstop reclaimed them. A user whose send failed on broadcast and retried immediately could see those coins as unavailable and hit a spurious "insufficient funds". Now a failed broadcast re-acquires the wallet-manager lock and calls `ManagedCoreFundsAccount::release_reservation(&tx)` before propagating the original error, so the inputs are selectable again right away. The account re-lookup is factored into a small `funds_account_mut` helper; a missing wallet/account on the release path is logged via `tracing::warn!` and does not alter the returned broadcast error. Adds a TDD test (`key-wallet/test-utils` dev-feature) that funds a wallet with a single UTXO, fails the first broadcast, and asserts an immediate retry succeeds — it fails against the old code with "No UTXOs available for selection". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claudius-Maginificent
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
✅ Review complete (commit eb54b37) |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
Small, well-scoped fix that releases the transaction builder's UTXO reservation when broadcast fails, preventing a spurious ~1h stranding of coins after a failed send. No blocking issues; a few suggestions and nitpicks around release-path locking, error semantics, and test coverage.
🟡 2 suggestion(s) | 💬 2 nitpick(s)
Source: reviewers opus general/rust-quality; claude-sonnet-5 general/rust-quality (ACP wrappers terminated after parseable output); gpt-5.5[high] general/rust-quality failed at initialize; verifier opus.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:168-189: Failure-path release takes an exclusive write lock it doesn't need
`ManagedCoreFundsAccount::release_reservation` takes `&self` (the `ReservationSet` uses an internal `Arc<Mutex<_>>`), so no mutable borrow of the account is required. The current code takes `self.wallet_manager.write().await` and looks the account up via `get_wallet_and_info_mut` + `get_mut` purely to satisfy `funds_account_mut`'s `&mut` signature. `WalletManager` stores all wallets in shared maps, so this write lock serializes against every other in-flight `send_to_addresses` / balance-read across every wallet managed by this instance for the duration of the cleanup — on the exact path meant to make retries fast. Switch this branch to `wallet_manager.read().await` + immutable `get_wallet_and_info` + `BTreeMap::get`, and either change `funds_account_mut` to return `&ManagedCoreFundsAccount` or introduce an immutable variant. Not a bug today (the section is short), but it's an avoidable point of contention on the exact code path meant to unblock retries.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:168-189: Releasing on every broadcaster Err assumes the tx never reached the network
Any `Err` from `broadcast_transaction` is treated as "tx definitely did not reach the network", which triggers an immediate release so a retry can reselect the same UTXOs. In practice a broadcaster can fail *after* the network has accepted the transaction — e.g. a dropped SPV/gRPC connection after send, a timeout waiting for ack, or a node error that still relayed the tx. In those cases the release + retry produces a second signed spend over the same inputs, turning a transient network hiccup into a self-inflicted double-spend attempt (the second broadcast usually then fails on the node, but the UX is confusing). This is defensible as a UX call — the pre-PR TTL-backstop behavior had the opposite failure mode — but it should be a deliberate contract. Either narrow the release to error variants that are unambiguously pre-send (transport/serialization/local rejection), or add a comment on `TransactionBroadcaster::broadcast` documenting that `Err` MUST mean "the network did not accept this transaction" so other broadcaster impls are held to the same contract.
- [NITPICK] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:287-348: Regression test only covers the BIP44 arm of `funds_account_mut`
The test exercises `StandardAccountType::BIP44Account` only. The helper and the release path also have a `BIP32Account` arm that the fix depends on to resolve the same account the builder reserved on. The two arms are structurally identical today, but a table-driven parameterization over `account_type` (or a second minimal case) would prevent silent divergence if a new `StandardAccountType` variant is added without updating the helper.
- [NITPICK] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:177-186: Warn logs on release-path fallback lack `wallet_id` for correlation
If the wallet or funds account has been removed between build and the failed broadcast, the release path logs `tracing::warn!` and returns the original broadcast error. The reservation is dropped along with the account, so state is fine, but neither warn arm includes `wallet_id`, which makes it hard to correlate an operator-visible warning with a specific send. Add `wallet_id=%self.wallet_id` to both arms, and for the outer arm also `?account_type, account_index`, so orphaned-reservation warnings are greppable.
| #[tokio::test] | ||
| async fn send_to_addresses_releases_reservation_on_broadcast_failure() { | ||
| // Testnet wallet funded with a single spendable UTXO — the whole | ||
| // balance rides on that one input, so a leaked reservation strands it. | ||
| let (ctx, _funding_tx) = TestWalletContext::new_random() | ||
| .with_mempool_funding(10_000_000) | ||
| .await; | ||
| let signer = WalletSigner { | ||
| wallet: ctx.wallet.clone(), | ||
| }; | ||
|
|
||
| let balance = Arc::new(WalletBalance::new()); | ||
| let info = PlatformWalletInfo { | ||
| core_wallet: ctx.managed_wallet, | ||
| balance: Arc::clone(&balance), | ||
| identity_manager: IdentityManager::new(), | ||
| tracked_asset_locks: BTreeMap::new(), | ||
| }; | ||
|
|
||
| let mut wm = WalletManager::<PlatformWalletInfo>::new(Network::Testnet); | ||
| let wallet_id = wm.insert_wallet(ctx.wallet, info).expect("insert wallet"); | ||
| let wallet_manager = Arc::new(RwLock::new(wm)); | ||
|
|
||
| let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); | ||
| let broadcaster = Arc::new(FailFirstBroadcaster::new()); | ||
| let core = CoreWallet::new( | ||
| sdk, | ||
| Arc::clone(&wallet_manager), | ||
| wallet_id, | ||
| broadcaster, | ||
| balance, | ||
| ); | ||
|
|
||
| let recipient = DashAddress::dummy(Network::Testnet, 42); | ||
| let outputs = vec![(recipient, 1_000_000u64)]; | ||
|
|
||
| // First attempt: build + sign succeed, broadcast fails. | ||
| let first = core | ||
| .send_to_addresses( | ||
| StandardAccountType::BIP44Account, | ||
| 0, | ||
| outputs.clone(), | ||
| &signer, | ||
| ) | ||
| .await; | ||
| assert!( | ||
| matches!(first, Err(PlatformWalletError::TransactionBroadcast(_))), | ||
| "first send should surface the broadcast failure, got {first:?}" | ||
| ); | ||
|
|
||
| // Immediate retry: only succeeds if the failed broadcast released the | ||
| // reservation. With the leak, coin selection sees no spendable UTXO and | ||
| // this fails with a build error instead. | ||
| let second = core | ||
| .send_to_addresses(StandardAccountType::BIP44Account, 0, outputs, &signer) | ||
| .await; | ||
| assert!( | ||
| second.is_ok(), | ||
| "retry after a failed broadcast should succeed once the reservation \ | ||
| is released, got {second:?}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Regression test only covers the BIP44 arm of funds_account_mut
The test exercises StandardAccountType::BIP44Account only. The helper and the release path also have a BIP32Account arm that the fix depends on to resolve the same account the builder reserved on. The two arms are structurally identical today, but a table-driven parameterization over account_type (or a second minimal case) would prevent silent divergence if a new StandardAccountType variant is added without updating the helper.
source: ['claude']
There was a problem hiding this comment.
Confirmed — the only regression test (send_to_addresses_releases_reservation_on_broadcast_failure) covers BIP44Account only; the BIP32Account arm in funds_account_mut is untested. Valid gap, not fixed in this PR yet — tracked as a follow-up.
🤖 Co-authored by Claudius the Magnificent AI Agent
There was a problem hiding this comment.
Resolved in eb54b37 — Regression test only covers the BIP44 arm of funds_account_mut no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| None => tracing::warn!( | ||
| ?account_type, | ||
| account_index, | ||
| "could not release UTXO reservation after failed broadcast: \ | ||
| funds account not found" | ||
| ), | ||
| }, | ||
| None => tracing::warn!( | ||
| "could not release UTXO reservation after failed broadcast: wallet not found" | ||
| ), |
There was a problem hiding this comment.
💬 Nitpick: Warn logs on release-path fallback lack wallet_id for correlation
If the wallet or funds account has been removed between build and the failed broadcast, the release path logs tracing::warn! and returns the original broadcast error. The reservation is dropped along with the account, so state is fine, but neither warn arm includes wallet_id, which makes it hard to correlate an operator-visible warning with a specific send. Add wallet_id=%self.wallet_id to both arms, and for the outer arm also ?account_type, account_index, so orphaned-reservation warnings are greppable.
source: ['claude']
There was a problem hiding this comment.
Confirmed — both tracing::warn! calls on the release-failure fallback paths omit wallet_id (and the outer one omits account_type/account_index too). Valid correlation gap, not fixed in this PR yet — tracked as a follow-up.
🤖 Co-authored by Claudius the Magnificent AI Agent
There was a problem hiding this comment.
Resolved in eb54b37 — Warn logs on release-path fallback lack wallet_id for correlation no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
Address reviewer findings on the send-to-addresses failure cleanup: - Take a read lock (not write) on the release path: release_reservation is &self and the manager map is untouched, so the cleanup no longer serializes concurrent sends and balance reads. Add an immutable funds_account helper and drop the now-unused funds_account_mut. - Only release the reservation for unambiguously pre-send broadcast failures (TransactionBroadcast / SpvError); ambiguous "possibly accepted" results are kept for the TTL backstop to avoid a double-spend on retry. Document the "Err means not accepted" contract on TransactionBroadcaster::broadcast. - Cover both BIP44 and BIP32 funds-account arms with a table-driven test plus a new test proving an ambiguous failure keeps the reservation, sharing a common funded-wallet helper. - Add wallet_id to both release-failure warn logs for correlation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Incremental review of eb54b37 vs prior f9c49ed. All four prior findings are FIXED at HEAD: release path now takes a read lock via immutable funds_account helper; release is gated by broadcast_failure_is_pre_send with the documented TransactionBroadcaster::broadcast Err-contract; regression tests iterate BIP44/BIP32 via funded_core_wallet and add an ambiguous-failure keep-reservation counterpart; both warn arms include wallet_id. One new in-scope suggestion: the documented Err = 'not accepted by the network' contract that broadcast_failure_is_pre_send relies on is not actually upheld by DapiBroadcaster (the default non-SPV path), because it flattens all gRPC errors — including DeadlineExceeded/Unavailable after Core may have accepted the tx — into TransactionBroadcast, which the predicate treats as safe to release. Verified against dash-spv afcff15: SpvError genuinely does mean pre-send (peer success counter). No blocking issues.
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/broadcaster.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/broadcaster.rs:41-67: `DapiBroadcaster` does not uphold the new `Err` = not-accepted contract, narrowing but not closing the double-spend-on-retry window
The new `broadcast_failure_is_pre_send` (broadcast.rs:44-49) classifies `PlatformWalletError::TransactionBroadcast(_)` as unambiguously pre-send and safe to release-and-retry, backed only by the doc comment on `TransactionBroadcaster::broadcast` (broadcaster.rs:23-24). `SpvBroadcaster` genuinely honors that contract — dash-spv (pinned rev `afcff15`) counts per-peer send results and only surfaces `Err` when zero peers received the tx. `DapiBroadcaster::broadcast` (broadcaster.rs:41-67) does not: it wraps *any* `DapiClientError` returned by `self.sdk.execute(...).into_inner()` into `TransactionBroadcast(String)`, with no discrimination by cause. The underlying `DapiClientError` includes `Transport(TransportError::Grpc(tonic::Status))` covering codes like `DeadlineExceeded` and `Unavailable` — the classic 'client gave up waiting for an ack, but the server-side call may have already put the tx in Core's mempool' case. A user hitting a gRPC deadline/connection drop after DAPI already relayed the tx will see `TransactionBroadcast`, get classified as pre-send, have the reservation released, and immediately retry — signing a second tx over the same inputs while the first is propagating. That is exactly the release-and-retry double-spend the PR's granular condition is meant to prevent, narrowed but not closed on the default broadcast path. Options: (a) in `DapiBroadcaster::broadcast`, inspect the `DapiClientError` and only return `TransactionBroadcast` for definite pre-send rejections (e.g. `Server(_)` decode/reject responses), routing ambiguous transport failures into a distinct 'possibly accepted' variant that `broadcast_failure_is_pre_send` treats as keep-reservation; or (b) drop `TransactionBroadcast` from the pre-send allowlist and rely on the TTL backstop for DAPI failures until (a) lands. Contract-by-doc-comment alone doesn't make it safe.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed_or_unparseable), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5[high] (rust-quality, failed_or_unparseable), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5[high] (ffi-engineer, failed_or_unparseable); verifier: opus; specialists: rust-quality, ffi-engineer
3 participants
Issue
CoreWallet::send_to_addresses(packages/rs-platform-wallet/src/wallet/core/broadcast.rs) builds and signs a transaction viaTransactionBuilder::build_signed, which reserves the selected UTXOs in the funds account'sReservationSet, then broadcasts. On broadcast failure the?propagated the error without releasing the reservation — the inputs stayed locked until the 24-block (~1h) TTL backstop reclaimed them.Not a funds-loss bug (self-heals via TTL), but a real UX rough edge: a user whose send fails on broadcast (network hiccup, node rejection, etc.) and immediately retries can see those specific UTXOs as unavailable and hit a spurious "insufficient funds", reachable in practice via dash-evo-tool's
WalletBackend::send_payment("Send Dash" screen).Depends on the reservation system introduced by the rust-dashcore bump to
afcff156in #3976 — this PR is based on that branch and should merge after (or alongside) it.Fix
On broadcast failure,
send_to_addressesreleases the reservation before propagating the original broadcast error, so the inputs are selectable again immediately instead of waiting on the TTL backstop:wallet_manager.read().await+ an immutablefunds_accountlookup (ManagedCoreFundsAccount::release_reservationtakes&self) — it no longer takes the manager's write lock, so it doesn't serialize against other in-flight sends/balance reads.TransactionBroadcast,SpvError) trigger a release; other variants (e.g.Shielded*Unconfirmed, where the tx may have partially propagated) keep the reservation and fall back to the TTL backstop, avoiding a release-and-retry double-spend attempt.TransactionBroadcaster::broadcast's doc comment now states thisErrcontract explicitly.tracing::warn!-logged (withwallet_idfor correlation) and does not alter the returned error. The wallet-manager lock is never held across the broadcastawait.Test plan
send_to_addresses_releases_reservation_on_broadcast_failurecovers bothBIP44AccountandBIP32Account— funds a wallet, fails the first broadcast via a fail-once mockTransactionBroadcaster, asserts an immediate retry succeeds for each account type.send_to_addresses_keeps_reservation_on_ambiguous_broadcast_failure(both account types) proves the granular-release condition: an ambiguous broadcaster error keeps the reservation, and an immediate retry fails at UTXO selection rather than silently double-spending.cargo fmt -p platform-walletcargo clippy -p platform-wallet --all-targets -- -D warnings— cleancargo test -p platform-wallet— 0 failures🤖 Generated with Claude Code
Co-authored by Claudius the Magnificent AI Agent