fix(chain): ban peer properly when receiving a bad block (#3029)

Currently we ban the peer who sends us the block if the block is bad. However, when we receive a block, we only validate its header before rebroadcasting it, which means that an honest peer can relay a bad block and we should not ban them. Otherwise a malicious attack can use this as an opportunity to cause peers to ban each other, which may lead to a network split.

Test plan
---------
* `test_not_ban_peer_for_invalid_block`
* `test_ban_peer_for_invalid_block_header`
* `test_not_process_height_twice`

Author: bowenwang1996

Cargo.lock

@@ -5,7 +5,7 @@ use std::time::{Duration as TimeDuration, Instant};
 use borsh::BorshSerialize;
 use chrono::Duration;
 use chrono::Utc;
-use log::{debug, error, info};
+use log::{debug, error, info, warn};
 use rand::rngs::StdRng;
 use rand::seq::SliceRandom;
 use rand::SeedableRng;
@@ -400,9 +400,9 @@ impl Chain {
     pub fn save_block(&mut self, block: &Block) -> Result<(), Error> {
         let mut chain_store_update = ChainStoreUpdate::new(&mut self.store);
 
-        if !block.check_validity() {
+        if let Err(e) = block.check_validity() {
             byzantine_assert!(false);
-            return Err(ErrorKind::Other("Invalid block".into()).into());
+            return Err(e.into());
         }
 
         chain_store_update.save_block(block.clone());
@@ -421,6 +421,14 @@ impl Chain {
         });
     }
 
+    fn save_block_height_processed(&mut self, block_height: BlockHeight) -> Result<(), Error> {
+        let mut chain_store_update = ChainStoreUpdate::new(&mut self.store);
+        if !chain_store_update.is_height_processed(block_height)? {
+            chain_store_update.save_block_height_processed(block_height);
+        }
+        Ok(())
+    }
+
     // GC CONTRACT
     // ===
     //
@@ -605,6 +613,13 @@ impl Chain {
         Ok(())
     }
 
+    /// Do Basic validation of a block upon receiving it. Check that header is valid
+    /// and block is well-formed (various roots match).
+    pub fn validate_block(&mut self, block: &Block) -> Result<(), Error> {
+        self.process_block_header(&block.header(), |_| {})?;
+        block.check_validity().map_err(|e| e.into())
+    }
+
     /// Process a block header received during "header first" propagation.
     pub fn process_block_header<F>(
         &mut self,
@@ -1020,10 +1035,6 @@ impl Chain {
     {
         near_metrics::inc_counter(&metrics::BLOCK_PROCESSED_TOTAL);
 
-        if block.chunks().len() != self.runtime_adapter.num_shards() as usize {
-            return Err(ErrorKind::IncorrectNumberOfChunkHeaders.into());
-        }
-
         let prev_head = self.store.head()?;
         let mut chain_update = ChainUpdate::new(
             &mut self.store,
@@ -1035,9 +1046,11 @@ impl Chain {
             self.doomslug_threshold_mode,
         );
         let maybe_new_head = chain_update.process_block(me, &block, &provenance, on_challenge);
+        let block_height = block.header().height();
 
         match maybe_new_head {
             Ok((head, needs_to_start_fetching_state)) => {
+                chain_update.chain_store_update.save_block_height_processed(block_height);
                 chain_update.commit()?;
 
                 if needs_to_start_fetching_state {
@@ -1079,62 +1092,64 @@ impl Chain {
 
                 Ok(head)
             }
-            Err(e) => match e.kind() {
-                ErrorKind::Orphan => {
-                    let tail_height = self.store.tail()?;
-                    // we only add blocks that couldn't have been gc'ed to the orphan pool.
-                    if block.header().height() >= tail_height {
+            Err(e) => {
+                match e.kind() {
+                    ErrorKind::Orphan => {
+                        let tail_height = self.store.tail()?;
+                        // we only add blocks that couldn't have been gc'ed to the orphan pool.
+                        if block_height >= tail_height {
+                            let block_hash = *block.hash();
+                            let orphan = Orphan { block, provenance, added: Instant::now() };
+
+                            self.orphans.add(orphan);
+
+                            debug!(
+                                target: "chain",
+                                "Process block: orphan: {:?}, # orphans {}{}",
+                                block_hash,
+                                self.orphans.len(),
+                                if self.orphans.len_evicted() > 0 {
+                                    format!(", # evicted {}", self.orphans.len_evicted())
+                                } else {
+                                    String::new()
+                                },
+                            );
+                        }
+                    }
+                    ErrorKind::ChunksMissing(missing_chunks) => {
                         let block_hash = *block.hash();
+                        block_misses_chunks(missing_chunks.clone());
                         let orphan = Orphan { block, provenance, added: Instant::now() };
 
-                        self.orphans.add(orphan);
+                        self.blocks_with_missing_chunks.add(orphan);
 
                         debug!(
                             target: "chain",
-                            "Process block: orphan: {:?}, # orphans {}{}",
-                            block_hash,
-                            self.orphans.len(),
-                            if self.orphans.len_evicted() > 0 {
-                                format!(", # evicted {}", self.orphans.len_evicted())
-                            } else {
-                                String::new()
-                            },
+                            "Process block: missing chunks. Block hash: {:?}. Missing chunks: {:?}",
+                            block_hash, missing_chunks,
                         );
                     }
-                    Err(e)
-                }
-                ErrorKind::ChunksMissing(missing_chunks) => {
-                    let block_hash = *block.hash();
-                    block_misses_chunks(missing_chunks.clone());
-                    let orphan = Orphan { block, provenance, added: Instant::now() };
-
-                    self.blocks_with_missing_chunks.add(orphan);
-
-                    debug!(
-                        target: "chain",
-                        "Process block: missing chunks. Block hash: {:?}. Missing chunks: {:?}",
-                        block_hash, missing_chunks,
-                    );
-                    Err(e)
-                }
-                ErrorKind::EpochOutOfBounds => {
-                    // Possibly block arrived before we finished processing all of the blocks for epoch before last.
-                    // Or someone is attacking with invalid chain.
-                    debug!(target: "chain", "Received block {}/{} ignored, as epoch is unknown", block.header().height(), block.hash());
-                    Err(e)
+                    ErrorKind::EpochOutOfBounds => {
+                        // Possibly block arrived before we finished processing all of the blocks for epoch before last.
+                        // Or someone is attacking with invalid chain.
+                        debug!(target: "chain", "Received block {}/{} ignored, as epoch is unknown", block_height, block.hash());
+                    }
+                    ErrorKind::Unfit(ref msg) => {
+                        debug!(
+                            target: "chain",
+                            "Block {} at {} is unfit at this time: {}",
+                            block.hash(),
+                            block_height,
+                            msg
+                        );
+                    }
+                    _ => {}
                 }
-                ErrorKind::Unfit(ref msg) => {
-                    debug!(
-                        target: "chain",
-                        "Block {} at {} is unfit at this time: {}",
-                        block.hash(),
-                        block.header().height(),
-                        msg
-                    );
-                    Err(ErrorKind::Unfit(msg.clone()).into())
+                if let Err(e) = self.save_block_height_processed(block_height) {
+                    warn!(target: "chain", "Failed to save processed height {}: {}", block_height, e);
                 }
-                _ => Err(e),
-            },
+                Err(e)
+            }
         }
     }
 
@@ -2823,6 +2838,10 @@ impl<'a> ChainUpdate<'a> {
     {
         debug!(target: "chain", "Process block {} at {}, approvals: {}, me: {:?}", block.hash(), block.header().height(), block.header().num_approvals(), me);
 
+        if block.chunks().len() != self.runtime_adapter.num_shards() as usize {
+            return Err(ErrorKind::IncorrectNumberOfChunkHeaders.into());
+        }
+
         // Check if we have already processed this block previously.
         self.check_known(block.header().hash())?;
 
@@ -2888,9 +2907,9 @@ impl<'a> ChainUpdate<'a> {
             return Err(ErrorKind::InvalidRandomnessBeaconOutput.into());
         }
 
-        if !block.check_validity() {
+        if let Err(e) = block.check_validity() {
             byzantine_assert!(false);
-            return Err(ErrorKind::Other("Invalid block".into()).into());
+            return Err(e.into());
         }
 
         let protocol_version =
@@ -3140,8 +3159,6 @@ impl<'a> ChainUpdate<'a> {
                 return Err(ErrorKind::InvalidApprovals.into());
             };
 
-            self.runtime_adapter.verify_block_signature(header)?;
-
             let stakes = self
                 .runtime_adapter
                 .get_epoch_block_approvers_ordered(header.prev_hash())?

chain/chain/src/chain.rs

@@ -5,6 +5,7 @@ use chrono::{DateTime, Utc};
 use failure::{Backtrace, Context, Fail};
 use log::error;
 
+use near_primitives::block::BlockValidityError;
 use near_primitives::challenge::{ChunkProofs, ChunkState};
 use near_primitives::errors::{EpochError, StorageError};
 use near_primitives::hash::CryptoHash;
@@ -46,9 +47,6 @@ pub enum ErrorKind {
     /// Invalid block proposed signature.
     #[fail(display = "Invalid Block Proposer Signature")]
     InvalidBlockProposer,
-    /// Invalid block confirmation signature.
-    #[fail(display = "Invalid Block Confirmation Signature")]
-    InvalidBlockConfirmation,
     /// Invalid state root hash.
     #[fail(display = "Invalid State Root Hash")]
     InvalidStateRoot,
@@ -76,6 +74,9 @@ pub enum ErrorKind {
     /// Invalid transactions in the block.
     #[fail(display = "Invalid Transactions")]
     InvalidTransactions,
+    /// Invalid Challenge Root (doesn't match actual challenge)
+    #[fail(display = "Invalid Challenge Root")]
+    InvalidChallengeRoot,
     /// Invalid challenge (wrong signature or format).
     #[fail(display = "Invalid Challenge")]
     InvalidChallenge,
@@ -241,7 +242,6 @@ impl Error {
             | ErrorKind::InvalidBlockFutureTime(_)
             | ErrorKind::InvalidBlockHeight
             | ErrorKind::InvalidBlockProposer
-            | ErrorKind::InvalidBlockConfirmation
             | ErrorKind::InvalidChunk
             | ErrorKind::InvalidChunkProofs(_)
             | ErrorKind::InvalidChunkState(_)
@@ -273,7 +273,8 @@ impl Error {
             | ErrorKind::InvalidStateRequest(_)
             | ErrorKind::InvalidRandomnessBeaconOutput
             | ErrorKind::InvalidBlockMerkleRoot
-            | ErrorKind::NotAValidator => true,
+            | ErrorKind::NotAValidator
+            | ErrorKind::InvalidChallengeRoot => true,
         }
     }
 
@@ -315,3 +316,17 @@ impl From<EpochError> for Error {
         .into()
     }
 }
+
+impl From<BlockValidityError> for Error {
+    fn from(error: BlockValidityError) -> Self {
+        match error {
+            BlockValidityError::InvalidStateRoot => ErrorKind::InvalidStateRoot,
+            BlockValidityError::InvalidReceiptRoot => ErrorKind::InvalidChunkReceiptsRoot,
+            BlockValidityError::InvalidTransactionRoot => ErrorKind::InvalidTxRoot,
+            BlockValidityError::InvalidChunkHeaderRoot => ErrorKind::InvalidChunkHeadersRoot,
+            BlockValidityError::InvalidNumChunksIncluded => ErrorKind::InvalidChunkMask,
+            BlockValidityError::InvalidChallengeRoot => ErrorKind::InvalidChallengeRoot,
+        }
+        .into()
+    }
+}