fix: add sanity checks to prepare_new_block

applications/tari_base_node/src/grpc/base_node_grpc_server.rs

@@ -462,6 +462,9 @@ impl tari_rpc::base_node_server::BaseNode for BaseNodeGrpcServer {
 
         let new_block = match handler.get_new_block(block_template).await {
             Ok(b) => b,
+            Err(CommsInterfaceError::ChainStorageError(ChainStorageError::InvalidArguments { message, .. })) => {
+                return Err(Status::invalid_argument(message));
+            },
             Err(CommsInterfaceError::ChainStorageError(ChainStorageError::CannotCalculateNonTipMmr(msg))) => {
                 let status = Status::with_details(
                     tonic::Code::FailedPrecondition,

applications/tari_mining_node/src/config.rs

@@ -108,7 +108,7 @@ impl MinerConfig {
         Duration::from_secs(10)
     }
 
-    pub fn validate_tip_timeout_sec(&self) -> Duration {
+    pub fn validate_tip_interval(&self) -> Duration {
         Duration::from_secs(self.validate_tip_timeout_sec)
     }
 }

applications/tari_mining_node/src/main.rs

@@ -297,7 +297,7 @@ async fn mining_cycle(
         } else {
             display_report(&report, config).await;
         }
-        if config.mine_on_tip_only && reporting_timeout.elapsed() > config.validate_tip_timeout_sec() {
+        if config.mine_on_tip_only && reporting_timeout.elapsed() > config.validate_tip_interval() {
             validate_tip(node_conn, report.height, bootstrap.mine_until_height).await?;
             reporting_timeout = Instant::now();
         }

base_layer/core/src/chain_storage/blockchain_database.rs

@@ -664,21 +664,67 @@ where B: BlockchainBackend
 
     pub fn prepare_new_block(&self, template: NewBlockTemplate) -> Result<Block, ChainStorageError> {
         let NewBlockTemplate { header, mut body, .. } = template;
+        if header.height == 0 {
+            return Err(ChainStorageError::InvalidArguments {
+                func: "prepare_new_block",
+                arg: "template",
+                message: "Invalid height for NewBlockTemplate: must be greater than 0".to_string(),
+            });
+        }
+
         body.sort(header.version);
         let mut header = BlockHeader::from(header);
         // If someone advanced the median timestamp such that the local time is less than the median timestamp, we need
         // to increase the timestamp to be greater than the median timestamp
-        let height = header.height - 1;
+        let prev_block_height = header.height - 1;
         let min_height = header.height.saturating_sub(
             self.consensus_manager
                 .consensus_constants(header.height)
                 .get_median_timestamp_count() as u64,
         );
+
         let db = self.db_read_access()?;
-        let timestamps = fetch_headers(&*db, min_height, height)?
+        let tip_header = db.fetch_tip_header()?;
+        if header.height != tip_header.height() + 1 {
+            return Err(ChainStorageError::InvalidArguments {
+                func: "prepare_new_block",
+                arg: "template",
+                message: format!(
+                    "Expected new block template height to be {} but was {}",
+                    tip_header.height() + 1,
+                    header.height
+                ),
+            });
+        }
+        if header.prev_hash != *tip_header.hash() {
+            return Err(ChainStorageError::InvalidArguments {
+                func: "prepare_new_block",
+                arg: "template",
+                message: format!(
+                    "Expected new block template previous hash to be set to the current tip hash ({}) but was {}",
+                    tip_header.hash().to_hex(),
+                    header.prev_hash.to_hex()
+                ),
+            });
+        }
+
+        let timestamps = fetch_headers(&*db, min_height, prev_block_height)?
             .iter()
             .map(|h| h.timestamp)
             .collect::<Vec<_>>();
+        if timestamps.is_empty() {
+            return Err(ChainStorageError::DataInconsistencyDetected {
+                function: "prepare_new_block",
+                details: format!(
+                    "No timestamps were returned within heights {} - {} by the database despite the tip header height \
+                     being {}",
+                    min_height,
+                    prev_block_height,
+                    tip_header.height()
+                ),
+            });
+        }
+
         let median_timestamp = calc_median_timestamp(&timestamps);
         if median_timestamp > header.timestamp {
             header.timestamp = median_timestamp.increase(1);

base_layer/core/src/chain_storage/tests/blockchain_database.rs

@@ -21,16 +21,20 @@
 //  USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 use crate::{
-    blocks::Block,
-    chain_storage::BlockchainDatabase,
+    blocks::{Block, BlockHeader, NewBlockTemplate},
+    chain_storage::{BlockchainDatabase, ChainStorageError},
     consensus::ConsensusManager,
+    proof_of_work::Difficulty,
     tari_utilities::Hashable,
     test_helpers::{
         blockchain::{create_new_blockchain, TempDatabase},
         create_block,
         BlockSpec,
     },
-    transactions::transaction::{Transaction, UnblindedOutput},
+    transactions::{
+        tari_amount::T,
+        transaction::{Transaction, UnblindedOutput},
+    },
 };
 use std::sync::Arc;
 use tari_common::configuration::Network;
@@ -160,6 +164,12 @@ mod fetch_headers {
         let db = setup();
         let headers = db.fetch_headers(0..).unwrap();
         assert_eq!(headers.len(), 1);
+        let headers = db.fetch_headers(0..0).unwrap();
+        assert_eq!(headers.len(), 1);
+        let headers = db.fetch_headers(0..=0).unwrap();
+        assert_eq!(headers.len(), 1);
+        let headers = db.fetch_headers(..).unwrap();
+        assert_eq!(headers.len(), 1);
     }
 
     #[test]
@@ -432,3 +442,41 @@ mod fetch_total_size_stats {
         assert_eq!(stats.sizes().len(), 20);
     }
 }
+
+mod prepare_new_block {
+    use super::*;
+
+    #[test]
+    fn it_errors_for_genesis_block() {
+        let db = setup();
+        let genesis = db.fetch_block(0).unwrap();
+        let template = NewBlockTemplate::from_block(genesis.block().clone(), Difficulty::min(), 5000 * T);
+        let err = db.prepare_new_block(template).unwrap_err();
+        assert!(matches!(err, ChainStorageError::InvalidArguments { .. }));
+    }
+
+    #[test]
+    fn it_errors_for_non_tip_template() {
+        let db = setup();
+        let genesis = db.fetch_block(0).unwrap();
+        let next_block = BlockHeader::from_previous(genesis.header());
+        let mut template = NewBlockTemplate::from_block(next_block.into_builder().build(), Difficulty::min(), 5000 * T);
+        // This would cause a panic if the sanity checks were not there
+        template.header.height = 100;
+        let err = db.prepare_new_block(template.clone()).unwrap_err();
+        assert!(matches!(err, ChainStorageError::InvalidArguments { .. }));
+        template.header.height = 1;
+        template.header.prev_hash[0] += 1;
+        let err = db.prepare_new_block(template).unwrap_err();
+        assert!(matches!(err, ChainStorageError::InvalidArguments { .. }));
+    }
+    #[test]
+    fn it_prepares_the_first_block() {
+        let db = setup();
+        let genesis = db.fetch_block(0).unwrap();
+        let next_block = BlockHeader::from_previous(genesis.header());
+        let template = NewBlockTemplate::from_block(next_block.into_builder().build(), Difficulty::min(), 5000 * T);
+        let block = db.prepare_new_block(template).unwrap();
+        assert_eq!(block.header.height, 1);
+    }
+}

base_layer/core/src/validation/helpers.rs

@@ -75,6 +75,10 @@ pub fn check_timestamp_ftl(
 }
 
 /// Returns the median timestamp for the provided timestamps.
+///
+/// ## Panics
+/// When an empty slice is given as this is undefined for median average.
+/// https://math.stackexchange.com/a/3451015
 pub fn calc_median_timestamp(timestamps: &[EpochTime]) -> EpochTime {
     assert!(
         !timestamps.is_empty(),

integration_tests/features/WalletMonitoring.feature

@@ -135,8 +135,8 @@ Feature: Wallet Monitoring
     And I have mining node MINER2 connected to base node NODE2 and wallet WALLET2
 
     When I co-mine <numBlocks> blocks via merge mining proxy PROXY1 and mining node MINER2
-    # This wait is here to give a chance for re-orgs to settle out
-    Then I wait 30 seconds
+#     This wait is here to give a chance for re-orgs to settle out
+    And I wait 30 seconds
     Then all nodes are on the same chain at height <numBlocks>
 
     And mining node MINER_SEED_A mines 5 blocks

integration_tests/features/support/steps.js

@@ -903,7 +903,11 @@ Then(
     await this.forEachClientAsync(async (client, name) => {
       await waitFor(async () => client.getTipHeight(), height, 115 * 1000);
       const currTip = await client.getTipHeader();
-      console.log("the node is at tip ", currTip);
+      console.log(
+        `${client.name} is at tip ${currTip.height} (${currTip.hash.toString(
+          "hex"
+        )})`
+      );
       expect(currTip.height).to.equal(height);
       if (!tipHash) {
         tipHash = currTip.hash.toString("hex");
@@ -929,7 +933,6 @@ Then(
         let height = null;
         let result = true;
         await this.forEachClientAsync(async (client, name) => {
-          await waitFor(async () => client.getTipHeight(), 115 * 1000);
           const currTip = await client.getTipHeader();
           if (!tipHash) {
             tipHash = currTip.hash.toString("hex");

integration_tests/helpers/config.js

@@ -24,7 +24,7 @@ function mapEnvs(options) {
     res.TARI_WALLET__TRANSACTION_BROADCAST_MONITORING_TIMEOUT = 3;
   }
   if ("mineOnTipOnly" in options) {
-    res.TARI_MINING_NODE__MINE_ON_TIP_ONLY = options.mineOnTipOnly;
+    res.TARI_MINING_NODE__MINE_ON_TIP_ONLY = options.mineOnTipOnly.toString();
   }
   if (options.numMiningThreads) {
     res.TARI_MINING_NODE__NUM_MINING_THREADS = options.numMiningThreads;

integration_tests/helpers/mergeMiningProxyClient.js

@@ -38,6 +38,7 @@ class MergeMiningProxyClient {
       id: "0",
       method: "submit_block",
       params: [block],
+      timeout: 60,
     });
     return res.data;
   }
@@ -96,7 +97,9 @@ class MergeMiningProxyClient {
       }
       await this.submitBlock(block);
     } while (tipHeight + 1 < height);
-    return await this.baseNodeClient.getTipHeight();
+    tipHeight = await this.baseNodeClient.getTipHeight();
+    console.log("[mmProxy client] Tip is at target height", tipHeight);
+    return tipHeight;
   }
 }
 

integration_tests/helpers/miningNodeProcess.js

@@ -175,7 +175,9 @@ class MiningNodeProcess {
     await this.init(numBlocks, height, minDifficulty, 9999999999, true, 1);
     await this.startNew();
     await this.stop();
-    return await this.baseNodeClient.getTipHeight();
+    const tipHeight = await this.baseNodeClient.getTipHeight();
+    console.log(`[${this.name}] Tip at ${tipHeight}`);
+    return tipHeight;
   }
 }