fix(dash-spv): recover InstantSend locks received before quorum sync completes#895
Open
bfoss765 wants to merge 2 commits into
Open
fix(dash-spv): recover InstantSend locks received before quorum sync completes#895bfoss765 wants to merge 2 commits into
bfoss765 wants to merge 2 commits into
Conversation
…completes A wallet integration observed a shielded fund-from-asset-lock stall for ~3.5 minutes on testnet. The wallet broadcast an asset-lock transaction and the network produced an InstantSend lock for it within ~2 seconds, but a freshly (re)started dash-spv client never used the islock — it fell back to waiting for the transaction to be mined before the asset-lock proof flow could proceed. Root cause: an islock that arrives before quorum sync has loaded the quorum needed to verify its BLS signature is correctly queued in `InstantSendManager::pending_instantlocks`, but re-validation of that queue was edge-triggered exclusively by the `MasternodeStateUpdated` sync event (`instantsend/sync_manager.rs`). After the initial masternode sync completes that event only re-fires on a new MnListDiff/QRInfo — i.e. on a new block — so a lock queued just after a fresh start is not re-checked until the next block, which for a just-broadcast transaction is effectively the block that mines it. `tick` only pruned the cache and never retried validation. Fix: make re-validation level-triggered. `tick` now re-runs `validate_pending` as soon as the masternode engine advances past the height at which the queue was last checked (tracked by `last_validated_engine_height`), so a queued islock is verified the moment the required quorum data lands instead of waiting for the next event. Pending expiry is switched from an attempt counter to a time-based TTL, since tick-driven retries would otherwise exhaust the counter long before the hour-long window elapses during a fresh sync. The existing `MasternodeStateUpdated` path is retained and also updates the marker. Tests (deterministic, no dashd required): - test_tick_revalidates_pending_when_engine_advances - test_tick_skips_revalidation_without_engine_advance - test_pending_expires_after_ttl_on_revalidation - test_on_disconnect_resets_validation_marker Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe InstantSend manager replaces retry-count expiry with TTL-based pending-lock retention, gates revalidation on masternode engine-height advancement, performs expiry during every tick, resets unvalidated state on disconnect, and transitions to ChangesInstantLock validation lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant InstantSendSyncManager
participant SharedEngine
participant InstantSendManager
InstantSendSyncManager->>InstantSendManager: inspect pending InstantLocks
InstantSendSyncManager->>InstantSendManager: request engine_height()
InstantSendManager->>SharedEngine: read masternode-list height
SharedEngine-->>InstantSendManager: current engine height
InstantSendSyncManager->>InstantSendSyncManager: compare validation height
InstantSendSyncManager->>InstantSendManager: expire_pending()
InstantSendSyncManager->>InstantSendManager: validate_pending()
InstantSendManager-->>InstantSendSyncManager: update pending and sync state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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/instantsend/sync_manager.rs`:
- Around line 117-123: Move pending-lock TTL expiry cleanup out of the
engine-advancement-gated path in tick, ensuring it runs whenever
pending_instantlocks is non-empty even when the engine height is unchanged.
Reuse the existing expiry logic without rerunning BLS verification, then retain
validate_pending for advancement-triggered validation.
- Around line 124-125: Update the tick-driven path in the sync manager around
validate_pending so it applies the synced-state transition after validation
instead of returning immediately. Preserve the validation result and invoke the
same transition logic used by handle_sync_event, ensuring final-lock validation
or expiry can move the manager out of Syncing or WaitForEvents.
- Around line 28-32: Update `on_disconnect` to also clear the cached unvalidated
instant locks alongside `pending_instantlocks`. Ensure the cache used by
`process_instantlock` for duplicate detection is emptied so locks announced
after reconnect are queued and validated again.
🪄 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: ef042840-586f-44ba-a699-c1d66e1f12bc
📒 Files selected for processing (2)
dash-spv/src/sync/instantsend/manager.rsdash-spv/src/sync/instantsend/sync_manager.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #895 +/- ##
==========================================
+ Coverage 74.54% 74.62% +0.07%
==========================================
Files 327 327
Lines 75032 75196 +164
==========================================
+ Hits 55936 56116 +180
+ Misses 19096 19080 -16
|
xdustinface
requested changes
Jul 16, 2026
xdustinface
left a comment
xdustinface
left a comment
Collaborator
There was a problem hiding this comment.
I think two of the coderabbit comments should be addressed.
…ependent of engine advance, synced-state transition after tick validation - on_disconnect now drops unvalidated instantlock cache entries (via clear_unvalidated_on_disconnect) alongside the pending queue, so a lock re-announced after reconnect is no longer deduplicated against a stale cache entry and is re-queued/re-validated fresh. - tick runs a cheap, advancement-independent expire_pending pass on every tick, so pending locks past PENDING_TTL are dropped even when the engine height is static; full BLS re-validation stays gated on engine advance. - tick applies the synced-state transition after resolving pending work (via the shared transition_synced_if_idle helper, also used by handle_sync_event), so a final lock resolved by tick no longer leaves the manager stuck in Syncing/WaitForEvents. - tests: re-announce after disconnect is not a duplicate; pending expires by TTL under a static engine height; tick resolving the last pending lock transitions out of Syncing. Existing tick tests updated for the new expiry/validation split. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Addressed all three review findings in
🤖 Generated with Claude Code |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dash-spv/src/sync/instantsend/manager.rs (1)
46-53: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse a monotonic clock for pending TTL
SystemTime::elapsed()can returnErrafter a backward clock change, which leaves the pending lock in the queue until wall time catches up. Storefirst_seenasInstantinstead, and update the test fixtures accordingly.🤖 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/instantsend/manager.rs` around lines 46 - 53, Change PendingInstantLock::first_seen from SystemTime to Instant and initialize it with the monotonic clock when the lock is received. Update the pending-lock expiration logic to use Instant::elapsed(), and adjust all related test fixtures and imports to construct Instant timestamps instead of SystemTime values.
🤖 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.
Outside diff comments:
In `@dash-spv/src/sync/instantsend/manager.rs`:
- Around line 46-53: Change PendingInstantLock::first_seen from SystemTime to
Instant and initialize it with the monotonic clock when the lock is received.
Update the pending-lock expiration logic to use Instant::elapsed(), and adjust
all related test fixtures and imports to construct Instant timestamps instead of
SystemTime values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ccc4157-95ab-40a3-b2ca-6416d25085a0
📒 Files selected for processing (2)
dash-spv/src/sync/instantsend/manager.rsdash-spv/src/sync/instantsend/sync_manager.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- dash-spv/src/sync/instantsend/sync_manager.rs
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.
Problem
A wallet integration observed a shielded fund-from-asset-lock stall for ~3.5 minutes on a freshly (re)started SPV client. The asset-lock transaction was broadcast through
dash-spv; the network's InstantSend lock for it appeared on the P2P network within ~2 seconds (a parallel engine on the same host received and verified it immediately), but this client — started ~21 seconds earlier — never used the islock. The asset-lock proof flow fell back to waiting for the transaction to be mined before it could proceed.Root cause
The islock is received and correctly queued into
pending_instantlockswhen its BLS signature can't yet be verified (quorum sync hasn't loaded the signing quorum) — it is not dropped. The defect is the re-validation trigger:MasternodeStateUpdatedevent.tickonly pruned the cache; it never retried validation.So an islock queued moments after a fresh start isn't re-checked until the next block — which, for a just-broadcast transaction, is effectively the block that mines it.
Fix
Make re-validation level-triggered:
last_validated_engine_heightand re-runvalidate_pendingfromtickwhenever the engine's masternode-list height has advanced past it (and pending locks exist), so a queued islock is verified as soon as the required quorum data lands — independent of whether aMasternodeStateUpdatedevent reaches the manager.on_disconnectresets the marker; the existingMasternodeStateUpdatedpath is retained and also updates the marker.Contained to
dash-spv/src/sync/instantsend/{manager.rs,sync_manager.rs}.Tests
test_tick_revalidates_pending_when_engine_advances(core red→green: pre-fixticknever retried)test_tick_skips_revalidation_without_engine_advance(throttle — no redundant BLS work per tick)test_pending_expires_after_ttl_on_revalidationtest_on_disconnect_resets_validation_markercargo test -p dash-spvgreen (483 lib tests incl. the 4 new);cargo clippy -p dash-spv --all-targetsclean;cargo fmt --checkclean.🤖 Generated with Claude Code
Summary by CodeRabbit