feat(dash-spv)!: check masternode list diffs against the block coinbase

dash-spv/src/storage/mod.rs

@@ -33,7 +33,9 @@ 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(crate) use crate::storage::masternode::feed_qrinfo_heights_to_engine;
+pub(crate) use crate::storage::masternode::{
+    feed_qrinfo_heights_to_engine, verify_diff_coinbase, verify_qr_info_coinbases,
+};
 pub use crate::storage::masternode::{MasternodeStorage, PersistentMasternodeStorage};
 pub use crate::storage::metadata::{MetadataStorage, PersistentMetadataStorage};
 pub use crate::storage::peers::{PeerStorage, PersistentPeerStorage};

dash-spv/src/sync/masternodes/sync_manager.rs

@@ -1,7 +1,10 @@
 use super::manager::PipelineMode;
 use crate::error::SyncResult;
 use crate::network::{Message, MessageType, RequestSender};
-use crate::storage::{feed_qrinfo_heights_to_engine, BlockHeaderStorage};
+use crate::storage::{
+    feed_qrinfo_heights_to_engine, verify_diff_coinbase, verify_qr_info_coinbases,
+    BlockHeaderStorage,
+};
 use crate::sync::{
     ManagerIdentifier, MasternodesManager, SyncEvent, SyncManager, SyncManagerProgress, SyncState,
 };
@@ -209,8 +212,18 @@ impl<H: BlockHeaderStorage> SyncManager for MasternodesManager<H> {
                 }
                 tracing::info!("Processing QRInfo message");
 
-                // Feed block heights to engine using internal storage
+                // Every diff's coinbase has to be the first transaction of its
+                // stored block before the engine checks the lists against it.
                 let storage = self.header_storage.read().await;
+                if let Err(e) = verify_qr_info_coinbases(&*storage, qr_info).await {
+                    drop(storage);
+                    tracing::error!("QRInfo does not match the stored block headers: {}", e);
+                    // Rejected like a response the engine refuses, see below.
+                    self.sync_state.qrinfo_rejected();
+                    return Err(SyncError::MasternodeSyncFailed(e.to_string()));
+                }
+
+                // Feed block heights to engine using internal storage
                 let mut engine = self.engine.write().await;
                 let fed = feed_qrinfo_heights_to_engine(&mut engine, qr_info, &*storage).await;
                 drop(storage);
@@ -378,28 +391,43 @@ impl<H: BlockHeaderStorage> SyncManager for MasternodesManager<H> {
                         return Ok(vec![]);
                     }
                 };
-                drop(storage);
 
-                // Apply diff to engine
-                let mut engine = self.engine.write().await;
-                engine.feed_block_height(target_height, diff.block_hash);
+                // The diff's coinbase has to be the first transaction of the
+                // stored block before the engine checks the list against it.
+                let proven = verify_diff_coinbase(&*storage, diff).await;
+                drop(storage);
 
-                let apply_ok = match engine.apply_diff(diff.clone(), Some(target_height), None) {
-                    Ok(_) => {
-                        self.sync_state.known_mn_list_heights.insert(target_height);
-                        tracing::debug!("Applied MnListDiff at height {}", target_height);
-                        true
+                let apply_ok = match proven {
+                    Ok(()) => {
+                        // Apply diff to engine
+                        let mut engine = self.engine.write().await;
+                        engine.feed_block_height(target_height, diff.block_hash);
+
+                        match engine.apply_diff(diff.clone(), Some(target_height), None) {
+                            Ok(_) => {
+                                self.sync_state.known_mn_list_heights.insert(target_height);
+                                tracing::debug!("Applied MnListDiff at height {}", target_height);
+                                true
+                            }
+                            Err(e) => {
+                                tracing::warn!(
+                                    "Failed to apply MnListDiff at height {}: {}",
+                                    target_height,
+                                    e
+                                );
+                                false
+                            }
+                        }
                     }
                     Err(e) => {
                         tracing::warn!(
-                            "Failed to apply MnListDiff at height {}: {}",
+                            "MnListDiff at height {} does not match its block header: {}",
                             target_height,
                             e
                         );
                         false
                     }
                 };
-                drop(engine);
 
                 if apply_ok {
                     if let Err(e) = self.store_diff(target_height, diff).await {
@@ -724,7 +752,7 @@ impl<H: BlockHeaderStorage> SyncManager for MasternodesManager<H> {
 
 #[cfg(test)]
 mod tests {
-    use super::super::manager::{MasternodeSyncState, QRInfoInFlight};
+    use super::super::manager::{MasternodeSyncState, PipelineMode, QRInfoInFlight};
     use super::{
         qrinfo_timeout_for, MAX_RETRY_ATTEMPTS, QRINFO_STALL_WATCHDOG, QRINFO_TIMEOUT_SCHEDULE_SECS,
     };
@@ -746,7 +774,7 @@ mod tests {
     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};
+    use dashcore::{BlockHash, Network, TxMerkleNode};
     use dashcore_hashes::Hash;
     use std::collections::HashMap;
     use std::sync::Arc;
@@ -828,7 +856,8 @@ mod tests {
             network: Network::Testnet,
             ..Default::default()
         };
-        feed_qrinfo_heights_to_engine(&mut engine, &qr_info, &MockHeaderStorage(height_map)).await;
+        feed_qrinfo_heights_to_engine(&mut engine, &qr_info, &MockHeaderStorage::new(height_map))
+            .await;
 
         for &b in expected_hashes {
             let hash = BlockHash::from_slice(&[b; 32]).unwrap();
@@ -1016,7 +1045,10 @@ mod tests {
             DiskStorageManager::new(&ClientConfig::testnet().with_storage_path(dir.path()))
                 .await
                 .unwrap();
-        let headers = Header::dummy_batch(0..target + 1);
+        let template = MnListDiff::dummy_between(0, 0);
+        let mut headers = Header::dummy_batch(0..target + 1);
+        // The stored header of the diff's block proves its coinbase.
+        headers[target as usize].merkle_root = template.dummy_block_merkle_root();
         let block_headers = storage.block_headers();
         block_headers
             .write()
@@ -1047,7 +1079,7 @@ mod tests {
         let diff = MnListDiff {
             base_block_hash: tip_hash,
             block_hash: target_hash,
-            ..MnListDiff::dummy_between(0, 0)
+            ..template
         };
         let peer = "127.0.0.1:19999".parse().unwrap();
         manager
@@ -1142,6 +1174,64 @@ mod tests {
         );
     }
 
+    /// A diff reaches the engine only once its coinbase proof leads to the
+    /// merkle root of the stored header of its block. A diff that fails is
+    /// dropped like one the engine refuses.
+    #[tokio::test]
+    async fn test_mnlistdiff_applies_only_when_its_block_header_proves_the_coinbase() {
+        let tip = 200u32;
+        let peer = "127.0.0.1:19999".parse().unwrap();
+        for header_proves_it in [true, false] {
+            let template = MnListDiff::dummy(0x00, 0x01);
+            let mut headers = Header::dummy_batch(0..tip + 1);
+            headers[tip as usize].merkle_root = if header_proves_it {
+                template.dummy_block_merkle_root()
+            } else {
+                TxMerkleNode::all_zeros()
+            };
+            let tip_hash = headers[tip as usize].block_hash();
+            let storage = DiskStorageManager::with_temp_dir().await.unwrap();
+            let block_headers = storage.block_headers();
+            block_headers
+                .write()
+                .await
+                .store_headers(&headers.iter().map(HashedBlockHeader::from).collect::<Vec<_>>())
+                .await
+                .unwrap();
+            let engine =
+                Arc::new(RwLock::new(MasternodeListEngine::default_for_network(Network::Regtest)));
+            let mut manager =
+                MasternodesManager::new(block_headers, Arc::clone(&engine), Network::Regtest, None)
+                    .await;
+            manager.set_state(SyncState::Synced);
+            manager.sync_state.pipeline_mode = PipelineMode::Incremental;
+            let (tx, _rx) = mpsc::unbounded_channel();
+            let requests = RequestSender::new(tx);
+            manager
+                .sync_state
+                .mnlistdiff_pipeline
+                .queue_requests(vec![(BlockHash::all_zeros(), tip_hash)]);
+            manager.sync_state.mnlistdiff_pipeline.send_pending(&requests).unwrap();
+
+            let diff = MnListDiff {
+                block_hash: tip_hash,
+                ..template
+            };
+            manager
+                .handle_message(Message::new(peer, NetworkMessage::MnListDiff(diff)), &requests)
+                .await
+                .expect("an unusable diff is dropped, not an error");
+
+            assert_eq!(
+                engine.read().await.masternode_lists.contains_key(&tip),
+                header_proves_it,
+                "header proves the coinbase: {header_proves_it}"
+            );
+            assert_eq!(manager.sync_state.known_mn_list_heights.contains(&tip), header_proves_it);
+            assert!(manager.sync_state.mnlistdiff_pipeline.is_complete());
+        }
+    }
+
     /// The stall watchdog is the backstop for every route into the stranded state
     /// that keeping the in-flight flag cannot cover - notably a
     /// `send_qrinfo_for_tip` that returns without dispatching *after* its caller

dash-spv/src/test_utils/header_storage.rs

@@ -2,15 +2,51 @@ use std::collections::HashMap;
 use std::ops::Range;
 
 use async_trait::async_trait;
-use dashcore::BlockHash;
+use dashcore::block::Header;
+use dashcore::network::message_sml::MnListDiff;
+use dashcore::{BlockHash, TxMerkleNode};
 
 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<BlockHash, u32>);
+/// A [`BlockHeaderStorage`] that answers hash lookups and header reads from
+/// maps and nothing else. Everything outside `get_header_height_by_hash` and
+/// `get_header` is inert.
+#[derive(Default)]
+pub struct MockHeaderStorage {
+    heights: HashMap<BlockHash, u32>,
+    headers: HashMap<u32, HashedBlockHeader>,
+}
+
+impl MockHeaderStorage {
+    /// Knows the height of every block in `heights` and holds no header.
+    pub fn new(heights: HashMap<BlockHash, u32>) -> Self {
+        MockHeaderStorage {
+            heights,
+            headers: HashMap::new(),
+        }
+    }
+
+    /// Holds a header for `block_hash`, whose height must be known, that
+    /// commits to `merkle_root`. The rest of the header is filler, and it is
+    /// stored under `block_hash` rather than its own hash.
+    pub fn with_header(mut self, block_hash: BlockHash, merkle_root: TxMerkleNode) -> Self {
+        let height = *self.heights.get(&block_hash).expect("the block's height is known");
+        let header = Header {
+            merkle_root,
+            ..Header::dummy(height)
+        };
+        self.headers.insert(height, HashedBlockHeader::with_trusted_hash(header, block_hash));
+        self
+    }
+
+    /// Holds the header of `diff`'s block that its coinbase proof leads to, as
+    /// [`MnListDiff::with_coinbase_committing_to`] sets it up.
+    pub fn with_header_for(self, diff: &MnListDiff) -> Self {
+        self.with_header(diff.block_hash, diff.dummy_block_merkle_root())
+    }
+}
 
 #[async_trait]
 impl BlockHeaderStorage for MockHeaderStorage {
@@ -27,6 +63,9 @@ impl BlockHeaderStorage for MockHeaderStorage {
     async fn load_headers(&self, _: Range<u32>) -> StorageResult<Vec<HashedBlockHeader>> {
         Ok(vec![])
     }
+    async fn get_header(&self, height: u32) -> StorageResult<Option<HashedBlockHeader>> {
+        Ok(self.headers.get(&height).cloned())
+    }
     async fn get_tip_height(&self) -> Option<u32> {
         None
     }
@@ -40,10 +79,11 @@ impl BlockHeaderStorage for MockHeaderStorage {
         0
     }
     async fn get_header_height_by_hash(&self, hash: &BlockHash) -> StorageResult<Option<u32>> {
-        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);
+        self.headers.retain(|h, _| *h <= target_height);
         Ok(())
     }
 }

dash/src/merkle_tree/block.rs

@@ -225,6 +225,23 @@ impl PartialMerkleTree {
         &self.hashes
     }
 
+    /// Builds a partial merkle tree from the parts it is serialized as: the block's total
+    /// transaction count, the hashes in depth-first order, and the flag bits packed eight per
+    /// byte, least significant bit first.
+    ///
+    /// Messages that embed a partial merkle tree field by field, such as `mnlistdiff`, use this to
+    /// check their proof with [`Self::extract_matches`]. Nothing is validated here.
+    pub fn from_parts(num_transactions: u32, hashes: Vec<TxMerkleNode>, flag_bytes: &[u8]) -> Self {
+        let bits = (0..flag_bytes.len() * 8)
+            .map(|p| (flag_bytes[p / 8] & (1 << (p % 8) as u8)) != 0)
+            .collect();
+        PartialMerkleTree {
+            num_transactions,
+            hashes,
+            bits,
+        }
+    }
+
     /// Construct a partial merkle tree
     /// The `txids` are the transaction hashes of the block and the `matches` is the contains flags
     /// wherever a tx hash should be included in the proof.
@@ -457,18 +474,8 @@ impl Decodable for PartialMerkleTree {
     fn consensus_decode<R: io::Read + ?Sized>(r: &mut R) -> Result<Self, encode::Error> {
         let num_transactions: u32 = Decodable::consensus_decode(r)?;
         let hashes: Vec<TxMerkleNode> = Decodable::consensus_decode(r)?;
-
         let bytes: Vec<u8> = Decodable::consensus_decode(r)?;
-        let mut bits: Vec<bool> = 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))
     }
 }