From 394e3721359fb6137bc18a0d933394eb63a0fcb1 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Wed, 15 Jul 2026 08:08:42 -0400 Subject: [PATCH 1/2] fix(dash-spv): recover InstantSend locks received before quorum sync completes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- dash-spv/src/sync/instantsend/manager.rs | 199 ++++++++++++++++-- dash-spv/src/sync/instantsend/sync_manager.rs | 23 ++ 2 files changed, 206 insertions(+), 16 deletions(-) diff --git a/dash-spv/src/sync/instantsend/manager.rs b/dash-spv/src/sync/instantsend/manager.rs index eb0ed5509..31c2a80e2 100644 --- a/dash-spv/src/sync/instantsend/manager.rs +++ b/dash-spv/src/sync/instantsend/manager.rs @@ -25,8 +25,12 @@ const MAX_CACHE_SIZE: usize = 5000; /// TTL for cached InstantLocks (1 hour). const CACHE_TTL: Duration = Duration::from_secs(3600); -/// Maximum retry attempts before dropping a pending InstantLock (~1 hour at 2.5min blocks). -const MAX_RETRIES: u32 = 24; +/// How long a pending InstantLock is retained while awaiting the quorum data +/// needed to verify it. Locks that stay unverifiable for longer than this are +/// dropped. Expiry is time-based (not attempt-based) because re-validation can +/// now be driven from `tick` as the engine advances, so an attempt counter +/// would overrun long before an hour elapsed during a fresh sync. +const PENDING_TTL: Duration = Duration::from_secs(3600); /// Entry in the InstantLock cache. #[derive(Debug, Clone)] @@ -39,13 +43,14 @@ pub struct InstantLockEntry { pub validated: bool, } -/// Pending InstantLock awaiting validation with retry tracking. +/// Pending InstantLock awaiting the quorum data required to verify it. #[derive(Debug, Clone)] pub(super) struct PendingInstantLock { /// The InstantLock data. instant_lock: InstantLock, - /// Number of validation retry attempts. - retry_count: u32, + /// When the lock was first received, used to expire locks that never + /// become verifiable. + first_seen: SystemTime, } /// InstantSend manager. @@ -62,8 +67,17 @@ pub struct InstantSendManager { engine: Arc>, /// InstantLocks indexed by txid. instantlocks: HashMap, - /// Pending InstantLocks awaiting validation with retry tracking. + /// Pending InstantLocks awaiting the quorum data required to verify them. pub(super) pending_instantlocks: Vec, + /// Highest masternode-list height at which every pending InstantLock has + /// already been (re)validated. + /// + /// `tick` uses this to re-run validation only once the engine has advanced + /// past this height, so a lock received before quorum sync completed is + /// re-checked as soon as new quorum data lands — instead of waiting for the + /// next `MasternodeStateUpdated` event, which for a freshly broadcast + /// transaction may not arrive until the block that mines it. + pub(super) last_validated_engine_height: Option, } impl InstantSendManager { @@ -74,6 +88,7 @@ impl InstantSendManager { engine, instantlocks: HashMap::new(), pending_instantlocks: Vec::new(), + last_validated_engine_height: None, } } @@ -107,7 +122,7 @@ impl InstantSendManager { } else { self.queue_pending(PendingInstantLock { instant_lock: instantlock.clone(), - retry_count: 0, + first_seen: SystemTime::now(), }); self.progress.update_pending(self.pending_instantlocks.len()); } @@ -201,21 +216,36 @@ impl InstantSendManager { } } - /// Validate pending InstantLocks after masternode engine becomes available. + /// Latest masternode-list height known to the shared engine, if any. + pub(super) async fn engine_height(&self) -> Option { + self.engine.read().await.masternode_lists.keys().next_back().copied() + } + + /// Re-validate every pending InstantLock against the current engine state. + /// + /// Called both when a `MasternodeStateUpdated` event arrives and from `tick` + /// once the engine has advanced. Locks that verify are emitted as validated + /// `InstantLockReceived` events; locks that still can't be verified are + /// re-queued until they either become verifiable or exceed [`PENDING_TTL`]. pub(super) async fn validate_pending(&mut self) -> SyncResult> { + // Snapshot the height we are validating against before doing the work so + // `tick` knows not to re-run until the engine advances past it. + let engine_height = self.engine_height().await; + let pending = std::mem::take(&mut self.pending_instantlocks); let mut events = Vec::new(); - for mut pending_lock in pending { - pending_lock.retry_count += 1; + for pending_lock in pending { let txid = pending_lock.instant_lock.txid; - // Check if max retries exceeded - if pending_lock.retry_count > MAX_RETRIES { + // Drop locks that have been awaiting quorum data for too long. + let expired = + pending_lock.first_seen.elapsed().map(|age| age > PENDING_TTL).unwrap_or(false); + if expired { tracing::warn!( - "Dropping InstantLock for txid {} after {} retries", + "Dropping InstantLock for txid {} after awaiting quorum data for over {}s", txid, - pending_lock.retry_count + PENDING_TTL.as_secs() ); self.progress.add_invalid(1); continue; @@ -239,6 +269,7 @@ impl InstantSendManager { } } + self.last_validated_engine_height = engine_height; self.progress.update_pending(self.pending_instantlocks.len()); Ok(events) } @@ -284,12 +315,38 @@ impl std::fmt::Debug for InstantSendManager { #[cfg(test)] mod tests { use super::*; - use crate::network::MessageType; + use crate::network::{MessageType, RequestSender}; use crate::sync::{ManagerIdentifier, SyncManager, SyncManagerProgress, SyncState}; use dashcore::bls_sig_utils::BLSSignature; use dashcore::hash_types::CycleHash; use dashcore::hashes::Hash; - use dashcore::OutPoint; + use dashcore::sml::masternode_list::MasternodeList; + use dashcore::{BlockHash, OutPoint}; + use tokio::sync::mpsc::unbounded_channel; + + /// Insert an empty masternode list at `height` so the shared engine reports a + /// new tip height. Empty lists carry no rotated quorums, so InstantLock + /// verification still fails — which is exactly what we want when exercising + /// the "engine advanced but the needed quorum is still absent" path. + async fn advance_engine_height(manager: &InstantSendManager, height: u32) { + let mut engine = manager.engine.write().await; + engine.masternode_lists.insert( + height, + MasternodeList::empty(BlockHash::from_byte_array([height as u8; 32]), height), + ); + } + + fn no_op_requests() -> RequestSender { + let (tx, _rx) = unbounded_channel(); + RequestSender::new(tx) + } + + fn expired_pending(txid: Txid) -> PendingInstantLock { + PendingInstantLock { + instant_lock: create_test_instantlock(txid), + first_seen: SystemTime::now() - Duration::from_secs(PENDING_TTL.as_secs() + 60), + } + } fn create_test_instantlock(txid: Txid) -> InstantLock { InstantLock { @@ -456,4 +513,114 @@ mod tests { // Should be capped at MAX_CACHE_SIZE assert!(manager.cached_count() <= MAX_CACHE_SIZE); } + + /// A queued InstantLock must be re-validated by `tick` as soon as the + /// masternode engine advances — not only when a `MasternodeStateUpdated` + /// event happens to arrive. This is the regression behind the on-device + /// incident: an islock received right after a fresh SPV start stayed pending + /// until the transaction was mined because nothing re-checked it. + #[tokio::test] + async fn test_tick_revalidates_pending_when_engine_advances() { + let mut manager = create_test_manager(); + let requests = no_op_requests(); + + // A lock arrives before quorum sync: the empty engine can't verify it, + // so it is queued. + let txid = Txid::from_byte_array([3u8; 32]); + let _ = manager.process_instantlock(&create_test_instantlock(txid)).await.unwrap(); + assert_eq!(manager.pending_count(), 1); + assert_eq!(manager.last_validated_engine_height, None); + + // Engine has not advanced yet (still empty): tick must not re-validate, + // and the pending-validation marker stays untouched. + let events = manager.tick(&requests).await.unwrap(); + assert!(events.is_empty()); + assert_eq!(manager.pending_count(), 1); + assert_eq!(manager.last_validated_engine_height, None); + + // The engine gains a new masternode-list height: tick now re-validates. + // The needed rotated quorum is still absent, so the lock is re-queued, + // but the marker records the height we validated against. + advance_engine_height(&manager, 200).await; + let events = manager.tick(&requests).await.unwrap(); + assert!(events.is_empty()); + assert_eq!(manager.pending_count(), 1); + assert_eq!(manager.last_validated_engine_height, Some(200)); + + // No further advance: tick must not spend work re-validating again. + let events = manager.tick(&requests).await.unwrap(); + assert!(events.is_empty()); + assert_eq!(manager.last_validated_engine_height, Some(200)); + + // Another advance re-arms re-validation. + advance_engine_height(&manager, 201).await; + let _ = manager.tick(&requests).await.unwrap(); + assert_eq!(manager.last_validated_engine_height, Some(201)); + } + + /// Without an engine advance, `tick` must leave pending locks alone — it must + /// not run (potentially expensive) BLS verification on every 100ms tick. + #[tokio::test] + async fn test_tick_skips_revalidation_without_engine_advance() { + let mut manager = create_test_manager(); + let requests = no_op_requests(); + + // Seed the engine and record that we already validated at that height. + advance_engine_height(&manager, 100).await; + manager.last_validated_engine_height = Some(100); + + // Queue a lock that is already past its TTL. If tick re-validated here it + // would expire and drop the lock — so surviving proves tick did not run + // validation while the engine height was unchanged. + manager.pending_instantlocks.push(expired_pending(Txid::from_byte_array([7u8; 32]))); + + let events = manager.tick(&requests).await.unwrap(); + assert!(events.is_empty()); + assert_eq!(manager.pending_count(), 1); + if let SyncManagerProgress::InstantSend(progress) = manager.progress() { + assert_eq!(progress.invalid(), 0); + } else { + panic!("Expected SyncManagerProgress::InstantSend"); + } + } + + /// A pending lock that never becomes verifiable is dropped once it exceeds + /// [`PENDING_TTL`], the next time re-validation runs after an engine advance. + #[tokio::test] + async fn test_pending_expires_after_ttl_on_revalidation() { + let mut manager = create_test_manager(); + let requests = no_op_requests(); + + manager.pending_instantlocks.push(expired_pending(Txid::from_byte_array([9u8; 32]))); + + // Advancing the engine triggers re-validation; the lock is past its TTL + // and still unverifiable, so it is dropped and counted invalid. + advance_engine_height(&manager, 300).await; + let events = manager.tick(&requests).await.unwrap(); + + assert!(events.is_empty()); + assert_eq!(manager.pending_count(), 0); + assert_eq!(manager.last_validated_engine_height, Some(300)); + if let SyncManagerProgress::InstantSend(progress) = manager.progress() { + assert_eq!(progress.invalid(), 1); + } else { + panic!("Expected SyncManagerProgress::InstantSend"); + } + } + + /// `on_disconnect` clears queued locks and resets the re-validation marker so + /// a lock re-received after reconnect is validated fresh. + #[tokio::test] + async fn test_on_disconnect_resets_validation_marker() { + let mut manager = create_test_manager(); + + advance_engine_height(&manager, 100).await; + manager.last_validated_engine_height = Some(100); + manager.pending_instantlocks.push(expired_pending(Txid::from_byte_array([1u8; 32]))); + + manager.on_disconnect(); + + assert_eq!(manager.pending_count(), 0); + assert_eq!(manager.last_validated_engine_height, None); + } } diff --git a/dash-spv/src/sync/instantsend/sync_manager.rs b/dash-spv/src/sync/instantsend/sync_manager.rs index e9ee4e96e..95079e670 100644 --- a/dash-spv/src/sync/instantsend/sync_manager.rs +++ b/dash-spv/src/sync/instantsend/sync_manager.rs @@ -27,6 +27,9 @@ impl SyncManager for InstantSendManager { fn on_disconnect(&mut self) { self.pending_instantlocks.clear(); + // Nothing is queued anymore, so the height at which we last validated is + // meaningless; reset it so re-validation runs again after reconnect. + self.last_validated_engine_height = None; } async fn handle_message( @@ -103,6 +106,26 @@ impl SyncManager for InstantSendManager { async fn tick(&mut self, _requests: &RequestSender) -> SyncResult> { // Prune old entries periodically self.prune_old_entries(); + + // Re-validate queued InstantLocks once the masternode engine has advanced + // past the height at which they were last checked. A lock received before + // quorum sync completed (e.g. right after a fresh SPV start, for a + // transaction the wallet just broadcast) is then verified as soon as the + // needed quorum data lands, rather than waiting for the next + // `MasternodeStateUpdated` event — which, for a just-broadcast + // transaction, may not arrive until the block that mines it. + if !self.pending_instantlocks.is_empty() { + let engine_height = self.engine_height().await; + let advanced = match (engine_height, self.last_validated_engine_height) { + (Some(current), Some(last)) => current > last, + (Some(_), None) => true, + (None, _) => false, + }; + if advanced { + return self.validate_pending().await; + } + } + Ok(vec![]) } From c0f0da9b5330882c9c5b946e4b4920f98d5a6ec6 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Thu, 16 Jul 2026 06:54:28 -0400 Subject: [PATCH 2/2] address review: clear unvalidated cache on disconnect, TTL expiry independent 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 --- dash-spv/src/sync/instantsend/manager.rs | 180 ++++++++++++++++-- dash-spv/src/sync/instantsend/sync_manager.rs | 39 +++- 2 files changed, 195 insertions(+), 24 deletions(-) diff --git a/dash-spv/src/sync/instantsend/manager.rs b/dash-spv/src/sync/instantsend/manager.rs index 31c2a80e2..ce25da675 100644 --- a/dash-spv/src/sync/instantsend/manager.rs +++ b/dash-spv/src/sync/instantsend/manager.rs @@ -14,7 +14,7 @@ use dashcore::Txid; use tokio::sync::RwLock; use crate::error::SyncResult; -use crate::sync::{InstantSendProgress, SyncEvent}; +use crate::sync::{InstantSendProgress, SyncEvent, SyncState}; /// Maximum number of pending InstantLocks awaiting validation. const MAX_PENDING_INSTANTLOCKS: usize = 500; @@ -282,6 +282,66 @@ impl InstantSendManager { }); } + /// Drop cached InstantLock entries that were never BLS-validated, and clear + /// the queued locks that fed them. + /// + /// Called on disconnect. The `instantlocks` cache is what + /// [`process_instantlock`](Self::process_instantlock) dedupes against, so if + /// an unvalidated entry survived a disconnect the same lock re-announced + /// after reconnect would be treated as a duplicate and never re-queued or + /// re-validated. Validated entries are kept — their locks are already final. + pub(super) fn clear_unvalidated_on_disconnect(&mut self) { + self.pending_instantlocks.clear(); + self.instantlocks.retain(|_, entry| entry.validated); + self.progress.update_pending(0); + } + + /// Drop pending locks that have outlived [`PENDING_TTL`] without running BLS + /// verification. + /// + /// This is the cheap, advancement-independent half of expiry: `tick` calls + /// it on every tick so a lock that never becomes verifiable is dropped after + /// an hour even when the masternode engine height is static. Full + /// re-validation (which does run BLS) stays in + /// [`validate_pending`](Self::validate_pending) and only runs when the engine + /// advances. Returns the number of locks expired. + pub(super) fn expire_pending(&mut self) -> usize { + let before = self.pending_instantlocks.len(); + self.pending_instantlocks.retain(|pending| { + let expired = + pending.first_seen.elapsed().map(|age| age > PENDING_TTL).unwrap_or(false); + if expired { + tracing::warn!( + "Dropping InstantLock for txid {} after awaiting quorum data for over {}s", + pending.instant_lock.txid, + PENDING_TTL.as_secs() + ); + } + !expired + }); + let expired = before - self.pending_instantlocks.len(); + if expired > 0 { + self.progress.add_invalid(expired as u32); + self.progress.update_pending(self.pending_instantlocks.len()); + } + expired + } + + /// Transition to [`SyncState::Synced`] once no pending validations remain. + /// + /// Shared by the event-driven (`handle_sync_event`) and tick-driven + /// revalidation paths so both resolve the manager's state the same way when + /// the last pending lock is validated or expired. No-op unless the manager + /// is currently `Syncing` or `WaitForEvents`. + pub(super) fn transition_synced_if_idle(&mut self) { + if self.pending_instantlocks.is_empty() + && matches!(self.progress.state(), SyncState::Syncing | SyncState::WaitForEvents) + { + self.progress.set_state(SyncState::Synced); + tracing::info!("InstantSend manager synced (no pending validations)"); + } + } + /// Get an InstantLock by transaction ID. pub fn get_instantlock(&self, txid: &Txid) -> Option<&InstantLockEntry> { self.instantlocks.get(txid) @@ -348,6 +408,13 @@ mod tests { } } + fn fresh_pending(txid: Txid) -> PendingInstantLock { + PendingInstantLock { + instant_lock: create_test_instantlock(txid), + first_seen: SystemTime::now(), + } + } + fn create_test_instantlock(txid: Txid) -> InstantLock { InstantLock { version: 1, @@ -558,25 +625,28 @@ mod tests { assert_eq!(manager.last_validated_engine_height, Some(201)); } - /// Without an engine advance, `tick` must leave pending locks alone — it must - /// not run (potentially expensive) BLS verification on every 100ms tick. + /// Without an engine advance, `tick` must not run (potentially expensive) BLS + /// re-validation. A *fresh* pending lock (well within its TTL) must survive + /// the tick untouched: the advancement-independent expiry pass leaves it in + /// place and validation never runs. #[tokio::test] async fn test_tick_skips_revalidation_without_engine_advance() { let mut manager = create_test_manager(); let requests = no_op_requests(); - // Seed the engine and record that we already validated at that height. + // Engine at height 100, but the marker is deliberately set ABOVE it so + // the advancement gate (current > last) stays closed. If `validate_pending` + // ran it would overwrite the marker down to 100 — so an unchanged marker + // proves it did not run. advance_engine_height(&manager, 100).await; - manager.last_validated_engine_height = Some(100); + manager.last_validated_engine_height = Some(150); - // Queue a lock that is already past its TTL. If tick re-validated here it - // would expire and drop the lock — so surviving proves tick did not run - // validation while the engine height was unchanged. - manager.pending_instantlocks.push(expired_pending(Txid::from_byte_array([7u8; 32]))); + manager.pending_instantlocks.push(fresh_pending(Txid::from_byte_array([7u8; 32]))); let events = manager.tick(&requests).await.unwrap(); assert!(events.is_empty()); assert_eq!(manager.pending_count(), 1); + assert_eq!(manager.last_validated_engine_height, Some(150)); if let SyncManagerProgress::InstantSend(progress) = manager.progress() { assert_eq!(progress.invalid(), 0); } else { @@ -584,8 +654,88 @@ mod tests { } } - /// A pending lock that never becomes verifiable is dropped once it exceeds - /// [`PENDING_TTL`], the next time re-validation runs after an engine advance. + /// TTL expiry must be independent of engine advancement (finding: expire + /// pending locks even when the engine height is static). A lock past its TTL + /// is dropped by `tick` without the engine ever advancing and without running + /// BLS re-validation. + #[tokio::test] + async fn test_tick_expires_pending_without_engine_advance() { + let mut manager = create_test_manager(); + let requests = no_op_requests(); + + // Close the advancement gate: engine at 100, already validated at 100. + // `validate_pending` therefore cannot run this tick. + advance_engine_height(&manager, 100).await; + manager.last_validated_engine_height = Some(100); + + manager.pending_instantlocks.push(expired_pending(Txid::from_byte_array([8u8; 32]))); + + let events = manager.tick(&requests).await.unwrap(); + + assert!(events.is_empty()); + // Expired purely by the advancement-independent pass. + assert_eq!(manager.pending_count(), 0); + // Marker untouched: only cheap expiry ran, not validation. + assert_eq!(manager.last_validated_engine_height, Some(100)); + if let SyncManagerProgress::InstantSend(progress) = manager.progress() { + assert_eq!(progress.invalid(), 1); + } else { + panic!("Expected SyncManagerProgress::InstantSend"); + } + } + + /// When `tick` resolves the final pending lock — here by expiring it under a + /// static engine height — the manager must leave `Syncing`, exactly as it + /// would had the resolution come from a `MasternodeStateUpdated` event via + /// `handle_sync_event`. Otherwise it stays stuck in `Syncing` indefinitely. + #[tokio::test] + async fn test_tick_transitions_synced_when_last_pending_resolved() { + let mut manager = create_test_manager(); + let requests = no_op_requests(); + + manager.set_state(SyncState::Syncing); + manager.pending_instantlocks.push(expired_pending(Txid::from_byte_array([4u8; 32]))); + + let events = manager.tick(&requests).await.unwrap(); + + assert!(events.is_empty()); + assert_eq!(manager.pending_count(), 0); + assert_eq!(manager.state(), SyncState::Synced); + } + + /// A lock re-announced with the same txid after a disconnect must be + /// processed fresh, not silently dropped as a duplicate. `on_disconnect` has + /// to evict the unvalidated cache entry that `process_instantlock` dedupes + /// against, otherwise the re-announced lock is never re-queued/re-validated. + #[tokio::test] + async fn test_reannounce_after_disconnect_is_not_duplicate() { + let mut manager = create_test_manager(); + + let txid = Txid::from_byte_array([5u8; 32]); + + // First announcement: the empty engine can't verify it, so it is queued + // and cached as an unvalidated entry. + let events = manager.process_instantlock(&create_test_instantlock(txid)).await.unwrap(); + assert_eq!(events.len(), 1); + assert_eq!(manager.pending_count(), 1); + assert!(manager.get_instantlock(&txid).is_some()); + + // Disconnect must clear the queue AND the unvalidated cache entry. + manager.on_disconnect(); + assert_eq!(manager.pending_count(), 0); + assert!(manager.get_instantlock(&txid).is_none()); + + // Re-announcement of the same txid is processed fresh, not deduped away. + let events = manager.process_instantlock(&create_test_instantlock(txid)).await.unwrap(); + assert_eq!(events.len(), 1); + assert_eq!(manager.pending_count(), 1); + } + + /// A pending lock past its [`PENDING_TTL`] is dropped and counted invalid, + /// even when the engine has also advanced. Expiry is handled by the cheap + /// advancement-independent pass, which drains the queue before the + /// validation gate — so `validate_pending` never runs and the marker is left + /// untouched (it is meaningless with no pending locks anyway). #[tokio::test] async fn test_pending_expires_after_ttl_on_revalidation() { let mut manager = create_test_manager(); @@ -593,14 +743,16 @@ mod tests { manager.pending_instantlocks.push(expired_pending(Txid::from_byte_array([9u8; 32]))); - // Advancing the engine triggers re-validation; the lock is past its TTL - // and still unverifiable, so it is dropped and counted invalid. + // The lock is past its TTL and still unverifiable, so it is dropped and + // counted invalid regardless of the engine advancing to height 300. advance_engine_height(&manager, 300).await; let events = manager.tick(&requests).await.unwrap(); assert!(events.is_empty()); assert_eq!(manager.pending_count(), 0); - assert_eq!(manager.last_validated_engine_height, Some(300)); + // Expiry drained the queue before the validation gate, so the marker was + // never set by a validation pass. + assert_eq!(manager.last_validated_engine_height, None); if let SyncManagerProgress::InstantSend(progress) = manager.progress() { assert_eq!(progress.invalid(), 1); } else { diff --git a/dash-spv/src/sync/instantsend/sync_manager.rs b/dash-spv/src/sync/instantsend/sync_manager.rs index 95079e670..1631483a1 100644 --- a/dash-spv/src/sync/instantsend/sync_manager.rs +++ b/dash-spv/src/sync/instantsend/sync_manager.rs @@ -26,7 +26,11 @@ impl SyncManager for InstantSendManager { } fn on_disconnect(&mut self) { - self.pending_instantlocks.clear(); + // Clear the queue *and* drop unvalidated cache entries. Leaving the + // latter behind would make `process_instantlock` treat a lock + // re-announced after reconnect as a duplicate, so it would never be + // re-queued or re-validated. + self.clear_unvalidated_on_disconnect(); // Nothing is queued anymore, so the height at which we last validated is // meaningless; reset it so re-validation runs again after reconnect. self.last_validated_engine_height = None; @@ -89,13 +93,8 @@ impl SyncManager for InstantSendManager { vec![] }; - // Transition to Synced when no pending validations after masternode sync - if self.pending_count() == 0 - && matches!(self.state(), SyncState::Syncing | SyncState::WaitForEvents) - { - self.set_state(SyncState::Synced); - tracing::info!("InstantSend manager synced (no pending validations)"); - } + // Transition to Synced when no pending validations after masternode sync. + self.transition_synced_if_idle(); return Ok(events); } @@ -107,6 +106,17 @@ impl SyncManager for InstantSendManager { // Prune old entries periodically self.prune_old_entries(); + // Whether there was work to resolve at the start of this tick. Guards the + // synced-state transition below so a freshly created manager sitting idle + // in `WaitForEvents` isn't marked `Synced` without ever having done work. + let had_pending = !self.pending_instantlocks.is_empty(); + + // Expire pending locks that have outlived their TTL. This runs on every + // tick, independent of engine advancement — otherwise, with a static + // engine height, `validate_pending` (the only other place expiry runs) + // would never be reached and a lock could stay pending forever. + self.expire_pending(); + // Re-validate queued InstantLocks once the masternode engine has advanced // past the height at which they were last checked. A lock received before // quorum sync completed (e.g. right after a fresh SPV start, for a @@ -114,6 +124,7 @@ impl SyncManager for InstantSendManager { // needed quorum data lands, rather than waiting for the next // `MasternodeStateUpdated` event — which, for a just-broadcast // transaction, may not arrive until the block that mines it. + let mut events = Vec::new(); if !self.pending_instantlocks.is_empty() { let engine_height = self.engine_height().await; let advanced = match (engine_height, self.last_validated_engine_height) { @@ -122,11 +133,19 @@ impl SyncManager for InstantSendManager { (None, _) => false, }; if advanced { - return self.validate_pending().await; + events = self.validate_pending().await?; } } - Ok(vec![]) + // If this tick started with pending work that has now fully drained (via + // expiry or validation), mirror `handle_sync_event` and resolve the + // manager's state — otherwise a final lock resolved by tick rather than a + // `MasternodeStateUpdated` event would leave it stuck in `Syncing`. + if had_pending { + self.transition_synced_if_idle(); + } + + Ok(events) } fn progress(&self) -> SyncManagerProgress {