From 39ddd337cc870932328250417755f2fa6a8201c5 Mon Sep 17 00:00:00 2001 From: Stanimal Date: Thu, 5 Aug 2021 14:30:48 +0400 Subject: [PATCH] fix: ban peer when merkle roots mismatch --- .../states/block_sync.rs | 1 + .../src/base_node/sync/block_sync/error.rs | 8 ++-- .../base_node/sync/block_sync/synchronizer.rs | 40 ++++++++++++++++--- base_layer/core/src/base_node/sync/config.rs | 6 --- .../sync/header_sync/synchronizer.rs | 12 +++--- 5 files changed, 45 insertions(+), 22 deletions(-) diff --git a/base_layer/core/src/base_node/state_machine_service/states/block_sync.rs b/base_layer/core/src/base_node/state_machine_service/states/block_sync.rs index 89960740c5..fb73b70fc2 100644 --- a/base_layer/core/src/base_node/state_machine_service/states/block_sync.rs +++ b/base_layer/core/src/base_node/state_machine_service/states/block_sync.rs @@ -58,6 +58,7 @@ impl BlockSync { shared: &mut BaseNodeStateMachine, ) -> StateEvent { let mut synchronizer = BlockSynchronizer::new( + shared.config.block_sync_config.clone(), shared.db.clone(), shared.connectivity.clone(), self.sync_peer.take(), diff --git a/base_layer/core/src/base_node/sync/block_sync/error.rs b/base_layer/core/src/base_node/sync/block_sync/error.rs index 297d700cf4..544a6644c3 100644 --- a/base_layer/core/src/base_node/sync/block_sync/error.rs +++ b/base_layer/core/src/base_node/sync/block_sync/error.rs @@ -20,7 +20,7 @@ // WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -use crate::{chain_storage::ChainStorageError, proof_of_work::PowError, validation::ValidationError}; +use crate::{chain_storage::ChainStorageError, validation::ValidationError}; use tari_comms::{ connectivity::ConnectivityError, protocol::rpc::{RpcError, RpcStatus}, @@ -42,10 +42,8 @@ pub enum BlockSyncError { ConnectivityError(#[from] ConnectivityError), #[error("No sync peers available")] NoSyncPeers, - #[error("Error fetching PoW: {0}")] - PowError(#[from] PowError), - //#[error("Expected to find header at height {0} however the header did not exist")] - // ExpectedHeaderNotFound(u64), #[error("Block validation failed: {0}")] ValidationError(#[from] ValidationError), + #[error("Failed to ban peer: {0}")] + FailedToBan(ConnectivityError), } diff --git a/base_layer/core/src/base_node/sync/block_sync/synchronizer.rs b/base_layer/core/src/base_node/sync/block_sync/synchronizer.rs index 9becd727fa..c2898962c8 100644 --- a/base_layer/core/src/base_node/sync/block_sync/synchronizer.rs +++ b/base_layer/core/src/base_node/sync/block_sync/synchronizer.rs @@ -22,7 +22,10 @@ use super::error::BlockSyncError; use crate::{ - base_node::sync::{hooks::Hooks, rpc}, + base_node::{ + sync::{hooks::Hooks, rpc}, + BlockSyncConfig, + }, chain_storage::{async_db::AsyncBlockchainDb, BlockchainBackend, ChainBlock}, proto::base_node::SyncBlocksRequest, tari_utilities::{hex::Hex, Hashable}, @@ -47,6 +50,7 @@ use tokio::task; const LOG_TARGET: &str = "c::bn::block_sync"; pub struct BlockSynchronizer { + config: BlockSyncConfig, db: AsyncBlockchainDb, connectivity: ConnectivityRequester, sync_peer: Option, @@ -56,12 +60,14 @@ pub struct BlockSynchronizer { impl BlockSynchronizer { pub fn new( + config: BlockSyncConfig, db: AsyncBlockchainDb, connectivity: ConnectivityRequester, sync_peer: Option, block_validator: Arc>, ) -> Self { Self { + config, db, connectivity, sync_peer, @@ -87,10 +93,17 @@ impl BlockSynchronizer { target: LOG_TARGET, "Attempting to synchronize blocks with `{}`", node_id ); - self.attempt_block_sync(peer_conn).await?; - - self.db.cleanup_orphans().await?; - Ok(()) + match self.attempt_block_sync(peer_conn).await { + Ok(_) => { + self.db.cleanup_orphans().await?; + Ok(()) + }, + Err(err @ BlockSyncError::ValidationError(_)) | Err(err @ BlockSyncError::ReceivedInvalidBlockBody(_)) => { + self.ban_peer(node_id, &err).await?; + Err(err) + }, + Err(err) => Err(err), + } } async fn get_next_sync_peer(&mut self) -> Result { @@ -267,4 +280,21 @@ impl BlockSynchronizer { .await .expect("block validator panicked") } + + async fn ban_peer(&mut self, node_id: NodeId, reason: T) -> Result<(), BlockSyncError> { + let reason = reason.to_string(); + if self.config.sync_peers.contains(&node_id) { + debug!( + target: LOG_TARGET, + "Not banning peer that is allowlisted for sync. Ban reason = {}", reason + ); + return Ok(()); + } + warn!(target: LOG_TARGET, "Banned sync peer because {}", reason); + self.connectivity + .ban_peer_until(node_id, self.config.ban_period, reason) + .await + .map_err(BlockSyncError::FailedToBan)?; + Ok(()) + } } diff --git a/base_layer/core/src/base_node/sync/config.rs b/base_layer/core/src/base_node/sync/config.rs index 2a4a379bc2..89965bb806 100644 --- a/base_layer/core/src/base_node/sync/config.rs +++ b/base_layer/core/src/base_node/sync/config.rs @@ -25,9 +25,6 @@ use tari_comms::peer_manager::NodeId; #[derive(Debug, Clone)] pub struct BlockSyncConfig { - pub max_sync_peers: usize, - pub num_tip_hashes: usize, - pub num_proof_headers: usize, pub ban_period: Duration, pub short_ban_period: Duration, pub sync_peers: Vec, @@ -36,9 +33,6 @@ pub struct BlockSyncConfig { impl Default for BlockSyncConfig { fn default() -> Self { Self { - max_sync_peers: 10, - num_tip_hashes: 500, - num_proof_headers: 100, ban_period: Duration::from_secs(30 * 60), short_ban_period: Duration::from_secs(60), sync_peers: Default::default(), diff --git a/base_layer/core/src/base_node/sync/header_sync/synchronizer.rs b/base_layer/core/src/base_node/sync/header_sync/synchronizer.rs index b5c9291f18..426e564a42 100644 --- a/base_layer/core/src/base_node/sync/header_sync/synchronizer.rs +++ b/base_layer/core/src/base_node/sync/header_sync/synchronizer.rs @@ -110,28 +110,28 @@ impl<'a, B: BlockchainBackend + 'static> HeaderSynchronizer<'a, B> { Ok(()) => return Ok(peer_conn), // Try another peer Err(err @ BlockHeaderSyncError::NotInSync) => { - debug!(target: LOG_TARGET, "{}", err); + warn!(target: LOG_TARGET, "{}", err); }, Err(err @ BlockHeaderSyncError::RpcError(RpcError::HandshakeError(RpcHandshakeError::TimedOut))) => { - debug!(target: LOG_TARGET, "{}", err); + warn!(target: LOG_TARGET, "{}", err); self.ban_peer_short(node_id, BanReason::RpcNegotiationTimedOut).await?; }, Err(BlockHeaderSyncError::ValidationFailed(err)) => { - debug!(target: LOG_TARGET, "Block header validation failed: {}", err); + warn!(target: LOG_TARGET, "Block header validation failed: {}", err); self.ban_peer_long(node_id, err.into()).await?; }, Err(BlockHeaderSyncError::ChainSplitNotFound(peer)) => { - debug!(target: LOG_TARGET, "Chain split not found for peer {}.", peer); + warn!(target: LOG_TARGET, "Chain split not found for peer {}.", peer); self.ban_peer_long(peer, BanReason::ChainSplitNotFound).await?; }, Err(err @ BlockHeaderSyncError::InvalidBlockHeight { .. }) => { - debug!(target: LOG_TARGET, "{}", err); + warn!(target: LOG_TARGET, "{}", err); self.ban_peer_long(node_id, BanReason::GeneralHeaderSyncFailure(err)) .await?; }, Err(err) => { - debug!( + error!( target: LOG_TARGET, "Failed to synchronize headers from peer `{}`: {}", node_id, err );