fix(wallet): trustless SPV broadcast acceptance with authoritative outcomes#4198
Merged
Conversation
…allback Network acceptance, not a successful peer socket write, now decides what a broadcast reports. Two authorities establish it: - DAPI/Core (primary): single-attempt submission through DAPI's Core sendrawtransaction bridge, classified accepted / already-known / rejected / uncertain from the gRPC code, with uncertain submissions reconciled via getTransaction. - SPV peer echo (trustless fallback): when DAPI is ambiguous or unreachable, the wallet falls back to dash-spv's broadcast acceptance detection (dashpay/rust-dashcore#913) — the transaction is sent to a subset of SPV peers and a withheld peer announcing the txid back proves it propagated into network mempools. A p2p reject resolves Rejected; no signal within 30s resolves MaybeSent. Rejected releases the transaction's UTXO reservation for immediate retry; MaybeSent keeps it (double-spend guard). Accepted transactions are relayed through SPV best-effort so the local mempool pipeline sees them immediately; a relay failure never downgrades an authoritative accept. The outcome surfaces through FFI as a dedicated rejection code (26) with the txid preserved on all network outcomes, and in Swift as CoreTransactionBroadcastOutcome (accepted/rejected/unknown) via broadcastTransactionWithOutcome, so the example app can no longer report payment success without network acceptance. Bumps rust-dashcore to 58f207e5 (dashpay/rust-dashcore#913). Supersedes #4181; carries over its DAPI classification, getTransaction reconciliation, FFI/Swift outcome plumbing, and reservation reconciliation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughBroadcasting now distinguishes authoritative acceptance, definitive rejection, and uncertain delivery across DAPI, SPV, FFI, and Swift. FFI txid ownership and result codes are updated, Swift exposes typed outcomes, and the example app presents outcome-specific states. ChangesBroadcast outcome pipeline
Dash Core dependency revision
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Wallet
participant DapiBroadcaster
participant DashCore
participant SpvRuntime
Wallet->>DapiBroadcaster: submit transaction
DapiBroadcaster->>DashCore: broadcast transaction
DashCore-->>DapiBroadcaster: accepted, rejected, or uncertain
DapiBroadcaster->>DashCore: reconcile uncertain transaction
DapiBroadcaster-->>Wallet: authoritative outcome
Wallet->>SpvRuntime: request peer-echo fallback
SpvRuntime-->>Wallet: acceptance verdict
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
rust-dashcore#913 dropped its dead BIP61 reject path (modern Dash Core never sends reject messages), so dash-spv's BroadcastResult is now Accepted/Uncertain only. The SPV acceptance fallback loses its Rejected arm: a transaction the p2p network refuses produces no echo and resolves MaybeSent via the timeout. DAPI/Core remains the only source of definitive rejections. Bumps rust-dashcore to c4625d18. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Small heads-up: while testing #4181, I found that |
…t the process Testing on the predecessor PR surfaced a stuck-reservation bug: DapiClientError::NoAvailableAddresses was classified as unknown, keeping the UTXO reservation, and the retry then failed with artificial insufficient funds even though nothing had ever been submitted. DAPI failures that provably precede any dispatch (empty address list, address-list errors) now classify as never-sent instead of uncertain. The SPV acceptance path gains a matching contract: an unstarted client or dash-spv's zero-connected-peers check (which fires before any local dispatch or deferred rebroadcast) surfaces as never-sent, pinned by boundary tests. When BOTH channels provably never sent, the combined outcome is a definitive rejection that releases the reservation for an immediate retry. Any path where bytes may have left the process — DAPI-uncertain, SPV echo timeout, banned-address retries — still keeps the reservation (double-spend guard). Never-sent DAPI submissions also skip the pointless getTransaction reconciliation, and the plain DapiBroadcaster reports them as definitive rejections directly. Thanks to @llbartekll for the report. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
marked this pull request as ready for review
July 22, 2026 15:17
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
July 22, 2026 15:17
Collaborator
|
🕓 Ready for review — 58 ahead in queue (commit c82fe1d) |
…'s send SPV wallets now broadcast exclusively over P2P through dash-spv's acceptance detection (rust-dashcore#913): the transaction goes to a subset of peers, and a withheld peer announcing it back, an InstantSend lock, or a confirmation proves network acceptance. Trustless, no DAPI round-trip, and no separate relay step (dash-spv injects the transaction into its local mempool pipeline as part of the broadcast). Outcome mapping is unchanged for callers: accepted resolves the send, no signal within 30s stays MaybeSent (reservation kept; dash-spv keeps rebroadcasting and a later echo/IS-lock/confirmation or the TTL backstop reconciles), and provably-never-sent failures (client not started, zero connected peers) release the reservation. DapiBroadcaster remains for standalone wallets without an SPV runtime, keeping its classification, getTransaction reconciliation, and never-sent handling. SpvRuntime's now-unused relay path is removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
changed the title
DapiBroadcaster has no call sites — the manager always constructs SpvBroadcaster — so the classification/reconciliation machinery built up for it (DapiSubmission, gRPC code mapping, single-attempt settings, DapiCoreClient seam, getTransaction reconciliation, never-sent handling, and its test suite) serviced a dead path. Revert it to its pre-PR form and revert the incidental comment/test churn in asset-lock recovery, test_support, and the shielded bind tests. The live path is untouched: SpvBroadcaster's pure-SPV acceptance mapping, the SPV-side never-sent classification, and the FFI/Swift outcome surface all remain. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #4198 +/- ##
==========================================
Coverage 87.50% 87.51%
==========================================
Files 2666 2666
Lines 336721 336866 +145
==========================================
+ Hits 294659 294800 +141
- Misses 42062 42066 +4
🚀 New features to boost your workflow:
|
This was referenced Jul 23, 2026
Merged
thepastaclaw
pushed a commit
to thepastaclaw/platform
that referenced
this pull request
Jul 23, 2026
The Swift SDK CI build compiles SwiftTests with warnings escalated to errors, and the integration-test support wallet still called the broadcastTransaction compatibility wrapper that dashpay#4198 deprecated, so any PR triggering the Swift workflow now fails to build. Switch TestWallet to broadcastTransactionWithOutcome and surface rejected/unknown outcomes as the same errors the wrapper raised. The base branch never notices because the Swift job is path-filtered and rs-only pushes skip it. Also make the coverage-artifact cleanup retry loop break only when the removal succeeded and target/llvm-cov-target is actually absent, so a concurrently recreated directory is retried instead of silently kept. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Note
Supersedes #4181. Built on dashpay/rust-dashcore#913 (merged); rust-dashcore is pinned to its merge commit
70d4bf8e.Problem
The wallet reported a broadcast as successful once bytes were written to an SPV peer socket. No network signal was consulted, so an invalid or dropped transaction still surfaced as "Payment sent", and UTXO reservation handling had only a coarse rejected/maybe-sent split derived from transport errors.
Approach
Network acceptance, not a socket write, decides the outcome. The authority depends on how the wallet runs:
inv, an InstantSend lock, or a confirmation proves the network accepted it →Accepted. Trustless — no DAPI involvement in the L1 send path, and no separate relay step (dash-spv injects the transaction into its local mempool pipeline as part of the broadcast). No signal within 30s →MaybeSent(dash-spv keeps rebroadcasting; a later echo/IS-lock/confirmation or the reservation-TTL backstop reconciles). Failures that provably precede any send (client not started, zero connected peers — pinned against dash-spv semantics by boundary tests) → definitiveRejected.v4.1-dev: submission via DAPI's gRPC endpoint with every failure conservatively classifiedMaybeSent. (This path has no production call sites today, so this PR deliberately leaves it untouched.)Outcome semantics through the stack:
Rejectedreleases the transaction's UTXO reservation for immediate retry;MaybeSentkeeps it (double-spend guard) — unchanged reconcile point inwallet/reservations.rs. Failures that provably never left the process (SPV client down, zero connected peers) release rather than lock the reservation.26(ErrorTransactionBroadcastRejected); the txid out-param is populated on all network outcomes (accepted, rejected, unknown), null only on operational errors.CoreTransactionBroadcastOutcome(accepted/rejected/unknown) returned bybroadcastTransactionWithOutcome; the old throwingbroadcastTransactionremains as a deprecated wrapper.SendViewModelapplies the outcome as a pure state transition — success is only shown on acceptance, and unknown outcomes warn against retrying.Testing
cargo test -p platform-wallet: 493 passed, 0 failed — the SPV broadcaster maps every dash-spv verdict (accepted / uncertain / never-sent / transport error) with the reservation released only on never-sent; boundary pins catch dash-spv semantic drift on the never-sent classification.cargo test -p platform-wallet-ffi: 236 passed, 0 failed — outcome classification carries the txid on all network outcomes; code-26 mapping round-trips.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests