fix(dash-spv): recover masternode sync from a rejected QRInfo

[bfoss765]

Supersedes #936 — same change, recreated on a dashpay/rust-dashcore branch per repo policy (no more personal-fork PRs). Commits and authorship unchanged; full review history on #936.

Summary

One rejected QRInfo response permanently strands masternode sync. qrinfo_received() cleared qrinfo_in_flight before the fallible feed_qr_info, so a validation failure returned Err and left the manager in Syncing with qrinfo_in_flight = None and an empty diff pipeline. The QRINFO_TIMEOUT_SCHEDULE_SECS = [10, 30, 60] retry ladder is armed solely by qrinfo_in_flight, so tick() fell through forever. The error surfaced only as a SyncEvent::ManagerError, which has no consumer — the run loop logs it and continues.

No watchdog existed. The comment at masternodes/manager.rs:544 already warned about stranding "Syncing with qrinfo_in_flight = None, which tick cannot recover", but only guarded the not-yet-Syncing entrance.

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_hash returns None), 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):

ERROR dash_spv::sync::masternodes::sync_manager: QRInfo feed into engine failed: All commitment aggregated signature not valid: invalid signature

then frozen for ~40 minutes until restart:

Masternodes: Syncing 0/1529065 | diffs_processed: 0, qr_infos_requested: 1

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:

grep	hits
Timeout waiting for QRInfo	0
Masternode task exiting	0
Lagged	0

Mainnet, Galaxy S22, 2026-08-09, restored wallet — reproduced twice in one day, in two separate processes:

2026-08-09T18:32:23.668969Z ERROR dash_spv::sync::masternodes::sync_manager: QRInfo feed into engine failed: All commitment aggregated signature not valid: invalid signature
Masternodes: Syncing 0/2519119 | diffs_processed: 0, qr_infos_requested: 1

Frozen 34+ minutes. DAPI logged total: 0 calls over 33.73 minutes and mnlist=0 while 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: 1 is the direct signature of the bug: the counter is bumped by send_qrinfo_for_tip, so a stuck 1 means 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 before queue_requests, after feed_qrinfo_heights_to_engine, feed_qr_info and build_mnlistdiff_request_pairs have all succeeded. It still has to run before the has_pending_requests() check that follows, which reads it.

On the feed_qr_info error branch the slot stays armed and the attempt is flagged rejected. tick treats 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 through send_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_tip continues to be set only on success, so the retry's response is not dropped as a duplicate by should_process_qrinfo.

One deviation from the agreed design, called out deliberately. The design said to increment qrinfo_retry_count on the error branch. Doing that double-counts: the tick timeout branch increments as well, so each rejection would burn two budget slots and the run would terminate after 2 dispatches, not the intended 3. Flagging the attempt and letting tick own the counter keeps the increment at exactly one per attempt, gives the full MAX_RETRY_ATTEMPTS = 3 dispatches, and reuses the tick branch verbatim instead of duplicating its dispatch and give-up logic.

2. Stall watchdog in tick (sync_manager.rs)

Syncing + no QRInfo in flight + empty diff pipeline for longer than QRINFO_STALL_WATCHDOG (60s) re-dispatches a QRInfo. This is the backstop for the routes 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.

Timed off a new MasternodeSyncState::last_qrinfo_dispatch, stamped inside start_waiting_for_qrinfo so it can never drift from the actual dispatch. Deliberately not progress.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_disconnect requeues instead of clearing (sync_manager.rs, manager.rs, pipeline.rs)

clear_pending() → requeue_in_flight(), mirroring BlocksManager (sync/blocks/sync_manager.rs:61-63) and FiltersManager. In-flight GetMnListDiffs move back to the front of the pending queue with their base_hashes mapping intact; the QRInfo slot stays armed so the ladder re-dispatches it. The old clear_pending() also discarded the qr_info_result carried on pipeline_mode, forcing a full QRInfo re-run after every disconnect. The qrinfo_retry_count and last_processed_qrinfo_tip resets 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: tick now 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 other send_pending call site hangs off a response handler, which cannot run while nothing is outstanding. It is a no-op for every pre-existing path, since queue_requests, handle_timeouts and requeue are all already followed by a send_pending.

Tests

Four new tests in sync/masternodes/sync_manager.rs, on a real MasternodesManager over DiskStorageManager with a bound RequestSender receiver:

• test_rejected_qrinfo_retries_against_another_peer_then_gives_up — drives a rejected QRInfo through handle_message + tick for the whole budget. Asserts the slot is not released and is flagged rejected, that the handler does not consume budget itself, that each tick dispatches exactly one retry GetQRInfo and re-arms a clean attempt, and that the run terminates at exactly MAX_RETRY_ATTEMPTS dispatches without parking the manager in Syncing. 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 — asserts tick actually puts a requeued GetMnListDiff back on the wire.

cargo test -p dash-spv (SKIP_DASHD_TESTS=1)   588 passed, 0 failed
cargo clippy -p dash-spv --all-targets -- -D warnings   clean
cargo fmt -p dash-spv   clean

Out of scope

Noted while root-causing, intentionally not touched here, for a separate ticket: sync/sync_manager.rs:281-285 and :308-312 treat a recoverable RecvError::Lagged(n) on the broadcast receivers as fatal and break out of the manager's run loop.

Summary by CodeRabbit

• Bug Fixes
  • Improved masternode synchronization reliability when peers reject requests or disconnect.
  • Automatically retries rejected synchronization requests.
  • Preserves pending work and retries it with another peer instead of discarding it.
  • Added recovery for stalled synchronization, including automatic redispatch after extended inactivity.
  • Improved request completion handling to prevent work from being released before processing finishes.
• Tests
  • Added coverage for rejection retries, disconnect recovery, stalled-sync recovery, and request redispatching.

[coderabbitai]

Warning

Review limit reached

@bfoss765, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 025ea464-369a-4c8e-9d88-4357139eb320

📥 Commits

Reviewing files that changed from the base of the PR and between 5a80bd7 and f4570c0.

📒 Files selected for processing (3)

• dash-spv/src/sync/masternodes/manager.rs
• dash-spv/src/sync/masternodes/pipeline.rs
• dash-spv/src/sync/masternodes/sync_manager.rs

---

_{Comment @coderabbitai help to get the list of available commands.}