From 40478414a981fa7ea4c71d7fac9f0c1f68178ecf Mon Sep 17 00:00:00 2001 From: Borja Castellano Date: Thu, 27 Aug 2026 14:27:57 +0000 Subject: [PATCH 1/4] test(dash-spv): assert the restart test's storage, not just its progress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `test_masternode_list_sync_with_restart` compared masternode sync progress either side of a restart. A from-scratch network re-sync produces the same progress as a restored one, so the test passed while the list was being rebuilt from nothing every time (dashpay/rust-dashcore#988). It now looks at the disk. After the first session's clean shutdown every directory that session earned must hold a file, and across the restart no directory may disappear or lose files. Fails as written: the first session builds four masternodes and writes no `masternodestate/`, while `block_headers/`, `filter_headers/`, `metadata/` and `peers/` all persist through the same shutdown to the same directory — so the storage layer and the shutdown are ruled out as causes. `filters/` and `blocks/` are left out of the must-hold set on purpose: the client stops as soon as the masternode phase reports `Synced`, which is before the filter phase leaves `WaitForEvents`, so they are legitimately empty here. The no-shrink check still covers them. The engine is read before the shutdown and the count carried into the failure message, so the assertion cannot be satisfied by a session that synced nothing — which is the shape dashpay/rust-dashcore#954 produces. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015NhHBDGiKfiGpy7FwooyfS --- dash-spv/tests/dashd_masternode/helpers.rs | 91 +++++++++++++++++++ dash-spv/tests/dashd_masternode/tests_sync.rs | 30 +++++- 2 files changed, 119 insertions(+), 2 deletions(-) diff --git a/dash-spv/tests/dashd_masternode/helpers.rs b/dash-spv/tests/dashd_masternode/helpers.rs index aa27b7a06..8166df9bb 100644 --- a/dash-spv/tests/dashd_masternode/helpers.rs +++ b/dash-spv/tests/dashd_masternode/helpers.rs @@ -1,3 +1,6 @@ +use std::collections::BTreeMap; +use std::path::Path; + use dash_spv::sync::{MasternodesProgress, SyncEvent, SyncProgress, SyncState}; use dashcore::ephemerealdata::instant_lock::InstantLock; use dashcore::sml::llmq_entry_verification::LLMQEntryVerificationStatus; @@ -14,6 +17,94 @@ use super::setup::{TestContext, SYNC_TIMEOUT}; /// Mine a DKG cycle and wait for the SPV to surface a `MasternodeStateUpdated` /// event above `baseline_height`. +/// Files held under each immediate subdirectory of the storage root, keyed by +/// directory name. +/// +/// A sync writes into these and never removes a whole class of state, so across +/// a restart every directory must still be there and hold at least as much — +/// see [`assert_storage_did_not_shrink`]. +pub(super) fn storage_snapshot(root: &Path) -> BTreeMap { + let mut counts = BTreeMap::new(); + let Ok(entries) = std::fs::read_dir(root) else { + return counts; + }; + for entry in entries.flatten() { + if !entry.path().is_dir() { + continue; + } + let files = walkdir_count(&entry.path()); + counts.insert(entry.file_name().to_string_lossy().into_owned(), files); + } + counts +} + +fn walkdir_count(dir: &Path) -> usize { + let Ok(entries) = std::fs::read_dir(dir) else { + return 0; + }; + entries + .flatten() + .map(|e| { + let path = e.path(); + if path.is_dir() { + walkdir_count(&path) + } else { + 1 + } + }) + .sum() +} + +/// Directories that must hold state once this test's first session has run, and +/// why. `filters` and `blocks` are deliberately absent: the client is stopped +/// as soon as the masternode phase reports `Synced`, which is before the filter +/// phase leaves `WaitForEvents`, so those stay legitimately empty here. +pub(super) const EXPECTED_STORAGE: &[(&str, &str)] = &[ + ("block_headers", "headers synced to the tip"), + ("filter_headers", "filter headers synced to the tip"), + ("metadata", "sync checkpoints"), + ("peers", "peer set and reputations"), + ("masternodestate", "the masternode list this session built"), +]; + +/// Assert every directory in [`EXPECTED_STORAGE`] exists and holds at least one +/// file, reporting all of them at once rather than the first to fail. +pub(super) fn assert_storage_persisted(snapshot: &BTreeMap, what: &str) { + let missing: Vec = EXPECTED_STORAGE + .iter() + .filter(|(dir, _)| snapshot.get(*dir).is_none_or(|files| *files == 0)) + .map(|(dir, why)| format!(" {dir}/ — {why}")) + .collect(); + assert!( + missing.is_empty(), + "{what}: {} storage director{} empty or absent after a clean shutdown:\n{}\n\nstorage holds {snapshot:?}", + missing.len(), + if missing.len() == 1 { "y is" } else { "ies are" }, + missing.join("\n"), + ); +} + +/// Every directory present before a restart must still be present after, with +/// at least as many files. A directory that vanishes or shrinks means a restart +/// threw away state that the previous session had already earned. +pub(super) fn assert_storage_did_not_shrink( + before: &BTreeMap, + after: &BTreeMap, + what: &str, +) { + for (dir, before_count) in before { + match after.get(dir) { + None => panic!( + "{what}: storage directory {dir:?} disappeared across the restart\n before: {before:?}\n after: {after:?}" + ), + Some(after_count) if after_count < before_count => panic!( + "{what}: storage directory {dir:?} shrank across the restart, {before_count} -> {after_count}\n before: {before:?}\n after: {after:?}" + ), + Some(_) => {} + } + } +} + pub(super) async fn mine_dkg_cycle_and_wait( ctx: &mut TestContext, sync_event_receiver: &mut broadcast::Receiver, diff --git a/dash-spv/tests/dashd_masternode/tests_sync.rs b/dash-spv/tests/dashd_masternode/tests_sync.rs index 805e469ad..b38d28783 100644 --- a/dash-spv/tests/dashd_masternode/tests_sync.rs +++ b/dash-spv/tests/dashd_masternode/tests_sync.rs @@ -10,8 +10,9 @@ use dashcore::sml::llmq_entry_verification::LLMQEntryVerificationStatus; use dashcore::sml::llmq_type::LLMQType; use super::helpers::{ - assert_all_rotated_quorums_verified, wait_for_chainlock_height_at_least, - wait_for_masternode_sync, wait_for_mn_state_event, wait_for_mn_state_event_above, + assert_all_rotated_quorums_verified, assert_storage_did_not_shrink, assert_storage_persisted, + storage_snapshot, wait_for_chainlock_height_at_least, wait_for_masternode_sync, + wait_for_mn_state_event, wait_for_mn_state_event_above, wait_for_mn_state_with_stored_cycle_above, }; use super::setup::{ @@ -103,9 +104,29 @@ async fn test_masternode_list_sync_with_restart() { let first_mn_progress = wait_for_masternode_sync(&mut client_handle.progress_receiver, SYNC_TIMEOUT).await; let first_height = first_mn_progress.current_height(); + + // Control: the first session really built a list, so the persistence + // assertion below cannot be satisfied by a client that synced nothing. + let first_masternodes = { + let engine = client_handle.engine.read().await; + engine.masternode_lists.values().map(|list| list.masternodes.len()).max().unwrap_or(0) + }; + assert!( + first_masternodes > 0, + "the first session must have a masternode list before its persistence can be tested" + ); + client_handle.stop().await; drop(client_handle); + // What the first session earned and wrote down. A clean shutdown of a + // fully-synced client must leave every sync phase's state on disk. + let after_first = storage_snapshot(ctx.storage_path()); + assert_storage_persisted( + &after_first, + &format!("after a first session that built {first_masternodes} masternode(s)"), + ); + // Restart with same storage tracing::info!("=== Restarting with same storage ==="); let mut client_handle = create_and_start_client(&config, Arc::clone(&wallet)).await; @@ -123,6 +144,11 @@ async fn test_masternode_list_sync_with_restart() { "Should reach Synced state after restart" ); + // A restart re-syncs on top of what it restored; it never discards a whole + // class of state it already had. + let after_second = storage_snapshot(ctx.storage_path()); + assert_storage_did_not_shrink(&after_first, &after_second, "masternode restart"); + tracing::info!( "Restart verified: first_height={}, second_height={}", first_height, From 1cf5bdd52a9e7207ac53c3554745b5d8bc9ca968 Mon Sep 17 00:00:00 2001 From: Borja Castellano Date: Tue, 8 Sep 2026 14:59:08 +0000 Subject: [PATCH 2/4] refactor(dash-spv): persist the masternode messages, not a snapshot of the engine The masternode storage wrote a serialized `MasternodeListEngine` plus its own copy of the block hash/height container. That second copy of a mapping the header storage already owns can only diverge - a reorg rewrites one of them - and the snapshot had to be rewritten whole on every update. It now stores the two network messages that produced the state, one file per message and height (`masternodes/diff_.dat`, `qrinfo_.dat`, atomic writes, indexed on open), and rebuilds the engine by replaying them. The replay runs the same path the live sync does, only reading from disk instead of waiting for peers: QRInfo through `feed_qrinfo_heights_to_engine`, MnListDiff through its file name plus a lookup of the base hash it extends. Heights are resolved against the header storage, which is injected at construction, so there is one mapping and it is the one the header chain maintains. Messages are written as they arrive, so this storage has no buffered state: `PersistentStorage` is gone from it, along with the no-op `persist` the background worker woke up every five seconds. `MasternodeState` and `storage/types.rs` go with it - the on-disk shape is no longer named outside `storage/`. `MasternodeStorage` takes and returns the engine rather than the file format, and knows its own network from `open`, so `load_engine()` and `masternode_list_at_or_before(height)` lose a parameter their callers were only forwarding. `ChainLockManager` loses the `network` field it carried for that. Retention is now bounded. `prune_obsolete_lists` keeps the engine to the span `quorum_entry_for_hash_at_or_before_height` can walk back over, and a ChainLock whose signing height falls outside it is verified against a list rebuilt from storage instead of failing. `masternode_list_at_or_before` caches that list with its validity range, so consecutive ChainLocks around one height replay once. Protocol rules that were spelled `- 8` at four call sites across both crates now have names where they are defined: `LLMQ_SIGN_HEIGHT_OFFSET` (DIP-0007, via `ChainLock::signing_height`) and `QUORUM_MEMBER_LIST_OFFSET` (DIP-0024), next to `WORK_DIFF_DEPTH`, which they are unrelated to despite sharing a value. The QRInfo's own shape stays in the engine: `qr_info_work_block_hashes` and `cycle_boundary_height` replace the hand-rolled diff enumeration dash-spv used to keep in step by hand. `verify_chain_lock_with_masternode_list` is public for the rebuilt-list path and derives its own request id, which the caller was having to fabricate. `prune_masternode_lists` is no longer gated on `quorum_validation`: nothing in it needs the feature, and bounding memory is not a validation concern. Verified against dashd regtest and the full unit suites: 590 dashcore, 570 dash-spv, 10 dashd_masternode, 32 dashd_sync. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SbgCpMiBjnpvW4CyEEsKXw --- dash-spv/src/client/lifecycle.rs | 17 +- dash-spv/src/storage/masternode.rs | 347 +++++++++++++++--- dash-spv/src/storage/mod.rs | 85 +++-- dash-spv/src/storage/types.rs | 16 - dash-spv/src/sync/chainlock/manager.rs | 52 ++- dash-spv/src/sync/masternodes/manager.rs | 42 ++- dash-spv/src/sync/masternodes/sync_manager.rs | 105 ++---- dash-spv/tests/dashd_masternode/helpers.rs | 17 +- dash-spv/tests/dashd_masternode/tests_sync.rs | 6 - dash/src/ephemerealdata/chain_lock.rs | 7 +- dash/src/sml/llmq_type/mod.rs | 10 + .../src/sml/masternode_list_engine/helpers.rs | 18 + .../message_request_verification.rs | 37 +- dash/src/sml/masternode_list_engine/mod.rs | 31 +- .../non_rotated_quorum_construction.rs | 33 +- 15 files changed, 589 insertions(+), 234 deletions(-) delete mode 100644 dash-spv/src/storage/types.rs diff --git a/dash-spv/src/client/lifecycle.rs b/dash-spv/src/client/lifecycle.rs index 46e26f71c..4b32c9b43 100644 --- a/dash-spv/src/client/lifecycle.rs +++ b/dash-spv/src/client/lifecycle.rs @@ -13,8 +13,9 @@ use crate::chain::checkpoints::CheckpointManager; use crate::error::{Result, SpvError}; use crate::network::NetworkManager; use crate::storage::{ - PersistentBlockHeaderStorage, PersistentBlockStorage, PersistentFilterHeaderStorage, - PersistentFilterStorage, PersistentMetadataStorage, StorageManager, + MasternodeStorage, PersistentBlockHeaderStorage, PersistentBlockStorage, + PersistentFilterHeaderStorage, PersistentFilterStorage, PersistentMetadataStorage, + StorageManager, }; use crate::sync::{ BlockHeadersManager, BlocksManager, ChainLockManager, FilterHeadersManager, FiltersManager, @@ -67,9 +68,13 @@ impl DashSpvClient DashSpvClient DashSpvClient; + +enum Pending { + Diff(Box), + QrInfo(Box), +} + +struct CachedList { + from: CoreBlockHeight, + until: Option, + list: Option, +} #[async_trait] -pub trait MasternodeStateStorage { - async fn store_masternode_state(&mut self, state: &MasternodeState) -> StorageResult<()>; +pub trait MasternodeStorage: Send + Sync + 'static { + async fn store_diff(&mut self, height: CoreBlockHeight, diff: &MnListDiff) + -> StorageResult<()>; + + async fn store_qr_info( + &mut self, + height: CoreBlockHeight, + qr_info: &QRInfo, + ) -> StorageResult<()>; + + async fn load_engine(&self) -> StorageResult; - async fn load_masternode_state(&self) -> StorageResult>; + async fn masternode_list_at_or_before( + &self, + height: CoreBlockHeight, + ) -> StorageResult>; } -pub struct PersistentMasternodeStateStorage { +pub struct PersistentMasternodeStorage { storage_path: PathBuf, + headers: Arc>, + network: Network, + diffs: IndexMap, + qr_infos: IndexMap, + cached_list: Mutex>, } -impl PersistentMasternodeStateStorage { - const FOLDER_NAME: &str = "masternodestate"; - const MASTERNODE_FILE_NAME: &str = "masternodestate.json"; -} +impl PersistentMasternodeStorage { + const FOLDER_NAME: &str = "masternodes"; + const DIFF_PREFIX: &str = "diff_"; + const QRINFO_PREFIX: &str = "qrinfo_"; + const EXTENSION: &str = "dat"; -#[async_trait] -impl PersistentStorage for PersistentMasternodeStateStorage { - async fn open(storage_path: impl Into + Send) -> StorageResult { - Ok(PersistentMasternodeStateStorage { - storage_path: storage_path.into(), + pub async fn open( + storage_path: impl Into + Send, + headers: Arc>, + network: Network, + ) -> StorageResult { + let storage_path = storage_path.into(); + let (diffs, qr_infos) = Self::index_folder(&storage_path.join(Self::FOLDER_NAME)).await?; + + Ok(PersistentMasternodeStorage { + storage_path, + headers, + network, + diffs, + qr_infos, + cached_list: Mutex::new(None), }) } - async fn persist(&mut self, _storage_path: impl Into + Send) -> StorageResult<()> { - // Current implementation persists data everytime data is stored - Ok(()) + fn folder(&self) -> PathBuf { + self.storage_path.join(Self::FOLDER_NAME) } -} -#[async_trait] -impl MasternodeStateStorage for PersistentMasternodeStateStorage { - async fn store_masternode_state(&mut self, state: &MasternodeState) -> StorageResult<()> { - let masternodestate_folder = self.storage_path.join(Self::FOLDER_NAME); - let path = masternodestate_folder.join(Self::MASTERNODE_FILE_NAME); + fn file_name(prefix: &str, height: CoreBlockHeight) -> String { + format!("{prefix}{height}.{}", Self::EXTENSION) + } - tokio::fs::create_dir_all(masternodestate_folder).await?; + fn height_from_file_name(name: &str, prefix: &str) -> Option { + name.strip_prefix(prefix)?.strip_suffix(&format!(".{}", Self::EXTENSION))?.parse().ok() + } - let json = serde_json::to_string_pretty(state).map_err(|e| { - crate::error::StorageError::Serialization(format!( - "Failed to serialize masternode state: {}", - e - )) - })?; + async fn index_folder(folder: &Path) -> StorageResult<(IndexMap, IndexMap)> { + let mut diffs = BTreeMap::new(); + let mut qr_infos = BTreeMap::new(); - atomic_write(&path, json.as_bytes()).await?; + if !folder.exists() { + return Ok((diffs, qr_infos)); + } + + let mut entries = tokio::fs::read_dir(folder).await?; + while let Some(entry) = entries.next_entry().await? { + let path = entry.path(); + let Some(name) = path.file_name().and_then(|n| n.to_str()) else { + continue; + }; + if let Some(height) = Self::height_from_file_name(name, Self::DIFF_PREFIX) { + diffs.insert(height, path); + } else if let Some(height) = Self::height_from_file_name(name, Self::QRINFO_PREFIX) { + qr_infos.insert(height, path); + } + } + + Ok((diffs, qr_infos)) + } + + async fn store_message( + folder: &Path, + index: &mut IndexMap, + prefix: &str, + height: CoreBlockHeight, + message: &T, + ) -> StorageResult<()> { + tokio::fs::create_dir_all(folder).await?; + let path = folder.join(Self::file_name(prefix, height)); + atomic_write(&path, &serialize(message)).await?; + index.insert(height, path); Ok(()) } - async fn load_masternode_state(&self) -> StorageResult> { - let path = self.storage_path.join(Self::FOLDER_NAME).join(Self::MASTERNODE_FILE_NAME); + async fn read_message(path: &Path) -> StorageResult { + let bytes = tokio::fs::read(path).await?; + deserialize(&bytes).map_err(|e| { + StorageError::Corruption(format!("Failed to decode {}: {e}", path.display())) + }) + } - if !path.exists() { - return Ok(None); + async fn cached_list_at(&self, height: CoreBlockHeight) -> Option> { + let cached = self.cached_list.lock().await; + let cached = cached.as_ref()?; + (height >= cached.from && cached.until.is_none_or(|until| height < until)) + .then(|| cached.list.clone()) + } + + fn invalidate_cached_list(&mut self) { + *self.cached_list.get_mut() = None; + } + + async fn replay(&self) -> StorageResult { + let mut engine = MasternodeListEngine::default_for_network(self.network); + + let mut ordered: Vec<(CoreBlockHeight, &PathBuf, bool)> = self + .qr_infos + .iter() + .map(|(height, path)| (*height, path, true)) + .chain(self.diffs.iter().map(|(height, path)| (*height, path, false))) + .collect(); + ordered.sort_by_key(|(height, _, is_qr_info)| (*height, *is_qr_info)); + + let mut queue: Vec<(CoreBlockHeight, Pending)> = Vec::with_capacity(ordered.len()); + { + let headers = self.headers.read().await; + for (height, path, is_qr_info) in ordered { + if is_qr_info { + match Self::read_message::(path).await { + Ok(qr_info) => { + feed_qrinfo_heights_to_engine(&mut engine, &qr_info, &*headers).await; + queue.push((height, Pending::QrInfo(Box::new(qr_info)))); + } + Err(e) => tracing::warn!("Skipping unreadable QRInfo at {height}: {e}"), + } + } else { + match Self::read_message::(path).await { + Ok(diff) => { + engine.feed_block_height(height, diff.block_hash); + if let Ok(Some(base_height)) = + headers.get_header_height_by_hash(&diff.base_block_hash).await + { + engine.feed_block_height(base_height, diff.base_block_hash); + } + queue.push((height, Pending::Diff(Box::new(diff)))); + } + Err(e) => tracing::warn!("Skipping unreadable MnListDiff at {height}: {e}"), + } + } + } } - let content = tokio::fs::read_to_string(path).await?; - let state = serde_json::from_str(&content).map_err(|e| { - crate::error::StorageError::Serialization(format!( - "Failed to deserialize masternode state: {}", - e - )) - })?; + let total = queue.len(); + let mut pending = Vec::new(); + for (height, message) in queue { + if let Some(unapplied) = Self::apply(&mut engine, height, message) { + pending.push((height, unapplied)); + } + } - Ok(Some(state)) + while !pending.is_empty() { + let remaining = pending.len(); + let mut still_pending = Vec::with_capacity(remaining); + for (height, message) in pending { + if let Some(unapplied) = Self::apply(&mut engine, height, message) { + still_pending.push((height, unapplied)); + } + } + pending = still_pending; + if pending.len() == remaining { + break; + } + } + + for (height, _) in &pending { + tracing::warn!("Message at {height} has no reachable base, leaving it to the network"); + } + + tracing::debug!( + "Replayed {}/{} masternode messages into {} masternode lists", + total - pending.len(), + total, + engine.masternode_lists.len() + ); + + Ok(engine) + } + + fn apply( + engine: &mut MasternodeListEngine, + height: CoreBlockHeight, + message: Pending, + ) -> Option { + match message { + Pending::QrInfo(qr_info) => engine + .feed_qr_info((*qr_info).clone(), true, true) + .is_err() + .then_some(Pending::QrInfo(qr_info)), + Pending::Diff(diff) => engine + .apply_diff((*diff).clone(), Some(height), false, None) + .is_err() + .then_some(Pending::Diff(diff)), + } + } +} + +#[async_trait] +impl MasternodeStorage for PersistentMasternodeStorage { + async fn store_diff( + &mut self, + height: CoreBlockHeight, + diff: &MnListDiff, + ) -> StorageResult<()> { + let folder = self.folder(); + self.invalidate_cached_list(); + Self::store_message(&folder, &mut self.diffs, Self::DIFF_PREFIX, height, diff).await + } + + async fn store_qr_info( + &mut self, + height: CoreBlockHeight, + qr_info: &QRInfo, + ) -> StorageResult<()> { + let folder = self.folder(); + self.invalidate_cached_list(); + Self::store_message(&folder, &mut self.qr_infos, Self::QRINFO_PREFIX, height, qr_info).await + } + + async fn load_engine(&self) -> StorageResult { + self.replay().await + } + + async fn masternode_list_at_or_before( + &self, + height: CoreBlockHeight, + ) -> StorageResult> { + if let Some(hit) = self.cached_list_at(height).await { + return Ok(hit); + } + + let engine = self.replay().await?; + let (before, after) = engine.masternode_lists_around_height(height); + let list = before.cloned(); + + *self.cached_list.lock().await = Some(CachedList { + from: before.map_or(0, |list| list.known_height), + until: after.map(|next| next.known_height), + list: list.clone(), + }); + + Ok(list) } } + +pub(crate) async fn feed_qrinfo_heights_to_engine( + engine: &mut MasternodeListEngine, + qr_info: &QRInfo, + storage: &S, +) { + let mut fed_count = 0; + for block_hash in MasternodeListEngine::qr_info_referenced_block_hashes(qr_info) { + if let Ok(Some(height)) = storage.get_header_height_by_hash(&block_hash).await { + engine.feed_block_height(height, block_hash); + fed_count += 1; + tracing::trace!("Fed height {} for block {}", height, block_hash); + } + } + + for work_block_hash in MasternodeListEngine::qr_info_work_block_hashes(qr_info) { + if let Ok(Some(work_block_height)) = + storage.get_header_height_by_hash(&work_block_hash).await + { + let cycle_boundary_height = + MasternodeListEngine::cycle_boundary_height(work_block_height); + if let Ok(Some(cycle_boundary_header)) = storage.get_header(cycle_boundary_height).await + { + let cycle_boundary_hash = *cycle_boundary_header.hash(); + engine.feed_block_height(cycle_boundary_height, cycle_boundary_hash); + fed_count += 1; + tracing::debug!( + "Fed cycle boundary height {} for block {}", + cycle_boundary_height, + cycle_boundary_hash + ); + } + } + } + + tracing::info!("Fed {} block heights to engine", fed_count); +} diff --git a/dash-spv/src/storage/mod.rs b/dash-spv/src/storage/mod.rs index 70a851acb..0e429bad1 100644 --- a/dash-spv/src/storage/mod.rs +++ b/dash-spv/src/storage/mod.rs @@ -1,7 +1,5 @@ //! Storage abstraction for the Dash SPV client. -pub mod types; - mod block_headers; mod blocks; mod filter_headers; @@ -18,7 +16,12 @@ use crate::types::{HashedBlock, HashedBlockHeader}; use crate::ClientConfig; use async_trait::async_trait; use dashcore::hash_types::FilterHeader; +use dashcore::network::message_qrinfo::QRInfo; +use dashcore::network::message_sml::MnListDiff; use dashcore::prelude::CoreBlockHeight; +use dashcore::sml::masternode_list::MasternodeList; +use dashcore::sml::masternode_list_engine::MasternodeListEngine; +use dashcore::Network; use std::ops::Range; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -31,12 +34,11 @@ pub use crate::storage::block_headers::{ pub use crate::storage::blocks::{BlockStorage, PersistentBlockStorage}; pub use crate::storage::filter_headers::{FilterHeaderStorage, PersistentFilterHeaderStorage}; pub use crate::storage::filters::{FilterStorage, PersistentFilterStorage}; -pub use crate::storage::masternode::{MasternodeStateStorage, PersistentMasternodeStateStorage}; +pub(crate) use crate::storage::masternode::feed_qrinfo_heights_to_engine; +pub use crate::storage::masternode::{MasternodeStorage, PersistentMasternodeStorage}; pub use crate::storage::metadata::{MetadataStorage, PersistentMetadataStorage}; pub use crate::storage::peers::{PeerStorage, PersistentPeerStorage}; -pub use types::*; - #[async_trait] pub trait PersistentStorage: Sized { /// If the storage_path contains persisted data the storage will use it, if not, @@ -53,7 +55,7 @@ pub trait StorageManager: + FilterStorage + BlockStorage + MetadataStorage - + MasternodeStateStorage + + MasternodeStorage + Send + Sync + 'static @@ -78,6 +80,9 @@ pub trait StorageManager: /// Returns shared access to the metadata storage. fn metadata(&self) -> Arc>; + + fn masternodes(&self) + -> Arc>>; } /// Disk-based storage manager with segmented files and async background saving. @@ -85,13 +90,14 @@ pub trait StorageManager: /// can exist at a time. pub struct DiskStorageManager { storage_path: PathBuf, + network: Network, block_headers: Arc>, filter_headers: Arc>, filters: Arc>, blocks: Arc>, metadata: Arc>, - masternodestate: Arc>, + masternodes: Arc>>, // Background worker worker_handle: Option>, @@ -132,21 +138,28 @@ impl DiskStorageManager { let lock_file = LockFile::new(lock_file)?; + let block_headers = + Arc::new(RwLock::new(PersistentBlockHeaderStorage::open(&storage_path).await?)); + let mut storage = Self { storage_path: storage_path.clone(), + network: config.network, - block_headers: Arc::new(RwLock::new( - PersistentBlockHeaderStorage::open(&storage_path).await?, - )), filter_headers: Arc::new(RwLock::new( PersistentFilterHeaderStorage::open(&storage_path).await?, )), filters: Arc::new(RwLock::new(PersistentFilterStorage::open(&storage_path).await?)), blocks: Arc::new(RwLock::new(PersistentBlockStorage::open(&storage_path).await?)), metadata: Arc::new(RwLock::new(PersistentMetadataStorage::open(&storage_path).await?)), - masternodestate: Arc::new(RwLock::new( - PersistentMasternodeStateStorage::open(&storage_path).await?, + masternodes: Arc::new(RwLock::new( + PersistentMasternodeStorage::open( + &storage_path, + Arc::clone(&block_headers), + config.network, + ) + .await?, )), + block_headers, worker_handle: None, @@ -173,7 +186,6 @@ impl DiskStorageManager { let filters = Arc::clone(&self.filters); let blocks = Arc::clone(&self.blocks); let metadata = Arc::clone(&self.metadata); - let masternodestate = Arc::clone(&self.masternodestate); let storage_path = self.storage_path.clone(); @@ -188,7 +200,6 @@ impl DiskStorageManager { let _ = filters.write().await.persist(&storage_path).await; let _ = blocks.write().await.persist(&storage_path).await; let _ = metadata.write().await.persist(&storage_path).await; - let _ = masternodestate.write().await.persist(&storage_path).await; } }); @@ -210,7 +221,6 @@ impl DiskStorageManager { let _ = self.filters.write().await.persist(storage_path).await; let _ = self.blocks.write().await.persist(storage_path).await; let _ = self.metadata.write().await.persist(storage_path).await; - let _ = self.masternodestate.write().await.persist(storage_path).await; } } @@ -247,8 +257,14 @@ impl StorageManager for DiskStorageManager { self.filters = Arc::new(RwLock::new(PersistentFilterStorage::open(storage_path).await?)); self.blocks = Arc::new(RwLock::new(PersistentBlockStorage::open(storage_path).await?)); self.metadata = Arc::new(RwLock::new(PersistentMetadataStorage::open(storage_path).await?)); - self.masternodestate = - Arc::new(RwLock::new(PersistentMasternodeStateStorage::open(storage_path).await?)); + self.masternodes = Arc::new(RwLock::new( + PersistentMasternodeStorage::open( + storage_path, + Arc::clone(&self.block_headers), + self.network, + ) + .await?, + )); // Restart the background worker for future operations self.start_worker().await; @@ -282,6 +298,12 @@ impl StorageManager for DiskStorageManager { fn metadata(&self) -> Arc> { Arc::clone(&self.metadata) } + + fn masternodes( + &self, + ) -> Arc>> { + Arc::clone(&self.masternodes) + } } #[async_trait] @@ -431,13 +453,32 @@ impl metadata::MetadataStorage for DiskStorageManager { } #[async_trait] -impl masternode::MasternodeStateStorage for DiskStorageManager { - async fn store_masternode_state(&mut self, state: &MasternodeState) -> StorageResult<()> { - self.masternodestate.write().await.store_masternode_state(state).await +impl masternode::MasternodeStorage for DiskStorageManager { + async fn store_diff( + &mut self, + height: CoreBlockHeight, + diff: &MnListDiff, + ) -> StorageResult<()> { + self.masternodes.write().await.store_diff(height, diff).await + } + + async fn store_qr_info( + &mut self, + height: CoreBlockHeight, + qr_info: &QRInfo, + ) -> StorageResult<()> { + self.masternodes.write().await.store_qr_info(height, qr_info).await + } + + async fn load_engine(&self) -> StorageResult { + self.masternodes.read().await.load_engine().await } - async fn load_masternode_state(&self) -> StorageResult> { - self.masternodestate.read().await.load_masternode_state().await + async fn masternode_list_at_or_before( + &self, + height: CoreBlockHeight, + ) -> StorageResult> { + self.masternodes.read().await.masternode_list_at_or_before(height).await } } diff --git a/dash-spv/src/storage/types.rs b/dash-spv/src/storage/types.rs deleted file mode 100644 index 678553caa..000000000 --- a/dash-spv/src/storage/types.rs +++ /dev/null @@ -1,16 +0,0 @@ -//! Storage-related types and structures. - -use serde::{Deserialize, Serialize}; - -/// Masternode state for storage. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct MasternodeState { - /// Last processed height. - pub last_height: u32, - - /// Serialized masternode list engine state. - pub engine_state: Vec, - - /// Last update timestamp. - pub last_update: u64, -} diff --git a/dash-spv/src/sync/chainlock/manager.rs b/dash-spv/src/sync/chainlock/manager.rs index c211919df..ba6f75d3e 100644 --- a/dash-spv/src/sync/chainlock/manager.rs +++ b/dash-spv/src/sync/chainlock/manager.rs @@ -14,7 +14,9 @@ use std::collections::HashSet; use tokio::sync::RwLock; use crate::error::SyncResult; -use crate::storage::{BlockHeaderStorage, MetadataStorage}; +use crate::storage::{ + BlockHeaderStorage, MasternodeStorage, MetadataStorage, PersistentMasternodeStorage, +}; use crate::sync::{ChainLockProgress, SyncEvent}; /// Metadata key for persisting the best validated ChainLock. @@ -36,6 +38,7 @@ pub struct ChainLockManager { metadata_storage: Arc>, /// Masternode engine for BLS signature validation. masternode_engine: Arc>, + masternode_storage: Option>>>, /// The best (highest height) validated ChainLock. best_chainlock: Option, /// ChainLock hashes that have been requested (to avoid duplicate requests). @@ -56,12 +59,14 @@ impl ChainLockManager { header_storage: Arc>, metadata_storage: Arc>, masternode_engine: Arc>, + masternode_storage: Option>>>, ) -> Self { let mut manager = Self { progress: ChainLockProgress::default(), header_storage, metadata_storage, masternode_engine, + masternode_storage, best_chainlock: None, requested_chainlocks: HashSet::new(), masternode_ready: false, @@ -254,6 +259,47 @@ impl ChainLockManager { "ChainLock signature verified for height {}", chainlock.block_height ); + return true; + } + Err(e) => tracing::debug!( + "ChainLock at height {} not verifiable against the retained lists: {}", + chainlock.block_height, + e + ), + } + drop(engine); + + self.validate_signature_from_storage(chainlock).await + } + + async fn validate_signature_from_storage(&self, chainlock: &ChainLock) -> bool { + let Some(storage) = &self.masternode_storage else { + return false; + }; + + let signing_height = chainlock.signing_height(); + let list = match storage.read().await.masternode_list_at_or_before(signing_height).await { + Ok(Some(list)) => list, + Ok(None) => return false, + Err(e) => { + tracing::warn!( + "Could not rebuild the masternode list for height {}: {}", + signing_height, + e + ); + return false; + } + }; + + let engine = self.masternode_engine.read().await; + + match engine.verify_chain_lock_with_masternode_list(chainlock, &list) { + Ok(()) => { + tracing::info!( + "ChainLock signature verified for height {} from a rebuilt list at {}", + chainlock.block_height, + list.known_height + ); true } Err(e) => { @@ -309,7 +355,7 @@ mod tests { let storage = DiskStorageManager::with_temp_dir().await.unwrap(); let engine = Arc::new(RwLock::new(MasternodeListEngine::default_for_network(Network::Testnet))); - ChainLockManager::new(storage.block_headers(), storage.metadata(), engine).await + ChainLockManager::new(storage.block_headers(), storage.metadata(), engine, None).await } async fn create_test_manager_with_storage( @@ -317,7 +363,7 @@ mod tests { ) -> TestChainLockManager { let engine = Arc::new(RwLock::new(MasternodeListEngine::default_for_network(Network::Testnet))); - ChainLockManager::new(storage.block_headers(), storage.metadata(), engine).await + ChainLockManager::new(storage.block_headers(), storage.metadata(), engine, None).await } fn create_test_chainlock(height: u32) -> ChainLock { diff --git a/dash-spv/src/sync/masternodes/manager.rs b/dash-spv/src/sync/masternodes/manager.rs index 428673535..1fdcba4ce 100644 --- a/dash-spv/src/sync/masternodes/manager.rs +++ b/dash-spv/src/sync/masternodes/manager.rs @@ -14,9 +14,10 @@ use tokio::sync::RwLock; use super::pipeline::MnListDiffPipeline; use crate::error::{SyncError, SyncResult}; use crate::network::RequestSender; -use crate::storage::BlockHeaderStorage; +use crate::storage::{BlockHeaderStorage, MasternodeStorage, PersistentMasternodeStorage}; use crate::sync::{MasternodesProgress, SyncEvent, SyncManager, SyncState}; use dashcore::network::message_qrinfo::QRInfo; +use dashcore::network::message_sml::MnListDiff; use dashcore::BlockHash; use std::collections::BTreeSet; @@ -299,6 +300,7 @@ pub struct MasternodesManager { network: dashcore::Network, /// Sync state tracking. pub(super) sync_state: MasternodeSyncState, + pub(super) message_storage: Option>>>, } impl MasternodesManager { @@ -307,6 +309,7 @@ impl MasternodesManager { header_storage: Arc>, engine: Arc>, network: dashcore::Network, + message_storage: Option>>>, ) -> Self { // Recover sync state from the engine's stored masternode lists so that a // restart can resume from where the previous run left off. @@ -337,9 +340,37 @@ impl MasternodesManager { engine, network, sync_state, + message_storage, } } + pub(super) async fn store_diff(&self, height: u32, diff: &MnListDiff) { + let Some(storage) = &self.message_storage else { + return; + }; + if let Err(e) = storage.write().await.store_diff(height, diff).await { + tracing::warn!("Could not store MnListDiff at {height}: {e}"); + } + } + + pub(super) async fn store_qr_info(&self, height: u32, qr_info: &QRInfo) { + let Some(storage) = &self.message_storage else { + return; + }; + if let Err(e) = storage.write().await.store_qr_info(height, qr_info).await { + tracing::warn!("Could not store QRInfo at {height}: {e}"); + } + } + + pub(super) async fn prune_obsolete_lists(&self, tip: u32) { + if self.message_storage.is_none() { + return; + } + + let pruned = self.engine.write().await.prune_obsolete_lists(tip); + tracing::debug!("Pruned {pruned} in-memory masternode lists at {tip}"); + } + /// Decide which [`PipelineMode`] to use when a new header lands at `tip_height` /// and masternode sync needs to catch up. The rule is: /// @@ -559,6 +590,7 @@ impl MasternodesManager { self.sync_state.last_synced_block_hash = Some(latest_block_hash); self.progress.update_current_height(height); + self.prune_obsolete_lists(height).await; tracing::debug!("Incremental MnListDiff complete at height {}", height); Ok(vec![SyncEvent::MasternodeStateUpdated { height, @@ -662,6 +694,10 @@ impl MasternodesManager { drop(engine); + if !events.is_empty() { + self.prune_obsolete_lists(self.progress.current_height()).await; + } + if is_initial_sync { self.set_state(SyncState::Synced); tracing::info!("Masternode sync complete at height {}", self.progress.current_height()); @@ -696,7 +732,7 @@ mod tests { async fn create_test_manager_for(network: dashcore::Network) -> TestMasternodesManager { let storage = DiskStorageManager::with_temp_dir().await.unwrap(); let engine = Arc::new(RwLock::new(MasternodeListEngine::default_for_network(network))); - MasternodesManager::new(storage.block_headers(), engine, network).await + MasternodesManager::new(storage.block_headers(), engine, network, None).await } async fn create_test_manager() -> TestMasternodesManager { @@ -733,6 +769,7 @@ mod tests { block_headers, Arc::new(RwLock::new(engine)), dashcore::Network::Regtest, + None, ) .await; manager.set_state(SyncState::Synced); @@ -964,6 +1001,7 @@ mod tests { storage.block_headers(), Arc::new(RwLock::new(engine)), dashcore::Network::Testnet, + None, ) .await; diff --git a/dash-spv/src/sync/masternodes/sync_manager.rs b/dash-spv/src/sync/masternodes/sync_manager.rs index 1a077a8a2..38c01d33a 100644 --- a/dash-spv/src/sync/masternodes/sync_manager.rs +++ b/dash-spv/src/sync/masternodes/sync_manager.rs @@ -1,15 +1,14 @@ use super::manager::PipelineMode; use crate::error::SyncResult; use crate::network::{Message, MessageType, RequestSender}; -use crate::storage::BlockHeaderStorage; +use crate::storage::{feed_qrinfo_heights_to_engine, BlockHeaderStorage}; use crate::sync::{ ManagerIdentifier, MasternodesManager, SyncEvent, SyncManager, SyncManagerProgress, SyncState, }; use crate::SyncError; use async_trait::async_trait; use dashcore::network::message::NetworkMessage; -use dashcore::network::message_qrinfo::QRInfo; -use dashcore::sml::masternode_list_engine::{MasternodeListEngine, WORK_DIFF_DEPTH}; +use dashcore::sml::llmq_type::QUORUM_MEMBER_LIST_OFFSET; use dashcore::{BlockHash, QuorumHash}; use dashcore_hashes::Hash; use std::collections::{BTreeSet, HashSet}; @@ -84,7 +83,7 @@ pub(super) async fn build_mnlistdiff_request_pairs( } }; - let validation_height = quorum_height.saturating_sub(8); + let validation_height = quorum_height.saturating_sub(QUORUM_MEMBER_LIST_OFFSET); // Skip if we already have this height if known_heights.contains(&validation_height) { @@ -159,63 +158,6 @@ pub(super) async fn build_mnlistdiff_request_pairs( Ok(pairs_with_height.into_iter().map(|(_, base, target)| (base, target)).collect()) } -/// Feed QRInfo block heights to the engine from storage. -/// -/// Resolves heights for every hash enumerated by -/// [`MasternodeListEngine::qr_info_referenced_block_hashes`], plus the cycle boundary -/// block for each work-block diff (`work_height + WORK_DIFF_DEPTH`), which is needed -/// for rotated quorum storage key calculation. -pub(super) async fn feed_qrinfo_heights_to_engine( - engine: &mut MasternodeListEngine, - qr_info: &QRInfo, - storage: &S, -) -> SyncResult { - let mut fed_count = 0; - for block_hash in MasternodeListEngine::qr_info_referenced_block_hashes(qr_info) { - if let Ok(Some(height)) = storage.get_header_height_by_hash(&block_hash).await { - engine.feed_block_height(height, block_hash); - fed_count += 1; - tracing::trace!("Fed height {} for block {}", height, block_hash); - } - } - - // Feed cycle boundary heights for all diffs (current and historical cycles). - // Each diff's block_hash is at the "work block" height; the cycle boundary is - // WORK_DIFF_DEPTH higher. - let mut work_block_hashes = vec![ - qr_info.mn_list_diff_h.block_hash, - qr_info.mn_list_diff_at_h_minus_c.block_hash, - qr_info.mn_list_diff_at_h_minus_2c.block_hash, - qr_info.mn_list_diff_at_h_minus_3c.block_hash, - ]; - - if let Some((_, diff)) = &qr_info.quorum_snapshot_and_mn_list_diff_at_h_minus_4c { - work_block_hashes.push(diff.block_hash); - } - - for work_block_hash in work_block_hashes { - if let Ok(Some(work_block_height)) = - storage.get_header_height_by_hash(&work_block_hash).await - { - let cycle_boundary_height = work_block_height + WORK_DIFF_DEPTH; - if let Ok(Some(cycle_boundary_header)) = storage.get_header(cycle_boundary_height).await - { - let cycle_boundary_hash = *cycle_boundary_header.hash(); - engine.feed_block_height(cycle_boundary_height, cycle_boundary_hash); - fed_count += 1; - tracing::debug!( - "Fed cycle boundary height {} for block {}", - cycle_boundary_height, - cycle_boundary_hash - ); - } - } - } - - tracing::info!("Fed {} block heights to engine", fed_count); - Ok(fed_count) -} - #[async_trait] impl SyncManager for MasternodesManager { fn identifier(&self) -> ManagerIdentifier { @@ -269,9 +211,8 @@ impl SyncManager for MasternodesManager { // Feed block heights to engine using internal storage let storage = self.header_storage.read().await; let mut engine = self.engine.write().await; - let fed = feed_qrinfo_heights_to_engine(&mut engine, qr_info, &*storage).await?; + feed_qrinfo_heights_to_engine(&mut engine, qr_info, &*storage).await; drop(storage); - tracing::info!("Fed {} block heights to engine", fed); // Feed QRInfo to engine first to populate masternode lists let qr_info_result = match engine.feed_qr_info(qr_info.clone(), true, true) { @@ -314,10 +255,23 @@ impl SyncManager for MasternodesManager { ) .await?; + let tip_hash = qr_info.mn_list_diff_tip.block_hash; + let tip_height = storage.get_header_height_by_hash(&tip_hash).await; + // Drop locks before potentially long operations drop(engine); drop(storage); + match tip_height { + Ok(Some(height)) => self.store_qr_info(height, qr_info).await, + Ok(None) => tracing::warn!( + "QRInfo tip {tip_hash} has no known height, rotated quorums will not survive a restart" + ), + Err(e) => { + tracing::warn!("Could not resolve QRInfo tip {tip_hash} height: {e}") + } + } + if let Some(ref qr_info_result) = qr_info_result { tracing::info!( "QRInfo processed: stored_cycle_height={:?}, rotated_quorum_count={}/{}, fully_verified_count={}, newly_qualified_count={}, cycle_key_unresolved={}, previous_cycle_invalid_count={}", @@ -431,6 +385,10 @@ impl SyncManager for MasternodesManager { }; drop(engine); + if apply_ok { + self.store_diff(target_height, diff).await; + } + self.progress.add_diffs_processed(1); self.sync_state.mnlistdiff_pipeline.receive(diff); self.sync_state.mnlistdiff_pipeline.send_pending(requests)?; @@ -747,14 +705,13 @@ impl SyncManager for MasternodesManager { mod tests { use super::super::manager::{MasternodeSyncState, QRInfoInFlight}; use super::{ - feed_qrinfo_heights_to_engine, qrinfo_timeout_for, MAX_RETRY_ATTEMPTS, - QRINFO_STALL_WATCHDOG, QRINFO_TIMEOUT_SCHEDULE_SECS, + qrinfo_timeout_for, MAX_RETRY_ATTEMPTS, QRINFO_STALL_WATCHDOG, QRINFO_TIMEOUT_SCHEDULE_SECS, }; use crate::error::StorageResult; use crate::network::{Message, NetworkRequest, RequestSender}; use crate::storage::{ - BlockHeaderStorage, BlockHeaderTip, DiskStorageManager, PersistentBlockHeaderStorage, - StorageManager, + feed_qrinfo_heights_to_engine, BlockHeaderStorage, BlockHeaderTip, DiskStorageManager, + PersistentBlockHeaderStorage, StorageManager, }; use crate::sync::{MasternodesManager, SyncManager, SyncState}; use crate::types::HashedBlockHeader; @@ -920,9 +877,7 @@ mod tests { network: Network::Testnet, ..Default::default() }; - feed_qrinfo_heights_to_engine(&mut engine, &qr_info, &MockHeaderStorage(height_map)) - .await - .unwrap(); + feed_qrinfo_heights_to_engine(&mut engine, &qr_info, &MockHeaderStorage(height_map)).await; for &b in expected_hashes { let hash = BlockHash::from_slice(&[b; 32]).unwrap(); @@ -1097,9 +1052,13 @@ mod tests { .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; + let mut manager = MasternodesManager::new( + block_headers, + Arc::new(RwLock::new(engine)), + Network::Regtest, + None, + ) + .await; manager.progress.update_block_header_tip_height(tip); let (tx, mut rx) = mpsc::unbounded_channel(); diff --git a/dash-spv/tests/dashd_masternode/helpers.rs b/dash-spv/tests/dashd_masternode/helpers.rs index 8166df9bb..d4b6d0269 100644 --- a/dash-spv/tests/dashd_masternode/helpers.rs +++ b/dash-spv/tests/dashd_masternode/helpers.rs @@ -17,12 +17,6 @@ use super::setup::{TestContext, SYNC_TIMEOUT}; /// Mine a DKG cycle and wait for the SPV to surface a `MasternodeStateUpdated` /// event above `baseline_height`. -/// Files held under each immediate subdirectory of the storage root, keyed by -/// directory name. -/// -/// A sync writes into these and never removes a whole class of state, so across -/// a restart every directory must still be there and hold at least as much — -/// see [`assert_storage_did_not_shrink`]. pub(super) fn storage_snapshot(root: &Path) -> BTreeMap { let mut counts = BTreeMap::new(); let Ok(entries) = std::fs::read_dir(root) else { @@ -55,20 +49,14 @@ fn walkdir_count(dir: &Path) -> usize { .sum() } -/// Directories that must hold state once this test's first session has run, and -/// why. `filters` and `blocks` are deliberately absent: the client is stopped -/// as soon as the masternode phase reports `Synced`, which is before the filter -/// phase leaves `WaitForEvents`, so those stay legitimately empty here. pub(super) const EXPECTED_STORAGE: &[(&str, &str)] = &[ ("block_headers", "headers synced to the tip"), ("filter_headers", "filter headers synced to the tip"), ("metadata", "sync checkpoints"), ("peers", "peer set and reputations"), - ("masternodestate", "the masternode list this session built"), + ("masternodes", "the masternode messages this session stored"), ]; -/// Assert every directory in [`EXPECTED_STORAGE`] exists and holds at least one -/// file, reporting all of them at once rather than the first to fail. pub(super) fn assert_storage_persisted(snapshot: &BTreeMap, what: &str) { let missing: Vec = EXPECTED_STORAGE .iter() @@ -84,9 +72,6 @@ pub(super) fn assert_storage_persisted(snapshot: &BTreeMap, what: ); } -/// Every directory present before a restart must still be present after, with -/// at least as many files. A directory that vanishes or shrinks means a restart -/// threw away state that the previous session had already earned. pub(super) fn assert_storage_did_not_shrink( before: &BTreeMap, after: &BTreeMap, diff --git a/dash-spv/tests/dashd_masternode/tests_sync.rs b/dash-spv/tests/dashd_masternode/tests_sync.rs index b38d28783..1647b9193 100644 --- a/dash-spv/tests/dashd_masternode/tests_sync.rs +++ b/dash-spv/tests/dashd_masternode/tests_sync.rs @@ -105,8 +105,6 @@ async fn test_masternode_list_sync_with_restart() { wait_for_masternode_sync(&mut client_handle.progress_receiver, SYNC_TIMEOUT).await; let first_height = first_mn_progress.current_height(); - // Control: the first session really built a list, so the persistence - // assertion below cannot be satisfied by a client that synced nothing. let first_masternodes = { let engine = client_handle.engine.read().await; engine.masternode_lists.values().map(|list| list.masternodes.len()).max().unwrap_or(0) @@ -119,8 +117,6 @@ async fn test_masternode_list_sync_with_restart() { client_handle.stop().await; drop(client_handle); - // What the first session earned and wrote down. A clean shutdown of a - // fully-synced client must leave every sync phase's state on disk. let after_first = storage_snapshot(ctx.storage_path()); assert_storage_persisted( &after_first, @@ -144,8 +140,6 @@ async fn test_masternode_list_sync_with_restart() { "Should reach Synced state after restart" ); - // A restart re-syncs on top of what it restored; it never discards a whole - // class of state it already had. let after_second = storage_snapshot(ctx.storage_path()); assert_storage_did_not_shrink(&after_first, &after_second, "masternode restart"); diff --git a/dash/src/ephemerealdata/chain_lock.rs b/dash/src/ephemerealdata/chain_lock.rs index f24bd69a9..fb8b07881 100644 --- a/dash/src/ephemerealdata/chain_lock.rs +++ b/dash/src/ephemerealdata/chain_lock.rs @@ -13,7 +13,7 @@ use crate::bls_sig_utils::BLSSignature; use crate::consensus::Encodable; use crate::hash_types::QuorumSigningSignId; use crate::internal_macros::impl_consensus_encoding; -use crate::sml::llmq_type::LLMQType; +use crate::sml::llmq_type::{LLMQ_SIGN_HEIGHT_OFFSET, LLMQType}; use crate::{BlockHash, QuorumHash, QuorumSigningRequestId, VarInt, io}; const CL_REQUEST_ID_PREFIX: &str = "clsig"; @@ -53,6 +53,11 @@ impl ChainLock { Ok(QuorumSigningRequestId::from_engine(engine)) } + /// Height of the masternode list that holds the quorum signing this ChainLock. + pub fn signing_height(&self) -> u32 { + self.block_height.saturating_sub(LLMQ_SIGN_HEIGHT_OFFSET) + } + pub fn sign_id( &self, quorum_type: LLMQType, diff --git a/dash/src/sml/llmq_type/mod.rs b/dash/src/sml/llmq_type/mod.rs index dc0fb52b7..46778da8e 100644 --- a/dash/src/sml/llmq_type/mod.rs +++ b/dash/src/sml/llmq_type/mod.rs @@ -49,6 +49,16 @@ pub struct LLMQParams { pub recovery_members: u32, } +/// Blocks below the tip whose active LLMQ set a signing session picks its quorum +/// from, per DIP-0007. Type-independent: it applies to every signing session, +/// ChainLocks among them. +pub const LLMQ_SIGN_HEIGHT_OFFSET: u32 = 8; + +/// Blocks below a quorum's own height whose masternode list its members are +/// selected from, per DIP-0024. The lag keeps the selection off a block that is +/// still being mined, which Dash Core needs for evoDB consistency. +pub const QUORUM_MEMBER_LIST_OFFSET: u32 = 8; + pub const DKG_TEST: DKGParams = DKGParams { interval: 24, phase_blocks: 2, diff --git a/dash/src/sml/masternode_list_engine/helpers.rs b/dash/src/sml/masternode_list_engine/helpers.rs index 9dfbaeccc..052f2a4e9 100644 --- a/dash/src/sml/masternode_list_engine/helpers.rs +++ b/dash/src/sml/masternode_list_engine/helpers.rs @@ -2,6 +2,7 @@ use crate::QuorumHash; use crate::prelude::CoreBlockHeight; use crate::sml::llmq_entry_verification::LLMQEntryVerificationStatus; use crate::sml::llmq_type::LLMQType; +use crate::sml::llmq_type::network::NetworkLLMQExt; use crate::sml::masternode_list::MasternodeList; use crate::sml::masternode_list_engine::MasternodeListEngine; use crate::sml::quorum_entry::qualified_quorum_entry::QualifiedQuorumEntry; @@ -14,6 +15,23 @@ use crate::sml::quorum_entry::qualified_quorum_entry::QualifiedQuorumEntry; const QUORUM_WALK_BACK_ACTIVE_WINDOWS: u32 = 4; impl MasternodeListEngine { + pub fn prune_obsolete_lists(&mut self, tip: CoreBlockHeight) -> usize { + let params = self.network.chain_locks_type().params(); + + let floor = tip.saturating_sub( + params + .signing_active_quorum_count + .saturating_mul(params.dkg_params.interval) + .saturating_mul(QUORUM_WALK_BACK_ACTIVE_WINDOWS), + ); + + let before = self.masternode_lists.len(); + + self.masternode_lists.retain(|height, _| *height >= floor); + + before - self.masternode_lists.len() + } + /// Retrieves the closest masternode lists before and after a given core block height. /// /// This function searches the `masternode_lists` map to find the nearest masternode lists diff --git a/dash/src/sml/masternode_list_engine/message_request_verification.rs b/dash/src/sml/masternode_list_engine/message_request_verification.rs index 662626ace..5fe12c9f7 100644 --- a/dash/src/sml/masternode_list_engine/message_request_verification.rs +++ b/dash/src/sml/masternode_list_engine/message_request_verification.rs @@ -230,7 +230,7 @@ impl MasternodeListEngine { } /// Retrieves the potential quorum for verifying a ChainLock from the masternode list **before or at** - /// block height **(chain_lock.block_height - 8)**. + /// the ChainLock's [signing height](ChainLock::signing_height). /// /// This function attempts to find the quorum responsible for signing the ChainLock by looking at /// the masternode list at or before the signing height, following DIP 24 logic. @@ -251,9 +251,7 @@ impl MasternodeListEngine { &self, chain_lock: &ChainLock, ) -> Result, MessageVerificationError> { - // Retrieve the masternode list at or before (block_height - 8) - let (before, _) = - self.masternode_lists_around_height(chain_lock.block_height.saturating_sub(8)); + let (before, _) = self.masternode_lists_around_height(chain_lock.signing_height()); // Compute the signing request ID let request_id = chain_lock.request_id().map_err(|e| e.to_string())?; @@ -271,7 +269,7 @@ impl MasternodeListEngine { } /// Retrieves the potential quorum for verifying a ChainLock from the masternode list **after** - /// block height **(chain_lock.block_height - 8)**. + /// the ChainLock's [signing height](ChainLock::signing_height). /// /// This function looks at the next available masternode list to determine if a quorum exists /// for signing the ChainLock, following DIP 24. @@ -292,9 +290,7 @@ impl MasternodeListEngine { &self, chain_lock: &ChainLock, ) -> Result, MessageVerificationError> { - // Retrieve the masternode list after (block_height - 8) - let (_, after) = - self.masternode_lists_around_height(chain_lock.block_height.saturating_sub(8)); + let (_, after) = self.masternode_lists_around_height(chain_lock.signing_height()); // Compute the signing request ID let request_id = chain_lock.request_id().map_err(|e| e.to_string())?; @@ -331,7 +327,7 @@ impl MasternodeListEngine { /// - `Other`: If computing the request ID or signing ID fails. /// /// # Implementation Details - /// - Retrieves masternode lists **before and after** `chain_lock.block_height - 8`. + /// - Retrieves masternode lists **before and after** the ChainLock's signing height. /// - Finds the **quorum with the lowest ordering hash** for the signing request. /// - Computes the **signing ID** and verifies the ChainLock signature. /// - If verification fails with the "before" list, it attempts verification with the "after" list. @@ -339,21 +335,14 @@ impl MasternodeListEngine { &self, chain_lock: &ChainLock, ) -> Result<(), MessageVerificationError> { - // Retrieve masternode lists surrounding the signing height (block_height - 8) - let (before, after) = - self.masternode_lists_around_height(chain_lock.block_height.saturating_sub(8)); + let (before, after) = self.masternode_lists_around_height(chain_lock.signing_height()); if before.is_none() && after.is_none() { return Err(MessageVerificationError::NoMasternodeLists); } - // Compute the signing request ID - let request_id = chain_lock.request_id().map_err(|e| e.to_string())?; - // Attempt verification using the "before" masternode list let initial_error = if let Some(before) = before { - let Err(e) = - self.verify_chain_lock_with_masternode_list(chain_lock, before, &request_id) - else { + let Err(e) = self.verify_chain_lock_with_masternode_list(chain_lock, before) else { return Ok(()); }; Some(e) @@ -373,7 +362,7 @@ impl MasternodeListEngine { true }; if do_check { - return self.verify_chain_lock_with_masternode_list(chain_lock, after, &request_id); + return self.verify_chain_lock_with_masternode_list(chain_lock, after); } else if let Some(initial_error) = initial_error { return Err(initial_error); } @@ -382,13 +371,13 @@ impl MasternodeListEngine { Ok(()) } - /// Helper function to verify a ChainLock using a specific masternode list. - fn verify_chain_lock_with_masternode_list( + pub fn verify_chain_lock_with_masternode_list( &self, chain_lock: &ChainLock, masternode_list: &MasternodeList, - request_id: &QuorumSigningRequestId, ) -> Result<(), MessageVerificationError> { + let request_id = chain_lock.request_id().map_err(|e| e.to_string())?; + // Get the quorum type for ChainLocks in the current network let chain_lock_quorum_type = self.network.chain_locks_type(); @@ -398,7 +387,7 @@ impl MasternodeListEngine { let quorum = quorums_of_type .values() - .min_by_key(|quorum| QuorumOrderingHash::create(&quorum.quorum_entry, request_id)) + .min_by_key(|quorum| QuorumOrderingHash::create(&quorum.quorum_entry, &request_id)) .ok_or(MessageVerificationError::MasternodeListHasNoQuorums( masternode_list.known_height, ))?; @@ -407,7 +396,7 @@ impl MasternodeListEngine { .sign_id( quorum.quorum_entry.llmq_type, quorum.quorum_entry.quorum_hash, - Some(*request_id), + Some(request_id), ) .map_err(|e| e.to_string())?; diff --git a/dash/src/sml/masternode_list_engine/mod.rs b/dash/src/sml/masternode_list_engine/mod.rs index 0be111c73..4df40bed6 100644 --- a/dash/src/sml/masternode_list_engine/mod.rs +++ b/dash/src/sml/masternode_list_engine/mod.rs @@ -764,6 +764,35 @@ impl MasternodeListEngine { hashes } + /// The work block of every rotation cycle the QRInfo carries: `h`, `h-c`, + /// `h-2c`, `h-3c` and, when shared, `h-4c`. `mn_list_diff_tip` is not a + /// cycle boundary and `mn_list_diff_list` carries no snapshot to key, so + /// neither is included. + /// + /// A caller holding its own header chain resolves these to heights and + /// feeds back the block at [`Self::cycle_boundary_height`], which is the + /// cycle base that keys the cycle's rotated quorums in storage. + pub fn qr_info_work_block_hashes(qr_info: &QRInfo) -> Vec { + let mut hashes = vec![ + qr_info.mn_list_diff_h.block_hash, + qr_info.mn_list_diff_at_h_minus_c.block_hash, + qr_info.mn_list_diff_at_h_minus_2c.block_hash, + qr_info.mn_list_diff_at_h_minus_3c.block_hash, + ]; + + if let Some((_, diff)) = &qr_info.quorum_snapshot_and_mn_list_diff_at_h_minus_4c { + hashes.push(diff.block_hash); + } + + hashes + } + + /// Cycle base height for a rotation cycle whose work block sits at + /// `work_block_height`. + pub fn cycle_boundary_height(work_block_height: CoreBlockHeight) -> CoreBlockHeight { + work_block_height.saturating_add(WORK_DIFF_DEPTH) + } + /// `true` iff `rotated_quorums_per_cycle` already holds a complete cycle /// for `cycle_hash`: one `Verified` entry for every active rotation slot. /// Used by the storage gate to refuse downgrading a cycle and by the @@ -2117,7 +2146,7 @@ mod tests { for (quorum_hash, quorum) in quorum_entries.iter() { if !quorum_type.is_rotating_quorum_type() { let (_, known_block_height) = mn_list_engine - .masternode_list_and_height_for_block_hash_8_blocks_ago( + .masternode_list_and_height_for_quorum_members( &quorum.quorum_entry.quorum_hash, ) .expect("expected to find validating masternode"); diff --git a/dash/src/sml/masternode_list_engine/non_rotated_quorum_construction.rs b/dash/src/sml/masternode_list_engine/non_rotated_quorum_construction.rs index 6d40f5571..2c1e4a04f 100644 --- a/dash/src/sml/masternode_list_engine/non_rotated_quorum_construction.rs +++ b/dash/src/sml/masternode_list_engine/non_rotated_quorum_construction.rs @@ -1,5 +1,6 @@ use crate::BlockHash; use crate::prelude::CoreBlockHeight; +use crate::sml::llmq_type::QUORUM_MEMBER_LIST_OFFSET; use crate::sml::masternode_list::MasternodeList; use crate::sml::masternode_list_engine::MasternodeListEngine; use crate::sml::masternode_list_entry::qualified_masternode_list_entry::QualifiedMasternodeListEntry; @@ -11,24 +12,22 @@ use crate::sml::quorum_validation_error::QuorumValidationError; impl MasternodeListEngine { #[allow(dead_code)] - pub(crate) fn masternode_list_and_height_for_block_hash_8_blocks_ago( + pub(crate) fn masternode_list_and_height_for_quorum_members( &self, block_hash: &BlockHash, ) -> Result<(&MasternodeList, CoreBlockHeight), QuorumValidationError> { - if let Some(height) = self.block_container.get_height(block_hash) { - if let Some(masternode_list) = self.masternode_lists.get(&(height.saturating_sub(8))) { - Ok((masternode_list, height.saturating_sub(8))) - } else { - Err(QuorumValidationError::RequiredMasternodeListNotPresent( - height.saturating_sub(8), - )) - } - } else { - Err(QuorumValidationError::RequiredBlockNotPresent( + let Some(height) = self.block_container.get_height(block_hash) else { + return Err(QuorumValidationError::RequiredBlockNotPresent( *block_hash, - "looking for masternode list and height for block hash 8 blocks ago".to_string(), - )) - } + "looking for the masternode list a quorum's members are selected from".to_string(), + )); + }; + + let member_list_height = height.saturating_sub(QUORUM_MEMBER_LIST_OFFSET); + self.masternode_lists + .get(&member_list_height) + .map(|masternode_list| (masternode_list, member_list_height)) + .ok_or(QuorumValidationError::RequiredMasternodeListNotPresent(member_list_height)) } #[allow(dead_code)] @@ -36,10 +35,8 @@ impl MasternodeListEngine { &self, quorum: &QualifiedQuorumEntry, ) -> Result, QuorumValidationError> { - let (masternode_list, known_block_height) = self - .masternode_list_and_height_for_block_hash_8_blocks_ago( - &quorum.quorum_entry.quorum_hash, - )?; + let (masternode_list, known_block_height) = + self.masternode_list_and_height_for_quorum_members(&quorum.quorum_entry.quorum_hash)?; let Some(VerifyingChainLockSignaturesType::NonRotating(chain_lock_sig)) = quorum.verifying_chain_lock_signature else { From 5910b2466e59a17244315ace2d2828a38509e0f3 Mon Sep 17 00:00:00 2001 From: Borja Castellano Date: Tue, 8 Sep 2026 16:34:16 +0000 Subject: [PATCH 3/4] test(dash-spv): masternode storage unit tests --- dash-spv/src/storage/masternode.rs | 207 ++++++++++++++++++ dash-spv/src/sync/masternodes/sync_manager.rs | 140 +++--------- dash-spv/src/test_utils/header_storage.rs | 49 +++++ dash-spv/src/test_utils/mod.rs | 2 + dash/src/test_utils/mod.rs | 1 + dash/src/test_utils/sml.rs | 96 ++++++++ 6 files changed, 381 insertions(+), 114 deletions(-) create mode 100644 dash-spv/src/test_utils/header_storage.rs create mode 100644 dash/src/test_utils/sml.rs diff --git a/dash-spv/src/storage/masternode.rs b/dash-spv/src/storage/masternode.rs index ade8dd3ec..99c61d985 100644 --- a/dash-spv/src/storage/masternode.rs +++ b/dash-spv/src/storage/masternode.rs @@ -327,3 +327,210 @@ pub(crate) async fn feed_qrinfo_heights_to_engine( tracing::info!("Fed {} block heights to engine", fed_count); } + +#[cfg(test)] +mod tests { + use super::*; + use crate::test_utils::MockHeaderStorage; + use dashcore::BlockHash; + use dashcore_hashes::Hash; + + use tempfile::TempDir; + + fn hash(byte: u8) -> BlockHash { + BlockHash::from_slice(&[byte; 32]).unwrap() + } + + async fn open_storage( + dir: &TempDir, + heights: &[(u8, u32)], + ) -> PersistentMasternodeStorage { + let map = heights.iter().map(|(b, h)| (hash(*b), *h)).collect(); + PersistentMasternodeStorage::open( + dir.path(), + Arc::new(RwLock::new(MockHeaderStorage(map))), + Network::Regtest, + ) + .await + .expect("open") + } + + #[tokio::test] + async fn cached_list_is_served_only_inside_its_validity_window() { + let dir = TempDir::new().unwrap(); + let storage = open_storage(&dir, &[]).await; + + *storage.cached_list.lock().await = Some(CachedList { + from: 100, + until: Some(200), + list: None, + }); + + assert!(storage.cached_list_at(99).await.is_none(), "below `from` is a miss"); + assert!(storage.cached_list_at(100).await.is_some(), "`from` itself is a hit"); + assert!(storage.cached_list_at(199).await.is_some(), "last height below `until` hits"); + assert!(storage.cached_list_at(200).await.is_none(), "`until` is exclusive"); + assert!(storage.cached_list_at(10_000).await.is_none(), "above the window is a miss"); + + *storage.cached_list.lock().await = Some(CachedList { + from: 100, + until: None, + list: None, + }); + + assert!(storage.cached_list_at(99).await.is_none(), "`from` still bounds an open window"); + assert!( + storage.cached_list_at(u32::MAX).await.is_some(), + "no upper bound covers every later height" + ); + } + + async fn prime_cache(storage: &PersistentMasternodeStorage) { + *storage.cached_list.lock().await = Some(CachedList { + from: 0, + until: None, + list: None, + }); + } + + #[tokio::test] + async fn storing_a_message_invalidates_the_cached_list() { + let dir = TempDir::new().unwrap(); + let mut storage = open_storage(&dir, &[]).await; + + prime_cache(&storage).await; + storage.store_diff(100, &MnListDiff::dummy(0x00, 0xAA)).await.expect("store diff"); + assert!(storage.cached_list.lock().await.is_none(), "store_diff must drop the cache"); + + prime_cache(&storage).await; + storage.store_qr_info(200, &QRInfo::dummy(0xBB)).await.expect("store qr_info"); + assert!(storage.cached_list.lock().await.is_none(), "store_qr_info must drop the cache"); + } + + #[tokio::test] + async fn replay_retries_until_no_more_messages_apply() { + let dir = TempDir::new().unwrap(); + let mut storage = + open_storage(&dir, &[(0x00, 0), (0xAA, 100), (0xBB, 60), (0xCC, 50), (0xDD, 40)]).await; + + // Heights descend while dependencies ascend, so each pass resolves one link. + storage.store_diff(100, &MnListDiff::dummy(0x00, 0xAA)).await.unwrap(); + storage.store_diff(60, &MnListDiff::dummy(0xAA, 0xBB)).await.unwrap(); + storage.store_diff(50, &MnListDiff::dummy(0xBB, 0xCC)).await.unwrap(); + storage.store_diff(40, &MnListDiff::dummy(0xEE, 0xDD)).await.unwrap(); + + let engine = storage.replay().await.expect("replay must not fail on an orphan"); + + assert!( + engine.masternode_lists.contains_key(&100), + "the genesis-based diff applies on the first pass" + ); + assert!( + engine.masternode_lists.contains_key(&60), + "the first link resolves on the second pass" + ); + assert!( + engine.masternode_lists.contains_key(&50), + "the loop keeps going while it is still making progress" + ); + assert!( + !engine.masternode_lists.contains_key(&40), + "the orphan has no reachable base and is left to the network" + ); + } + + async fn storage_with_lists_at_100_200_300( + dir: &TempDir, + ) -> PersistentMasternodeStorage { + let mut storage = + open_storage(dir, &[(0x00, 0), (0xAA, 100), (0xBB, 200), (0xCC, 300)]).await; + storage.store_diff(100, &MnListDiff::dummy(0x00, 0xAA)).await.unwrap(); + storage.store_diff(200, &MnListDiff::dummy(0xAA, 0xBB)).await.unwrap(); + storage.store_diff(300, &MnListDiff::dummy(0xBB, 0xCC)).await.unwrap(); + storage + } + + #[tokio::test] + async fn lookup_caches_the_window_between_the_lists_around_the_height() { + let dir = TempDir::new().unwrap(); + let storage = storage_with_lists_at_100_200_300(&dir).await; + + let list = storage.masternode_list_at_or_before(250).await.unwrap(); + assert_eq!(list.map(|l| l.known_height), Some(200), "the list at or below 250"); + + let cached = storage.cached_list.lock().await.take().expect("lookup must cache"); + assert_eq!(cached.from, 200, "valid from the list it returned"); + assert_eq!(cached.until, Some(300), "and only up to the next one"); + } + + #[tokio::test] + async fn lookup_below_every_list_caches_the_absence_up_to_the_first_one() { + let dir = TempDir::new().unwrap(); + let storage = storage_with_lists_at_100_200_300(&dir).await; + + assert!(storage.masternode_list_at_or_before(50).await.unwrap().is_none()); + + let cached = storage.cached_list.lock().await.take().expect("an absence is cacheable too"); + assert_eq!(cached.from, 0, "nothing below the first list, all the way down"); + assert_eq!(cached.until, Some(100), "up to the first list there is"); + assert!(cached.list.is_none()); + } + + #[tokio::test] + async fn lookup_at_or_above_the_newest_list_caches_an_open_window() { + let dir = TempDir::new().unwrap(); + let storage = storage_with_lists_at_100_200_300(&dir).await; + + let list = storage.masternode_list_at_or_before(10_000).await.unwrap(); + assert_eq!(list.map(|l| l.known_height), Some(300)); + + let cached = storage.cached_list.lock().await.take().expect("lookup must cache"); + assert_eq!(cached.from, 300); + assert_eq!(cached.until, None, "no later list, so nothing bounds it"); + } + + #[tokio::test] + async fn storing_the_same_height_twice_keeps_the_newer_message() { + let dir = TempDir::new().unwrap(); + let mut storage = open_storage(&dir, &[]).await; + + storage.store_diff(100, &MnListDiff::dummy(0x00, 0xAA)).await.unwrap(); + storage.store_diff(100, &MnListDiff::dummy(0x00, 0xBB)).await.unwrap(); + + assert_eq!(storage.diffs.len(), 1, "one file per height"); + let stored: MnListDiff = + PersistentMasternodeStorage::::read_message(&storage.diffs[&100]) + .await + .expect("read back"); + assert_eq!(stored.block_hash, hash(0xBB), "the second write wins"); + } + + #[tokio::test] + async fn replay_survives_unreadable_files_and_ignores_foreign_names() { + let dir = TempDir::new().unwrap(); + let folder = dir.path().join(PersistentMasternodeStorage::::FOLDER_NAME); + tokio::fs::create_dir_all(&folder).await.unwrap(); + + { + let mut storage = open_storage(&dir, &[(0x00, 0), (0xAA, 100)]).await; + storage.store_diff(100, &MnListDiff::dummy(0x00, 0xAA)).await.unwrap(); + } + + tokio::fs::write(folder.join("diff_50.dat"), b"not a diff").await.unwrap(); + tokio::fs::write(folder.join("diff_abc.dat"), b"x").await.unwrap(); + + let storage = open_storage(&dir, &[(0x00, 0), (0xAA, 100)]).await; + assert_eq!( + storage.diffs.keys().copied().collect::>(), + vec![50, 100], + "only well-formed names are indexed" + ); + assert!(storage.qr_infos.is_empty()); + + let engine = storage.replay().await.expect("a corrupt file must not fail the load"); + assert!( + engine.masternode_lists.contains_key(&100), + "the readable message still rebuilds its list" + ); + } +} diff --git a/dash-spv/src/sync/masternodes/sync_manager.rs b/dash-spv/src/sync/masternodes/sync_manager.rs index 38c01d33a..fb2f899a5 100644 --- a/dash-spv/src/sync/masternodes/sync_manager.rs +++ b/dash-spv/src/sync/masternodes/sync_manager.rs @@ -707,95 +707,35 @@ mod tests { use super::{ qrinfo_timeout_for, MAX_RETRY_ATTEMPTS, QRINFO_STALL_WATCHDOG, QRINFO_TIMEOUT_SCHEDULE_SECS, }; - use crate::error::StorageResult; + use crate::network::{Message, NetworkRequest, RequestSender}; use crate::storage::{ - feed_qrinfo_heights_to_engine, BlockHeaderStorage, BlockHeaderTip, DiskStorageManager, + feed_qrinfo_heights_to_engine, BlockHeaderStorage, DiskStorageManager, PersistentBlockHeaderStorage, StorageManager, }; use crate::sync::{MasternodesManager, SyncManager, SyncState}; + use crate::test_utils::MockHeaderStorage; use crate::types::HashedBlockHeader; use crate::SyncError; - use async_trait::async_trait; + use dashcore::block::Header; use dashcore::bls_sig_utils::{BLSPublicKey, BLSSignature}; use dashcore::hash_types::QuorumVVecHash; use dashcore::network::message::NetworkMessage; - use dashcore::network::message_qrinfo::{MNSkipListMode, QRInfo, QuorumSnapshot}; + use dashcore::network::message_qrinfo::{QRInfo, QuorumSnapshot}; use dashcore::network::message_sml::MnListDiff; use dashcore::sml::llmq_type::LLMQType; use dashcore::sml::masternode_list_engine::MasternodeListEngine; use dashcore::transaction::special_transaction::quorum_commitment::QuorumEntry; - use dashcore::{BlockHash, Network, Transaction}; + use dashcore::{BlockHash, Network}; use dashcore_hashes::Hash; use std::collections::HashMap; - use std::ops::Range; + use std::sync::Arc; use std::time::Duration; use std::time::Instant; use tokio::sync::{mpsc, RwLock}; - struct MockHeaderStorage(HashMap); - - #[async_trait] - impl BlockHeaderStorage for MockHeaderStorage { - async fn store_headers(&mut self, _: &[HashedBlockHeader]) -> StorageResult<()> { - Ok(()) - } - async fn store_headers_at_height( - &mut self, - _: &[HashedBlockHeader], - _: u32, - ) -> StorageResult<()> { - Ok(()) - } - async fn load_headers(&self, _: Range) -> StorageResult> { - Ok(vec![]) - } - async fn get_tip_height(&self) -> Option { - None - } - async fn get_tip(&self) -> Option { - None - } - async fn get_start_height(&self) -> Option { - None - } - async fn get_stored_headers_len(&self) -> u32 { - 0 - } - async fn get_header_height_by_hash(&self, hash: &BlockHash) -> StorageResult> { - Ok(self.0.get(hash).copied()) - } - async fn truncate_above(&mut self, target_height: u32) -> StorageResult<()> { - self.0.retain(|_, h| *h <= target_height); - Ok(()) - } - } - - fn make_diff(base_byte: u8, tip_byte: u8) -> MnListDiff { - MnListDiff { - version: 1, - base_block_hash: BlockHash::from_slice(&[base_byte; 32]).unwrap(), - block_hash: BlockHash::from_slice(&[tip_byte; 32]).unwrap(), - total_transactions: 0, - merkle_hashes: vec![], - merkle_flags: vec![], - coinbase_tx: Transaction { - version: 1, - lock_time: 0, - input: vec![], - output: vec![], - special_transaction_payload: None, - }, - deleted_masternodes: vec![], - new_masternodes: vec![], - deleted_quorums: vec![], - new_quorums: vec![], - quorums_chainlock_signatures: vec![], - } - } - fn make_quorum_entry(hash_byte: u8, index: i16) -> QuorumEntry { QuorumEntry { version: 1, @@ -811,14 +751,6 @@ mod tests { } } - fn make_snapshot() -> QuorumSnapshot { - QuorumSnapshot { - skip_list_mode: MNSkipListMode::NoSkipping, - active_quorum_members: vec![], - skip_list: vec![], - } - } - /// Verifies that `feed_qrinfo_heights_to_engine` feeds the engine's /// `block_container` with heights for every hash source in a `QRInfo` message: /// - base and tip hashes for each of the five standard diffs @@ -852,25 +784,25 @@ mod tests { } let qr_info = QRInfo { - quorum_snapshot_at_h_minus_c: make_snapshot(), - quorum_snapshot_at_h_minus_2c: make_snapshot(), - quorum_snapshot_at_h_minus_3c: make_snapshot(), - mn_list_diff_tip: make_diff(0x01, 0x02), - mn_list_diff_h: make_diff(0x03, 0x04), - mn_list_diff_at_h_minus_c: make_diff(0x05, 0x06), - mn_list_diff_at_h_minus_2c: make_diff(0x07, 0x08), - mn_list_diff_at_h_minus_3c: make_diff(0x09, 0x0A), + quorum_snapshot_at_h_minus_c: QuorumSnapshot::dummy(), + quorum_snapshot_at_h_minus_2c: QuorumSnapshot::dummy(), + quorum_snapshot_at_h_minus_3c: QuorumSnapshot::dummy(), + mn_list_diff_tip: MnListDiff::dummy_empty(0x01, 0x02), + mn_list_diff_h: MnListDiff::dummy_empty(0x03, 0x04), + mn_list_diff_at_h_minus_c: MnListDiff::dummy_empty(0x05, 0x06), + mn_list_diff_at_h_minus_2c: MnListDiff::dummy_empty(0x07, 0x08), + mn_list_diff_at_h_minus_3c: MnListDiff::dummy_empty(0x09, 0x0A), quorum_snapshot_and_mn_list_diff_at_h_minus_4c: Some(( - make_snapshot(), - make_diff(0x0B, 0x0C), + QuorumSnapshot::dummy(), + MnListDiff::dummy_empty(0x0B, 0x0C), )), - mn_list_diff_list: vec![make_diff(0x0D, 0x0E)], + mn_list_diff_list: vec![MnListDiff::dummy_empty(0x0D, 0x0E)], last_commitment_per_index: [0x80u8, 0x81, 0x82, 0x83] .iter() .enumerate() .map(|(i, &b)| make_quorum_entry(b, i as i16)) .collect(), - quorum_snapshot_list: vec![make_snapshot()], + quorum_snapshot_list: vec![QuorumSnapshot::dummy()], }; let mut engine = MasternodeListEngine { @@ -917,25 +849,6 @@ mod tests { assert_eq!(qrinfo_timeout_for(u8::MAX).as_secs(), last); } - /// Build a minimal `QRInfo` whose `mn_list_diff_tip.block_hash` is `[tip_byte; 32]`. - /// Only the tip hash is read by `should_process_qrinfo`; every other field is filler. - fn qrinfo_with_tip(tip_byte: u8) -> QRInfo { - QRInfo { - quorum_snapshot_at_h_minus_c: make_snapshot(), - quorum_snapshot_at_h_minus_2c: make_snapshot(), - quorum_snapshot_at_h_minus_3c: make_snapshot(), - mn_list_diff_tip: make_diff(0x00, tip_byte), - mn_list_diff_h: make_diff(0x00, 0x00), - mn_list_diff_at_h_minus_c: make_diff(0x00, 0x00), - mn_list_diff_at_h_minus_2c: make_diff(0x00, 0x00), - mn_list_diff_at_h_minus_3c: make_diff(0x00, 0x00), - quorum_snapshot_and_mn_list_diff_at_h_minus_4c: None, - mn_list_diff_list: vec![], - last_commitment_per_index: vec![], - quorum_snapshot_list: vec![], - } - } - /// `should_process_qrinfo` is the dedup gate at the QRInfo handler entry. It /// must: /// 1. Drop a response carrying the same `mn_list_diff_tip.block_hash` as the @@ -964,7 +877,7 @@ mod tests { ..Default::default() }; assert!( - !state.should_process_qrinfo(&qrinfo_with_tip(0xAA)), + !state.should_process_qrinfo(&QRInfo::dummy(0xAA)), "duplicate of last processed tip must be dropped" ); @@ -973,7 +886,7 @@ mod tests { let state = MasternodeSyncState::default(); assert!(state.qrinfo_in_flight.is_none()); assert!( - !state.should_process_qrinfo(&qrinfo_with_tip(0xBB)), + !state.should_process_qrinfo(&QRInfo::dummy(0xBB)), "unsolicited response must be dropped" ); @@ -984,7 +897,7 @@ mod tests { ..Default::default() }; assert!( - state.should_process_qrinfo(&qrinfo_with_tip(0xBB)), + state.should_process_qrinfo(&QRInfo::dummy(0xBB)), "response matching the active request tip must be accepted" ); @@ -997,7 +910,7 @@ mod tests { ..Default::default() }; assert!( - !state.should_process_qrinfo(&qrinfo_with_tip(0xAA)), + !state.should_process_qrinfo(&QRInfo::dummy(0xAA)), "duplicate must be dropped even when no request is in flight" ); @@ -1010,15 +923,14 @@ mod tests { ..Default::default() }; assert!( - !state.should_process_qrinfo(&qrinfo_with_tip(0xCC)), + !state.should_process_qrinfo(&QRInfo::dummy(0xCC)), "response for non-active request tip must be dropped" ); } - /// 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. + /// [`QRInfo::dummy`] 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); + let mut qr_info = QRInfo::dummy(0x00); qr_info.mn_list_diff_tip.block_hash = tip; qr_info } diff --git a/dash-spv/src/test_utils/header_storage.rs b/dash-spv/src/test_utils/header_storage.rs new file mode 100644 index 000000000..50141ca33 --- /dev/null +++ b/dash-spv/src/test_utils/header_storage.rs @@ -0,0 +1,49 @@ +use std::collections::HashMap; +use std::ops::Range; + +use async_trait::async_trait; +use dashcore::BlockHash; + +use crate::error::StorageResult; +use crate::storage::{BlockHeaderStorage, BlockHeaderTip}; +use crate::types::HashedBlockHeader; + +/// A [`BlockHeaderStorage`] that answers hash lookups from a map and nothing else. +/// Everything outside `get_header_height_by_hash` is inert. +pub struct MockHeaderStorage(pub HashMap); + +#[async_trait] +impl BlockHeaderStorage for MockHeaderStorage { + async fn store_headers(&mut self, _: &[HashedBlockHeader]) -> StorageResult<()> { + Ok(()) + } + async fn store_headers_at_height( + &mut self, + _: &[HashedBlockHeader], + _: u32, + ) -> StorageResult<()> { + Ok(()) + } + async fn load_headers(&self, _: Range) -> StorageResult> { + Ok(vec![]) + } + async fn get_tip_height(&self) -> Option { + None + } + async fn get_tip(&self) -> Option { + None + } + async fn get_start_height(&self) -> Option { + None + } + async fn get_stored_headers_len(&self) -> u32 { + 0 + } + async fn get_header_height_by_hash(&self, hash: &BlockHash) -> StorageResult> { + Ok(self.0.get(hash).copied()) + } + async fn truncate_above(&mut self, target_height: u32) -> StorageResult<()> { + self.0.retain(|_, h| *h <= target_height); + Ok(()) + } +} diff --git a/dash-spv/src/test_utils/mod.rs b/dash-spv/src/test_utils/mod.rs index 61acedda0..39e91fded 100644 --- a/dash-spv/src/test_utils/mod.rs +++ b/dash-spv/src/test_utils/mod.rs @@ -5,6 +5,7 @@ mod context; mod event_handler; mod filter; mod fs_helpers; +mod header_storage; pub(crate) mod masternode_network; mod network; mod node; @@ -19,6 +20,7 @@ pub const SYNC_TIMEOUT: Duration = Duration::from_secs(180); pub use context::DashdTestContext; pub use event_handler::TestEventHandler; pub use fs_helpers::retain_test_dir; +pub use header_storage::MockHeaderStorage; pub use masternode_network::MasternodeTestContext; pub use network::{test_socket_address, MockNetworkManager}; pub use node::{DashCoreNode, TestChain, WalletFile}; diff --git a/dash/src/test_utils/mod.rs b/dash/src/test_utils/mod.rs index 287757dfc..4630e0247 100644 --- a/dash/src/test_utils/mod.rs +++ b/dash/src/test_utils/mod.rs @@ -4,4 +4,5 @@ mod chainlock; mod filter; mod instantlock; mod network; +mod sml; mod transaction; diff --git a/dash/src/test_utils/sml.rs b/dash/src/test_utils/sml.rs new file mode 100644 index 000000000..dcabb32bf --- /dev/null +++ b/dash/src/test_utils/sml.rs @@ -0,0 +1,96 @@ +use std::net::SocketAddr; + +use hashes::Hash; + +use crate::bls_sig_utils::BLSPublicKey; +use crate::hash_types::{MerkleRootMasternodeList, ProTxHash}; +use crate::network::message_qrinfo::{MNSkipListMode, QRInfo, QuorumSnapshot}; +use crate::network::message_sml::MnListDiff; +use crate::sml::masternode_list_entry::{ + EntryMasternodeType, MasternodeListEntry, MasternodeNetInfo, +}; +use crate::{BlockHash, PubkeyHash, Transaction}; + +fn dummy_hash(byte: u8) -> BlockHash { + BlockHash::from_slice(&[byte; 32]).unwrap() +} + +impl MasternodeListEntry { + pub fn dummy(byte: u8) -> Self { + MasternodeListEntry { + version: 1, + pro_reg_tx_hash: ProTxHash::from_slice(&[byte; 32]).unwrap(), + confirmed_hash: None, + service_address: MasternodeNetInfo::Legacy(SocketAddr::from(([127, 0, 0, 1], 19999))), + operator_public_key: BLSPublicKey::from([0u8; 48]), + key_id_voting: PubkeyHash::from_slice(&[byte; 20]).unwrap(), + is_valid: true, + mn_type: EntryMasternodeType::Regular, + } + } +} + +impl MnListDiff { + /// Carries one masternode and one merkle hash, the minimum + /// [`MasternodeList`](crate::sml::masternode_list::MasternodeList) conversion + /// accepts. Use this when the diff has to apply to an engine. + pub fn dummy(base_byte: u8, tip_byte: u8) -> Self { + MnListDiff { + total_transactions: 1, + merkle_hashes: vec![MerkleRootMasternodeList::from([tip_byte; 32])], + merkle_flags: vec![1], + new_masternodes: vec![MasternodeListEntry::dummy(tip_byte)], + ..MnListDiff::dummy_empty(base_byte, tip_byte) + } + } + + /// Hashes only. An engine rejects this as an incomplete diff, which is what + /// makes it useful for exercising the rejection paths. + pub fn dummy_empty(base_byte: u8, tip_byte: u8) -> Self { + MnListDiff { + version: 1, + base_block_hash: dummy_hash(base_byte), + block_hash: dummy_hash(tip_byte), + total_transactions: 0, + merkle_hashes: vec![], + merkle_flags: vec![], + coinbase_tx: Transaction::dummy_empty(), + deleted_masternodes: vec![], + new_masternodes: vec![], + deleted_quorums: vec![], + new_quorums: vec![], + quorums_chainlock_signatures: vec![], + } + } +} + +impl QuorumSnapshot { + pub fn dummy() -> Self { + QuorumSnapshot { + skip_list_mode: MNSkipListMode::NoSkipping, + active_quorum_members: vec![], + skip_list: vec![], + } + } +} + +impl QRInfo { + /// Built from [`MnListDiff::dummy_empty`], so an engine rejects it. Only + /// `mn_list_diff_tip` is distinguished, by `[tip_byte; 32]`. + pub fn dummy(tip_byte: u8) -> Self { + QRInfo { + quorum_snapshot_at_h_minus_c: QuorumSnapshot::dummy(), + quorum_snapshot_at_h_minus_2c: QuorumSnapshot::dummy(), + quorum_snapshot_at_h_minus_3c: QuorumSnapshot::dummy(), + mn_list_diff_tip: MnListDiff::dummy_empty(0x00, tip_byte), + mn_list_diff_h: MnListDiff::dummy_empty(0x00, 0x00), + mn_list_diff_at_h_minus_c: MnListDiff::dummy_empty(0x00, 0x00), + mn_list_diff_at_h_minus_2c: MnListDiff::dummy_empty(0x00, 0x00), + mn_list_diff_at_h_minus_3c: MnListDiff::dummy_empty(0x00, 0x00), + quorum_snapshot_and_mn_list_diff_at_h_minus_4c: None, + last_commitment_per_index: vec![], + quorum_snapshot_list: vec![], + mn_list_diff_list: vec![], + } + } +} From d6083c6bccfafd9db8dbc897507735482ce2ead5 Mon Sep 17 00:00:00 2001 From: Borja Castellano Date: Tue, 8 Sep 2026 19:39:46 +0000 Subject: [PATCH 4/4] fix(dash-spv): coderabbit comments addressed --- dash-spv/src/storage/masternode.rs | 9 ++++ dash-spv/src/sync/chainlock/manager.rs | 51 ++++++++++++++++++ dash-spv/src/sync/masternodes/manager.rs | 27 ++++++---- dash-spv/src/sync/masternodes/sync_manager.rs | 27 +++++++++- dash-spv/tests/dashd_masternode/setup.rs | 23 ++++++-- dash-spv/tests/dashd_masternode/tests_sync.rs | 33 ++++++++++-- .../src/sml/masternode_list_engine/helpers.rs | 52 ++++++++++++++++++- 7 files changed, 198 insertions(+), 24 deletions(-) diff --git a/dash-spv/src/storage/masternode.rs b/dash-spv/src/storage/masternode.rs index 99c61d985..f7d92beac 100644 --- a/dash-spv/src/storage/masternode.rs +++ b/dash-spv/src/storage/masternode.rs @@ -243,6 +243,15 @@ impl PersistentMasternodeStorage { } } +#[cfg(any(test, feature = "test-utils"))] +impl PersistentMasternodeStorage { + /// The validity window the last lookup cached, so a caller's test can tell + /// whether this storage was consulted and for which height. + pub async fn cached_window(&self) -> Option<(CoreBlockHeight, Option)> { + self.cached_list.lock().await.as_ref().map(|cached| (cached.from, cached.until)) + } +} + #[async_trait] impl MasternodeStorage for PersistentMasternodeStorage { async fn store_diff( diff --git a/dash-spv/src/sync/chainlock/manager.rs b/dash-spv/src/sync/chainlock/manager.rs index ba6f75d3e..4aca7b688 100644 --- a/dash-spv/src/sync/chainlock/manager.rs +++ b/dash-spv/src/sync/chainlock/manager.rs @@ -342,10 +342,13 @@ mod tests { use crate::storage::{ DiskStorageManager, PersistentBlockHeaderStorage, PersistentMetadataStorage, StorageManager, }; + use crate::storage::{MasternodeStorage, PersistentMasternodeStorage}; use crate::sync::{ManagerIdentifier, SyncManager, SyncManagerProgress, SyncState}; + use crate::test_utils::MockHeaderStorage; use crate::Network; use dashcore::bls_sig_utils::BLSSignature; use dashcore::hashes::Hash; + use dashcore::network::message_sml::MnListDiff; use dashcore::BlockHash; type TestChainLockManager = @@ -366,6 +369,54 @@ mod tests { ChainLockManager::new(storage.block_headers(), storage.metadata(), engine, None).await } + async fn manager_with_replayable_storage( + dir: &tempfile::TempDir, + metadata: Arc>, + ) -> ChainLockManager { + let heights = [(0x00u8, 0u32), (0xAA, 100), (0xBB, 200), (0xCC, 300)] + .into_iter() + .map(|(b, h)| (BlockHash::from_slice(&[b; 32]).unwrap(), h)) + .collect(); + let headers = Arc::new(RwLock::new(MockHeaderStorage(heights))); + let mut storage = + PersistentMasternodeStorage::open(dir.path(), Arc::clone(&headers), Network::Regtest) + .await + .unwrap(); + storage.store_diff(100, &MnListDiff::dummy(0x00, 0xAA)).await.unwrap(); + storage.store_diff(200, &MnListDiff::dummy(0xAA, 0xBB)).await.unwrap(); + storage.store_diff(300, &MnListDiff::dummy(0xBB, 0xCC)).await.unwrap(); + + ChainLockManager::new( + headers, + metadata, + Arc::new(RwLock::new(MasternodeListEngine::default_for_network(Network::Regtest))), + Some(Arc::new(RwLock::new(storage))), + ) + .await + } + + /// The engine holds no lists, so validation can only get anywhere by falling + /// back to a list replayed from storage. That fallback has to ask for the + /// height that signed the ChainLock, not the height it locks. + #[tokio::test] + async fn validation_falls_back_to_storage_at_the_signing_height() { + let disk = DiskStorageManager::with_temp_dir().await.unwrap(); + let dir = tempfile::TempDir::new().unwrap(); + let manager = manager_with_replayable_storage(&dir, disk.metadata()).await; + + // 205 - 8 = 197, which sits between the lists at 100 and 200. Using the + // locked height instead would land in the window above. + let chainlock = create_test_chainlock(205); + assert!(!manager.validate_signature(&chainlock).await, "a dummy signature cannot verify"); + + let storage = manager.masternode_storage.as_ref().expect("storage was wired"); + assert_eq!( + storage.read().await.cached_window().await, + Some((100, Some(200))), + "the fallback looked up the signing height, not the locked height" + ); + } + fn create_test_chainlock(height: u32) -> ChainLock { ChainLock { block_height: height, diff --git a/dash-spv/src/sync/masternodes/manager.rs b/dash-spv/src/sync/masternodes/manager.rs index 1fdcba4ce..422d19a1d 100644 --- a/dash-spv/src/sync/masternodes/manager.rs +++ b/dash-spv/src/sync/masternodes/manager.rs @@ -12,6 +12,7 @@ use dashcore::sml::masternode_list_engine::{MasternodeListEngine, QRInfoFeedResu use tokio::sync::RwLock; use super::pipeline::MnListDiffPipeline; +use crate::error::StorageResult; use crate::error::{SyncError, SyncResult}; use crate::network::RequestSender; use crate::storage::{BlockHeaderStorage, MasternodeStorage, PersistentMasternodeStorage}; @@ -89,6 +90,10 @@ pub(super) struct QRInfoInFlight { pub(super) struct MasternodeSyncState { /// Heights where the engine has masternode lists (for chaining diffs). pub(super) known_mn_list_heights: BTreeSet, + /// Heights whose message could not be written, so their list is only in + /// memory and pruning it would lose it. Pruning is the one place that trades + /// memory for a copy on disk, so it has to skip these. + pub(super) unpersisted_heights: BTreeSet, /// Pipeline for MnListDiff requests. pub(super) mnlistdiff_pipeline: MnListDiffPipeline, /// What the pipeline is currently being used for. See [`PipelineMode`]. @@ -344,22 +349,18 @@ impl MasternodesManager { } } - pub(super) async fn store_diff(&self, height: u32, diff: &MnListDiff) { + pub(super) async fn store_diff(&self, height: u32, diff: &MnListDiff) -> StorageResult<()> { let Some(storage) = &self.message_storage else { - return; + return Ok(()); }; - if let Err(e) = storage.write().await.store_diff(height, diff).await { - tracing::warn!("Could not store MnListDiff at {height}: {e}"); - } + storage.write().await.store_diff(height, diff).await } - pub(super) async fn store_qr_info(&self, height: u32, qr_info: &QRInfo) { + pub(super) async fn store_qr_info(&self, height: u32, qr_info: &QRInfo) -> StorageResult<()> { let Some(storage) = &self.message_storage else { - return; + return Ok(()); }; - if let Err(e) = storage.write().await.store_qr_info(height, qr_info).await { - tracing::warn!("Could not store QRInfo at {height}: {e}"); - } + storage.write().await.store_qr_info(height, qr_info).await } pub(super) async fn prune_obsolete_lists(&self, tip: u32) { @@ -367,7 +368,11 @@ impl MasternodesManager { return; } - let pruned = self.engine.write().await.prune_obsolete_lists(tip); + let pruned = self + .engine + .write() + .await + .prune_obsolete_lists(tip, &self.sync_state.unpersisted_heights); tracing::debug!("Pruned {pruned} in-memory masternode lists at {tip}"); } diff --git a/dash-spv/src/sync/masternodes/sync_manager.rs b/dash-spv/src/sync/masternodes/sync_manager.rs index fb2f899a5..a950a67ec 100644 --- a/dash-spv/src/sync/masternodes/sync_manager.rs +++ b/dash-spv/src/sync/masternodes/sync_manager.rs @@ -263,7 +263,18 @@ impl SyncManager for MasternodesManager { drop(storage); match tip_height { - Ok(Some(height)) => self.store_qr_info(height, qr_info).await, + Ok(Some(height)) => match self.store_qr_info(height, qr_info).await { + Ok(()) => { + self.sync_state.unpersisted_heights.remove(&height); + } + Err(e) => { + tracing::warn!( + "Could not store QRInfo at {height}: {e}. Its list stays in memory \ + and is excluded from pruning, so a restart resyncs from further back" + ); + self.sync_state.unpersisted_heights.insert(height); + } + }, Ok(None) => tracing::warn!( "QRInfo tip {tip_hash} has no known height, rotated quorums will not survive a restart" ), @@ -386,7 +397,19 @@ impl SyncManager for MasternodesManager { drop(engine); if apply_ok { - self.store_diff(target_height, diff).await; + match self.store_diff(target_height, diff).await { + Ok(()) => { + self.sync_state.unpersisted_heights.remove(&target_height); + } + Err(e) => { + tracing::warn!( + "Could not store MnListDiff at {target_height}: {e}. Its list stays \ + in memory and is excluded from pruning, so a restart resyncs from \ + further back" + ); + self.sync_state.unpersisted_heights.insert(target_height); + } + } } self.progress.add_diffs_processed(1); diff --git a/dash-spv/tests/dashd_masternode/setup.rs b/dash-spv/tests/dashd_masternode/setup.rs index 773dd5c07..ff990562f 100644 --- a/dash-spv/tests/dashd_masternode/setup.rs +++ b/dash-spv/tests/dashd_masternode/setup.rs @@ -41,6 +41,11 @@ pub(super) struct ClientHandle { } impl ClientHandle { + pub(super) fn start(&mut self) { + let run_client = self.client.clone(); + self.run_handle = Some(tokio::task::spawn(async move { run_client.run().await })); + } + pub(super) async fn stop(&mut self) { tracing::info!("Stopping client run loop..."); self.client.stop().await.expect("client stop failed"); @@ -161,7 +166,7 @@ pub(super) async fn receive_address( next_unused_receive_address(wallet, wallet_id).await } -pub(super) async fn create_and_start_client( +pub(super) async fn create_client( config: &ClientConfig, wallet: Arc>>, ) -> ClientHandle { @@ -183,13 +188,10 @@ pub(super) async fn create_and_start_client( let engine = client.masternode_list_engine().expect("Engine should be initialized after creation"); - let run_client = client.clone(); - - let run_handle = tokio::task::spawn(async move { run_client.run().await }); ClientHandle { client, - run_handle: Some(run_handle), + run_handle: None, progress_receiver, sync_event_receiver, wallet_event_receiver, @@ -197,3 +199,14 @@ pub(super) async fn create_and_start_client( engine, } } + +/// Built and started. Use [`create_client`] plus [`ClientHandle::start`] instead when +/// the test needs to look at the client before it touches the network. +pub(super) async fn create_and_start_client( + config: &ClientConfig, + wallet: Arc>>, +) -> ClientHandle { + let mut handle = create_client(config, wallet).await; + handle.start(); + handle +} diff --git a/dash-spv/tests/dashd_masternode/tests_sync.rs b/dash-spv/tests/dashd_masternode/tests_sync.rs index 1647b9193..b1a252089 100644 --- a/dash-spv/tests/dashd_masternode/tests_sync.rs +++ b/dash-spv/tests/dashd_masternode/tests_sync.rs @@ -16,7 +16,8 @@ use super::helpers::{ wait_for_mn_state_with_stored_cycle_above, }; use super::setup::{ - create_and_start_client, create_dummy_wallet, create_mn_test_config, TestContext, SYNC_TIMEOUT, + create_and_start_client, create_client, create_dummy_wallet, create_mn_test_config, + TestContext, SYNC_TIMEOUT, }; /// Sync masternode list against a pre-generated regtest controller node. @@ -105,10 +106,16 @@ async fn test_masternode_list_sync_with_restart() { wait_for_masternode_sync(&mut client_handle.progress_receiver, SYNC_TIMEOUT).await; let first_height = first_mn_progress.current_height(); - let first_masternodes = { + let first_tip = { let engine = client_handle.engine.read().await; - engine.masternode_lists.values().map(|list| list.masternodes.len()).max().unwrap_or(0) + engine + .masternode_lists + .iter() + .next_back() + .map(|(height, list)| (*height, list.block_hash, list.masternodes.len())) }; + let (_, _, first_masternodes) = + first_tip.expect("the first session must build a masternode list before testing restart"); assert!( first_masternodes > 0, "the first session must have a masternode list before its persistence can be tested" @@ -125,7 +132,25 @@ async fn test_masternode_list_sync_with_restart() { // Restart with same storage tracing::info!("=== Restarting with same storage ==="); - let mut client_handle = create_and_start_client(&config, Arc::clone(&wallet)).await; + let mut client_handle = create_client(&config, Arc::clone(&wallet)).await; + + // Read before the run loop is spawned, so nothing has come off the network yet: + // this is what replaying the stored messages produced on its own. + let replayed_tip = { + let engine = client_handle.engine.read().await; + engine + .masternode_lists + .iter() + .next_back() + .map(|(height, list)| (*height, list.block_hash, list.masternodes.len())) + }; + assert_eq!( + replayed_tip, first_tip, + "startup must rebuild the first session's masternode list from storage, \ + not default and let a fresh dashd sync cover for it" + ); + + client_handle.start(); let second_mn_progress = wait_for_masternode_sync(&mut client_handle.progress_receiver, SYNC_TIMEOUT).await; let second_height = second_mn_progress.current_height(); diff --git a/dash/src/sml/masternode_list_engine/helpers.rs b/dash/src/sml/masternode_list_engine/helpers.rs index 052f2a4e9..e3d180e00 100644 --- a/dash/src/sml/masternode_list_engine/helpers.rs +++ b/dash/src/sml/masternode_list_engine/helpers.rs @@ -6,6 +6,7 @@ use crate::sml::llmq_type::network::NetworkLLMQExt; use crate::sml::masternode_list::MasternodeList; use crate::sml::masternode_list_engine::MasternodeListEngine; use crate::sml::quorum_entry::qualified_quorum_entry::QualifiedQuorumEntry; +use std::collections::BTreeSet; /// How many active windows below the lookup height [`MasternodeListEngine::quorum_entry_for_hash_at_or_before_height`] /// searches before giving up. A signing quorum referenced by a proof was selected at a lagged @@ -15,7 +16,13 @@ use crate::sml::quorum_entry::qualified_quorum_entry::QualifiedQuorumEntry; const QUORUM_WALK_BACK_ACTIVE_WINDOWS: u32 = 4; impl MasternodeListEngine { - pub fn prune_obsolete_lists(&mut self, tip: CoreBlockHeight) -> usize { + /// `keep` names heights the caller cannot recover, so they are retained + /// however old they are: dropping one loses the list outright. + pub fn prune_obsolete_lists( + &mut self, + tip: CoreBlockHeight, + keep: &BTreeSet, + ) -> usize { let params = self.network.chain_locks_type().params(); let floor = tip.saturating_sub( @@ -27,7 +34,7 @@ impl MasternodeListEngine { let before = self.masternode_lists.len(); - self.masternode_lists.retain(|height, _| *height >= floor); + self.masternode_lists.retain(|height, _| *height >= floor || keep.contains(height)); before - self.masternode_lists.len() } @@ -265,3 +272,44 @@ mod tests { ); } } + +#[cfg(test)] +mod prune_tests { + use super::*; + use crate::sml::masternode_list::MasternodeList; + use crate::sml::masternode_list_engine::MasternodeListEngine; + use crate::{BlockHash, Network}; + use hashes::Hash; + + fn engine_with_lists(heights: &[CoreBlockHeight]) -> MasternodeListEngine { + let mut engine = MasternodeListEngine::default_for_network(Network::Mainnet); + for height in heights { + let hash = BlockHash::from_slice(&[*height as u8; 32]).unwrap(); + engine.masternode_lists.insert(*height, MasternodeList::empty(hash, *height)); + } + engine + } + + #[test] + fn prune_drops_lists_below_the_walk_back_floor() { + let mut engine = engine_with_lists(&[1_000, 500_000, 1_000_000]); + let pruned = engine.prune_obsolete_lists(1_000_000, &BTreeSet::new()); + + assert_eq!(pruned, 2); + assert_eq!(engine.masternode_lists.keys().copied().collect::>(), vec![1_000_000]); + } + + #[test] + fn prune_keeps_a_list_the_caller_could_not_persist() { + let mut engine = engine_with_lists(&[1_000, 500_000, 1_000_000]); + let keep = BTreeSet::from([1_000]); + let pruned = engine.prune_obsolete_lists(1_000_000, &keep); + + assert_eq!(pruned, 1, "only the persisted old list goes"); + assert!( + engine.masternode_lists.contains_key(&1_000), + "dropping an unpersisted list would lose it outright" + ); + assert!(!engine.masternode_lists.contains_key(&500_000)); + } +}