feat: rebroadcast unconfirmed self-sent transactions#627
Open
xdustinface wants to merge 1 commit intov0.42-devfrom
Open
feat: rebroadcast unconfirmed self-sent transactions#627xdustinface wants to merge 1 commit intov0.42-devfrom
xdustinface wants to merge 1 commit intov0.42-devfrom
Conversation
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 (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA mempool rebroadcast feature was added: the mempool manager identifies recently-sent unconfirmed transactions due for rebroadcast and issues Changes
Sequence Diagram(s)sequenceDiagram
participant Sync as Sync Tick
participant MempoolMgr as Mempool Manager
participant State as MempoolState
participant Request as RequestSender
participant NetMgr as Network Manager
participant Peers as Peers
Sync->>MempoolMgr: tick()
MempoolMgr->>MempoolMgr: send_queued(requests)
MempoolMgr->>MempoolMgr: rebroadcast_if_due(requests)
MempoolMgr->>State: take_rebroadcast_due(interval)
State-->>MempoolMgr: Vec<Transaction>
loop per transaction
MempoolMgr->>Request: broadcast(NetworkMessage::Tx)
Request->>NetMgr: enqueue BroadcastMessage
NetMgr->>Peers: send to all peers (async)
Peers-->>NetMgr: per-peer results
NetMgr-->>MempoolMgr: counts of failures/total
MempoolMgr->>MempoolMgr: log if failures > 0
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #627 +/- ##
=============================================
+ Coverage 67.81% 67.87% +0.05%
=============================================
Files 318 318
Lines 67976 68069 +93
=============================================
+ Hits 46100 46201 +101
+ Misses 21876 21868 -8
|
![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. |
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
b747d56 to
ea9cd8e
Compare
github-actions
bot
removed
the
merge-conflict
Collaborator
Author
|
@CodeRabbit review! |
Contributor
|
✅ Actions performedReview triggered.
|
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
1 participant
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
Improvements
Bug Fixes