Skip to content

Commit 9dea26e

Browse files
shumkovclaude
andauthored
fix(dash-spv): drop masternode lists above a followed fork, refuse forks below the checkpoint (#1201)
* fix(dash-spv): drop masternode lists built above a followed fork `handle_fork` truncated the stored headers, filters, blocks and wallets above the fork, but not the masternode list engine. Lists built on the dropped blocks stayed in memory, kept answering quorum lookups for blocks that had left the chain, and kept serving as the base of the next tip diff. The engine now drops, right after the headers: - every list above the fork; - every rotated cycle whose base block left the chain, or whose mining window ends above the fork, since its commitments may have been mined on the dropped branch. So the engine never holds lists above the stored tip, even if a later truncation fails. The truncation steps move into `truncate_chain_above`, so they can be tested without a running sync loop. Test would have caught this in CI: `following_a_fork_drops_the_masternode_lists_above_it` fails without the engine truncation (lists above the fork remain) and passes with it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(dash-spv): refuse forks below the newest reached checkpoint `fork_height` refused a branch off below the best ChainLock, but not one below a compiled-in checkpoint (`CheckpointManager::should_reject_fork` was never called). A peer could make the client drop and re-sync a checkpointed part of its chain. A branch off below the newest checkpoint at or below the stored tip is no longer treated as a fork. A checkpoint above the tip fixes nothing yet, so `should_reject_fork`, which compares against the newest compiled checkpoint whatever the tip, is not used. Test would have caught this in CI: `a_branch_off_below_the_newest_reached_checkpoint_is_no_fork` ("below the checkpoint") fails without the floor and passes with it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 70a1256 commit 9dea26e

4 files changed

Lines changed: 175 additions & 18 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
111111
`key::Error::InvalidWifCompressionFlag`
112112
- Accept an IL of zero in `key-wallet` private child derivation
113113
- `transaction_sign_input` in `key-wallet-ffi` rejects flags above `0xff`
114+
- `dash-spv` drops the masternode lists, and the rotated quorum cycles, built
115+
above a fork it follows, so they no longer answer for blocks that left the
116+
chain
117+
- `dash-spv` no longer follows a fork that branches off below the newest
118+
checkpoint its stored chain reaches
114119

115120
### Removed
116121

‎dash-spv/src/client/lifecycle.rs‎

Lines changed: 61 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -269,28 +269,37 @@ impl<W: WalletInterface, N: NetworkManager> DashSpvClient<W, N> {
269269
self.stop_locked(forked).await;
270270

271271
tracing::warn!("Fork at height {}, dropping the stored chain above it", fork_height);
272-
// The wallets go first: a wallet refusing the fork leaves storage as
273-
// it is, and storage failing after them keeps the fork unstored, so
274-
// it is detected and retried.
275-
let truncated = async {
276-
self.wallet.write().await.truncate_above(fork_height)?;
277-
let mut storage = self.storage.lock().await;
278-
BlockHeaderStorage::truncate_above(&mut *storage, fork_height).await?;
279-
FilterHeaderStorage::truncate_above(&mut *storage, fork_height).await?;
280-
FilterStorage::truncate_above(&mut *storage, fork_height).await?;
281-
BlockStorage::truncate_above(&mut *storage, fork_height).await?;
282-
Ok::<_, Box<dyn std::error::Error + Send + Sync>>(())
283-
}
284-
.await;
285-
286-
if let Err(e) = truncated {
272+
if let Err(e) = self.truncate_chain_above(fork_height).await {
287273
tracing::warn!("Fork at height {} not followed: {}", fork_height, e);
288274
}
289275

290276
self.run_locked(&mut sync_loop).await
291277
})
292278
}
293279

280+
/// Drops what the wallets, the stored chain and the masternode engine hold
281+
/// above `height`.
282+
///
283+
/// The wallets go first: a wallet refusing the fork leaves storage as it
284+
/// is, and storage failing after them keeps the fork unstored, so it is
285+
/// detected and retried. The engine goes right after the headers, which it
286+
/// looks rotated cycles up in, so it never holds lists above the stored tip.
287+
async fn truncate_chain_above(
288+
&self,
289+
height: u32,
290+
) -> std::result::Result<(), Box<dyn std::error::Error + Send + Sync>> {
291+
self.wallet.write().await.truncate_above(height)?;
292+
BlockHeaderStorage::truncate_above(&mut *self.storage.lock().await, height).await?;
293+
if let Some(engine) = &self.masternode_engine {
294+
engine.write().await.truncate_above(height).await;
295+
}
296+
let mut storage = self.storage.lock().await;
297+
FilterHeaderStorage::truncate_above(&mut *storage, height).await?;
298+
FilterStorage::truncate_above(&mut *storage, height).await?;
299+
BlockStorage::truncate_above(&mut *storage, height).await?;
300+
Ok(())
301+
}
302+
294303
/// Stop `sync_loop` and everything it drives. The caller holds the lock.
295304
pub(super) async fn stop_locked(
296305
&self,
@@ -445,3 +454,40 @@ impl<W: WalletInterface, N: NetworkManager> DashSpvClient<W, N> {
445454
Ok(())
446455
}
447456
}
457+
458+
#[cfg(test)]
459+
mod tests {
460+
use super::*;
461+
use crate::test_utils::MockNetworkManager;
462+
use dashcore::sml::masternode_list::MasternodeList;
463+
use dashcore::BlockHash;
464+
use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo;
465+
use key_wallet_manager::WalletManager;
466+
use tempfile::TempDir;
467+
468+
/// Following a fork drops the masternode lists built above it, so they stop
469+
/// answering for blocks that left the chain.
470+
#[tokio::test]
471+
async fn following_a_fork_drops_the_masternode_lists_above_it() {
472+
let temp_dir = TempDir::new().unwrap();
473+
let mut config =
474+
ClientConfig::mainnet().without_filters().with_storage_path(temp_dir.path());
475+
config.start_from_height = Some(2_500_000);
476+
let wallet = Arc::new(RwLock::new(WalletManager::<ManagedWalletInfo>::new(config.network)));
477+
let client = DashSpvClient::new(config, MockNetworkManager::new(), wallet, vec![])
478+
.await
479+
.expect("client construction must succeed");
480+
let fork = client.tip_header().await.expect("anchored at the checkpoint").height();
481+
482+
let engine = client.masternode_list_engine().expect("masternodes are enabled");
483+
for height in [fork - 1, fork, fork + 1, fork + 2] {
484+
let list = MasternodeList::empty(BlockHash::dummy(height), height);
485+
engine.write().await.masternode_lists.insert(height, list);
486+
}
487+
488+
client.truncate_chain_above(fork).await.expect("nothing refuses the fork");
489+
490+
let heights: Vec<_> = engine.read().await.masternode_lists.keys().copied().collect();
491+
assert_eq!(heights, vec![fork - 1, fork]);
492+
}
493+
}

‎dash-spv/src/sml_engine/helpers.rs‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,30 @@ impl<H: BlockHeaderStorage> MasternodeListEngine<H> {
8181
self.rotated_quorums_per_cycle.retain(|hash, _| !below_floor.contains(hash));
8282
}
8383

84+
/// Drops what the engine built on blocks above `height`, once the header
85+
/// chain has dropped them for another branch: the lists above it, and the
86+
/// rotated cycles whose base block left the chain or whose mining window
87+
/// ends above it, since their commitments may have been mined on the
88+
/// dropped branch.
89+
pub(crate) async fn truncate_above(&mut self, height: CoreBlockHeight) {
90+
if let Some(above) = height.checked_add(1) {
91+
self.masternode_lists.split_off(&above);
92+
}
93+
94+
let mining_window_end = self.network.isd_llmq_type().params().dkg_params.mining_window_end;
95+
let mut dropped = BTreeSet::new();
96+
for cycle_hash in self.rotated_quorums_per_cycle.keys() {
97+
let mined_below = self
98+
.height_of(cycle_hash)
99+
.await
100+
.is_some_and(|base| base.saturating_add(mining_window_end) <= height);
101+
if !mined_below {
102+
dropped.insert(*cycle_hash);
103+
}
104+
}
105+
self.rotated_quorums_per_cycle.retain(|hash, _| !dropped.contains(hash));
106+
}
107+
84108
/// Sets the verification status of a quorum in every list holding it.
85109
/// Lists that shared a quorum map keep sharing the updated one.
86110
pub(super) fn set_quorum_status(
@@ -435,6 +459,35 @@ mod prune_tests {
435459
assert_eq!(heights(&engine), vec![floor, TIP, TIP + 1]);
436460
}
437461

462+
/// Once the header chain follows another branch at `fork`, nothing built on
463+
/// the dropped blocks may answer for them: the lists above the fork go, and
464+
/// so does a rotated cycle whose base left the chain or whose commitments
465+
/// could have been mined above the fork.
466+
#[tokio::test]
467+
async fn truncating_above_a_fork_drops_what_the_dropped_blocks_built() {
468+
let fork = TIP;
469+
let window_end = Network::Mainnet.isd_llmq_type().params().dkg_params.mining_window_end;
470+
let mut engine = TestEngine::dummy_with_lists(&[fork - 100, fork, fork + 1, fork + 50]);
471+
let mined_below = fork - window_end;
472+
let mined_above = mined_below + 1;
473+
let left_the_chain = fork + 10;
474+
for base in [mined_below, mined_above, left_the_chain] {
475+
engine.feed_block_height(base, BlockHash::dummy(base));
476+
engine.rotated_quorums_per_cycle.insert(BlockHash::dummy(base), Default::default());
477+
}
478+
for height in [fork + 1, left_the_chain, fork + 50] {
479+
engine.forget_block(&BlockHash::dummy(height));
480+
}
481+
482+
engine.truncate_above(fork).await;
483+
484+
assert_eq!(heights(&engine), vec![fork - 100, fork]);
485+
assert_eq!(
486+
engine.rotated_quorums_per_cycle.keys().collect::<Vec<_>>(),
487+
vec![&BlockHash::dummy(mined_below)]
488+
);
489+
}
490+
438491
/// As a legacy QRInfo or a batch of work-block lists does.
439492
#[tokio::test]
440493
async fn diffs_sharing_an_older_base_all_apply() {

‎dash-spv/src/sync/block_headers/manager.rs‎

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,8 @@ pub struct BlockHeadersManager<H: BlockHeaderStorage, M: MetadataStorage> {
4545
pub(super) metadata_storage: Arc<RwLock<M>>,
4646
/// Pipeline for parallel header downloads (used for both initial sync and post-sync).
4747
pub(super) pipeline: HeadersPipeline,
48+
/// Compiled-in checkpoints, which no fork may branch off below.
49+
checkpoint_manager: Arc<CheckpointManager>,
4850
/// Pending block announcements waiting for headers message (post-sync).
4951
pub(super) pending_announcements: HashMap<BlockHash, Instant>,
5052
/// Peers we've sent a GetHeaders to after sync, so Dash Core knows our tip
@@ -90,7 +92,8 @@ impl<H: BlockHeaderStorage, M: MetadataStorage> BlockHeadersManager<H, M> {
9092
progress: initial_progress,
9193
header_storage,
9294
metadata_storage,
93-
pipeline: HeadersPipeline::new(checkpoint_manager),
95+
pipeline: HeadersPipeline::new(checkpoint_manager.clone()),
96+
checkpoint_manager,
9497
pending_announcements: HashMap::new(),
9598
announced_peers: HashSet::new(),
9699
})
@@ -186,8 +189,8 @@ impl<H: BlockHeaderStorage, M: MetadataStorage> BlockHeadersManager<H, M> {
186189
}
187190

188191
/// Height where `headers` branch off the stored chain below its tip, if they
189-
/// do. A branch off below the best ChainLock is not followed: the blocks it
190-
/// locks are final.
192+
/// do. A branch off below the best ChainLock, or below the newest checkpoint
193+
/// the stored chain reaches, is not followed: the blocks they fix are final.
191194
async fn fork_height(&self, headers: &[HashedBlockHeader]) -> SyncResult<Option<u32>> {
192195
let storage = self.header_storage.read().await;
193196

@@ -204,6 +207,18 @@ impl<H: BlockHeaderStorage, M: MetadataStorage> BlockHeadersManager<H, M> {
204207
return Ok(None);
205208
};
206209

210+
if let Some(checkpoint) = tip
211+
.and_then(|tip| self.checkpoint_manager.last_checkpoint_before_height(tip))
212+
.filter(|checkpoint| fork_height < checkpoint.height)
213+
{
214+
tracing::warn!(
215+
"Ignoring a fork at height {}, below the checkpoint at {}",
216+
fork_height,
217+
checkpoint.height
218+
);
219+
return Ok(None);
220+
}
221+
207222
let chain_lock = self
208223
.metadata_storage
209224
.read()
@@ -352,6 +367,7 @@ impl<H: BlockHeaderStorage, M: MetadataStorage> BlockHeadersManager<H, M> {
352367
mod tests {
353368
use super::*;
354369
use crate::chain::checkpoints::testnet_checkpoints;
370+
use crate::chain::Checkpoint;
355371
use crate::network::{MessageType, NetworkEvent, NetworkRequest, RequestSender};
356372
use crate::storage::{
357373
DiskStorageManager, PersistentBlockHeaderStorage, PersistentMetadataStorage, StorageManager,
@@ -425,6 +441,43 @@ mod tests {
425441
);
426442
}
427443

444+
/// Blocks below the newest checkpoint the stored chain reaches are final, as
445+
/// below the best ChainLock. A checkpoint above the tip fixes nothing yet,
446+
/// or no fork could ever be followed before the chain reached it.
447+
#[test_case(5, None; "below the checkpoint")]
448+
#[test_case(6, Some(6); "at the checkpoint")]
449+
#[test_case(7, Some(7); "above the checkpoint")]
450+
#[tokio::test]
451+
async fn a_branch_off_below_the_newest_reached_checkpoint_is_no_fork(
452+
branch_off: u32,
453+
expected: Option<u32>,
454+
) {
455+
let mut storage = DiskStorageManager::with_temp_dir().await.unwrap();
456+
let stored = Header::dummy_batch(0..10);
457+
storage
458+
.store_headers(&stored.iter().map(HashedBlockHeader::from).collect::<Vec<_>>())
459+
.await
460+
.unwrap();
461+
let checkpoints = [0, 6, 20].map(Checkpoint::dummy).to_vec();
462+
let manager = BlockHeadersManager::new(
463+
storage.block_headers(),
464+
storage.metadata(),
465+
Arc::new(CheckpointManager::new(checkpoints)),
466+
)
467+
.await
468+
.unwrap();
469+
470+
let branch = Header {
471+
prev_blockhash: stored[branch_off as usize].block_hash(),
472+
..Header::dummy(1_000)
473+
};
474+
475+
assert_eq!(
476+
manager.fork_height(&[HashedBlockHeader::from(&branch)]).await.unwrap(),
477+
expected
478+
);
479+
}
480+
428481
/// Create a manager in synced state with an initialized pipeline.
429482
async fn create_synced_manager() -> TestBlockHeadersManager {
430483
let mut manager = create_test_manager().await;

0 commit comments

Comments
 (0)