fix: adds bans for horizon sync (tari-project#5661)

Description --- Adds bans for horizon sync bad behavior Cleans up the horizonsync errors Motivation and Context --- We should ban bad behavior, which we don't do currently. Audit Finding Number --- TARI-017 --------- Co-authored-by: stringhandler <[email protected]>
brianp · Aug 30, 2023 · 826473d · 826473d
1 parent 90a8a21
commit 826473d
Show file tree

Hide file tree

Showing 2 changed files with 44 additions and 10 deletions.
diff --git a/base_layer/core/src/base_node/sync/horizon_state_sync/error.rs b/base_layer/core/src/base_node/sync/horizon_state_sync/error.rs
@@ -34,7 +34,6 @@ use thiserror::Error;
 use tokio::task;
 
 use crate::{
-    base_node::comms_interface::CommsInterfaceError,
     chain_storage::{ChainStorageError, MmrTree},
     transactions::transaction_components::TransactionError,
     validation::ValidationError,
@@ -46,8 +45,6 @@ pub enum HorizonSyncError {
     IncorrectResponse(String),
     #[error("Chain storage error: {0}")]
     ChainStorageError(#[from] ChainStorageError),
-    #[error("Comms interface error: {0}")]
-    CommsInterfaceError(#[from] CommsInterfaceError),
     #[error("Final state validation failed: {0}")]
     FinalStateValidationFailed(ValidationError),
     #[error("Join error: {0}")]
@@ -56,8 +53,6 @@ pub enum HorizonSyncError {
     RangeProofError(String),
     #[error("An invalid transaction has been encountered: {0}")]
     TransactionError(#[from] TransactionError),
-    #[error("Invalid kernel signature: {0}")]
-    InvalidKernelSignature(TransactionError),
     #[error("MMR did not match for {mmr_tree} at height {at_height}. Expected {actual_hex} to equal {expected_hex}")]
     InvalidMmrRoot {
         mmr_tree: MmrTree,
@@ -67,8 +62,6 @@ pub enum HorizonSyncError {
     },
     #[error("Invalid MMR position {mmr_position} at height {at_height}")]
     InvalidMmrPosition { at_height: u64, mmr_position: u64 },
-    #[error("Invalid range proof for output: {0} : {1}")]
-    InvalidRangeProof(String, String),
     #[error("RPC error: {0}")]
     RpcError(#[from] RpcError),
     #[error("RPC status: {0}")]

diff --git a/base_layer/core/src/base_node/sync/horizon_state_sync/synchronizer.rs b/base_layer/core/src/base_node/sync/horizon_state_sync/synchronizer.rs
@@ -196,18 +196,21 @@ impl<'a, B: BlockchainBackend + 'static> HorizonStateSynchronization<'a, B> {
                 Ok(_) => match self.finalize_horizon_sync(sync_peer).await {
                     Ok(_) => return Ok(()),
                     Err(err) => {
+                        self.ban_peer_on_bannable_error(sync_peer, &err).await?;
                         warn!(target: LOG_TARGET, "Error during sync:{}", err);
                         return Err(err);
                     },
                 },
                 Err(err @ HorizonSyncError::RpcError(RpcError::ReplyTimeout)) |
                 Err(err @ HorizonSyncError::MaxLatencyExceeded { .. }) => {
+                    self.ban_peer_on_bannable_error(sync_peer, &err).await?;
                     warn!(target: LOG_TARGET, "{}", err);
                     if i == self.sync_peers.len() - 1 {
                         return Err(HorizonSyncError::AllSyncPeersExceedLatency);
                     }
                 },
                 Err(err) => {
+                    self.ban_peer_on_bannable_error(sync_peer, &err).await?;
                     warn!(target: LOG_TARGET, "Error during sync:{}", err);
                     return Err(err);
                 },
@@ -217,6 +220,46 @@ impl<'a, B: BlockchainBackend + 'static> HorizonStateSynchronization<'a, B> {
         Err(HorizonSyncError::FailedSyncAllPeers)
     }
 
+    async fn ban_peer_on_bannable_error(
+        &mut self,
+        peer: &SyncPeer,
+        error: &HorizonSyncError,
+    ) -> Result<(), HorizonSyncError> {
+        match error {
+            HorizonSyncError::ChainStorageError(_) |
+            HorizonSyncError::JoinError(_) |
+            HorizonSyncError::RpcError(_) |
+            HorizonSyncError::RpcStatus(_) |
+            HorizonSyncError::ConnectivityError(_) |
+            HorizonSyncError::NoSyncPeers |
+            HorizonSyncError::FailedSyncAllPeers |
+            HorizonSyncError::AllSyncPeersExceedLatency => {
+                // these are local errors so we dont ban die per
+            },
+            HorizonSyncError::MaxLatencyExceeded { .. } => {
+                warn!(target: LOG_TARGET, "Banned sync peer for short while because peer exceeded max latency: {}",error.to_string());
+                if let Err(err) = self
+                    .connectivity
+                    .ban_peer_until(peer.node_id().clone(), self.config.short_ban_period, error.to_string())
+                    .await
+                {
+                    error!(target: LOG_TARGET, "Failed to ban peer: {}", err);
+                }
+            },
+            _ => {
+                warn!(target: LOG_TARGET, "Banned sync peer for because: {}",error.to_string());
+                if let Err(err) = self
+                    .connectivity
+                    .ban_peer_until(peer.node_id().clone(), self.config.ban_period, error.to_string())
+                    .await
+                {
+                    error!(target: LOG_TARGET, "Failed to ban peer: {}", err);
+                }
+            },
+        }
+        Ok(())
+    }
+
     async fn begin_sync(
         &mut self,
         sync_peer: SyncPeer,
@@ -313,9 +356,7 @@ impl<'a, B: BlockchainBackend + 'static> HorizonStateSynchronization<'a, B> {
             let latency = last_sync_timer.elapsed();
             avg_latency.add_sample(latency);
             let kernel: TransactionKernel = kernel?.try_into().map_err(HorizonSyncError::ConversionError)?;
-            kernel
-                .verify_signature()
-                .map_err(HorizonSyncError::InvalidKernelSignature)?;
+            kernel.verify_signature()?;
 
             kernel_hashes.push(kernel.hash());