From b22c583dfdb4dc23b7d750238dd55ff4cd2ff63b Mon Sep 17 00:00:00 2001 From: Stanimal Date: Tue, 12 Oct 2021 16:04:01 +0400 Subject: [PATCH] fix: add sanity checks to prepare_new_block --- .../src/grpc/base_node_grpc_server.rs | 3 ++ applications/tari_mining_node/src/config.rs | 2 +- applications/tari_mining_node/src/main.rs | 2 +- .../src/chain_storage/blockchain_database.rs | 50 ++++++++++++++++- .../tests/blockchain_database.rs | 54 +++++++++++++++++-- base_layer/core/src/validation/helpers.rs | 4 ++ .../features/WalletMonitoring.feature | 4 +- integration_tests/features/support/steps.js | 7 ++- integration_tests/helpers/config.js | 2 +- .../helpers/mergeMiningProxyClient.js | 5 +- .../helpers/miningNodeProcess.js | 4 +- 11 files changed, 123 insertions(+), 14 deletions(-) diff --git a/applications/tari_base_node/src/grpc/base_node_grpc_server.rs b/applications/tari_base_node/src/grpc/base_node_grpc_server.rs index 3a24c91249..fcf4cfb691 100644 --- a/applications/tari_base_node/src/grpc/base_node_grpc_server.rs +++ b/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, diff --git a/applications/tari_mining_node/src/config.rs b/applications/tari_mining_node/src/config.rs index c65446d051..6b46e8e195 100644 --- a/applications/tari_mining_node/src/config.rs +++ b/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) } } diff --git a/applications/tari_mining_node/src/main.rs b/applications/tari_mining_node/src/main.rs index 2de68f4ccb..bb36f4bf5c 100644 --- a/applications/tari_mining_node/src/main.rs +++ b/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(); } diff --git a/base_layer/core/src/chain_storage/blockchain_database.rs b/base_layer/core/src/chain_storage/blockchain_database.rs index 9fdaad9851..6053ccef95 100644 --- a/base_layer/core/src/chain_storage/blockchain_database.rs +++ b/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 { 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::>(); + 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(×tamps); if median_timestamp > header.timestamp { header.timestamp = median_timestamp.increase(1); diff --git a/base_layer/core/src/chain_storage/tests/blockchain_database.rs b/base_layer/core/src/chain_storage/tests/blockchain_database.rs index f0eabd8a48..f5cb1b9802 100644 --- a/base_layer/core/src/chain_storage/tests/blockchain_database.rs +++ b/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); + } +} diff --git a/base_layer/core/src/validation/helpers.rs b/base_layer/core/src/validation/helpers.rs index b76c065a45..ef6bb8e0db 100644 --- a/base_layer/core/src/validation/helpers.rs +++ b/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(), diff --git a/integration_tests/features/WalletMonitoring.feature b/integration_tests/features/WalletMonitoring.feature index 845690916f..d7a1156048 100644 --- a/integration_tests/features/WalletMonitoring.feature +++ b/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 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 And mining node MINER_SEED_A mines 5 blocks diff --git a/integration_tests/features/support/steps.js b/integration_tests/features/support/steps.js index 56a0efb4db..bcdbcff24e 100644 --- a/integration_tests/features/support/steps.js +++ b/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"); diff --git a/integration_tests/helpers/config.js b/integration_tests/helpers/config.js index 200fac6fbb..812eb11b6b 100644 --- a/integration_tests/helpers/config.js +++ b/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; diff --git a/integration_tests/helpers/mergeMiningProxyClient.js b/integration_tests/helpers/mergeMiningProxyClient.js index b990d9e4e5..4477b73532 100644 --- a/integration_tests/helpers/mergeMiningProxyClient.js +++ b/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; } } diff --git a/integration_tests/helpers/miningNodeProcess.js b/integration_tests/helpers/miningNodeProcess.js index 002a72c938..7a09c98767 100644 --- a/integration_tests/helpers/miningNodeProcess.js +++ b/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; } }