feat: rebroadcast unconfirmed self-sent transactions#627
Merged
xdustinface merged 3 commits intov0.42-devfrom Apr 17, 2026
Merged
feat: rebroadcast unconfirmed self-sent transactions#627xdustinface merged 3 commits intov0.42-devfrom
xdustinface merged 3 commits intov0.42-devfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA broadcast request variant and sender were added to the network layer; the network manager now spawns tasks to broadcast messages to all peers. The mempool manager periodically rebroadcasts aged unconfirmed transactions (10 minutes) by using the new broadcast path during its sync tick. Changes
Sequence Diagram(s)sequenceDiagram
participant MM as MempoolManager
participant RS as RequestSender
participant NM as NetworkManager
participant P as Peer
MM->>MM: tick()
MM->>MM: identify txs with last_broadcast >= REBROADCAST_INTERVAL
MM->>RS: broadcast(NetworkMessage::Tx)
activate RS
RS->>NM: enqueue BroadcastMessage
deactivate RS
NM->>NM: spawn task to broadcast to peers
activate NM
NM->>P: send tx to Peer (for each)
P-->>NM: success / failure per peer
NM->>NM: aggregate results & log failures
deactivate NM
MM->>MM: update recent_sends timestamps
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #627 +/- ##
=============================================
- Coverage 67.99% 67.98% -0.01%
=============================================
Files 318 319 +1
Lines 67877 67970 +93
=============================================
+ Hits 46153 46210 +57
- Misses 21724 21760 +36
|
![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
🧹 Nitpick comments (1)
dash-spv/src/sync/mempool/manager.rs (1)
349-353: Keeprecent_sendscleanup insideMempoolState::remove_transaction.Handling it only here fixes confirmations, but it leaves the invariant split across callers. Any other path removing a tx in
dash-spv/src/types.rscan still orphan arecent_sendsentry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/sync/mempool/manager.rs` around lines 349 - 353, The loop in manager.rs is performing recent_sends cleanup outside the state, splitting the invariant; update MempoolState::remove_transaction (in dash-spv/src/types.rs) to also remove the txid from its recent_sends map when a transaction is removed and return a clear indicator (e.g., Option or bool) that the tx was removed, then delete the external recent_sends removal from the loop in mempool/manager.rs and use the returned indicator to populate removed; ensure the unique symbol names referenced are MempoolState::remove_transaction, recent_sends, and the loop over txids in the mempool_state.write() block so all callers rely on the single internal cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-spv/src/sync/mempool/manager.rs`:
- Around line 423-427: Current code calls take_rebroadcast_due() which resets
timestamps up front and then ignores the Result from RequestSender::broadcast(),
causing txs to be suppressed even on send failure; change the flow to first
collect due txs without consuming the rebroadcast window (e.g., add or use a
non-destructive accessor like list_rebroadcast_due or a peek_rebroadcast_due on
mempool_state), attempt to broadcast each tx and check the Result from
RequestSender::broadcast(), and only call the mutation that marks a tx as
rebroadcasted (the counterpart of take_rebroadcast_due, e.g., mark_rebroadcasted
or consuming method) after a successful send; also stop discarding the broadcast
Result so failures can be retried or the timestamp left unchanged.
- Around line 371-374: The code marks a transaction InstantSend
(tx.is_instant_send = true) but doesn't move its pending amount into the
InstantSend bucket, causing pending balance drift and negative totals when
remove_transaction() later subtracts from pending_instant_balance. Fix by
transferring the transaction's pending amount from state.pending_balance into
state.pending_instant_balance at the same spot where you set tx.is_instant_send
(i.e., decrement state.pending_balance and increment
state.pending_instant_balance by the transaction's pending amount field), and
ensure the same unique field name you use for the tx pending amount is used so
remove_transaction() and other logic remain consistent.
---
Nitpick comments:
In `@dash-spv/src/sync/mempool/manager.rs`:
- Around line 349-353: The loop in manager.rs is performing recent_sends cleanup
outside the state, splitting the invariant; update
MempoolState::remove_transaction (in dash-spv/src/types.rs) to also remove the
txid from its recent_sends map when a transaction is removed and return a clear
indicator (e.g., Option or bool) that the tx was removed, then delete the
external recent_sends removal from the loop in mempool/manager.rs and use the
returned indicator to populate removed; ensure the unique symbol names
referenced are MempoolState::remove_transaction, recent_sends, and the loop over
txids in the mempool_state.write() block so all callers rely on the single
internal cleanup.
🪄 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: c86f9796-b03b-4c13-9304-093f3d938b20
📒 Files selected for processing (5)
dash-spv/src/network/manager.rsdash-spv/src/network/mod.rsdash-spv/src/sync/mempool/manager.rsdash-spv/src/sync/mempool/sync_manager.rsdash-spv/src/types.rs
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
force-pushed
the
feat/rebroadcast-unconfirmed-tx
branch
from
b747d56 to
ea9cd8e
Compare
github-actions
bot
removed
the
merge-conflict
Collaborator
Author
|
@CodeRabbit review! |
Contributor
|
✅ Actions performedReview triggered.
|
xdustinface
force-pushed
the
feat/rebroadcast-unconfirmed-tx
branch
from
ea9cd8e to
1e54e4c
Compare
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
Each self-sent transaction in `recent_sends` tracks when it was last broadcast. Transactions whose last broadcast was more than 10 minutes ago are rebroadcast to all peers on each tick. Add `BroadcastMessage` variant to `NetworkRequest` so the mempool manager can request a broadcast via the request queue. Clean up `recent_sends` when a transaction is confirmed or IS-locked, since there is no need to rebroadcast finalized transactions.
xdustinface
force-pushed
the
feat/rebroadcast-unconfirmed-tx
branch
from
1e54e4c to
821c4f8
Compare
github-actions
bot
removed
the
merge-conflict
Project `now` forward via a private `rebroadcast_if_due_at` helper instead of subtracting `REBROADCAST_INTERVAL` from `Instant::now()`. On Windows, `Instant` is backed by `QueryPerformanceCounter` measured from boot, so on a freshly provisioned runner with uptime below the interval the subtraction underflows and panics.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
dash-spv/src/sync/mempool/manager.rs (1)
457-469:⚠️ Potential issue | 🟠 MajorDon’t consume the rebroadcast window before enqueue succeeds.
Line 463 updates
last_broadcastbefore Line 468 enqueues the broadcast, and the enqueue result is discarded. If the request channel is closed, the tx is still delayed for another interval even though nothing was queued.Proposed fix
async fn rebroadcast_if_due_at(&mut self, requests: &RequestSender, now: Instant) { - let mut due: Vec<Transaction> = Vec::new(); - for (txid, last_broadcast) in &mut self.recent_sends { + let mut due: Vec<(Txid, Transaction)> = Vec::new(); + for (txid, last_broadcast) in &self.recent_sends { if now.saturating_duration_since(*last_broadcast) < REBROADCAST_INTERVAL { continue; } if let Some(unconfirmed) = self.transactions.get(txid) { - due.push(unconfirmed.transaction.clone()); - *last_broadcast = now; + due.push((*txid, unconfirmed.transaction.clone())); } } - for tx in &due { - let _ = requests.broadcast(NetworkMessage::Tx(tx.clone())); + let mut sent = 0usize; + for (txid, tx) in due { + if requests.broadcast(NetworkMessage::Tx(tx)).is_ok() { + if let Some(last_broadcast) = self.recent_sends.get_mut(&txid) { + *last_broadcast = now; + } + sent += 1; + } else { + tracing::warn!("Failed to enqueue rebroadcast for tx {}", txid); + } } - if !due.is_empty() { - tracing::info!("Rebroadcast {} unconfirmed transaction(s) to all peers", due.len()); + if sent > 0 { + tracing::info!("Rebroadcast {} unconfirmed transaction(s) to all peers", sent); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/sync/mempool/manager.rs` around lines 457 - 469, The code updates the rebroadcast timestamp (in self.recent_sends via last_broadcast) before attempting to enqueue the transaction (requests.broadcast), so a failed enqueue still consumes the rebroadcast window; change the flow in the loop that builds due and updates last_broadcast: instead of setting *last_broadcast = now when pushing to due, only collect txids (or references) into due and leave last_broadcast unchanged, then when iterating over due call requests.broadcast(NetworkMessage::Tx(tx.clone())) and only on success update the corresponding entry in self.recent_sends (using the txid) to now; ensure failed sends do not update last_broadcast and consider logging or propagating the enqueue error from requests.broadcast to aid debugging.
🧹 Nitpick comments (1)
dash-spv/src/sync/mempool/manager.rs (1)
1643-1704: Add a failure-path rebroadcast test (queue send failure).These tests cover due/not-due timing well, but they don’t cover the case where enqueue fails (e.g., dropped request receiver). Add one test asserting
recent_sendsis not advanced when broadcast enqueue returnsErr, so this behavior doesn’t regress.As per coding guidelines, "Write unit tests for new functionality."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/sync/mempool/manager.rs` around lines 1643 - 1704, Add a new #[tokio::test] that simulates enqueue failure by dropping the request receiver before calling the rebroadcast path (use create_test_manager() to get (manager, requests, rx), then drop(rx) so sending to requests will Err), insert an UnconfirmedTransaction and set its recent_sends entry to a past Instant (use manager.transactions.insert with UnconfirmedTransaction::new and manager.recent_sends.insert), call manager.rebroadcast_if_due_at(&requests, later).await (or manager.rebroadcast_if_due(&requests).await) and finally assert that manager.recent_sends still contains the original timestamp (i.e., was not advanced) to ensure failed enqueue does not update recent_sends.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@dash-spv/src/sync/mempool/manager.rs`:
- Around line 457-469: The code updates the rebroadcast timestamp (in
self.recent_sends via last_broadcast) before attempting to enqueue the
transaction (requests.broadcast), so a failed enqueue still consumes the
rebroadcast window; change the flow in the loop that builds due and updates
last_broadcast: instead of setting *last_broadcast = now when pushing to due,
only collect txids (or references) into due and leave last_broadcast unchanged,
then when iterating over due call
requests.broadcast(NetworkMessage::Tx(tx.clone())) and only on success update
the corresponding entry in self.recent_sends (using the txid) to now; ensure
failed sends do not update last_broadcast and consider logging or propagating
the enqueue error from requests.broadcast to aid debugging.
---
Nitpick comments:
In `@dash-spv/src/sync/mempool/manager.rs`:
- Around line 1643-1704: Add a new #[tokio::test] that simulates enqueue failure
by dropping the request receiver before calling the rebroadcast path (use
create_test_manager() to get (manager, requests, rx), then drop(rx) so sending
to requests will Err), insert an UnconfirmedTransaction and set its recent_sends
entry to a past Instant (use manager.transactions.insert with
UnconfirmedTransaction::new and manager.recent_sends.insert), call
manager.rebroadcast_if_due_at(&requests, later).await (or
manager.rebroadcast_if_due(&requests).await) and finally assert that
manager.recent_sends still contains the original timestamp (i.e., was not
advanced) to ensure failed enqueue does not update recent_sends.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: afd6b863-eadf-4398-ae24-a04eec395def
📒 Files selected for processing (1)
dash-spv/src/sync/mempool/manager.rs
ZocoLini
previously approved these changes
Apr 16, 2026
Collaborator
ZocoLini
left a comment
ZocoLini
left a comment
There was a problem hiding this comment.
Not really blocking my comment, its a performance related issue but not really a problem
…ecting into a `Vec` Addresses ZocoLini review comment on PR #627 #627 (comment)
xdustinface
dismissed stale reviews from ZocoLini and coderabbitai[bot]
via
0ca9b39
ZocoLini
approved these changes
Apr 16, 2026
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.
Each self-sent transaction in
recent_sendstracks when it was last broadcast. Transactions whose last broadcast was more than 10 minutes ago are rebroadcast to all peers on each tick.Add
BroadcastMessagevariant toNetworkRequestso the mempool manager can request a broadcast via the request queue.Clean up
recent_sendswhen a transaction is confirmed or IS-locked, since there is no need to rebroadcast finalized transactions.This will only work properly with #626 merged since we don't record self sent transactions without that PR.
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests