fix(dash-spv): recover masternode sync from a rejected QRInfo - #936
Closed
bfoss765 wants to merge 3 commits into
Closed
fix(dash-spv): recover masternode sync from a rejected QRInfo#936bfoss765 wants to merge 3 commits into
bfoss765 wants to merge 3 commits into
Conversation
One QRInfo response the engine rejects permanently strands masternode sync. `qrinfo_received()` cleared `qrinfo_in_flight` before the fallible `feed_qr_info`, so a validation failure returned `Err` leaving the manager in `Syncing` with nothing in flight and an empty diff pipeline. The [10, 30, 60] retry ladder is armed solely by `qrinfo_in_flight`, so `tick` fell through forever - a frozen `qr_infos_requested: 1` for the life of the process, with the error surfacing only as a `SyncEvent::ManagerError` that has no consumer. Masternode sync gates overall SYNCED, so while stalled every InstantLock fails verification, DAPI has no masternode list, and platform features are dead until a restart. Three changes: 1. Release the request slot only after the last fallible step, just before `queue_requests`. On the `feed_qr_info` error branch keep the slot armed and flag the attempt `rejected`, which `tick` treats as an elapsed timeout: the retry rotates to the next peer via `send_distributed`'s round-robin, on the existing budget, so a deterministically-bad response still terminates after MAX_RETRY_ATTEMPTS dispatches. `last_processed_qrinfo_tip` continues to be set only on success, so the retry is not dropped as a duplicate. 2. Add a stall watchdog to `tick`: `Syncing` with no QRInfo in flight and an empty diff pipeline for longer than 60s re-dispatches a QRInfo. This covers the routes the in-flight flag cannot, notably a `send_qrinfo_for_tip` that fails after its caller already cleared the slot. Timed off a new `last_qrinfo_dispatch` rather than `progress.last_activity()`, which unrelated block events keep bumping. 3. `on_disconnect` requeues in-flight `GetMnListDiff`s instead of clearing, mirroring `BlocksManager` and `FiltersManager`, and leaves the QRInfo slot armed. `tick` now flushes the pending queue whenever the pipeline is non-empty, which is what actually reissues requeued requests - every other `send_pending` call site hangs off a response handler that cannot run while nothing is in flight.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe masternode sync flow tracks QRInfo rejection and dispatch time, preserves work across disconnects, retries rejected requests, redispatches pending MnListDiff requests, and recovers stalled synchronization through a watchdog. ChangesMasternode sync reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SyncManager
participant Peer
participant MnListDiffPipeline
SyncManager->>SyncManager: tick()
alt QRInfo is rejected or timed out
SyncManager->>Peer: retry QRInfo
else MnListDiff work is pending
SyncManager->>MnListDiffPipeline: send_pending()
MnListDiffPipeline->>Peer: dispatch MnListDiff
else sync is stalled for 60 seconds
SyncManager->>Peer: redispatch QRInfo
end
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🤖 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/src/sync/masternodes/sync_manager.rs`:
- Around line 32-43: Make QRINFO_STALL_WATCHDOG configurable by sourcing its
duration from the existing synchronization or client configuration instead of
defining a fixed Duration::from_secs(60). Update the SyncManager initialization
and tick logic to use the configured value while preserving the watchdog’s
empty-slot Syncing behavior.
- Around line 1047-1293: Add an external integration test under dash-spv/tests
that exercises the public synchronization API through a recovery scenario, such
as a rejected QRInfo or peer disconnect, and verifies that synchronization
re-dispatches or resumes successfully. Keep the existing in-module tests
unchanged, and drive the scenario through publicly accessible setup, message,
and synchronization-flow interfaces rather than inspecting private manager
state.
- Around line 347-350: Move the `last_processed_qrinfo_tip` assignment from
before `build_mnlistdiff_request_pairs(...).await?` to immediately before
`self.sync_state.qrinfo_received()`, after all fallible QRInfo processing
succeeds. Add a regression test covering request-pair construction failure
followed by a retry for the same tip, verifying the retry is processed
successfully.
🪄 Autofix
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 Plus
Run ID: fe140a20-2aa5-4e54-9463-4f5a67c4a89b
📒 Files selected for processing (3)
dash-spv/src/sync/masternodes/manager.rsdash-spv/src/sync/masternodes/pipeline.rsdash-spv/src/sync/masternodes/sync_manager.rs
Comment on lines
+32
to
+43
| /// How long the manager may sit in `Syncing` with no QRInfo in flight and an empty | ||
| /// MnListDiff pipeline before `tick` re-dispatches a QRInfo. | ||
| /// | ||
| /// That combination is a dead state: every retry path in `tick` is armed solely by | ||
| /// `qrinfo_in_flight`, so once the slot is empty while `Syncing`, nothing fires | ||
| /// again and masternode sync is stranded for the life of the process - which also | ||
| /// strands InstantSend verification and every other masternode-list consumer. | ||
| /// | ||
| /// Chosen shorter than the full retry ladder (`sum(QRINFO_TIMEOUT_SCHEDULE_SECS) | ||
| /// = 100s`) because the two can never overlap: the ladder only runs while the slot | ||
| /// is occupied, the watchdog only while it is empty. | ||
| const QRINFO_STALL_WATCHDOG: Duration = Duration::from_secs(60); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Make the watchdog interval configurable.
Duration::from_secs(60) hardcodes a network recovery parameter. Source this value from synchronization or client configuration.
As per coding guidelines, **/*.rs: “Never hardcode network parameters, addresses, or keys”.
🤖 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/masternodes/sync_manager.rs` around lines 32 - 43, Make
QRINFO_STALL_WATCHDOG configurable by sourcing its duration from the existing
synchronization or client configuration instead of defining a fixed
Duration::from_secs(60). Update the SyncManager initialization and tick logic to
use the configured value while preserving the watchdog’s empty-slot Syncing
behavior.
Source: Coding guidelines
Comment on lines
+1047
to
+1293
| /// Same filler `QRInfo` as [`qrinfo_with_tip`], but carrying an explicit tip | ||
| /// hash so it can be aimed at whatever tip a live manager actually requested. | ||
| fn qrinfo_with_tip_hash(tip: BlockHash) -> QRInfo { | ||
| let mut qr_info = qrinfo_with_tip(0x00); | ||
| qr_info.mn_list_diff_tip.block_hash = tip; | ||
| qr_info | ||
| } | ||
|
|
||
| type TestMasternodesManager = MasternodesManager<PersistentBlockHeaderStorage>; | ||
|
|
||
| /// Build a regtest manager whose header storage holds dummy headers up to | ||
| /// `tip`, then fire the initial QRInfo so it sits in `Syncing` with a real | ||
| /// in-flight request, exactly as a fresh wallet does once headers catch up. | ||
| /// | ||
| /// Returns the manager, the `RequestSender`, the matching receiver (the caller | ||
| /// must bind it - the channel closes when it drops), and the tip hash the | ||
| /// in-flight request was made for. A response has to echo that hash to get | ||
| /// past `should_process_qrinfo`. The initial `GetQRInfo` is drained from the | ||
| /// receiver so callers only see what they trigger themselves. | ||
| async fn syncing_manager_awaiting_qrinfo( | ||
| tip: u32, | ||
| ) -> (TestMasternodesManager, RequestSender, mpsc::UnboundedReceiver<NetworkRequest>, BlockHash) | ||
| { | ||
| let storage = DiskStorageManager::with_temp_dir().await.unwrap(); | ||
| let block_headers = storage.block_headers(); | ||
| block_headers | ||
| .write() | ||
| .await | ||
| .store_headers( | ||
| &Header::dummy_batch(0..tip + 1) | ||
| .iter() | ||
| .map(HashedBlockHeader::from) | ||
| .collect::<Vec<_>>(), | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
| let engine = MasternodeListEngine::default_for_network(Network::Regtest); | ||
| let mut manager = | ||
| MasternodesManager::new(block_headers, Arc::new(RwLock::new(engine)), Network::Regtest) | ||
| .await; | ||
| manager.progress.update_block_header_tip_height(tip); | ||
|
|
||
| let (tx, mut rx) = mpsc::unbounded_channel(); | ||
| let requests = RequestSender::new(tx); | ||
| manager.send_qrinfo_for_tip(&requests).await.expect("initial QRInfo dispatch succeeds"); | ||
| assert_eq!(manager.state(), SyncState::Syncing); | ||
| let tip_hash = manager.sync_state.qrinfo_in_flight.expect("QRInfo in flight").tip; | ||
| rx.try_recv().expect("initial GetQRInfo is queued"); | ||
| (manager, requests, rx, tip_hash) | ||
| } | ||
|
|
||
| /// A QRInfo the engine rejects must not release the request slot. | ||
| /// | ||
| /// `qrinfo_in_flight` is the only thing that arms `tick`'s retry ladder, so | ||
| /// the pre-fix ordering - release the slot, *then* run the fallible engine | ||
| /// feed - left the manager `Syncing` with nothing in flight and an empty diff | ||
| /// pipeline on any validation failure. `tick` has no branch for that state, so | ||
| /// masternode sync never resumed: observed once on testnet and twice on | ||
| /// mainnet as a frozen `Masternodes: Syncing 0/N | diffs_processed: 0, | ||
| /// qr_infos_requested: 1` for as long as the process lived, with InstantSend | ||
| /// verification and every masternode-list consumer dead behind it. | ||
| /// | ||
| /// With the slot kept armed and the attempt flagged, `tick` retries on the | ||
| /// existing budget. Each dispatch goes out through `send_distributed`, whose | ||
| /// round-robin lands it on a different peer than the one that served the bad | ||
| /// response, and a deterministically-bad response still terminates - after | ||
| /// `MAX_RETRY_ATTEMPTS` dispatches, not silently and not never. | ||
| #[tokio::test] | ||
| async fn test_rejected_qrinfo_retries_against_another_peer_then_gives_up() { | ||
| let (mut manager, requests, mut rx, tip_hash) = syncing_manager_awaiting_qrinfo(200).await; | ||
| let peer = "127.0.0.1:19999".parse().unwrap(); | ||
| let bad_response = | ||
| || Message::new(peer, NetworkMessage::QRInfo(qrinfo_with_tip_hash(tip_hash))); | ||
|
|
||
| for attempt in 0..MAX_RETRY_ATTEMPTS - 1 { | ||
| let err = manager | ||
| .handle_message(bad_response(), &requests) | ||
| .await | ||
| .expect_err("the engine must reject this filler QRInfo"); | ||
| assert!( | ||
| matches!(err, SyncError::MasternodeSyncFailed(_)), | ||
| "expected MasternodeSyncFailed, got {:?}", | ||
| err | ||
| ); | ||
|
|
||
| let in_flight = manager | ||
| .sync_state | ||
| .qrinfo_in_flight | ||
| .expect("a rejected response must NOT release the request slot"); | ||
| assert!(in_flight.rejected, "the spent attempt must be flagged for retry"); | ||
| assert_eq!( | ||
| manager.sync_state.qrinfo_retry_count, attempt, | ||
| "the handler must not consume budget itself; `tick` owns the counter" | ||
| ); | ||
|
|
||
| manager.tick(&requests).await.expect("tick retries the rejected attempt"); | ||
| assert_eq!(manager.sync_state.qrinfo_retry_count, attempt + 1); | ||
| assert!( | ||
| matches!( | ||
| rx.try_recv().expect("tick must dispatch a retry GetQRInfo"), | ||
| NetworkRequest::SendMessage(NetworkMessage::GetQRInfo(_)) | ||
| ), | ||
| "the retry must be a GetQRInfo" | ||
| ); | ||
| assert!( | ||
| !manager.sync_state.qrinfo_in_flight.expect("slot re-armed").rejected, | ||
| "the retry must start as a clean attempt" | ||
| ); | ||
| } | ||
|
|
||
| // Budget spent. The last rejection must terminate rather than dispatch | ||
| // again, and must not park the manager in `Syncing` forever. | ||
| manager | ||
| .handle_message(bad_response(), &requests) | ||
| .await | ||
| .expect_err("the engine must reject this filler QRInfo"); | ||
| let _ = manager.tick(&requests).await; | ||
| assert!( | ||
| rx.try_recv().is_err(), | ||
| "the retry budget must stop dispatching after MAX_RETRY_ATTEMPTS" | ||
| ); | ||
| assert_eq!( | ||
| manager.progress.qr_infos_requested(), | ||
| MAX_RETRY_ATTEMPTS as u32, | ||
| "exactly MAX_RETRY_ATTEMPTS dispatches: the initial one plus its retries" | ||
| ); | ||
| assert_ne!( | ||
| manager.state(), | ||
| SyncState::Syncing, | ||
| "giving up must resolve the state, not leave it silently Syncing" | ||
| ); | ||
| } | ||
|
|
||
| /// The stall watchdog is the backstop for every route into the stranded state | ||
| /// that keeping the in-flight flag cannot cover - notably a | ||
| /// `send_qrinfo_for_tip` that fails *after* its caller already cleared the | ||
| /// slot, which is exactly what `tick`'s own retry path does. `Syncing` with | ||
| /// nothing in flight and an empty diff pipeline has no other exit. | ||
| #[tokio::test] | ||
| async fn test_tick_watchdog_redispatches_after_stall() { | ||
| let (mut manager, requests, mut rx, _) = syncing_manager_awaiting_qrinfo(200).await; | ||
|
|
||
| // Strand the manager the way a dispatch failure would. | ||
| manager.sync_state.clear_pending(); | ||
| assert_eq!(manager.state(), SyncState::Syncing); | ||
| assert!(manager.sync_state.qrinfo_in_flight.is_none()); | ||
| assert!(manager.sync_state.mnlistdiff_pipeline.is_complete()); | ||
|
|
||
| manager.tick(&requests).await.expect("tick succeeds"); | ||
| assert!( | ||
| rx.try_recv().is_err(), | ||
| "the watchdog must stay quiet until its interval has elapsed" | ||
| ); | ||
|
|
||
| manager.sync_state.last_qrinfo_dispatch = Some( | ||
| Instant::now() | ||
| .checked_sub(QRINFO_STALL_WATCHDOG + Duration::from_secs(1)) | ||
| .expect("test host uptime must exceed the watchdog interval"), | ||
| ); | ||
|
|
||
| manager.tick(&requests).await.expect("the watchdog re-dispatch succeeds"); | ||
| assert!( | ||
| matches!( | ||
| rx.try_recv().expect("the watchdog must re-dispatch a GetQRInfo"), | ||
| NetworkRequest::SendMessage(NetworkMessage::GetQRInfo(_)) | ||
| ), | ||
| "the watchdog must re-dispatch a GetQRInfo" | ||
| ); | ||
| assert!( | ||
| manager.sync_state.qrinfo_in_flight.is_some(), | ||
| "the re-dispatch must re-arm the request slot" | ||
| ); | ||
| assert_eq!(manager.sync_state.qrinfo_retry_count, 0, "recovery starts a fresh budget"); | ||
| } | ||
|
|
||
| /// A disconnect must not throw masternode work away. `BlocksManager` and | ||
| /// `FiltersManager` both requeue their in-flight network slots; the masternode | ||
| /// manager used to call `clear_pending()`, which dropped the queued | ||
| /// `GetMnListDiff`s, the QRInfo slot, and any `qr_info_result` carried on | ||
| /// `pipeline_mode`, forcing a full QRInfo re-run on every reconnect. | ||
| #[tokio::test] | ||
| async fn test_on_disconnect_requeues_instead_of_clearing() { | ||
| let (mut manager, requests, _rx, tip_hash) = syncing_manager_awaiting_qrinfo(200).await; | ||
|
|
||
| let base = BlockHash::from_slice(&[0x11; 32]).unwrap(); | ||
| let target = BlockHash::from_slice(&[0x22; 32]).unwrap(); | ||
| manager.sync_state.mnlistdiff_pipeline.queue_requests(vec![(base, target)]); | ||
| manager.sync_state.mnlistdiff_pipeline.send_pending(&requests).expect("send succeeds"); | ||
| assert_eq!(manager.sync_state.mnlistdiff_pipeline.active_count(), 1); | ||
|
|
||
| manager.on_disconnect(); | ||
|
|
||
| assert_eq!( | ||
| manager.sync_state.mnlistdiff_pipeline.active_count(), | ||
| 0, | ||
| "the dead peer's network slot must be released" | ||
| ); | ||
| assert!( | ||
| !manager.sync_state.mnlistdiff_pipeline.is_complete(), | ||
| "but the request itself must survive, back in the pending queue" | ||
| ); | ||
| assert_eq!( | ||
| manager.sync_state.qrinfo_in_flight.map(|in_flight| in_flight.tip), | ||
| Some(tip_hash), | ||
| "the QRInfo slot must stay armed so tick's ladder re-dispatches it" | ||
| ); | ||
| assert_eq!( | ||
| manager.sync_state.qrinfo_retry_count, 0, | ||
| "a fresh peer set earns a fresh retry budget" | ||
| ); | ||
| assert_eq!( | ||
| manager.sync_state.last_processed_qrinfo_tip, None, | ||
| "the dedup guard must not reject the reconnect's response" | ||
| ); | ||
| } | ||
|
|
||
| /// `tick` must reissue MnListDiff requests that a disconnect moved back to | ||
| /// pending. Nothing else can: every other `send_pending` call site hangs off a | ||
| /// response handler, and after a disconnect no response is coming. | ||
| #[tokio::test] | ||
| async fn test_tick_reissues_requeued_mnlistdiffs() { | ||
| let (mut manager, requests, mut rx, _) = syncing_manager_awaiting_qrinfo(200).await; | ||
| manager.sync_state.qrinfo_received(); | ||
|
|
||
| let base = BlockHash::from_slice(&[0x11; 32]).unwrap(); | ||
| let target = BlockHash::from_slice(&[0x22; 32]).unwrap(); | ||
| manager.sync_state.mnlistdiff_pipeline.queue_requests(vec![(base, target)]); | ||
| manager.sync_state.mnlistdiff_pipeline.send_pending(&requests).expect("send succeeds"); | ||
| while rx.try_recv().is_ok() {} | ||
|
|
||
| manager.on_disconnect(); | ||
| assert_eq!(manager.sync_state.mnlistdiff_pipeline.active_count(), 0); | ||
|
|
||
| manager.tick(&requests).await.expect("tick reissues the requeued request"); | ||
| assert!( | ||
| matches!( | ||
| rx.try_recv().expect("tick must reissue the GetMnListDiff"), | ||
| NetworkRequest::SendMessage(NetworkMessage::GetMnListD(_)) | ||
| ), | ||
| "the reissued request must be a GetMnListDiff" | ||
| ); | ||
| assert_eq!( | ||
| manager.sync_state.mnlistdiff_pipeline.active_count(), | ||
| 1, | ||
| "the reissued request must be tracked in flight again" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add an external integration test for the recovery flow.
These in-module tests validate internal state transitions. Add at least one recovery scenario under dash-spv/tests/ that drives the public synchronization flow across a rejected QRInfo or peer disconnect.
As per coding guidelines, **/*.rs: “Write unit tests for new functionality”. As per path instructions, dash-spv/**/{src,tests}/**/*.rs: “Implement comprehensive unit tests in-module for individual components using #[cfg(test)] and integration tests in the tests/ directory”.
🤖 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/masternodes/sync_manager.rs` around lines 1047 - 1293, Add
an external integration test under dash-spv/tests that exercises the public
synchronization API through a recovery scenario, such as a rejected QRInfo or
peer disconnect, and verifies that synchronization re-dispatches or resumes
successfully. Keep the existing in-module tests unchanged, and drive the
scenario through publicly accessible setup, message, and synchronization-flow
interfaces rather than inspecting private manager state.
Sources: Coding guidelines, Path instructions
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #936 +/- ##
==========================================
+ Coverage 75.18% 75.53% +0.34%
==========================================
Files 328 328
Lines 78194 79283 +1089
==========================================
+ Hits 58792 59884 +1092
+ Misses 19402 19399 -3
|
Contributor
Author
|
Additional field evidence, this time from a production device (v11.9.x wallet, dashj 22.0.5, mainnet, 5 days of continuous logs): the same rejection occurred organically 26 times across two days — plus a So the reject-then-retry scenario is not an edge case — it happens repeatedly in the field under normal mainnet conditions, and retrying is the established, proven recovery behavior in the reference client. This PR gives the SPV client that same behavior; without it the first such rejection permanently strands masternode sync (no retry is ever armed). |
…llible step Recording last_processed_qrinfo_tip before build_mnlistdiff_request_pairs turned the dedup gate against the retry ladder: a failure in that step left the request slot armed, but every retried response for the same tip was rejected at the handler entry by should_process_qrinfo, so the retry budget burned down with no way to succeed - reintroducing the permanent stall this branch exists to fix, through a different fallible step. Defer the record to the success path, next to qrinfo_received(). A straggler can only arrive after the handler returns, so the dedup gate loses nothing. Re-feeding the engine on such a retry is already accepted by design - the on_disconnect path clears the recorded tip for the same reason. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Closed in favor of the in-repo recreation: #947 (same commits and authorship; no more personal-fork PRs). |
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.
Summary
One rejected QRInfo response permanently strands masternode sync.
qrinfo_received()clearedqrinfo_in_flightbefore the falliblefeed_qr_info, so a validation failure returnedErrand left the manager inSyncingwithqrinfo_in_flight = Noneand an empty diff pipeline. TheQRINFO_TIMEOUT_SCHEDULE_SECS = [10, 30, 60]retry ladder is armed solely byqrinfo_in_flight, sotick()fell through forever. The error surfaced only as aSyncEvent::ManagerError, which has no consumer — the run loop logs it and continues.No watchdog existed. The comment at
masternodes/manager.rs:544already warned about stranding "Syncingwithqrinfo_in_flight = None, whichtickcannot recover", but only guarded the not-yet-Syncingentrance.Masternode sync gates overall
SYNCED(sync/progress.rs:79-97), so while stalled every InstantLock fails verification, DAPI has no masternode list, and platform features are dead until the process restarts. It is intermittent because a fresh wallet chains diffs from genesis (compute_qrinfo_anchor_hashreturnsNone), a much wider validation surface than an incremental catch-up.On-device evidence
Testnet, Galaxy S21, 2026-08-06, fresh wallet (
dash_spv/run.log):then frozen for ~40 minutes until restart:
Discriminating greps over the same log — these are what prove the timeout branch never ran, rather than the task dying or the log being lossy:
Timeout waiting for QRInfoMasternode task exitingLaggedMainnet, Galaxy S22, 2026-08-09, restored wallet — reproduced twice in one day, in two separate processes:
Frozen 34+ minutes. DAPI logged
total: 0calls over 33.73 minutes andmnlist=0while the chain sat at tip. User-visible symptoms: "unable to connect", and an invitation-creation confirm loop (asset-lock InstantSend verification is impossible with no quorums).The frozen
qr_infos_requested: 1is the direct signature of the bug: the counter is bumped bysend_qrinfo_for_tip, so a stuck1means no retry was ever dispatched.Changes
1. Release the request slot only after the last fallible step (
sync_manager.rs)qrinfo_received()moves from handler entry to just beforequeue_requests, afterfeed_qrinfo_heights_to_engine,feed_qr_infoandbuild_mnlistdiff_request_pairshave all succeeded. It still has to run before thehas_pending_requests()check that follows, which reads it.On the
feed_qr_infoerror branch the slot stays armed and the attempt is flaggedrejected.ticktreats that flag as an elapsed timeout, so the retry goes out on the next tick rather than waiting out a timeout the peer has already answered. Each dispatch goes throughsend_distributed, whose round-robin (network/manager.rs:1194-1201) lands it on a different peer than the one that served the bad response.last_processed_qrinfo_tipcontinues to be set only on success, so the retry's response is not dropped as a duplicate byshould_process_qrinfo.2. Stall watchdog in
tick(sync_manager.rs)Syncing+ no QRInfo in flight + empty diff pipeline for longer thanQRINFO_STALL_WATCHDOG(60s) re-dispatches a QRInfo. This is the backstop for the routes the in-flight flag cannot cover — notably asend_qrinfo_for_tipthat fails after its caller already cleared the slot, which is exactly whattick's own retry path does.Timed off a new
MasternodeSyncState::last_qrinfo_dispatch, stamped insidestart_waiting_for_qrinfoso it can never drift from the actual dispatch. Deliberately notprogress.last_activity(), which unrelated block-header events keep bumping (masternodes/progress.rs) and which would therefore reset the watchdog while masternode sync itself made no progress at all.The watchdog re-stamps before dispatching, so a dispatch that fails (no peers, empty header storage) re-arms it for another full interval instead of becoming a 10 Hz retry loop. It cannot race the retry ladder: the ladder only runs while the slot is occupied, the watchdog only while it is empty.
3.
on_disconnectrequeues instead of clearing (sync_manager.rs,manager.rs,pipeline.rs)clear_pending()→requeue_in_flight(), mirroringBlocksManager(sync/blocks/sync_manager.rs:61-63) andFiltersManager. In-flightGetMnListDiffs move back to the front of the pending queue with theirbase_hashesmapping intact; the QRInfo slot stays armed so the ladder re-dispatches it. The oldclear_pending()also discarded theqr_info_resultcarried onpipeline_mode, forcing a full QRInfo re-run after every disconnect. Theqrinfo_retry_countandlast_processed_qrinfo_tipresets are kept — a fresh peer set earns a fresh budget, and the dedup guard must not reject the reconnect's response.This required one supporting change to make the requeue actionable:
ticknow flushes the pending queue whenever the pipeline is non-empty, not only when something is in flight. That is the only path that can reissue requeued requests — every othersend_pendingcall site hangs off a response handler, which cannot run while nothing is outstanding. It is a no-op for every pre-existing path, sincequeue_requests,handle_timeoutsandrequeueare all already followed by asend_pending.Tests
Four new tests in
sync/masternodes/sync_manager.rs, on a realMasternodesManageroverDiskStorageManagerwith a boundRequestSenderreceiver:test_rejected_qrinfo_retries_against_another_peer_then_gives_up— drives a rejected QRInfo throughhandle_message+tickfor the whole budget. Asserts the slot is not released and is flaggedrejected, that the handler does not consume budget itself, that eachtickdispatches exactly one retryGetQRInfoand re-arms a clean attempt, and that the run terminates at exactlyMAX_RETRY_ATTEMPTSdispatches without parking the manager inSyncing. Verified to fail against the pre-fix ordering (panics on "a rejected response must NOT release the request slot").test_tick_watchdog_redispatches_after_stall— asserts the watchdog stays quiet inside its interval and re-dispatches with a fresh budget once it elapses.test_on_disconnect_requeues_instead_of_clearing— asserts the dead peer's slot is released but the request survives in pending, and the QRInfo slot stays armed at the right tip.test_tick_reissues_requeued_mnlistdiffs— assertstickactually puts a requeuedGetMnListDiffback on the wire.Out of scope
Noted while root-causing, intentionally not touched here, for a separate ticket:
sync/sync_manager.rs:281-285and:308-312treat a recoverableRecvError::Lagged(n)on the broadcast receivers as fatal andbreakout of the manager's run loop.Summary by CodeRabbit