fix(platform-wallet): release UTXO reservation when broadcast fails#3985
fix(platform-wallet): release UTXO reservation when broadcast fails#3985Claudius-Maginificent wants to merge 24 commits into
Conversation
Bump all 8 rust-dashcore workspace dependencies (dashcore, dash-spv, key-wallet, key-wallet-ffi, key-wallet-manager, dash-network, dash-network-seeds, dashcore-rpc) from 5c0113e7 to 78d10022 (rust-dashcore `dev` head, 15 commits, 0.43.0 -> 0.45.0). key-wallet's `Signer` trait gained a required `extended_public_key` method. Implement it for MnemonicResolverCoreSigner (real BIP-32 xpub derivation with private-scalar zeroization on every return path) and add stubs to the two rs-dpp test signers. Resolver + derive boilerplate extracted into a shared `resolve_and_derive` helper (DRY); the master scalar is wiped on both the success and derive-error paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MoY6vhmqZuHzNsMfJ8wakQ <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…st stub The rust-dashcore bump added `Signer::extended_public_key` as a required trait method. The shielded `shield_from_asset_lock_transition` signing test's `FixedKeySigner` stub was missed: it is feature-gated behind `all(test, state-transition-signing, core_key_wallet, shielded-client)` and is not compiled under default features, so the local workspace check passed while CI (which enables those features) failed with E0046. Add the stub matching the two sibling test signers (returns Err — the fixed-key stub carries no chain code). Verified with `cargo check -p dpp --tests --features state-transition-signing,core_key_wallet,shielded-client`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MoY6vhmqZuHzNsMfJ8wakQ <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Move the 8 rust-dashcore workspace deps (dashcore, dash-spv, key-wallet, key-wallet-ffi, key-wallet-manager, dash-network, dash-network-seeds, dashcore-rpc) from rev 78d10022 to f42498e0 — the head of rust-dashcore PR #833, which adds `impl zeroize::Zeroize for ExtendedPrivKey` in key-wallet/src/bip32.rs. Package versions stay at 0.45.0; Cargo.lock change is rev-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…erase Now that key-wallet's ExtendedPrivKey implements Zeroize (rust-dashcore PR #833), hold the master and derived extended keys in Zeroizing<_> and let RAII wipe them on every exit path — success, ?-early-return, and panic-unwind. This removes the 4 manual `.private_key.non_secure_erase()` calls in resolve_and_derive / derive_priv / extended_public_key that only covered the paths they were hand-placed on. resolve_and_derive now returns Zeroizing<ExtendedPrivKey>; its two callers are updated. The two `secret.non_secure_erase()` calls in sign_ecdsa / public_key stay: secp256k1::SecretKey has no Zeroize impl, so those raw SecretKey copies still need an explicit wipe. Module and per-fn zeroization docs rewritten to describe the current Zeroizing approach. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ia extract closure resolve_and_derive now takes `extract: impl FnOnce(&ExtendedPrivKey) -> T` and returns T, instead of handing back Zeroizing<ExtendedPrivKey>. The derived key is wrapped in Zeroizing the instant derive_priv returns and is only ever borrowed by the closure, so no raw or wrapped ExtendedPrivKey crosses the function boundary — closing the Copy<ExtendedPrivKey> stack-residue gap flagged in review. The one remaining by-value copy is derive_priv's own return slot, which is upstream's (key-wallet) API to own. derive_priv and extended_public_key become thin closure callers. Also add a #[tokio::test] driving extended_public_key against an independently derived ExtendedPubKey from the same BIP-39 vector / network / path, asserting full-struct equality plus explicit chain_code / depth / network spot-checks so metadata loss is caught, not just the public point (fixes codecov/patch gap). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ing; doc irreducible xpriv transient Reorder the extended_public_key test so each field-level assert (public_key, chain_code, depth, network) runs before the full-struct assert_eq!, which otherwise short-circuits and leaves the per-field metadata checks unreachable on a regression (i.e. vacuous). Proven non-vacuous: temporarily corrupting only `depth` in the impl fails the test at "depth must match", where a pubkey-only check would have passed. Also document the one irreducible transient in resolve_and_derive: key-wallet's ExtendedPrivKey::derive_priv returns by value (Copy), so its return slot holds an un-wiped copy until the same-line Zeroizing::new takes ownership — noted so the zeroization contract doesn't over-promise "zero copies ever". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…edPrivKey on Drop) Advances the 8 rust-dashcore workspace deps from rev f42498e0d04257e28b4e457c16629904a872ab61 to a8c57fe863c96ac9c7e33833549e7a4f75ac9b5e (PR #833 head). The new commit drops `Copy` from `ExtendedPrivKey` and gives it a `Drop` impl that zeroizes its secret material on scope exit — so the key wipes itself automatically instead of relying on caller-side wrappers, and non-`Copy` moves leave no stray bitwise duplicate behind. No workspace call sites relied on `ExtendedPrivKey: Copy`: the Copy-removal impact is absorbed upstream in `bip32::derive_priv` (clones `self` instead of `*self`), so `cargo check --workspace --all-targets` is clean with no source changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…roizing wrap `ExtendedPrivKey` now zeroizes on `Drop` (rust-dashcore rev a8c57fe863c96ac9c7e33833549e7a4f75ac9b5e), so wrapping the master and derived keys in `Zeroizing` inside `resolve_and_derive` only bought a harmless double-wipe. Unwrap both and let the type's own `Drop` do the work; `Zeroizing` stays on the plain byte buffers that have no `Drop` of their own (mnemonic buffer, BIP-39 seed, final 32-byte scalar). Rewrites the module `# Zeroization` block and the `resolve_and_derive` contract to the present three-mechanism reality (self-wiping key + Zeroizing buffers + non_secure_erase on SecretKey). The previously documented "irreducible transient" caveat is fully closed: a non-`Copy` move leaves no bitwise duplicate, so there is no un-wiped return slot. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ashcore-dev # Conflicts: # Cargo.lock # Cargo.toml
The prior merge commit updated Cargo.toml's rust-dashcore rev pins to the current dev HEAD but left Cargo.lock staged at the pre-cargo-update state (still pinned to v0.44.0/991c6ebe), an oversight caught during PR comment verification. Stage the already-regenerated lockfile so both files agree on the a8a096838b829cf5bec3c2374a23511640a0c35c pin. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The committed lockfile still pinned all 12 rust-dashcore workspace crates at the stale rev 991c6eb (v0.44.0), even though root Cargo.toml was already re-pinned to a8a096838b829cf5bec3c2374a23511640a0c35c (v0.45.0). Cargo silently re-resolves the lock on the next build, so --locked builds (CI) would fail against the mismatch. Regenerate the lock to match: dashcore, dashcore-private, dashcore_hashes, dashcore-rpc, dashcore-rpc-json, dash-network, dash-network-seeds, dash-spv, git-state, key-wallet, key-wallet-ffi, key-wallet-manager all move 991c6eb -> a8a0968. No other crate churn. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…a0968 Two accuracy fixes forced by the rust-dashcore pin move to a8a0968 (squashed PR #833 form), verified against key-wallet/src/bip32.rs at that rev — ExtendedPrivKey now derives Clone (not Copy) and has an explicit `impl Drop` that calls `self.zeroize()`: - signer_simple.rs: the `dash_sdk_sign_with_mnemonic_and_path` comment still claimed "the ExtendedPrivKey itself doesn't zeroize — derived falls out of scope intact". That was true when written (PR #3541, pre-Zeroize), but is now false: `derived` self-wipes on Drop. Reword to state present reality and clarify the Zeroizing wrapper is there because `secret_bytes()` copies the scalar into a fresh array with no Drop of its own. - mnemonic_resolver_core_signer.rs: re-point two rev citations from a8c57fe (rebased away upstream, no longer reachable from dev) to the current reachable pin a8a0968. The behavioral claims are unchanged and still hold. Comment-only; no functional change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…mp-rust-dashcore-dev
Move all eight rust-dashcore workspace deps (dashcore, dash-spv, key-wallet, key-wallet-ffi, key-wallet-manager, dash-network, dash-network-seeds, dashcore-rpc) from a8a09683 to dev HEAD afcff156, and regenerate Cargo.lock so every rust-dashcore-sourced crate (including the transitive dashcore_hashes, dashcore-private, dashcore-rpc-json, git-state) resolves to the new rev. Upstream refactors the signing API: `extended_public_key` moves out of the `Signer` trait into a dedicated `ExtendedPubKeySigner: Signer` trait, so implementing an extended-pubkey export is now opt-in rather than a required `Signer` method. `ExtendedPrivKey` keeps its zeroize-on-`Drop`, non-`Copy` shape. The three rs-dpp test-signer stubs added earlier in this branch (`FixedKeySigner` in `state_transition/mod.rs` and its two `signing_tests.rs` call sites) only existed to satisfy the previously required `Signer::extended_public_key` method. With that method moved to the opt-in trait, the stubs no longer compile and nothing needs them as `ExtendedPubKeySigner` -- revert those three files to their v4.1-dev content. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Implement `ExtendedPubKeySigner::extended_public_key` on `MnemonicResolverCoreSigner`, tracking the upstream split of that method out of the base `Signer` trait. Callers that need offline non-hardened descendant derivation (e.g. DashPay contact-payment addresses) can now obtain the full BIP-32 extended public key -- public point plus chain code -- from the mnemonic-resolver signer in one call. Funnel all private-key material through a single `resolve_and_derive` helper that owns the mnemonic -> seed -> master -> child `ExtendedPrivKey` stack and hands the derived key to a caller-supplied closure by reference. The extended key never crosses the call boundary and self-wipes on `Drop` (`ExtendedPrivKey` zeroizes and is not `Copy`); the mnemonic and seed byte buffers ride `Zeroizing` wrappers. Both `derive_priv` (raw scalar for signing) and `extended_public_key` (xpub) share this one entry point. Add a unit test asserting the exported xpub matches an independent BIP-39 derivation field-by-field (public key, chain code, depth, network) before the full-struct equality, so a dropped BIP-32 metadatum fails with a precise message rather than a vacuous assert. Also correct the `signer_simple.rs` zeroization comment, now that `ExtendedPrivKey` self-wipes on `Drop` -- the `Zeroizing` wrap there guards only the separate `secret_bytes()` array copy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
`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 79caf9b) |
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR classifies broadcast outcomes as rejected or maybe-sent, releases wallet reservations only on rejection, routes wallet broadcast paths through a shared helper, and updates Rust, FFI, Swift, and test coverage for the new behavior. ChangesBroadcast failure handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WalletFlow
participant BroadcastHelper
participant Broadcaster
participant WalletManager
participant ReservationSet
WalletFlow->>BroadcastHelper: broadcast_releasing_on_rejection(...)
BroadcastHelper->>Broadcaster: broadcast(tx)
alt rejected
BroadcastHelper->>WalletManager: read lock, locate account
WalletManager-->>BroadcastHelper: managed account
BroadcastHelper->>ReservationSet: release_reservation(tx)
else maybe sent or success
BroadcastHelper-->>WalletFlow: txid or error
end
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/wallet/core/broadcast.rs (1)
409-444: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
SpvErrorpre-send arm in the retry test.
broadcast_failure_is_pre_sendreleases for bothTransactionBroadcastandSpvError, but this test only exercisesTransactionBroadcast. Parameterize the first failure so theSpvErrorrelease path cannot regress unnoticed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/core/broadcast.rs` around lines 409 - 444, The retry test only covers the TransactionBroadcast pre-send failure path, so the SpvError release branch in the broadcast flow is still untested. Update send_to_addresses_releases_reservation_on_broadcast_failure to parameterize the first failure source using FailFirstBroadcaster or a similar test helper so it can trigger both TransactionBroadcast and SpvError cases, and keep the retry assertion in the send_to_addresses path to verify the reservation is released in both scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/broadcaster.rs`:
- Around line 23-24: Update the trait-level docs in the broadcaster contract so
they no longer imply every Err is safe to retry; instead, explicitly distinguish
between pre-send failures where the transaction was not accepted and callers may
release reservations, and ambiguous or post-submit failures where reservations
must be kept. Use the send_to_addresses semantics as the reference point, and
make sure broadcaster implementers understand not to collapse uncertain errors
into the safe-retry class.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- Around line 409-444: The retry test only covers the TransactionBroadcast
pre-send failure path, so the SpvError release branch in the broadcast flow is
still untested. Update
send_to_addresses_releases_reservation_on_broadcast_failure to parameterize the
first failure source using FailFirstBroadcaster or a similar test helper so it
can trigger both TransactionBroadcast and SpvError cases, and keep the retry
assertion in the send_to_addresses path to verify the reservation is released in
both scenarios.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2b03dbc-be99-480b-950e-ce454975459b
📒 Files selected for processing (3)
packages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/broadcaster.rspackages/rs-platform-wallet/src/wallet/core/broadcast.rs
There was a problem hiding this comment.
Code Review
HEAD c81f9d2 is a no-op merge of v4.1-dev into the fix branch — git diff eb54b37001..c81f9d2c81 -- packages/rs-platform-wallet/ is empty. No new delta findings. The one prior in-scope finding (DapiBroadcaster does not uphold the Err=not-accepted contract that broadcast_failure_is_pre_send depends on) remains STILL_VALID at HEAD and is carried forward as a suggestion. All 6 agents converged on this single finding.
🟡 1 suggestion(s)
🤖 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` (packages/rs-platform-wallet/src/wallet/core/broadcast.rs:44-49) classifies `PlatformWalletError::TransactionBroadcast(_)` as unambiguously pre-send and therefore safe to release the UTXO reservation and let the caller retry. That predicate is backed only by the doc comment added on `TransactionBroadcaster::broadcast` (broadcaster.rs:23-24): "Err must mean the transaction was not accepted by the network".
`SpvBroadcaster` genuinely upholds that contract — dash-spv (pinned rev afcff156) counts per-peer send results and only surfaces `Err` when zero peers received the tx. `DapiBroadcaster::broadcast` does not: at broadcaster.rs:56-63 it wraps *any* error returned by `self.sdk.execute(request, RequestSettings::default()).await.into_inner()` into `PlatformWalletError::TransactionBroadcast(String)` with no discrimination by cause. The underlying `DapiClientError` includes `Transport(TransportError::Grpc(tonic::Status))`, which covers codes such as `DeadlineExceeded`, `Unavailable`, `Cancelled`, and mid-response resets — 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 or connection drop after DAPI already relayed the tx will see `TransactionBroadcast`, be classified pre-send by the predicate, have the reservation released, and immediately retry — signing a second tx over the same inputs while the first may already be propagating. That is exactly the release-and-retry double-spend the granular condition is meant to prevent, narrowed but not closed on the default (non-SPV) broadcast path. On the security side, the input-conflict property means at most one tx confirms (no funds theft), but a hostile or degraded DAPI relay can deliberately drop the response after forwarding to Core to coerce the wallet into duplicate broadcasts (mempool spam using the user's key, retry-policy fingerprinting).
The contract is also enforced only by convention (a doc comment), not the type system, so any future `TransactionBroadcaster` impl that raises `TransactionBroadcast` for an ambiguous submission will silently violate it.
Options: (a) in `DapiBroadcaster::broadcast`, match on `DapiClientError` and only return `TransactionBroadcast` for definite pre-send rejections (e.g. `Server(_)` decode/reject responses, connection-refused before the request was written), routing ambiguous transport failures (`Transport(Grpc(_))` with `DeadlineExceeded`/`Unavailable`/`Cancelled`/`Unknown`, or any post-write-side error) into a distinct "possibly accepted" `PlatformWalletError` 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 — SPV keeps its fast-path.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed_or_unparseable), opus (security-auditor), claude-sonnet-5 (security-auditor), gpt-5.5[high] (security-auditor, failed_or_unparseable), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5[high] (rust-quality, failed_or_unparseable); verifier: opus; specialists: security-auditor, rust-quality
|
Handled the CodeRabbit follow-up in helper PR #3996: #3996 Current status: #3996 is docs-only per maintainer request. It clarifies the broadcaster error contract in Latest checks are green: PR title, action-semantic-pull-request, and CodeRabbit manual review. Local validation before the docs-only push included |
…-release seam
Review follow-ups on the release-on-broadcast-failure fix:
- TransactionBroadcaster::broadcast now returns a typed BroadcastError
(Rejected vs MaybeSent) so the release decision is made where the
transport knowledge lives, instead of variant-matching catch-all
PlatformWalletError strings. DapiBroadcaster classifies every failure
MaybeSent (its internal multi-node retries make any failure ambiguous);
SpvRuntime classifies "client not started" and dash-spv's zero-peer
NotConnected as Rejected (both fire before any bytes leave the process)
and everything else MaybeSent. MaybeSent surfaces as the new
PlatformWalletError::TransactionBroadcastUnconfirmed.
- The release logic moves from a send_to_addresses-only block into
wallet::reservations::broadcast_releasing_on_rejection, and the two
other build-then-broadcast paths that leaked reservations the same way
now use it too: send_dashpay_payment and create_funded_asset_lock_proof.
The asset-lock recovery rebroadcast intentionally does not release (it
re-drives a previously built transaction, not a fresh reserve).
- funds_account dispatch replaced with key-wallet's existing
bip{44,32}_managed_account_at_index accessors; the nested release
match flattened to one lookup chain with a single warn.
- Test fixture reuses ctx.receive_address / ctx.check_transaction from
TestWalletContext instead of re-deriving the BIP44 address and
hand-rolling the mempool check; mocks updated to the typed error.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Prior finding prior-1 is resolved at HEAD 3145402: the doc-comment-only pre-send contract in DapiBroadcaster is replaced by a typed BroadcastError { Rejected, MaybeSent } enum, DapiBroadcaster conservatively classifies every DAPI failure as MaybeSent, and the release logic flows through a shared broadcast_releasing_on_rejection seam consumed by all three build-then-broadcast callers. Two new latest-delta findings surface: (1) the new PlatformWalletError::TransactionBroadcastUnconfirmed variant flattens to ErrorUnknown at the FFI boundary because the analogous arms added for ShieldedBroadcastUnconfirmed/ShieldedSpendUnconfirmed were not added for the new variant; (2) on BroadcastError::Rejected the asset-lock path releases the funding UTXO reservation while leaving a persisted Built tracked lock that resume_asset_lock will re-broadcast if later invoked with that outpoint. Both are suggestions, not blockers — the DAPI path structurally never returns Rejected today, so the second is currently only reachable via the SPV path.
🟡 2 suggestion(s)
The FFI-boundary suggestion is included body-only in the details below because packages/rs-platform-wallet-ffi/src/error.rs is outside the PR diff.
Source: reviewers opus/opus, sonnet5/claude-sonnet-5, codex/gpt-5.5 for general/security-auditor/rust-quality/ffi-engineer; verifier opus/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-ffi/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/error.rs:231-240: New `TransactionBroadcastUnconfirmed` variant flattens to `ErrorUnknown` at the FFI boundary, erasing the PR's retry-safety signal
This PR's entire point is a typed distinction between broadcast outcomes that are safe to retry immediately (`BroadcastError::Rejected` → `PlatformWalletError::TransactionBroadcast`) and outcomes that must NOT be retried because the transaction may already be propagating (`BroadcastError::MaybeSent` → the new `PlatformWalletError::TransactionBroadcastUnconfirmed`, per broadcaster.rs:45-56 and error.rs:60-77). The distinction is enforced by the type system in Rust, but is lost crossing the C ABI.
`From<PlatformWalletError> for PlatformWalletFFIResult` at rs-platform-wallet-ffi/src/error.rs:205-244 has dedicated arms for the structurally identical shielded siblings — `ShieldedBroadcastFailed`, `ShieldedBroadcastUnconfirmed`, `ShieldedSpendUnconfirmed` — precisely because the in-code comment (lines 221-230) documents that flattening these to `ErrorUnknown` would 'defeat the slot-holding contract.' The new `TransactionBroadcastUnconfirmed` variant carries the same slot-holding contract (its own doc at error.rs:63-77 names `ShieldedSpendUnconfirmed` as its direct sibling) but has no arm here, so every ambiguous broadcast on the Core-wallet, DashPay, and asset-lock send paths crosses the boundary as generic code 99. Since every DAPI broadcast failure now maps to `MaybeSent` unconditionally, this is the primary error class the DAPI-backed send paths (`core_wallet_send_to_addresses`, `platform_wallet_send_dashpay_payment`, `asset_lock_manager_create_funded_proof`) will emit on failure.
Swift's `PlatformWalletError.init(result:)` is a pure code switch with no message-string fallback, and `SendViewModel` already special-cases `shieldedSpendUnconfirmed` as non-retryable — that special-casing cannot be replicated for the Core-wallet path until the FFI code exists. The safety property is still preserved by the TTL-backstopped reservation, but the UX degrades from a specific 'tx may already be on network — do not retry' to a generic error, followed by a misleading `NoSpendableInputs` on retry.
Add a dedicated `ErrorTransactionBroadcastUnconfirmed` variant to `PlatformWalletFFIResultCode`, add the matching arm here mirroring the shielded siblings, and add the corresponding case to Swift's `PlatformWalletError`. The PR diff is currently confined to `packages/rs-platform-wallet/`, but this variant is only useful once foreign callers can act on it.
In `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:329-361: Rejected asset-lock broadcast releases the funding reservation while leaving a resumable `Built` lock behind
`create_funded_asset_lock_proof` tracks and persists the transaction as `AssetLockStatus::Built` (lines 329-341) before calling `broadcast_releasing_on_rejection`. On `BroadcastError::Rejected`, that helper releases the BIP44 funding reservation and returns via `?` without advancing or removing the tracked lock, leaving the persisted `Built` row live in `tracked_asset_locks`. The inline comment at lines 348-352 documents this as intentional ('the tracked lock stays `Built`, so a later resume can still re-drive the same transaction while its inputs remain unspent') — but with the reservation now released, the 'inputs remain unspent' precondition can no longer be relied on. If the caller retries `create_funded_asset_lock_proof` successfully with a fresh transaction spending the same UTXOs, `resume_asset_lock(&old_out_point)` (recovery.rs:251-261, driven by `FundingResolution` orchestration or a `list_tracked_locks`-based recovery UI) will re-broadcast the stale `Built` transaction whose inputs have already been consumed.
The worst-case fallout is not a double-spend — the second broadcast will fail mempool acceptance because the inputs are already spent — but it does re-broadcast a superseded transaction, produce confusing failures, and leave clutter in the tracked-lock persistence. This is currently only reachable via the SPV path (`DapiBroadcaster` never returns `Rejected`; only `SpvRuntime::broadcast_transaction`'s zero-peer / client-not-started arms do).
On `BroadcastError::Rejected`, either remove/tombstone the tracked lock (so it isn't offered up by `list_tracked_locks` for resume), or keep the reservation held for the `Built` row and only release it once the lock is definitively abandoned. Either shape restores the invariant the doc comment relies on.
…nfirmed-broadcast code across FFI Review follow-ups: - On BroadcastError::Rejected, create_funded_asset_lock_proof now untracks the Built lock (in-memory + persisted row via the changeset's removed set). With the funding reservation released for re-selection, leaving the row resumable would let a later resume_asset_lock re-broadcast a transaction whose inputs may already be re-spent. MaybeSent failures keep both the reservation and the resumable row. broadcast_releasing_on_rejection returns the typed BroadcastError so call sites with rejection-specific cleanup can branch on it; `?` still converts via From. - PlatformWalletError::TransactionBroadcastUnconfirmed now crosses the FFI boundary as a dedicated ErrorTransactionBroadcastUnconfirmed code (19) instead of flattening to ErrorUnknown, mirroring the shielded unconfirmed siblings, with the matching Swift PlatformWalletResultCode / PlatformWalletError cases so hosts can special-case the do-not-retry outcome. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Both prior findings (prior-1 FFI flattening of TransactionBroadcastUnconfirmed and prior-2 rejected asset-lock leaving a resumable Built row) are fully resolved at head 12ab17f. Carried-forward findings: none. New in-scope findings from the latest delta: two suggestion-level items — (a) the new asset-lock untrack-on-Rejected branch in create_funded_asset_lock_proof has no direct regression test, unlike the core-send sibling that got table-driven tests; (b) a narrow window between broadcast_releasing_on_rejection releasing the UTXO reservation and untrack_asset_lock removing the Built row lets a concurrent resume_asset_lock re-broadcast the just-rejected transaction using inputs now eligible for re-selection. Neither rises to blocking.
🟡 2 suggestion(s)
🤖 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/asset_lock/build.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:356-371: New untrack-on-Rejected branch in create_funded_asset_lock_proof has no direct regression test
The PR added table-driven tests (`send_to_addresses_releases_reservation_on_broadcast_failure` / `..._keeps_reservation_on_ambiguous_broadcast_failure`) for the parallel core-send path, but the newly added asset-lock branch here — matching `BroadcastError::Rejected` and calling `untrack_asset_lock`, while `MaybeSent` deliberately leaves the reservation and Built row intact — has no equivalent coverage. `build.rs` has no `#[cfg(test)]` module, and no test elsewhere in the crate exercises `AssetLockManager` together with `BroadcastError::Rejected`. Since this is the exact invariant that closes prior-2 (a `resume_asset_lock` on a rejected outpoint must not re-broadcast a stale tx whose inputs may already be re-spent), a future refactor could silently regress the branch or the `matches!` guard without breaking any existing test. A direct test that asserts a `Rejected` outcome (i) queues a changeset with the outpoint in `removed`, (ii) empties the in-memory `tracked_asset_locks` entry, and that a `MaybeSent` outcome does neither, would pin the invariant.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:356-371: Reservation release and Built-row untrack are not atomic, leaving a window where a concurrent resume_asset_lock can re-broadcast the rejected tx
`broadcast_releasing_on_rejection` releases the funding UTXO reservation and returns `Err(BroadcastError::Rejected)`; only afterward does this call site acquire the write lock via `untrack_asset_lock` to remove the `Built` row. Between those two lock acquisitions, the `Built` entry for `out_point` is still visible via `list_tracked_locks()` / `tracked_asset_locks.values()`. `resume_asset_lock` (sync/recovery.rs:209-261) for `Built` status calls `self.broadcaster.broadcast(&tx).await?` directly — it does not go through `broadcast_releasing_on_rejection` and does not re-check reservation state. If a host recovery/resume flow interleaves precisely with the rejection cleanup, it can re-drive the just-rejected transaction using inputs whose reservation is already released and therefore concurrently eligible for re-selection by a new build. `untrack_asset_lock` also has no `AssetLockStatus::Built` guard, so if `resume_asset_lock` advanced the row to `Broadcast` in that window, the untrack still removes it — clobbering genuine progress. Blast radius is bounded: Core's mempool resolves double-spend at the network layer, so this is not a fund-loss bug, but it can produce a confusing state where the wallet believes a new build succeeded while the confirmed tx is actually the resumed one. Consider combining the reservation release and row untrack under a single write-lock section, or gating `untrack_asset_lock` on the status still being `Built`.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5 (general), opus (security-auditor), claude-sonnet-5 (security-auditor), gpt-5.5 (security-auditor), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5 (rust-quality), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5 (ffi-engineer); verifier: opus; specialists: security-auditor, rust-quality, ffi-engineer
… branches with tests Review follow-ups: - Untrack-before-release ordering: on a Rejected asset-lock broadcast the Built row is now removed BEFORE the funding reservation is released. While the reservation is held the inputs cannot be re-selected by a new build, and once the row is gone resume_asset_lock cannot re-drive the rejected transaction — so at no point is the row resumable while its inputs are re-spendable. untrack_asset_lock is additionally guarded on the row still being Built, so a concurrent resume that advanced it to Broadcast is kept rather than clobbered. The release block moved into release_reservation_after_rejected_broadcast, which broadcast_releasing_on_rejection now delegates to. - Regression tests for the asset-lock branch: a Rejected broadcast empties tracked_asset_locks, queues the outpoint in the changeset's removed set, and a fresh build reselects the released inputs; a MaybeSent failure keeps the Built row, queues no deletion, and a fresh build fails at input selection while the reservation is held. The mocks/signer/funded-wallet fixture moved to a shared cfg(test) test_support module used by both the core-send and asset-lock test suites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Both prior findings from 12ab17f are FIXED at head 25ea9d2: dedicated Rejected/MaybeSent regression tests were added to build.rs, and the cleanup order was inverted (untrack before release) with untrack_asset_lock newly guarded on status == Built. However, the same reorder introduced one new in-scope logic gap flagged by 5 of 12 reviewers: the reservation release in create_funded_asset_lock_proof runs unconditionally even when the untrack guard fires (i.e. a concurrent resume_asset_lock already advanced the row past Built), freeing UTXOs of a transaction the wallet is still tracking as Broadcast. That is a variant of the exact race this commit's title claims to close, and the fix is a one-line conditional on cs_untrack.removed.contains(&out_point).
🔴 1 blocking
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5 (general), opus (security-auditor), claude-sonnet-5 (security-auditor), gpt-5.5 (security-auditor), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5 (rust-quality), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5 (ffi-engineer); verifier: opus; specialists: security-auditor, rust-quality, ffi-engineer.
🤖 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/asset_lock/build.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:357-371: Release the funding reservation only when untrack_asset_lock actually removed the Built row
The reorder at build.rs:357-371 correctly untracks the `Built` asset-lock row before releasing the funding reservation, and `untrack_asset_lock` (sync/tracking.rs:46-64) is now status-guarded so a concurrent `resume_asset_lock` that advanced the row to `Broadcast` in the window between this call's rejected broadcast and its cleanup is preserved (logged, not clobbered) — returning an empty changeset in that case.
But `release_reservation_after_rejected_broadcast` on line 361 runs unconditionally inside the `Rejected` branch, regardless of whether `untrack_asset_lock` actually removed the entry. When the guard fires, the row is left tracked as `Broadcast` (positive evidence the transaction reached the network via the concurrent resume), yet the funding reservation is still released, making its inputs immediately selectable by a fresh build. This is strictly worse than the `MaybeSent` case this PR is designed to protect (there the outcome is merely ambiguous; here the status transition is direct evidence of a successful send), and it reopens a variant of the exact double-spend race the commit message ("close the rejected-asset-lock race") claims to eliminate.
The invariant the doc comment at build.rs:348-356 claims — "at no point is the row resumable while its inputs are re-spendable" — is violated in this interleaving: after cleanup the row is resumable/tracked *and* the inputs are re-spendable. `reservations.rs:70-75` explicitly states the ordering precondition, and that precondition is only satisfied when the untrack actually succeeded. Gate the release on `cs_untrack.removed.contains(&out_point)`.
…lt row actually being removed If the untrack guard fires — a concurrent resume_asset_lock advanced the row past Built in the rejection window, positive evidence the transaction reached the network — the funding reservation must stay held exactly like a MaybeSent outcome, or the still-tracked row would be resumable while its inputs are re-spendable. Release now runs only when untrack_asset_lock actually removed the row (cs_untrack.removed.contains(&out_point)). Pinned by rejected_broadcast_racing_concurrent_resume_keeps_row_and_reservation: a mock broadcaster advances the row to Broadcast mid-broadcast and then returns Rejected; the test asserts the row survives as Broadcast, no persisted-row deletion is queued, and a fresh build still fails at input selection because the reservation was kept. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review from 25ea9d2 to 5df222b. The sole prior blocking finding (prior-1) is verified FIXED: create_funded_asset_lock_proof now captures removed_built_row = cs_untrack.removed.contains(&out_point) before queuing the changeset and gates the reservation release on that flag, exactly matching the suggested fix. untrack_asset_lock only populates cs.removed when the row was actually removed (guarded on status == Built), so the flag is a correct proxy for the race outcome. A new regression test rejected_broadcast_racing_concurrent_resume_keeps_row_and_reservation pins the concurrent-resume interleaving. No new in-scope findings in the latest delta or cumulative PR.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5 (general), opus (security-auditor), claude-sonnet-5 (security-auditor), gpt-5.5 (security-auditor), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5 (rust-quality), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5 (ffi-engineer); verifier: opus; specialists: security-auditor, rust-quality, ffi-engineer.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #3985 +/- ##
=========================================
Coverage 87.18% 87.18%
=========================================
Files 2632 2632
Lines 327563 327563
=========================================
Hits 285592 285592
Misses 41971 41971
🚀 New features to boost your workflow:
|
…nchor error v4.0-dev already ships ErrorShieldedNoRecordedAnchor = 19; move ErrorTransactionBroadcastUnconfirmed to 20 (Rust enum + Swift mirror) so the FFI codes stay numerically identical across both release lines when the 4.0 -> 4.1 sync lands, following the ErrorArithmeticOverflow reserved-slot precedent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
fix/core-wallet-release-reservation-on-broadcast-failure
branch
from
aef46b9 to
04ab5cd
Compare
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: none — the prior review at 5df222b had zero unresolved findings and nothing in the current head aef46b9 regresses it. New findings in the latest delta: one suggestion, independently raised by two sonnet5 reviewers (security + rust-quality), that the safety-critical SPV Rejected/MaybeSent classification in SpvRuntime::broadcast_transaction has no direct unit test coverage against dash-spv's error semantics. Verified in-tree: the core reservation-release seam (BroadcastError split, broadcast_releasing_on_rejection, asset-lock untrack-before-release with the removed_built_row guard) is correct, read-lock discipline is sound, and the FFI code renumbering to 20 (to accommodate ErrorShieldedNoRecordedAnchor=19 from the v4.0-dev merge) is mirrored consistently in the Swift PlatformWalletResult. The DapiBroadcaster blanket-MaybeSent behavior flagged by opus-rust-quality is an author-acknowledged, code-commented tradeoff (safe, conservative, documented as a follow-up), not a defect — dropped.
🟡 1 suggestion(s)
🤖 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/spv/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:104-128: SpvRuntime::broadcast_transaction's Rejected/MaybeSent classification is untested against dash-spv error semantics
This method is the concrete classification point that decides whether an SPV broadcast failure is safe to treat as `Rejected` (release the UTXO reservation and let a later resume re-spend the inputs) versus `MaybeSent` (must keep the reservation to prevent double-spend). The safety of releasing on `SpvError::Network(NetworkError::NotConnected)` depends entirely on an external dash-spv invariant: that `NotConnected` is only raised when zero peers are connected *before* any send attempt, never after the client has already handed the tx to at least one peer. dash-spv is pinned by git rev and its error contract is not covered by any test in this crate — `mod tests` is absent from `spv/runtime.rs`, and every test in `wallet/core/broadcast.rs` and `wallet/asset_lock/build.rs` uses mock broadcasters that never construct a real `SpvRuntime`. A future dash-spv change that raises `NotConnected` post-partial-send would compile cleanly and silently reintroduce the double-spend window this PR is designed to prevent, with no test signal. At minimum, add a targeted unit test that stubs the internal SPV client and pins both match arms (unstarted client → `Rejected`, `NetworkError::NotConnected` → `Rejected`, any other `SpvError` → `MaybeSent`), so a dash-spv semantic change is caught at the crate boundary rather than the trust boundary.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5 (general), opus (security-auditor), claude-sonnet-5 (security-auditor), gpt-5.5 (security-auditor), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5 (rust-quality), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer, failed_or_unparseable), gpt-5.5 (ffi-engineer); verifier: opus; specialists: security-auditor, rust-quality, ffi-engineer
…ification Extract the dash-spv error mapping into classify_spv_broadcast_error and pin all three arms at the crate boundary: unstarted client -> Rejected, NetworkError::NotConnected -> Rejected (dash-spv raises it from the zero-peers check before any bytes leave the process), and every other SpvError -> MaybeSent. A dash-spv semantic change now trips a test here instead of silently reopening the release-and-retry double-spend window. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Prior finding on untested SPV Rejected/MaybeSent classification is FIXED at 79caf9b by the extracted classify_spv_broadcast_error and its three-arm test module (unstarted-client, NotConnected→Rejected, all other SpvError→MaybeSent). Reservation-release seam, DAPI conservative MaybeSent, asset-lock untrack-before-release ordering, and FFI code-20 mapping (with Swift mirror) all verify clean. No in-scope blocking or suggestion findings remain.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5 (general), opus (security-auditor), claude-sonnet-5 (security-auditor, failed_or_unparseable), gpt-5.5 (security-auditor), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5 (rust-quality), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5 (ffi-engineer); verifier: opus; specialists: security-auditor, rust-quality, ffi-engineer.
4 participants
Issue being fixed or feature implemented
TransactionBuilder::build_signedreserves the selected UTXOs in the funding account'sReservationSetand leaves the reservation held on success, expecting the broadcast to follow. When the broadcast failed, nothing released the reservation — the inputs stayed locked until the 24-block (~1h) TTL backstop reclaimed them. A user whose send failed and who immediately retried hit a spurious "insufficient funds".This affected every build-then-broadcast path in the crate:
CoreWallet::send_to_addresses,send_dashpay_payment, andcreate_funded_asset_lock_proof(identity funding).Not a funds-loss bug (self-heals via TTL), but a real UX rough edge, reachable in practice via dash-evo-tool's
WalletBackend::send_payment("Send Dash" screen) and the SwiftExampleApp send flows.Depends on the reservation system introduced by the rust-dashcore bump to
afcff156in #3976 (merged).What was done?
TransactionBroadcaster::broadcastnow returnsBroadcastErrorwith two variants:Rejected(the transaction provably never reached the network — safe to release its inputs and retry) andMaybeSent(outcome unknown — the reservation must be kept for the TTL backstop, since releasing and retrying could double-spend). The classification is made inside each broadcaster implementation, where the transport knowledge lives, rather than by matching catch-all error variants at the wallet layer:DapiBroadcasterclassifies every failureMaybeSent: its internal multi-node retries (RequestSettings::default()) mean the surfaced error is only the last attempt's — an earlier attempt may have delivered the transaction (e.g. a node accepts the tx but its gRPC response times out).SpvRuntimeclassifies "client not started" and dash-spv's zero-peerNetworkError::NotConnectedasRejected— both fire before any bytes leave the process — and everything elseMaybeSent(a later failure may follow a partial peer send).MaybeSentsurfaces to callers as the newPlatformWalletError::TransactionBroadcastUnconfirmed, whose message tells the user not to immediately re-spend.wallet::reservations::broadcast_releasing_on_rejection, used by all three build-then-broadcast paths (send_to_addresses,send_dashpay_payment,create_funded_asset_lock_proof), so a rejected broadcast releases the reserved inputs immediately on every path instead of only core sends. The asset-lock recovery rebroadcast intentionally does not release — it re-drives a previously built transaction and is not paired with a fresh reserve. The release path is read-lock only (release_reservationtakes&self), so it doesn't serialize concurrent sends, and the wallet-manager lock is never held across the broadcastawait.bip44_managed_account_at_index/bip32_managed_account_at_indexaccessors instead of hand-rolled map dispatch; a missing wallet/account on the release path istracing::warn!-logged (withwallet_id) without altering the returned error.How Has This Been Tested?
send_to_addresses_releases_reservation_on_broadcast_failurecovers bothBIP44AccountandBIP32Account: funds a wallet, fails the first broadcast via aRejected-returning mock broadcaster, asserts an immediate retry succeeds for each account type.send_to_addresses_keeps_reservation_on_ambiguous_broadcast_failure(both account types) proves the conservative side: aMaybeSentbroadcaster error keeps the reservation, and an immediate retry fails at UTXO selection rather than silently double-spending.cargo fmt --allcargo clippy -p platform-wallet --all-targets -- -D warnings— cleancargo test -p platform-wallet— 204 passed, 0 failedcargo check --all-features --testsforplatform-wallet, plusplatform-wallet-ffi,platform-wallet-storage, andrs-unified-sdk-ffi(all dependents of the changed trait)Breaking Changes
None (not consensus-affecting).
TransactionBroadcaster::broadcast's error type changed fromPlatformWalletErrortoBroadcastError— all in-workspace implementations and callers are updated in this PR; aFrom<BroadcastError> for PlatformWalletErrorimpl preserves the existing?ergonomics.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit