From 04996569818d59b8d7016232dcac23c82e91e93a Mon Sep 17 00:00:00 2001 From: Borja Castellano Date: Tue, 20 Oct 2026 06:29:47 -0700 Subject: [PATCH 1/3] fix(dash-spv)!: verify MnListDiff against its block's coinbase before applying A peer's MnListDiff applied without proving anything, so a malicious peer could inject masternodes and quorums, and with them ChainLocks it signed itself. The engine now applies a diff only once `MasternodeListValidator` checks that its merkle proof proves its coinbase in the stored header, and that the list it builds hashes to the masternode and quorum roots that coinbase commits to. This covers single diffs and every diff of a QRInfo, and the replay of stored diffs. BREAKING CHANGE: `MasternodeList::has_valid_mn_list_root` and `has_valid_llmq_list_root` are replaced by `verify_coinbase_roots`, and `SmlError` gains `InvalidCoinbaseProof` and `CoinbaseCommitmentMismatch`. Closes #1117 Co-Authored-By: Claude Opus 5.5 --- dash-spv/src/sml_engine.rs | 94 +++++++++++-- dash-spv/src/storage/masternode.rs | 28 +++- dash-spv/src/sync/masternodes/sync_manager.rs | 10 +- dash-spv/src/test_utils/header_storage.rs | 31 +++- dash-spv/src/validation/masternode_list.rs | 57 ++++++++ dash-spv/src/validation/mod.rs | 2 + dash/src/merkle_tree/block.rs | 40 ++++-- dash/src/network/message_sml.rs | 45 +++++- dash/src/sml/error.rs | 6 + dash/src/sml/masternode_list/from_diff.rs | 10 +- dash/src/sml/masternode_list/merkle_roots.rs | 133 +++++++++++------- 11 files changed, 356 insertions(+), 100 deletions(-) create mode 100644 dash-spv/src/validation/masternode_list.rs diff --git a/dash-spv/src/sml_engine.rs b/dash-spv/src/sml_engine.rs index 0434c0480..beb757bd4 100644 --- a/dash-spv/src/sml_engine.rs +++ b/dash-spv/src/sml_engine.rs @@ -6,6 +6,7 @@ use std::collections::{BTreeMap, BTreeSet}; use std::sync::Arc; use crate::storage::BlockHeaderStorage; +use crate::validation::MasternodeListValidator; use dashcore::bls_sig_utils::BLSSignature; use dashcore::hashes::Hash; use dashcore::network::constants::NetworkExt; @@ -59,6 +60,7 @@ pub struct MasternodeListEngine { pub masternode_lists: BTreeMap, rotated_quorums_per_cycle: BTreeMap>, network: Network, + validator: Option>, } /// The most quorums of `llmq_type` a run of `blocks` blocks can add: one per @@ -161,11 +163,26 @@ fn cycle_quarter_work_heights( impl MasternodeListEngine { /// An empty engine for `network` that looks block heights up in `headers`. pub fn new(network: Network, headers: Arc>) -> Self { + let validator = MasternodeListValidator::new(Arc::clone(&headers)); + Self::with_validator(network, headers, Some(validator)) + } + + #[cfg(any(test, feature = "test-utils"))] + pub fn new_without_validation(network: Network, headers: Arc>) -> Self { + Self::with_validator(network, headers, None) + } + + fn with_validator( + network: Network, + headers: Arc>, + validator: Option>, + ) -> Self { Self { headers, masternode_lists: BTreeMap::new(), rotated_quorums_per_cycle: BTreeMap::new(), network, + validator, } } @@ -782,6 +799,8 @@ impl MasternodeListEngine { /// Applies a diff, then verifies the newest list's non-rotating quorums: the /// diff may have brought one, or the work-block list one needs. + /// The diff must prove its coinbase in its block's header and build the + /// lists that coinbase commits to. pub async fn apply_diff(&mut self, masternode_list_diff: MnListDiff) -> Result<(), SmlError> { self.apply_and_prune(masternode_list_diff).await?; self.verify_newest_quorums().await; @@ -814,9 +833,23 @@ impl MasternodeListEngine { .ok_or(SmlError::BlockHashLookupFailed(base_block_hash))?; let diff_end_height = self.height_of(&block_hash).await.ok_or(SmlError::BlockHashLookupFailed(block_hash))?; + + let coinbase_tx = match &self.validator { + Some(validator) => { + validator.validate_coinbase_proof(&masternode_list_diff, diff_end_height).await?; + Some(masternode_list_diff.coinbase_tx.clone()) + } + None => None, + }; + if base_height == 0 { let masternode_list = MasternodeList::from_diff(masternode_list_diff, diff_end_height, self.network)?; + + if let (Some(validator), Some(coinbase_tx)) = (&self.validator, &coinbase_tx) { + validator.validate_coinbase_roots(&masternode_list, coinbase_tx)?; + } + self.masternode_lists.insert(diff_end_height, masternode_list); return Ok(()); } @@ -826,6 +859,11 @@ impl MasternodeListEngine { }; let masternode_list = base_masternode_list.apply_diff(masternode_list_diff, diff_end_height, self.network)?; + + if let (Some(validator), Some(coinbase_tx)) = (&self.validator, &coinbase_tx) { + validator.validate_coinbase_roots(&masternode_list, coinbase_tx)?; + } + // A quorum the diff brings may already have a status in another list. let mut changes = Vec::new(); for (quorum_type, quorums) in masternode_list.quorums.iter() { @@ -955,28 +993,43 @@ pub(crate) mod test_support { network: Network, heights: HashMap, ) -> Self { - Self::new(network, Arc::new(RwLock::new(MockHeaderStorage(heights)))) + Self::new_without_validation( + network, + Arc::new(RwLock::new(MockHeaderStorage::new(heights))), + ) } pub(crate) fn headers(&self) -> Arc> { self.headers.clone() } + /// Stands in for the headers a fixture lacks. + pub(crate) fn feed_merkle_roots<'a>( + &mut self, + diffs: impl IntoIterator, + ) { + let mut headers = self.headers.try_write().expect("unlocked"); + for diff in diffs { + let merkle_root = diff.coinbase_merkle_root().expect("proves its coinbase"); + headers.merkle_roots.insert(diff.block_hash, merkle_root); + } + } + pub(crate) fn feed_block_height(&mut self, height: CoreBlockHeight, block_hash: BlockHash) { - self.headers.try_write().expect("unlocked").0.insert(block_hash, height); + self.headers.try_write().expect("unlocked").heights.insert(block_hash, height); } pub(crate) fn forget_block(&mut self, block_hash: &BlockHash) { - self.headers.try_write().expect("unlocked").0.remove(block_hash); + self.headers.try_write().expect("unlocked").heights.remove(block_hash); } pub(crate) fn block_height(&self, block_hash: &BlockHash) -> Option { - self.headers.try_read().expect("unlocked").0.get(block_hash).copied() + self.headers.try_read().expect("unlocked").heights.get(block_hash).copied() } pub(crate) fn block_hash_at(&self, height: CoreBlockHeight) -> Option { let headers = self.headers.try_read().expect("unlocked"); - headers.0.iter().find(|(_, h)| **h == height).map(|(block_hash, _)| *block_hash) + headers.heights.iter().find(|(_, h)| **h == height).map(|(block_hash, _)| *block_hash) } /// Verifies `chain_lock` as the ChainLock manager does. @@ -1023,7 +1076,8 @@ pub(crate) mod test_support { #[cfg(test)] mod tests { use super::test_support::{fixture_heights, quorum_entry, TestEngine}; - use super::{build_cycle_quorum_map, WORK_DIFF_DEPTH}; + use super::{build_cycle_quorum_map, qr_info_diffs, WORK_DIFF_DEPTH}; + use crate::test_utils::MockHeaderStorage; use bincode::{config, decode_from_slice, Decode}; use dashcore::bls_sig_utils::{BLSPublicKey, BLSSignature}; use dashcore::consensus::deserialize; @@ -1031,6 +1085,7 @@ mod tests { use dashcore::network::message_qrinfo::QRInfo; use dashcore::network::message_sml::{MnListDiff, QuorumCLSigObject}; use dashcore::prelude::CoreBlockHeight; + use dashcore::sml::error::SmlError; use dashcore::sml::llmq_entry_verification::{ LLMQEntryVerificationSkipStatus, LLMQEntryVerificationStatus, }; @@ -1038,6 +1093,7 @@ mod tests { use dashcore::sml::llmq_type::network::NetworkLLMQExt; use dashcore::sml::llmq_type::{LLMQType, QUORUM_MEMBER_LIST_OFFSET}; use dashcore::sml::masternode_list::MasternodeList; + use dashcore::sml::masternode_list_entry::MasternodeListEntry; use dashcore::sml::quorum_entry::qualified_quorum_entry::{ QualifiedQuorumEntry, VerifyingChainLockSignaturesType, }; @@ -1046,6 +1102,8 @@ mod tests { use dashcore::{BlockHash, Network, QuorumHash}; use std::collections::{BTreeMap, BTreeSet}; use std::sync::Arc; + use test_case::test_case; + use tokio::sync::RwLock; fn make_qualified_quorum_entry( llmq_type: LLMQType, @@ -1364,21 +1422,33 @@ mod tests { /// engine state mirrors production cold start: empty masternode lists /// over the captured header heights. fn load_qrinfo_2518986_fixture() -> (TestEngine, QRInfo) { - let engine = TestEngine::knowing( + let heights = fixture_heights(include_bytes!( + "../../dash/tests/data/test_DML_diffs/block_container_2518986.dat" + )); + let mut engine = TestEngine::new( Network::Mainnet, - fixture_heights(include_bytes!( - "../../dash/tests/data/test_DML_diffs/block_container_2518986.dat" - )), + Arc::new(RwLock::new(MockHeaderStorage::new(heights))), ); let qr_info: QRInfo = decode_fixture(include_bytes!( "../../dash/tests/data/test_DML_diffs/qrinfo_2518986.dat" )); + engine.feed_merkle_roots(qr_info_diffs(&qr_info)); (engine, qr_info) } + #[test_case(true; "matching its commitments")] + #[test_case(false; "off its masternode list root")] #[tokio::test] - async fn validate_first_qr_info_on_fresh_engine_with_mixed_cycle_quorums() { - let (mut engine, qr_info) = load_qrinfo_2518986_fixture(); + async fn validate_first_qr_info_on_fresh_engine_with_mixed_cycle_quorums(committed: bool) { + let (mut engine, mut qr_info) = load_qrinfo_2518986_fixture(); + if !committed { + let tip = &mut qr_info.mn_list_diff_tip; + tip.new_masternodes.push(MasternodeListEntry::dummy(0x11)); + let tip_hash = tip.block_hash; + let result = engine.feed_qr_info(qr_info).await; + assert_eq!(result, Err(SmlError::CoinbaseCommitmentMismatch(tip_hash).into())); + return; + } // The fixture's active set mixes cycles: quorum index 0 carries the // previous cycle's commitment (its DKG failed in the current cycle), diff --git a/dash-spv/src/storage/masternode.rs b/dash-spv/src/storage/masternode.rs index 3b6e095bc..2417d2c37 100644 --- a/dash-spv/src/storage/masternode.rs +++ b/dash-spv/src/storage/masternode.rs @@ -20,6 +20,8 @@ use crate::storage::{io::atomic_write, BlockHeaderStorage}; type IndexMap = BTreeMap; +type NewEngine = fn(Network, Arc>) -> MasternodeListEngine; + enum Message { Diff(Box), QrInfo(Box), @@ -50,6 +52,7 @@ pub struct PersistentMasternodeStorage { network: Network, diffs: IndexMap, qr_infos: IndexMap, + new_engine: NewEngine, } /// The stored messages as of when it was taken. A replay runs on this, so the @@ -59,6 +62,7 @@ pub struct MessageLog { network: Network, diffs: IndexMap, qr_infos: IndexMap, + new_engine: NewEngine, } impl PersistentMasternodeStorage { @@ -81,9 +85,17 @@ impl PersistentMasternodeStorage { network, diffs, qr_infos, + new_engine: MasternodeListEngine::new, }) } + /// Replays into engines that do not validate diffs against their block. + #[cfg(any(test, feature = "test-utils"))] + pub fn without_validation(mut self) -> Self { + self.new_engine = MasternodeListEngine::new_without_validation; + self + } + fn folder(&self) -> PathBuf { self.storage_path.join(Self::FOLDER_NAME) } @@ -154,6 +166,7 @@ impl PersistentMasternodeStorage { network: self.network, diffs: self.diffs.clone(), qr_infos: self.qr_infos.clone(), + new_engine: self.new_engine, } } @@ -173,7 +186,7 @@ impl MessageLog { until: CoreBlockHeight, mut visit: impl FnMut(&MasternodeListEngine), ) -> MasternodeListEngine { - let mut engine = MasternodeListEngine::new(self.network, Arc::clone(&self.headers)); + let mut engine = (self.new_engine)(self.network, Arc::clone(&self.headers)); let entries = self.diffs.range(..=until).map(|(height, path)| (*height, Kind::Diff, path)).chain( @@ -365,11 +378,12 @@ mod tests { let map = heights.iter().map(|(b, h)| (hash(*b), *h)).collect(); PersistentMasternodeStorage::open( dir.path(), - Arc::new(RwLock::new(MockHeaderStorage(map))), + Arc::new(RwLock::new(MockHeaderStorage::new(map))), Network::Regtest, ) .await .expect("open") + .without_validation() } #[tokio::test] @@ -624,11 +638,12 @@ mod tests { let heights = (1..=tip).map(|height| (BlockHash::dummy(height), height)).collect(); let storage = S::open( dir.path(), - Arc::new(RwLock::new(MockHeaderStorage(heights))), + Arc::new(RwLock::new(MockHeaderStorage::new(heights))), Network::Regtest, ) .await - .unwrap(); + .unwrap() + .without_validation(); let engine = storage.load_engine().await; assert!(engine @@ -685,11 +700,12 @@ mod tests { let dir = TempDir::new().unwrap(); let mut storage = PersistentMasternodeStorage::open( dir.path(), - Arc::new(RwLock::new(MockHeaderStorage(heights))), + Arc::new(RwLock::new(MockHeaderStorage::new(heights))), Network::Mainnet, ) .await - .unwrap(); + .unwrap() + .without_validation(); storage.store_qr_info(tip_height, &qr_info).await.unwrap(); let diff = MnListDiff { base_block_hash: base, diff --git a/dash-spv/src/sync/masternodes/sync_manager.rs b/dash-spv/src/sync/masternodes/sync_manager.rs index 9108923d8..280381e91 100644 --- a/dash-spv/src/sync/masternodes/sync_manager.rs +++ b/dash-spv/src/sync/masternodes/sync_manager.rs @@ -737,7 +737,10 @@ mod tests { ) .await .unwrap(); - let engine = MasternodeListEngine::new(Network::Regtest, Arc::clone(&block_headers)); + let engine = MasternodeListEngine::new_without_validation( + Network::Regtest, + Arc::clone(&block_headers), + ); let mut manager = MasternodesManager::new( block_headers, Arc::new(RwLock::new(engine)), @@ -811,7 +814,10 @@ mod tests { .await .unwrap(); let hashes: Vec = headers.iter().map(|header| header.block_hash()).collect(); - let mut engine = MasternodeListEngine::new(Network::Regtest, Arc::clone(&block_headers)); + let mut engine = MasternodeListEngine::new_without_validation( + Network::Regtest, + Arc::clone(&block_headers), + ); for list in lists(&hashes) { engine.masternode_lists.insert(list.known_height, list); } diff --git a/dash-spv/src/test_utils/header_storage.rs b/dash-spv/src/test_utils/header_storage.rs index 85bbb2162..33c8d843a 100644 --- a/dash-spv/src/test_utils/header_storage.rs +++ b/dash-spv/src/test_utils/header_storage.rs @@ -3,7 +3,7 @@ use std::ops::Range; use async_trait::async_trait; use dashcore::block::Header as BlockHeader; -use dashcore::BlockHash; +use dashcore::{BlockHash, TxMerkleNode}; use crate::error::StorageResult; use crate::storage::{BlockHeaderStorage, BlockHeaderTip}; @@ -11,8 +11,21 @@ use crate::types::HashedBlockHeader; /// A [`BlockHeaderStorage`] that answers hash and height lookups from a map /// and nothing else. A header it returns carries the mapped hash on a dummy -/// header. Everything else is inert. -pub struct MockHeaderStorage(pub HashMap); +/// header, with its mapped merkle root if any. Everything else is inert. +#[derive(Default)] +pub struct MockHeaderStorage { + pub heights: HashMap, + pub merkle_roots: HashMap, +} + +impl MockHeaderStorage { + pub fn new(heights: HashMap) -> Self { + Self { + heights, + merkle_roots: HashMap::new(), + } + } +} #[async_trait] impl BlockHeaderStorage for MockHeaderStorage { @@ -42,15 +55,19 @@ impl BlockHeaderStorage for MockHeaderStorage { 0 } async fn get_header(&self, height: u32) -> StorageResult> { - Ok(self.0.iter().find(|(_, h)| **h == height).map(|(hash, _)| { - HashedBlockHeader::with_trusted_hash(BlockHeader::dummy(height), *hash) + Ok(self.heights.iter().find(|(_, h)| **h == height).map(|(hash, _)| { + let mut header = BlockHeader::dummy(height); + if let Some(merkle_root) = self.merkle_roots.get(hash) { + header.merkle_root = *merkle_root; + } + HashedBlockHeader::with_trusted_hash(header, *hash) })) } async fn get_header_height_by_hash(&self, hash: &BlockHash) -> StorageResult> { - Ok(self.0.get(hash).copied()) + Ok(self.heights.get(hash).copied()) } async fn truncate_above(&mut self, target_height: u32) -> StorageResult<()> { - self.0.retain(|_, h| *h <= target_height); + self.heights.retain(|_, h| *h <= target_height); Ok(()) } } diff --git a/dash-spv/src/validation/masternode_list.rs b/dash-spv/src/validation/masternode_list.rs new file mode 100644 index 000000000..f85697d21 --- /dev/null +++ b/dash-spv/src/validation/masternode_list.rs @@ -0,0 +1,57 @@ +use std::sync::Arc; + +use dashcore::network::message_sml::MnListDiff; +use dashcore::prelude::CoreBlockHeight; +use dashcore::sml::error::SmlError; +use dashcore::sml::masternode_list::MasternodeList; +use dashcore::Transaction; +use tokio::sync::RwLock; + +use crate::storage::BlockHeaderStorage; + +/// Validates a masternode list diff against its block: its merkle proof must +/// prove its coinbase in the stored header, and the list it builds must hash +/// to the roots that coinbase commits to. +pub struct MasternodeListValidator { + headers: Arc>, +} + +impl MasternodeListValidator { + pub fn new(headers: Arc>) -> Self { + Self { + headers, + } + } + + pub async fn validate_coinbase_proof( + &self, + diff: &MnListDiff, + height: CoreBlockHeight, + ) -> Result<(), SmlError> { + let header = match self.headers.read().await.get_header(height).await { + Ok(header) => header, + Err(e) => { + tracing::warn!("Could not look up the block at {height}: {e}"); + None + } + }; + + let header = header + .filter(|header| *header.hash() == diff.block_hash) + .ok_or(SmlError::BlockHashLookupFailed(diff.block_hash))?; + + if diff.coinbase_merkle_root()? != header.header().merkle_root { + return Err(SmlError::InvalidCoinbaseProof(diff.block_hash)); + } + + Ok(()) + } + + pub fn validate_coinbase_roots( + &self, + masternode_list: &MasternodeList, + coinbase_tx: &Transaction, + ) -> Result<(), SmlError> { + masternode_list.verify_coinbase_roots(coinbase_tx) + } +} diff --git a/dash-spv/src/validation/mod.rs b/dash-spv/src/validation/mod.rs index 46ee7dd31..52e6275a1 100644 --- a/dash-spv/src/validation/mod.rs +++ b/dash-spv/src/validation/mod.rs @@ -5,12 +5,14 @@ mod chainlock; mod filter; mod header; mod instantlock; +mod masternode_list; pub use block::BlockValidator; pub use chainlock::ChainLockValidator; pub use filter::{FilterValidationInput, FilterValidator}; pub use header::BlockHeaderValidator; pub use instantlock::{InstantLockStructureValidator, InstantLockValidator}; +pub use masternode_list::MasternodeListValidator; use crate::error::ValidationResult; diff --git a/dash/src/merkle_tree/block.rs b/dash/src/merkle_tree/block.rs index 5dfd9e6ac..fb96ba00d 100644 --- a/dash/src/merkle_tree/block.rs +++ b/dash/src/merkle_tree/block.rs @@ -50,6 +50,7 @@ use crate::blockdata::transaction::Transaction; use crate::consensus::encode::{self, Decodable, Encodable}; use crate::hash_types::{TxMerkleNode, Txid}; use crate::io; +use crate::network::message_sml::MnListDiff; use crate::prelude::*; /// Data structure that represents a block header paired to a partial merkle tree. @@ -208,6 +209,34 @@ pub struct PartialMerkleTree { } impl PartialMerkleTree { + /// Constructs a partial merkle tree from the given `MnListDiff`. + pub fn from_mn_list_diff(mn_list_diff: &MnListDiff) -> Self { + PartialMerkleTree::from_parts( + mn_list_diff.total_transactions, + mn_list_diff + .merkle_hashes + .iter() + .map(|hash| TxMerkleNode::from_raw_hash(hash.to_raw_hash())) + .collect(), + &mn_list_diff.merkle_flags, + ) + } + + /// `bytes` packs the bits per 8 in a byte, least significant bit first. + fn from_parts(num_transactions: u32, hashes: Vec, bytes: &[u8]) -> Self { + let mut bits: Vec = vec![false; bytes.len() * 8]; + + for (p, bit) in bits.iter_mut().enumerate() { + *bit = (bytes[p / 8] & (1 << (p % 8) as u8)) != 0; + } + + PartialMerkleTree { + num_transactions, + bits, + hashes, + } + } + /// Returns the total number of transactions in the block. pub fn num_transactions(&self) -> u32 { self.num_transactions @@ -456,16 +485,7 @@ impl Decodable for PartialMerkleTree { let hashes: Vec = Decodable::consensus_decode(r)?; let bytes: Vec = Decodable::consensus_decode(r)?; - let mut bits: Vec = vec![false; bytes.len() * 8]; - - for (p, bit) in bits.iter_mut().enumerate() { - *bit = (bytes[p / 8] & (1 << (p % 8) as u8)) != 0; - } - Ok(PartialMerkleTree { - num_transactions, - hashes, - bits, - }) + Ok(PartialMerkleTree::from_parts(num_transactions, hashes, &bytes)) } } diff --git a/dash/src/network/message_sml.rs b/dash/src/network/message_sml.rs index e3d206d95..0740e6e00 100644 --- a/dash/src/network/message_sml.rs +++ b/dash/src/network/message_sml.rs @@ -2,8 +2,10 @@ use bincode::{Decode, Encode}; use crate::bls_sig_utils::BLSSignature; -use crate::hash_types::MerkleRootMasternodeList; +use crate::hash_types::{MerkleRootMasternodeList, TxMerkleNode}; use crate::internal_macros::impl_consensus_encoding; +use crate::merkle_tree::PartialMerkleTree; +use crate::sml::error::SmlError; use crate::sml::llmq_type::LLMQType; use crate::sml::masternode_list_entry::MasternodeListEntry; use crate::transaction::special_transaction::quorum_commitment::QuorumEntry; @@ -76,6 +78,26 @@ impl_consensus_encoding!( quorums_chainlock_signatures ); +impl MnListDiff { + /// The block merkle root the proof yields, once it proves `coinbase_tx` first. + pub fn coinbase_merkle_root(&self) -> Result { + let proof = PartialMerkleTree::from_mn_list_diff(self); + + let mut matches = vec![]; + let mut indexes = vec![]; + + let root = proof + .extract_matches(&mut matches, &mut indexes) + .map_err(|_| SmlError::InvalidCoinbaseProof(self.block_hash))?; + + if matches != [self.coinbase_tx.txid()] || indexes != [0] { + return Err(SmlError::InvalidCoinbaseProof(self.block_hash)); + } + + Ok(root) + } +} + #[derive(PartialEq, Eq, Clone, Debug)] #[cfg_attr(feature = "bincode", derive(Encode, Decode))] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] @@ -100,9 +122,30 @@ impl_consensus_encoding!(DeletedQuorum, llmq_type, quorum_hash); mod tests { use assert_matches::assert_matches; + use test_case::test_case; + use crate::consensus::{deserialize, serialize}; use crate::network::message::{NetworkMessage, RawNetworkMessage}; use crate::network::message_sml::MnListDiff; + use crate::sml::error::SmlError; + + #[test_case(|_| {}, true; "proving its coinbase")] + #[test_case(|diff| diff.coinbase_tx.lock_time += 1, false; "of another coinbase")] + #[test_case(|diff| diff.merkle_flags.clear(), false; "malformed")] + fn coinbase_merkle_root(tamper: fn(&mut MnListDiff), proves: bool) { + let mut diff: MnListDiff = deserialize(include_bytes!( + "../../tests/data/test_DML_diffs/mn_list_diff_0_2227096.bin" + )) + .expect("expected to deserialize"); + tamper(&mut diff); + + let result = diff.coinbase_merkle_root(); + + assert_eq!(result.is_ok(), proves); + if !proves { + assert_eq!(result, Err(SmlError::InvalidCoinbaseProof(diff.block_hash))); + } + } #[test] fn deserialize_mn_list_diff() { diff --git a/dash/src/sml/error.rs b/dash/src/sml/error.rs index 5005ee777..2543cebf0 100644 --- a/dash/src/sml/error.rs +++ b/dash/src/sml/error.rs @@ -42,4 +42,10 @@ pub enum SmlError { /// Error indicating the quorum signature set is incomplete (some slots were not filled). #[error("Incomplete quorum signature set; not all slots were filled")] IncompleteSignatureSet, + + #[error("The MnListDiff merkle proof does not prove the coinbase of block {0}")] + InvalidCoinbaseProof(BlockHash), + + #[error("The coinbase of block {0} does not commit to the lists the MnListDiff builds")] + CoinbaseCommitmentMismatch(BlockHash), } diff --git a/dash/src/sml/masternode_list/from_diff.rs b/dash/src/sml/masternode_list/from_diff.rs index 0b756457e..029a2a4cf 100644 --- a/dash/src/sml/masternode_list/from_diff.rs +++ b/dash/src/sml/masternode_list/from_diff.rs @@ -95,15 +95,7 @@ impl MasternodeList { }, ); - // Construct `MasternodeList` - Ok(MasternodeList { - block_hash: diff.block_hash, - known_height, - masternode_merkle_root: diff.merkle_hashes.first().cloned(), - llmq_merkle_root: None, // Adjust based on real data availability - masternodes: masternodes.into(), - quorums: quorums.into(), - }) + Ok(MasternodeList::build(masternodes, quorums, diff.block_hash, known_height).build()) } } diff --git a/dash/src/sml/masternode_list/merkle_roots.rs b/dash/src/sml/masternode_list/merkle_roots.rs index 1d4e4bcbf..4ba79c53b 100644 --- a/dash/src/sml/masternode_list/merkle_roots.rs +++ b/dash/src/sml/masternode_list/merkle_roots.rs @@ -2,6 +2,7 @@ use hashes::{Hash, sha256d}; use crate::Transaction; use crate::hash_types::{MerkleRootMasternodeList, MerkleRootQuorums}; +use crate::sml::error::SmlError; use crate::sml::masternode_list::MasternodeList; use crate::transaction::special_transaction::TransactionPayload; @@ -44,66 +45,34 @@ pub fn merkle_root_from_hashes(hashes: Vec) -> Option bool { + /// The quorum root is in the coinbase payload from its version 2. An empty + /// list hashes to the all-zero root, as in Dash Core. + pub fn verify_coinbase_roots( + &self, + coinbase_transaction: &Transaction, + ) -> Result<(), SmlError> { let Some(TransactionPayload::CoinbasePayloadType(coinbase_payload)) = &coinbase_transaction.special_transaction_payload else { - return false; + return Err(SmlError::CoinbaseCommitmentMismatch(self.block_hash)); }; - // we need to check that the coinbase is in the transaction hashes we got back - // and is in the merkle block - if let Some(mn_merkle_root) = self.masternode_merkle_root { - coinbase_payload.merkle_root_masternode_list == mn_merkle_root - } else { - false - } - } - /// Validates whether the stored LLMQ list Merkle root matches the one in the coinbase transaction. - /// - /// This function compares the calculated quorum Merkle root with the one provided - /// in the coinbase transaction payload to verify the integrity of the quorum list. - /// - /// # Parameters - /// - /// - `coinbase_transaction`: The coinbase transaction containing the expected Merkle root. - /// - /// # Returns - /// - /// - `true` if the Merkle root matches. - /// - `false` otherwise. - pub fn has_valid_llmq_list_root(&self, coinbase_transaction: &Transaction) -> bool { - let Some(TransactionPayload::CoinbasePayloadType(coinbase_payload)) = - &coinbase_transaction.special_transaction_payload - else { - return false; - }; + let calculated = self + .calculate_masternodes_merkle_root(self.known_height) + .unwrap_or_else(MerkleRootMasternodeList::all_zeros); + if calculated != coinbase_payload.merkle_root_masternode_list { + return Err(SmlError::CoinbaseCommitmentMismatch(self.block_hash)); + } - let q_merkle_root = self.llmq_merkle_root; - let coinbase_merkle_root_quorums = coinbase_payload.merkle_root_quorums; - let has_valid_quorum_list_root = - q_merkle_root.is_some() && coinbase_merkle_root_quorums == q_merkle_root.unwrap(); - if !has_valid_quorum_list_root { - // warn!("LLMQ Merkle root not valid for DML on block {} version {} ({:?} wanted - {:?} calculated)", - // tx.height, - // tx.base.version, - // tx.merkle_root_llmq_list.map(|q| q.to_hex()).unwrap_or("None".to_string()), - // self.llmq_merkle_root.map(|q| q.to_hex()).unwrap_or("None".to_string())); + if coinbase_payload.version >= 2 { + let calculated = + self.calculate_llmq_merkle_root().unwrap_or_else(MerkleRootQuorums::all_zeros); + if calculated != coinbase_payload.merkle_root_quorums { + return Err(SmlError::CoinbaseCommitmentMismatch(self.block_hash)); + } } - has_valid_quorum_list_root + + Ok(()) } /// Computes the Merkle root for the masternode list at a given block height. @@ -184,3 +153,61 @@ impl MasternodeList { llmq_commitment_hashes } } + +#[cfg(test)] +mod tests { + use hashes::Hash; + use test_case::test_case; + + use crate::consensus::deserialize; + use crate::hash_types::{MerkleRootMasternodeList, MerkleRootQuorums}; + use crate::network::message_sml::MnListDiff; + use crate::sml::error::SmlError; + use crate::sml::masternode_list::MasternodeList; + use crate::transaction::special_transaction::TransactionPayload; + use crate::transaction::special_transaction::coinbase::CoinbasePayload; + use crate::{BlockHash, Network}; + + #[test_case(|_| {}, None; "matching")] + #[test_case( + |payload| payload.merkle_root_masternode_list = MerkleRootMasternodeList::all_zeros(), + Some(SmlError::CoinbaseCommitmentMismatch); + "off the masternode list root" + )] + #[test_case( + |payload| payload.merkle_root_quorums = MerkleRootQuorums::all_zeros(), + Some(SmlError::CoinbaseCommitmentMismatch); + "off the quorum list root" + )] + #[test_case( + |payload| { + payload.version = 1; + payload.merkle_root_quorums = MerkleRootQuorums::all_zeros(); + }, + None; + "a version 1 coinbase committing to no quorums" + )] + fn verify_coinbase_roots( + tamper: fn(&mut CoinbasePayload), + error: Option SmlError>, + ) { + let diff: MnListDiff = deserialize(include_bytes!( + "../../../tests/data/test_DML_diffs/mn_list_diff_0_2227096.bin" + )) + .expect("expected to deserialize"); + let mut coinbase = diff.coinbase_tx.clone(); + let list = MasternodeList::from_diff(diff, 2_227_096, Network::Mainnet).expect("builds"); + let Some(TransactionPayload::CoinbasePayloadType(payload)) = + &mut coinbase.special_transaction_payload + else { + panic!("coinbase payload"); + }; + // The capture's masternodes do not hash to its coinbase's root, its quorums do. + payload.merkle_root_masternode_list = list.masternode_merkle_root.expect("masternodes"); + tamper(payload); + + let result = list.verify_coinbase_roots(&coinbase); + + assert_eq!(result, error.map_or(Ok(()), |error| Err(error(list.block_hash)))); + } +} From 55e4015ad2395e6788c602dae234a4b3dd45da2e Mon Sep 17 00:00:00 2001 From: Borja Castellano Date: Mon, 26 Oct 2026 13:17:33 -0700 Subject: [PATCH 2/3] test(dash-spv): adapt the coinbase-proof tests to FeedError after merging dev Co-Authored-By: Claude Opus 5.5 --- dash-spv/src/sml_engine.rs | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/dash-spv/src/sml_engine.rs b/dash-spv/src/sml_engine.rs index 91ad58f1a..918888a75 100644 --- a/dash-spv/src/sml_engine.rs +++ b/dash-spv/src/sml_engine.rs @@ -1711,7 +1711,15 @@ mod tests { tip.new_masternodes.push(MasternodeListEntry::dummy(0x11)); let tip_hash = tip.block_hash; let result = engine.feed_qr_info(qr_info).await; - assert_eq!(result, Err(SmlError::CoinbaseCommitmentMismatch(tip_hash).into())); + assert!( + matches!( + result, + Err(FeedError::Refused(QuorumValidationError::SMLError( + SmlError::CoinbaseCommitmentMismatch(hash) + ))) if hash == tip_hash + ), + "got {result:?}" + ); return; } @@ -2148,14 +2156,22 @@ mod tests { unreadable: BlockHash, readable: usize, ) -> MasternodeListEngine { + let inner = { + let headers = engine.headers(); + let headers = headers.try_read().expect("unlocked"); + MockHeaderStorage { + heights: headers.heights.clone(), + merkle_roots: headers.merkle_roots.clone(), + } + }; let headers = Arc::new(RwLock::new(FailingFrom { - inner: MockHeaderStorage(engine.headers().try_read().expect("unlocked").0.clone()), + inner, unreadable, readable, reads: AtomicUsize::new(0), hangs: false, })); - let mut failing = MasternodeListEngine::new(engine.network, headers); + let mut failing = MasternodeListEngine::new_without_validation(engine.network, headers); failing.masternode_lists = engine.masternode_lists.clone(); failing.rotated_quorums_per_cycle = engine.rotated_quorums_per_cycle.clone(); failing From 25165965fd7bbf9c85c9d4c86833307e3b09cf64 Mon Sep 17 00:00:00 2001 From: Borja Castellano Date: Wed, 28 Oct 2026 05:05:15 -0700 Subject: [PATCH 3/3] fix(dash-spv): fail the feed on a header read error during coinbase validation validate_coinbase_proof turned a header storage read error into BlockHashLookupFailed, which refused the message. It now returns FeedError and propagates the error as HeaderStorage, keeping only a missing header or a mismatched hash as refusals. Co-Authored-By: Claude Opus 5.5 --- dash-spv/src/sml_engine.rs | 33 ++++++++++++++++++++++ dash-spv/src/validation/masternode_list.rs | 24 ++++++++-------- 2 files changed, 46 insertions(+), 11 deletions(-) diff --git a/dash-spv/src/sml_engine.rs b/dash-spv/src/sml_engine.rs index d1da887c3..586631a16 100644 --- a/dash-spv/src/sml_engine.rs +++ b/dash-spv/src/sml_engine.rs @@ -1299,6 +1299,7 @@ mod tests { use crate::storage::{BlockHeaderStorage, BlockHeaderTip}; use crate::test_utils::MockHeaderStorage; use crate::types::HashedBlockHeader; + use crate::validation::MasternodeListValidator; use bincode::{config, decode_from_slice, Decode}; use dashcore::bls_sig_utils::{BLSPublicKey, BLSSignature}; use dashcore::consensus::deserialize; @@ -2260,6 +2261,38 @@ mod tests { } } + /// Our header storage failing to read a diff's block while its coinbase + /// proof is checked fails the feed with `HeaderStorage` rather than + /// refusing the QRInfo, for every diff the QRInfo carries. + #[tokio::test] + async fn a_header_storage_failure_never_refuses_a_validated_qr_info() { + let (engine, qr_info) = load_qrinfo_2518986_fixture(); + let blocks: Vec = + qr_info_diffs(&qr_info).iter().map(|diff| diff.block_hash).collect(); + + for unreadable in blocks { + let validating = |readable| { + let mut failing = with_reads_failing(&engine, unreadable, readable); + failing.validator = + Some(MasternodeListValidator::new(Arc::clone(&failing.headers))); + failing + }; + let mut fed = validating(usize::MAX); + let result = fed.feed_qr_info(qr_info.clone()).await; + assert!(result.is_ok(), "the QRInfo feeds while the storage reads, got {result:?}"); + let reads = fed.headers.read().await.reads.load(Ordering::Relaxed); + assert!(reads > 0, "the feed reads {unreadable}"); + for readable in 0..reads { + let result = validating(readable).feed_qr_info(qr_info.clone()).await; + assert!( + !matches!(result, Err(FeedError::Refused(_))), + "reading {unreadable} failing from its read {readable} on refused the \ + QRInfo: {result:?}" + ); + } + } + } + /// Stopping the client, as following a fork does, aborts a manager task /// still running after a grace period, dropping a QRInfo feed part way. /// That leaves the engine as it was, as a refusal does, so a run started diff --git a/dash-spv/src/validation/masternode_list.rs b/dash-spv/src/validation/masternode_list.rs index f85697d21..a9fcc7b08 100644 --- a/dash-spv/src/validation/masternode_list.rs +++ b/dash-spv/src/validation/masternode_list.rs @@ -7,6 +7,7 @@ use dashcore::sml::masternode_list::MasternodeList; use dashcore::Transaction; use tokio::sync::RwLock; +use crate::sml_engine::FeedError; use crate::storage::BlockHeaderStorage; /// Validates a masternode list diff against its block: its merkle proof must @@ -23,25 +24,26 @@ impl MasternodeListValidator { } } + /// Fails with `HeaderStorage` when the header cannot be read, and refuses + /// the diff when its block is not the one stored at `height` or its proof + /// does not match the header's merkle root. pub async fn validate_coinbase_proof( &self, diff: &MnListDiff, height: CoreBlockHeight, - ) -> Result<(), SmlError> { - let header = match self.headers.read().await.get_header(height).await { - Ok(header) => header, - Err(e) => { - tracing::warn!("Could not look up the block at {height}: {e}"); - None - } - }; - - let header = header + ) -> Result<(), FeedError> { + let header = self + .headers + .read() + .await + .get_header(height) + .await + .map_err(FeedError::HeaderStorage)? .filter(|header| *header.hash() == diff.block_hash) .ok_or(SmlError::BlockHashLookupFailed(diff.block_hash))?; if diff.coinbase_merkle_root()? != header.header().merkle_root { - return Err(SmlError::InvalidCoinbaseProof(diff.block_hash)); + return Err(SmlError::InvalidCoinbaseProof(diff.block_hash).into()); } Ok(())