Refactor block struct (#62)

* Refactor block struct

consensus/core/src/block.rs

@@ -1,33 +1,45 @@
 use std::sync::Arc;
 
-use crate::{header::Header, tx::Transaction, BlueWorkType};
+use crate::{header::Header, tx::Transaction};
 use hashes::Hash;
 
+/// A mutable block structure where header and transactions within can still be mutated.
 #[derive(Debug, Clone)]
-pub struct Block {
+pub struct MutableBlock {
     pub header: Header,
+    pub transactions: Vec<Transaction>,
+}
+
+impl MutableBlock {
+    pub fn new(header: Header, txs: Vec<Transaction>) -> Self {
+        Self { header, transactions: txs }
+    }
+
+    pub fn from_header(header: Header) -> Self {
+        Self::new(header, vec![])
+    }
+
+    pub fn to_immutable(self) -> Block {
+        Block::new(self.header, self.transactions)
+    }
+}
+
+/// A block structure where the inner header and transactions are wrapped by Arcs for
+/// cheap cloning and for cross-thread safety and immutability. Note: no need to wrap
+/// this struct with an additional Arc.
+#[derive(Debug, Clone)]
+pub struct Block {
+    pub header: Arc<Header>,
     pub transactions: Arc<Vec<Transaction>>,
 }
 
 impl Block {
-    pub fn new(
-        version: u16,
-        parents: Vec<Hash>,
-        timestamp: u64,
-        bits: u32,
-        nonce: u64,
-        daa_score: u64,
-        blue_work: BlueWorkType,
-        blue_score: u64,
-    ) -> Self {
-        Self {
-            header: Header::new(version, parents, Default::default(), timestamp, bits, nonce, daa_score, blue_work, blue_score),
-            transactions: Arc::new(Vec::new()),
-        }
+    pub fn new(header: Header, txs: Vec<Transaction>) -> Self {
+        Self { header: Arc::new(header), transactions: Arc::new(txs) }
     }
 
     pub fn from_header(header: Header) -> Self {
-        Self { header, transactions: Arc::new(Vec::new()) }
+        Self { header: Arc::new(header), transactions: Arc::new(Vec::new()) }
     }
 
     pub fn is_header_only(&self) -> bool {

consensus/core/src/header.rs

@@ -50,6 +50,7 @@ impl Header {
         header
     }
 
+    /// Finalizes the header and recomputes the header hash
     pub fn finalize(&mut self) {
         self.hash = hashing::header::hash(self);
     }

consensus/src/consensus/mod.rs

@@ -334,7 +334,7 @@ impl Consensus {
         ]
     }
 
-    pub fn validate_and_insert_block(&self, block: Arc<Block>) -> impl Future<Output = BlockProcessResult<BlockStatus>> {
+    pub fn validate_and_insert_block(&self, block: Block) -> impl Future<Output = BlockProcessResult<BlockStatus>> {
         let (tx, rx): (BlockResultSender, _) = oneshot::channel();
         self.block_sender.send(BlockTask::Process(block, vec![tx])).unwrap();
         async { rx.await.unwrap() }

consensus/src/consensus/test_consensus.rs

@@ -5,7 +5,12 @@ use std::{
 };
 
 use consensus_core::{
-    block::Block, header::Header, merkle::calc_hash_merkle_root, subnets::SUBNETWORK_ID_COINBASE, tx::Transaction, BlockHashSet,
+    block::{Block, MutableBlock},
+    header::Header,
+    merkle::calc_hash_merkle_root,
+    subnets::SUBNETWORK_ID_COINBASE,
+    tx::Transaction,
+    BlockHashSet,
 };
 use hashes::Hash;
 use kaspa_core::{core::Core, service::Service};
@@ -71,10 +76,15 @@ impl TestConsensus {
     }
 
     pub fn add_block_with_parents(&self, hash: Hash, parents: Vec<Hash>) -> impl Future<Output = BlockProcessResult<BlockStatus>> {
-        self.validate_and_insert_block(Arc::new(self.build_block_with_parents(hash, parents)))
+        self.validate_and_insert_block(self.build_block_with_parents(hash, parents).to_immutable())
     }
 
-    pub fn build_block_with_parents_and_transactions(&self, hash: Hash, parents: Vec<Hash>, txs: Vec<Transaction>) -> Block {
+    pub fn build_block_with_parents_and_transactions(
+        &self,
+        hash: Hash,
+        parents: Vec<Hash>,
+        mut txs: Vec<Transaction>,
+    ) -> MutableBlock {
         let mut header = self.build_header_with_parents(hash, parents);
         let cb_payload: Vec<u8> = header.blue_score.to_le_bytes().iter().copied() // Blue score
             .chain(self.consensus.coinbase_manager.calc_block_subsidy(header.daa_score).to_le_bytes().iter().copied()) // Subsidy
@@ -83,16 +93,16 @@ impl TestConsensus {
             .collect();
 
         let cb = Transaction::new(TX_VERSION, vec![], vec![], 0, SUBNETWORK_ID_COINBASE, 0, cb_payload, 0);
-        let final_txs = vec![vec![cb], txs].concat();
-        header.hash_merkle_root = calc_hash_merkle_root(final_txs.iter());
-        Block { header, transactions: Arc::new(final_txs) }
+        txs.insert(0, cb);
+        header.hash_merkle_root = calc_hash_merkle_root(txs.iter());
+        MutableBlock::new(header, txs)
     }
 
-    pub fn build_block_with_parents(&self, hash: Hash, parents: Vec<Hash>) -> Block {
-        Block::from_header(self.build_header_with_parents(hash, parents))
+    pub fn build_block_with_parents(&self, hash: Hash, parents: Vec<Hash>) -> MutableBlock {
+        MutableBlock::from_header(self.build_header_with_parents(hash, parents))
     }
 
-    pub fn validate_and_insert_block(&self, block: Arc<Block>) -> impl Future<Output = BlockProcessResult<BlockStatus>> {
+    pub fn validate_and_insert_block(&self, block: Block) -> impl Future<Output = BlockProcessResult<BlockStatus>> {
         self.consensus.validate_and_insert_block(block)
     }
 

consensus/src/pipeline/body_processor/body_validation_in_context.rs

@@ -107,59 +107,57 @@ mod tests {
         {
             let block = consensus.build_block_with_parents_and_transactions(2.into(), vec![1.into()], vec![]);
             // We expect a missing parents error since the parent is header only.
-            assert_match!(body_processor.validate_body_in_context(&block), Err(RuleError::MissingParents(_)));
+            assert_match!(body_processor.validate_body_in_context(&block.to_immutable()), Err(RuleError::MissingParents(_)));
         }
 
         let valid_block = consensus.build_block_with_parents_and_transactions(3.into(), vec![params.genesis_hash], vec![]);
-        consensus.validate_and_insert_block(Arc::new(valid_block)).await.unwrap();
+        consensus.validate_and_insert_block(valid_block.to_immutable()).await.unwrap();
         {
             let mut block = consensus.build_block_with_parents_and_transactions(2.into(), vec![3.into()], vec![]);
-            Arc::make_mut(&mut block.transactions)[0].payload[8..16].copy_from_slice(&(5_u64).to_le_bytes());
+            block.transactions[0].payload[8..16].copy_from_slice(&(5_u64).to_le_bytes());
             block.header.hash_merkle_root = calc_hash_merkle_root(block.transactions.iter());
 
-            let block = Arc::new(block);
             assert_match!(
-                consensus.validate_and_insert_block(block.clone()).await, Err(RuleError::WrongSubsidy(expected,_)) if expected == 50000000000);
+                consensus.validate_and_insert_block(block.clone().to_immutable()).await, Err(RuleError::WrongSubsidy(expected,_)) if expected == 50000000000);
 
             // The second time we send an invalid block we expect it to be a known invalid.
-            assert_match!(consensus.validate_and_insert_block(block).await, Err(RuleError::KnownInvalid));
+            assert_match!(consensus.validate_and_insert_block(block.to_immutable()).await, Err(RuleError::KnownInvalid));
         }
 
         {
             let mut block = consensus.build_block_with_parents_and_transactions(4.into(), vec![3.into()], vec![]);
-            Arc::make_mut(&mut block.transactions)[0].payload[0..8].copy_from_slice(&(100_u64).to_le_bytes());
+            block.transactions[0].payload[0..8].copy_from_slice(&(100_u64).to_le_bytes());
             block.header.hash_merkle_root = calc_hash_merkle_root(block.transactions.iter());
 
-            let block = Arc::new(block);
-            assert_match!(consensus.validate_and_insert_block(block.clone()).await, Err(RuleError::BadCoinbasePayloadBlueScore(_, _)));
+            assert_match!(
+                consensus.validate_and_insert_block(block.to_immutable()).await,
+                Err(RuleError::BadCoinbasePayloadBlueScore(_, _))
+            );
         }
 
         {
             let mut block = consensus.build_block_with_parents_and_transactions(5.into(), vec![3.into()], vec![]);
-            Arc::make_mut(&mut block.transactions)[0].payload = vec![];
+            block.transactions[0].payload = vec![];
             block.header.hash_merkle_root = calc_hash_merkle_root(block.transactions.iter());
 
-            let block = Arc::new(block);
-            assert_match!(consensus.validate_and_insert_block(block.clone()).await, Err(RuleError::BadCoinbasePayload(_)));
+            assert_match!(consensus.validate_and_insert_block(block.to_immutable()).await, Err(RuleError::BadCoinbasePayload(_)));
         }
 
-        let valid_block_child = Arc::new(consensus.build_block_with_parents_and_transactions(6.into(), vec![3.into()], vec![]));
-        consensus.validate_and_insert_block(valid_block_child.clone()).await.unwrap();
+        let valid_block_child = consensus.build_block_with_parents_and_transactions(6.into(), vec![3.into()], vec![]);
+        consensus.validate_and_insert_block(valid_block_child.clone().to_immutable()).await.unwrap();
         {
             // The block DAA score is 2, so the subsidy should be calculated according to the deflationary stage.
             let mut block = consensus.build_block_with_parents_and_transactions(7.into(), vec![6.into()], vec![]);
-            Arc::make_mut(&mut block.transactions)[0].payload[8..16].copy_from_slice(&(5_u64).to_le_bytes());
+            block.transactions[0].payload[8..16].copy_from_slice(&(5_u64).to_le_bytes());
             block.header.hash_merkle_root = calc_hash_merkle_root(block.transactions.iter());
-            assert_match!(consensus.validate_and_insert_block(Arc::new(block)).await, Err(RuleError::WrongSubsidy(expected,_)) if expected == 44000000000);
+            assert_match!(consensus.validate_and_insert_block(block.to_immutable()).await, Err(RuleError::WrongSubsidy(expected,_)) if expected == 44000000000);
         }
 
         {
             // Check that the same daa score as the block's daa score or higher fails, but lower passes.
             let tip_daa_score = valid_block_child.header.daa_score + 1;
             check_for_lock_time_and_sequence(&consensus, valid_block_child.header.hash, 8.into(), tip_daa_score + 1, 0, false).await;
-
             check_for_lock_time_and_sequence(&consensus, valid_block_child.header.hash, 9.into(), tip_daa_score, 0, false).await;
-
             check_for_lock_time_and_sequence(&consensus, valid_block_child.header.hash, 10.into(), tip_daa_score - 1, 0, true).await;
 
             let valid_block_child_gd = consensus.ghostdag_store().get_data(valid_block_child.header.hash).unwrap();
@@ -170,9 +168,7 @@ mod tests {
             let tip_daa_score = valid_block_child.header.daa_score + 1;
             check_for_lock_time_and_sequence(&consensus, valid_block_child.header.hash, 11.into(), past_median_time + 1, 0, false)
                 .await;
-
             check_for_lock_time_and_sequence(&consensus, valid_block_child.header.hash, 12.into(), past_median_time, 0, false).await;
-
             check_for_lock_time_and_sequence(&consensus, valid_block_child.header.hash, 13.into(), past_median_time - 1, 0, true)
                 .await;
 
@@ -219,10 +215,10 @@ mod tests {
         );
 
         if should_pass {
-            consensus.validate_and_insert_block(Arc::new(block)).await.unwrap();
+            consensus.validate_and_insert_block(block.to_immutable()).await.unwrap();
         } else {
             assert_match!(
-                consensus.validate_and_insert_block(Arc::new(block)).await, 
+                consensus.validate_and_insert_block(block.to_immutable()).await, 
                 Err(RuleError::TxInContextFailed(_, e)) if matches!(e, TxRuleError::NotFinalized(_)));
         }
     }

consensus/src/pipeline/body_processor/body_validation_in_isolation.rs

@@ -110,7 +110,7 @@ mod tests {
     use std::sync::Arc;
 
     use consensus_core::{
-        block::Block,
+        block::MutableBlock,
         header::Header,
         merkle::calc_hash_merkle_root,
         subnets::{SUBNETWORK_ID_COINBASE, SUBNETWORK_ID_NATIVE},
@@ -128,8 +128,8 @@ mod tests {
         let wait_handles = consensus.init();
 
         let body_processor = consensus.block_body_processor();
-        let example_block = Block {
-            header: Header::new(
+        let example_block = MutableBlock::new(
+            Header::new(
                 0,
                 vec![
                     Hash::from_slice(&[
@@ -152,7 +152,7 @@ mod tests {
                 0,
                 9,
             ),
-            transactions: Arc::new(vec![
+            vec![
                 Transaction::new(
                     0,
                     vec![],
@@ -376,57 +376,61 @@ mod tests {
                     vec![],
                     0,
                 ),
-            ]),
-        };
+            ],
+        );
 
-        body_processor.validate_body_in_isolation(&example_block).unwrap();
+        body_processor.validate_body_in_isolation(&example_block.clone().to_immutable()).unwrap();
 
         let mut block = example_block.clone();
-        Arc::make_mut(&mut block.transactions)[1].version += 1;
-        assert_match!(body_processor.validate_body_in_isolation(&block), Err(RuleError::BadMerkleRoot(_, _)));
+        let txs = &mut block.transactions;
+        txs[1].version += 1;
+        assert_match!(body_processor.validate_body_in_isolation(&block.to_immutable()), Err(RuleError::BadMerkleRoot(_, _)));
 
         let mut block = example_block.clone();
-        let txs = Arc::make_mut(&mut block.transactions);
+        let txs = &mut block.transactions;
         Arc::make_mut(&mut txs[1].inputs[0]).sig_op_count = 255;
         Arc::make_mut(&mut txs[1].inputs[1]).sig_op_count = 255;
         block.header.hash_merkle_root = calc_hash_merkle_root(txs.iter());
-        assert_match!(body_processor.validate_body_in_isolation(&block), Err(RuleError::ExceedsMassLimit(_)));
+        assert_match!(body_processor.validate_body_in_isolation(&block.to_immutable()), Err(RuleError::ExceedsMassLimit(_)));
 
         let mut block = example_block.clone();
-        let txs = Arc::make_mut(&mut block.transactions);
+        let txs = &mut block.transactions;
         txs.push(txs[1].clone());
         block.header.hash_merkle_root = calc_hash_merkle_root(txs.iter());
-        assert_match!(body_processor.validate_body_in_isolation(&block), Err(RuleError::DuplicateTransactions(_)));
+        assert_match!(body_processor.validate_body_in_isolation(&block.to_immutable()), Err(RuleError::DuplicateTransactions(_)));
 
         let mut block = example_block.clone();
-        let txs = Arc::make_mut(&mut block.transactions);
+        let txs = &mut block.transactions;
         txs[1].subnetwork_id = SUBNETWORK_ID_COINBASE;
         block.header.hash_merkle_root = calc_hash_merkle_root(txs.iter());
-        assert_match!(body_processor.validate_body_in_isolation(&block), Err(RuleError::MultipleCoinbases(_)));
+        assert_match!(body_processor.validate_body_in_isolation(&block.to_immutable()), Err(RuleError::MultipleCoinbases(_)));
 
         let mut block = example_block.clone();
-        let txs = Arc::make_mut(&mut block.transactions);
+        let txs = &mut block.transactions;
         Arc::make_mut(&mut txs[2].inputs[0]).previous_outpoint = txs[1].inputs[0].previous_outpoint;
         block.header.hash_merkle_root = calc_hash_merkle_root(txs.iter());
-        assert_match!(body_processor.validate_body_in_isolation(&block), Err(RuleError::DoubleSpendInSameBlock(_)));
+        assert_match!(body_processor.validate_body_in_isolation(&block.to_immutable()), Err(RuleError::DoubleSpendInSameBlock(_)));
 
         let mut block = example_block.clone();
-        let txs = Arc::make_mut(&mut block.transactions);
+        let txs = &mut block.transactions;
         txs[0].subnetwork_id = SUBNETWORK_ID_NATIVE;
         block.header.hash_merkle_root = calc_hash_merkle_root(txs.iter());
-        assert_match!(body_processor.validate_body_in_isolation(&block), Err(RuleError::FirstTxNotCoinbase));
+        assert_match!(body_processor.validate_body_in_isolation(&block.to_immutable()), Err(RuleError::FirstTxNotCoinbase));
 
         let mut block = example_block.clone();
-        let txs = Arc::make_mut(&mut block.transactions);
+        let txs = &mut block.transactions;
         txs[1].inputs = vec![];
         block.header.hash_merkle_root = calc_hash_merkle_root(txs.iter());
-        assert_match!(body_processor.validate_body_in_isolation(&block), Err(RuleError::TxInIsolationValidationFailed(_, _)));
+        assert_match!(
+            body_processor.validate_body_in_isolation(&block.to_immutable()),
+            Err(RuleError::TxInIsolationValidationFailed(_, _))
+        );
 
         let mut block = example_block;
-        let txs = Arc::make_mut(&mut block.transactions);
+        let txs = &mut block.transactions;
         Arc::make_mut(&mut txs[3].inputs[0]).previous_outpoint = TransactionOutpoint { transaction_id: txs[2].id(), index: 0 };
         block.header.hash_merkle_root = calc_hash_merkle_root(txs.iter());
-        assert_match!(body_processor.validate_body_in_isolation(&block), Err(RuleError::ChainedTransaction(_)));
+        assert_match!(body_processor.validate_body_in_isolation(&block.to_immutable()), Err(RuleError::ChainedTransaction(_)));
 
         consensus.shutdown(wait_handles);
     }
@@ -438,22 +442,20 @@ mod tests {
         let wait_handles = consensus.init();
 
         let mut block = consensus.build_block_with_parents_and_transactions(1.into(), vec![params.genesis_hash], vec![]);
-        Arc::make_mut(&mut block.transactions)[0].version += 1;
+        block.transactions[0].version += 1;
 
-        let block = Arc::new(block);
-        assert_match!(consensus.validate_and_insert_block(block.clone()).await, Err(RuleError::BadMerkleRoot(_, _)));
+        assert_match!(consensus.validate_and_insert_block(block.clone().to_immutable()).await, Err(RuleError::BadMerkleRoot(_, _)));
 
         // BadMerkleRoot shouldn't mark the block as known invalid
-        assert_match!(consensus.validate_and_insert_block(block).await, Err(RuleError::BadMerkleRoot(_, _)));
+        assert_match!(consensus.validate_and_insert_block(block.to_immutable()).await, Err(RuleError::BadMerkleRoot(_, _)));
 
         let mut block = consensus.build_block_with_parents_and_transactions(1.into(), vec![params.genesis_hash], vec![]);
         block.header.parents_by_level[0][0] = 0.into();
 
-        let block = Arc::new(block);
-        assert_match!(consensus.validate_and_insert_block(block.clone()).await, Err(RuleError::MissingParents(_)));
+        assert_match!(consensus.validate_and_insert_block(block.clone().to_immutable()).await, Err(RuleError::MissingParents(_)));
 
         // MissingParents shouldn't mark the block as known invalid
-        assert_match!(consensus.validate_and_insert_block(block).await, Err(RuleError::MissingParents(_)));
+        assert_match!(consensus.validate_and_insert_block(block.to_immutable()).await, Err(RuleError::MissingParents(_)));
 
         consensus.shutdown(wait_handles);
     }