feat(dash-spv): track network acceptance of broadcast transactions#913
Merged
Merged
Conversation
Broadcasts are now sent to a subset of connected peers (default: half, per BroadcastHoldout::Half) while being withheld from the rest. Since peers never re-announce a transaction to whoever sent it to them, an inv for our txid from a withheld peer proves the transaction relayed through the network — the broadcast is then reported Accepted. InstantSend locks, block confirmations, and already-have rejects also count as acceptance; p2p reject messages (best-effort on modern dashd) report Rejected, distinguishing txn-mempool-conflict (rejection) from txn-already-in-mempool (acceptance) under the shared Duplicate code; a configurable timeout reports Uncertain, upgradeable by a late echo. Outcomes surface as SyncEvent::TransactionBroadcastResult, an awaitable DashSpvClient::broadcast_transaction_and_wait API, and matching FFI callbacks, config setters, and a blocking FFI wait function. Rebroadcast keeps respecting the holdout while pending, re-picks holdouts from never-sent peers on disconnect, skips rejected entries, and sends deferred broadcasts as soon as a peer connects. Broadcast state survives disconnects. Verified with a new two-node dashd integration test: the transaction is sent to one node and the withheld node's inv echo confirms acceptance; a double-spend leg documents that dashd 23 sends no BIP61 rejects and correctly resolves Uncertain. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThe PR adds configurable transaction broadcast acceptance tracking with holdout and timeout policies, accepted and uncertain outcomes, sync events, client waiting APIs, integration coverage, and FFI configuration, result, and callback support. ChangesTransaction broadcast acceptance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MempoolManager
participant Peers
participant SyncEvents
Client->>MempoolManager: broadcast_transaction_and_wait
MempoolManager->>Peers: send transaction using holdout policy
Peers-->>MempoolManager: inventory echo
MempoolManager->>SyncEvents: emit TransactionBroadcastResult
SyncEvents-->>Client: return Accepted or Uncertain
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@dash-spv-ffi/src/config.rs`:
- Around line 364-406: Add unit coverage for
dash_spv_ffi_config_set_broadcast_acceptance_threshold and
dash_spv_ffi_config_set_broadcast_acceptance_timeout_secs: verify zero values
return InvalidArgument and set the expected error, while nonzero values return
Success and map to the corresponding ClientConfig fields. Use the existing FFI
config test helpers and assertion patterns.
In `@dash-spv/src/client/transactions.rs`:
- Around line 85-99: Update the transaction broadcast result handling around the
SyncEvent::TransactionBroadcastResult match so an Uncertain result does not
return immediately; continue waiting for a later Accepted or Rejected result.
Return Uncertain only when the caller’s deadline expires, while preserving the
existing handling for unrelated events, lagged streams, and closed channels.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89bed8d6-5666-4fd7-a9db-f79937a716c2
📒 Files selected for processing (18)
dash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/client.rsdash-spv-ffi/src/config.rsdash-spv-ffi/tests/dashd_sync/callbacks.rsdash-spv-ffi/tests/unit/test_async_operations.rsdash-spv/src/client/config.rsdash-spv/src/client/lifecycle.rsdash-spv/src/client/transactions.rsdash-spv/src/lib.rsdash-spv/src/network/mod.rsdash-spv/src/sync/events.rsdash-spv/src/sync/mempool/broadcast.rsdash-spv/src/sync/mempool/manager.rsdash-spv/src/sync/mempool/mod.rsdash-spv/src/sync/mempool/sync_manager.rsdash-spv/src/sync/mod.rsdash-spv/tests/dashd_sync/tests_mempool.rs
Collaborator
This was referenced Jul 22, 2026
Merged
…nctions Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Modern Dash Core removed the BIP61 reject message entirely (no enablebip61, NetMsgType::REJECT, or reject codes remain in dashpay/dash), and the two-node integration test confirmed dashd 23.1.0 sends nothing for a refused transaction. The reject path was therefore dead code: remove the Reject message subscription and handler, the BroadcastResult::Rejected and BroadcastStatus::Rejected variants, and the FFI reject fields (FFIBroadcastStatus is now Accepted=0/Uncertain=1, the callback and FFIBroadcastResult lose reject_code/reject_reason, and the result struct no longer owns strings so its destroy function is gone). A transaction the network refuses now uniformly surfaces as Uncertain: no echo arrives within the acceptance timeout. The integration test's double-spend leg asserts exactly that. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dash-spv/src/sync/mempool/sync_manager.rs (1)
105-113: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not rebroadcast broadcasts that are already
Accepted.This now invokes rebroadcast bookkeeping before the sync-state guard, while
dash-spv/src/sync/mempool/manager.rs:625-682sends transactions for every non-Pendingstatus. Consequently, terminalAcceptedbroadcasts are repeatedly resent instead of being skipped, violating the stated “avoid resending transactions that have reached a terminal state” behavior. Restrict rebroadcasting toPendingandUncertain, or explicitly skipBroadcastStatus::Acceptedin the helper.Proposed fix
- } else { + } else if state.status == BroadcastStatus::Uncertain { let _ = requests.broadcast(NetworkMessage::Tx(state.transaction.clone())); }🤖 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 `@dash-spv/src/sync/mempool/sync_manager.rs` around lines 105 - 113, The rebroadcast flow in tick must not resend broadcasts with terminal Accepted status. Update rebroadcast_if_due and its related status handling to process only Pending and Uncertain broadcasts, or explicitly skip BroadcastStatus::Accepted, while preserving expiry bookkeeping and existing sync-state behavior.dash-spv-ffi/src/callbacks.rs (1)
230-257: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftVersion the broadcast FFI ABI
OnTransactionBroadcastResultCallback,FFISyncEventCallbacks, andFFIBroadcastResultchanged incompatibly. Regenerated headers don’t protect already-built binaries, so add versioned*_v2symbols/types or make this a documented major ABI bump.🤖 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 `@dash-spv-ffi/src/callbacks.rs` around lines 230 - 257, The broadcast FFI changes require explicit ABI versioning: add compatible *_v2 symbols/types for OnTransactionBroadcastResultCallback, FFISyncEventCallbacks, and FFIBroadcastResult, preserving the existing ABI for already-built clients. Update dash-spv-ffi/src/callbacks.rs lines 230-257 and 282, plus dash-spv-ffi/src/client.rs lines 351-362, so the new callback and result paths use the versioned definitions; alternatively, document and enforce this as a major ABI bump if versioned symbols are not added.Source: Coding guidelines
🤖 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 `@dash-spv-ffi/FFI_API.md`:
- Around line 276-283: Update the description for
dash_spv_ffi_config_set_broadcast_acceptance_timeout_secs to remove references
to rejection signals and state that broadcasts without an acceptance signal are
reported as Uncertain. Keep the timeout and safety requirements unchanged.
---
Outside diff comments:
In `@dash-spv-ffi/src/callbacks.rs`:
- Around line 230-257: The broadcast FFI changes require explicit ABI
versioning: add compatible *_v2 symbols/types for
OnTransactionBroadcastResultCallback, FFISyncEventCallbacks, and
FFIBroadcastResult, preserving the existing ABI for already-built clients.
Update dash-spv-ffi/src/callbacks.rs lines 230-257 and 282, plus
dash-spv-ffi/src/client.rs lines 351-362, so the new callback and result paths
use the versioned definitions; alternatively, document and enforce this as a
major ABI bump if versioned symbols are not added.
In `@dash-spv/src/sync/mempool/sync_manager.rs`:
- Around line 105-113: The rebroadcast flow in tick must not resend broadcasts
with terminal Accepted status. Update rebroadcast_if_due and its related status
handling to process only Pending and Uncertain broadcasts, or explicitly skip
BroadcastStatus::Accepted, while preserving expiry bookkeeping and existing
sync-state behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b56568a-101f-4745-bb61-58128ac67330
📒 Files selected for processing (10)
dash-spv-ffi/FFI_API.mddash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/client.rsdash-spv/src/client/transactions.rsdash-spv/src/sync/events.rsdash-spv/src/sync/mempool/broadcast.rsdash-spv/src/sync/mempool/manager.rsdash-spv/src/sync/mempool/sync_manager.rsdash-spv/tests/dashd_sync/tests_mempool.rs
💤 Files with no reviewable changes (1)
- dash-spv-ffi/src/bin/ffi_cli.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- dash-spv/src/sync/events.rs
- dash-spv/tests/dashd_sync/tests_mempool.rs
- dash-spv/src/client/transactions.rs
- dash-spv/src/sync/mempool/manager.rs
- broadcast_transaction_and_wait no longer resolves on the manager's interim Uncertain event: a late echo can still upgrade the outcome to Accepted, so callers with a timeout longer than the configured acceptance timeout keep waiting until their deadline. Uncertain is now only returned on deadline expiry (or bus close). - Add FFI unit tests for the broadcast config setters: zero threshold/timeout are rejected with InvalidArgument without modifying the config; valid values map onto the ClientConfig fields; holdout half/count setters round-trip. - Drop the obsolete "or rejection signal" wording from the timeout setter doc (BIP61 reject handling was removed) and regenerate FFI_API.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
xdustinface
approved these changes
Jul 22, 2026
Member
Author
|
Tested in iOS simulator. |
QuantumExplorer
added a commit
to dashpay/platform
that referenced
this pull request
Jul 22, 2026
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765
pushed a commit
to bfoss765/platform
that referenced
this pull request
Jul 23, 2026
…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 dashpay#4181; carries over its DAPI classification, getTransaction reconciliation, FFI/Swift outcome plumbing, and reservation reconciliation. 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
2 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.
Problem
broadcast_transactionsent the transaction to all connected peers and immediately reported it as detected via the local re-inject — no network signal was ever consulted.invechoes of our own txid were discarded by the dedup gate inhandle_inv, p2prejectmessages had no subscriber, and there was no accepted/rejected/uncertain state. Until an InstantSend lock or block confirmation arrived, the client genuinely did not know whether the network accepted a broadcast.Approach
Implements the classic SPV acceptance heuristic. Key protocol fact: a peer never re-announces a transaction to the peer it received it from, so broadcasting to everyone means no echo can ever arrive. Instead the mempool manager now:
BroadcastHoldout::{Half, Count(n)}), withholding it from the rest;invfor the txid from a non-recipient peer as proof the transaction relayed through the network →Accepted { relayed_by };Acceptedon InstantSend lock, block confirmation, or an "already-have" reject;rejectas a best-effort negative signal →Rejected { code, reason }, distinguishingtxn-mempool-conflict(a real rejection, e.g. double-spend) fromtxn-already-in-mempool(acceptance) under the sharedDuplicatecode;Uncertainafter a configurable timeout (default 60s), upgradeable by a late echo.State transitions (
Pending → Accepted | Rejected | Uncertain,Uncertain → Accepted) are the single duplicate-event guard; each transition emits exactly oneSyncEvent::TransactionBroadcastResult.Rebroadcast keeps respecting the holdout while pending, re-picks holdouts from never-sent peers when they disconnect, never resends rejected transactions, and sends deferred (zero-peer) broadcasts as soon as a peer connects. Broadcast state survives disconnects.
API
SyncEvent::TransactionBroadcastResult { txid, result: BroadcastResult }DashSpvClient::broadcast_transaction_and_wait(&tx, timeout) -> Result<BroadcastResult>(subscribes before sending so the outcome cannot be missed)ClientConfig:broadcast_holdout,broadcast_acceptance_threshold,broadcast_acceptance_timeout(+ builders, validation)on_transaction_broadcast_resultcallback onFFISyncEventCallbacks,dash_spv_ffi_client_broadcast_transaction_and_waitreturningFFIBroadcastResult, and four config settersenable_mempool_tracking = falsethe legacy broadcast-to-all behavior is retained (documented as untracked)Testing
Duplicatereject cases, non-tx rejects ignored, timeout→Uncertain→late-echo upgrade, sticky/re-picked holdouts, threshold=2, zero-peer deferral,clear_pendingsurvival, broadcast pruning; FFI dispatch test for all three outcome variantstest_broadcast_transaction_network_acceptance): SPV connects to two connected dashd nodes, sends to exactly one, and the withheld node's echo resolvesAccepted(relayed_by=1)(~119ms on regtest). The double-spend leg resolvesUncertain, documenting that dashd 23.1.0 sends no BIP61 rejects — which is why the echo, not the reject, is the primary signal.-D warnings, fmt clean.Related: dashpay/platform#4181 solves the same problem for the platform wallet by treating Core/DAPI as authoritative; this PR is the complementary pure-SPV path for when no trusted Core is available.
🤖 Generated with Claude Code
Summary by CodeRabbit
Fixes #664
Fixes #734