fix(dash-spv): remove overly-strict is_synced gate from broadcast#656
fix(dash-spv): remove overly-strict is_synced gate from broadcast#656QuantumExplorer wants to merge 1 commit intov0.42-devfrom
Conversation
The `dispatch_local` fix in #626 added a gate that rejected `broadcast_transaction` whenever `sync_progress().is_synced()` returned false. `is_synced()` requires *every* sync manager (headers / filter_headers / filters / blocks / masternodes) to be in `SyncState::Synced` simultaneously — any manager transitioning to `WaitForEvents` (e.g. the blocks manager briefly reacting to a new chain tip) flips the aggregate to "not synced" and blocks broadcasts. In end-to-end tests this manifested as intermittent `SpvBroadcastFailed { detail: "Client is not synced" }` errors in the middle of normal operation, hours after initial sync had completed. The gate served no purpose the underlying broadcast protocol doesn't already handle — peers themselves reject unfit-for-propagation txs. Drop it.
|
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 (1)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #656 +/- ##
=============================================
- Coverage 68.08% 68.05% -0.04%
=============================================
Files 319 319
Lines 67646 67643 -3
=============================================
- Hits 46057 46032 -25
- Misses 21589 21611 +22
|
ZocoLini
left a comment
ZocoLini
left a comment
There was a problem hiding this comment.
I believe I asked Kevin for this check in the past and he gave me a reason for its existence, I would like to wait for him to approve this. About the intermittent errors after sync completed, I have never experienced that situation, but if it is a possibility, it means we have a bug and is_synced is returning false after sync, would you be able to share a case where this can happen??
|
It will return false temporarily for incremental sync updates. I think instead of removing this check it would be better to make it so that we check if the initial sync was done rather. I will not block on this PR if you wanna get it in do it but i dont think its a good idea because the underlying issue it might cause is not handled. |
|
There's no reason not to broadcast a transaction while we are syncing. The worst thing that could happen is that you haven't fully synced and the transaction will be rejected. Generally this will only happen on imported wallets, and imo should be a different wallet level check on new imported wallets. |
|
There is a reason. In this path we inject the broadcasted transactions back into our network message queue as local message since there is no mempool rejection reponse and the peers wont relay our own tx back to us. Which means we will process the transaction and store it in the wallet even though the utxos used in there might already be spent in chain or mempool which then is not recoverable because this cases are not handled internally. Yes having this check doesnt solve all the issues but its a small safe guard to at least avoid some. Anyway, i just thought it would be good to least put some thougts into all this before just removing this check. In my opinion a wallet always should be synced before it can be used. Same for creating transactions and anything else. Synced should be a requirement not an option since having it optional just increases lots of weird edge cases and compexity that needs to be handled. |
4 participants
Summary
The
dispatch_localgate added in #626 rejectedbroadcast_transactionwheneversync_progress().is_synced()returnedfalse.is_synced()is the aggregate of every sync manager (headers / filter_headers / filters / blocks / masternodes) — any single manager briefly transitioning toWaitForEvents(e.g. the blocks manager reacting to a new chain tip) flips the aggregate and blocks broadcasts.In end-to-end tests this surfaced as intermittent
SpvBroadcastFailed { detail: "Client is not synced" }errors well after initial sync had completed. The gate also doesn't protect anything the peer-level broadcast path doesn't already handle — peers reject unfit-for-propagation txs on their own. Drop it.Extracted from
This commit was cherry-picked from
feat/platform-wallet2(the in-progress platform-wallet branch, draft PR #655). Pulling it into its own PR so it can land independently.Test plan
cargo build -p dash-spv --all-featurescargo test -p dash-spv --all-features --lib— 370 passed / 0 faileddash-spv/src/client/transactions.rs, −5 / +1).🤖 Extracted with Claude Code
Summary by CodeRabbit
Bug Fixes