From ee757df369c4ee56f1ed242020f790b3869ff955 Mon Sep 17 00:00:00 2001 From: SW van Heerden Date: Wed, 10 Aug 2022 16:37:00 +0200 Subject: [PATCH] fix: fix new block handling of non-tip blocks (#4431) Description --- Currently, the base node cannot process a block that was received out of order (aka orphaned). The `reconcile_block` function makes the assumption that the block is always on the current tip and tries to calculate the MMR roots of the block as a sanity check. The MMR roots can only be checked by a block that is built on the current local tip. This changes the `reconcile_block` function to first check orphan status, and if true, it will require the full block from the peer. This Pr also changes how the db lock is processed. Currently, it will print out a warn followed by an error. This suppresses the error and only prints the warn. How Has This Been Tested? --- Manual Fixes: https://github.com/tari-project/tari/issues/4352 --- applications/tari_console_wallet/src/main.rs | 2 +- .../comms_interface/inbound_handlers.rs | 41 +++++++++++++++++-- .../core/src/base_node/service/service.rs | 12 ++++-- .../src/chain_storage/blockchain_database.rs | 4 +- 4 files changed, 49 insertions(+), 10 deletions(-) diff --git a/applications/tari_console_wallet/src/main.rs b/applications/tari_console_wallet/src/main.rs index 983baf7526..a411271833 100644 --- a/applications/tari_console_wallet/src/main.rs +++ b/applications/tari_console_wallet/src/main.rs @@ -100,7 +100,7 @@ fn main_inner() -> Result<(), ExitError> { )?; #[cfg_attr(not(all(unix, feature = "libtor")), allow(unused_mut))] - let mut config = ApplicationConfig::load_from(&cfg)?; + let config = ApplicationConfig::load_from(&cfg)?; let runtime = tokio::runtime::Builder::new_multi_thread() .enable_all() diff --git a/base_layer/core/src/base_node/comms_interface/inbound_handlers.rs b/base_layer/core/src/base_node/comms_interface/inbound_handlers.rs index b49d262f87..d6375953ff 100644 --- a/base_layer/core/src/base_node/comms_interface/inbound_handlers.rs +++ b/base_layer/core/src/base_node/comms_interface/inbound_handlers.rs @@ -584,7 +584,36 @@ where B: BlockchainBackend + 'static coinbase_output, kernel_excess_sigs: excess_sigs, } = new_block; + // If the block is empty, we dont have to check ask for the block, as we already have the full block available + // to us. + if excess_sigs.is_empty() { + let block = BlockBuilder::new(header.version) + .with_coinbase_utxo(coinbase_output, coinbase_kernel) + .with_header(header) + .build(); + return Ok(Arc::new(block)); + } + let block_hash = header.hash(); + // We check the current tip and orphan status of the block because we cannot guarantee that mempool state is + // correct and the mmr root calculation is only valid if the block is building on the tip. + let current_meta = self.blockchain_db.get_chain_metadata().await?; + if header.prev_hash != *current_meta.best_block() { + debug!( + target: LOG_TARGET, + "Orphaned block #{}: ({}), current tip is: #{} ({}). We need to fetch the complete block from peer: \ + ({})", + header.height, + block_hash.to_hex(), + current_meta.height_of_longest_chain(), + current_meta.best_block().to_hex(), + source_peer, + ); + let block = self.request_full_block_from_peer(source_peer, block_hash).await?; + return Ok(block); + } + + // We know that the block is neither and orphan or a coinbase, so lets ask our mempool for the transactions let (known_transactions, missing_excess_sigs) = self.mempool.retrieve_by_excess_sigs(excess_sigs).await?; let known_transactions = known_transactions.into_iter().map(|tx| (*tx).clone()).collect(); @@ -599,7 +628,7 @@ where B: BlockchainBackend + 'static target: LOG_TARGET, "All transactions for block #{} ({}) found in mempool", header.height, - header.hash().to_hex() + block_hash.to_hex() ); } else { debug!( @@ -623,7 +652,6 @@ where B: BlockchainBackend + 'static } if !not_found.is_empty() { - let block_hash = header.hash(); warn!( target: LOG_TARGET, "Peer {} was not able to return all transactions for block #{} ({}). {} transaction(s) not found. \ @@ -655,9 +683,14 @@ where B: BlockchainBackend + 'static // Perform a sanity check on the reconstructed block, if the MMR roots don't match then it's possible one or // more transactions in our mempool had the same excess/signature for a *different* transaction. // This is extremely unlikely, but still possible. In case of a mismatch, request the full block from the peer. - let (block, mmr_roots) = self.blockchain_db.calculate_mmr_roots(block).await?; + let (block, mmr_roots) = match self.blockchain_db.calculate_mmr_roots(block).await { + Err(_) => { + let block = self.request_full_block_from_peer(source_peer, block_hash).await?; + return Ok(block); + }, + Ok(v) => v, + }; if let Err(e) = helpers::check_mmr_roots(&header, &mmr_roots) { - let block_hash = block.hash(); warn!( target: LOG_TARGET, "Reconstructed block #{} ({}) failed MMR check validation!. Requesting full block. Error: {}", diff --git a/base_layer/core/src/base_node/service/service.rs b/base_layer/core/src/base_node/service/service.rs index c3e29faf38..fb8076a348 100644 --- a/base_layer/core/src/base_node/service/service.rs +++ b/base_layer/core/src/base_node/service/service.rs @@ -55,7 +55,7 @@ use crate::{ StateMachineHandle, }, blocks::{Block, NewBlock}, - chain_storage::BlockchainBackend, + chain_storage::{BlockchainBackend, ChainStorageError}, proto as shared_protos, proto::base_node as proto, }; @@ -297,8 +297,14 @@ where B: BlockchainBackend + 'static task::spawn(async move { let result = handle_incoming_block(inbound_nch, new_block).await; - if let Err(e) = result { - error!(target: LOG_TARGET, "Failed to handle incoming block message: {:?}", e); + match result { + Err(BaseNodeServiceError::CommsInterfaceError(CommsInterfaceError::ChainStorageError( + ChainStorageError::AddBlockOperationLocked, + ))) => { + // Special case, dont log this again as an error + }, + Err(e) => error!(target: LOG_TARGET, "Failed to handle incoming block message: {:?}", e), + _ => {}, } }); } diff --git a/base_layer/core/src/chain_storage/blockchain_database.rs b/base_layer/core/src/chain_storage/blockchain_database.rs index ed43b23e80..75a9d9f50d 100644 --- a/base_layer/core/src/chain_storage/blockchain_database.rs +++ b/base_layer/core/src/chain_storage/blockchain_database.rs @@ -892,7 +892,7 @@ where B: BlockchainBackend if self.is_add_block_disabled() { warn!( target: LOG_TARGET, - "add_block is disabled. Ignoring candidate block #{} ({})", + "add_block is disabled, node busy syncing. Ignoring candidate block #{} ({})", block.header.height, block.hash().to_hex() ); @@ -1230,7 +1230,7 @@ pub fn calculate_mmr_roots(db: &T, block: &Block) -> Resul let metadata = db.fetch_chain_metadata()?; if header.prev_hash != *metadata.best_block() { return Err(ChainStorageError::CannotCalculateNonTipMmr(format!( - "Block (#{}) previous hash is {} but the current tip is #{} {}", + "Block (#{}) is not building on tip, previous hash is {} but the current tip is #{} {}", header.height, header.prev_hash.to_hex(), metadata.height_of_longest_chain(),