From 7e73e27cb62a2149f6b26aed6bd2ed2bfb27502e Mon Sep 17 00:00:00 2001
From: Michael Birch <birchmd8@gmail.com>
Date: Mon, 27 Jul 2020 09:09:29 -0400
Subject: [PATCH] fix(chunks): properly handle missing block, previously
 causing chunk signature check to fail (#3026)

Description:
For partial encoded chunks, the signature check includes looking up the chunk producer, based on the epoch ID. In addition it checks whether this chunk producer has been slashed. These checks require the node to know the parent block the chunk header references, and thus fails if it is not present. This change ensures we properly catch and handle such errors since we will be able to process the chunk later when the node eventually learns about the missing block.

Testing plan:
* New unit test for processing a chunk with an unknown parent block
---
 chain/chunks/src/lib.rs    | 12 ++++++------
 chain/client/src/client.rs | 35 +++++++++++++++++++++++++++++++++--
 2 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/chain/chunks/src/lib.rs b/chain/chunks/src/lib.rs
index 9b0fe34b3ee..a94d4f6b895 100644
--- a/chain/chunks/src/lib.rs
+++ b/chain/chunks/src/lib.rs
@@ -935,6 +935,12 @@ impl ShardsManager {
             return Err(Error::InvalidPartMessage);
         }
 
+        // check part merkle proofs
+        let num_total_parts = self.runtime_adapter.num_total_parts();
+        for part_info in forward.parts.iter() {
+            self.validate_part(forward.merkle_root, part_info, num_total_parts)?;
+        }
+
         // check signature
         let valid_signature = self.runtime_adapter.verify_chunk_signature_with_header_parts(
             &forward.chunk_hash,
@@ -948,12 +954,6 @@ impl ShardsManager {
             return Err(Error::InvalidChunkSignature);
         }
 
-        // check part merkle proofs
-        let num_total_parts = self.runtime_adapter.num_total_parts();
-        for part_info in forward.parts.iter() {
-            self.validate_part(forward.merkle_root, part_info, num_total_parts)?;
-        }
-
         Ok(())
     }
 
diff --git a/chain/client/src/client.rs b/chain/client/src/client.rs
index 53e1b8025c8..f56474cb364 100644
--- a/chain/client/src/client.rs
+++ b/chain/client/src/client.rs
@@ -704,9 +704,12 @@ impl Client {
         &mut self,
         forward: PartialEncodedChunkForwardMsg,
     ) -> Result<Vec<AcceptedBlock>, Error> {
-        self.shards_mgr.validate_partial_encoded_chunk_forward(&forward)?;
+        let maybe_header = self
+            .shards_mgr
+            .validate_partial_encoded_chunk_forward(&forward)
+            .and_then(|_| self.shards_mgr.get_partial_encoded_chunk_header(&forward.chunk_hash));
 
-        let header = match self.shards_mgr.get_partial_encoded_chunk_header(&forward.chunk_hash) {
+        let header = match maybe_header {
             Ok(header) => Ok(header),
             Err(near_chunks::Error::UnknownChunk) => {
                 // We don't know this chunk yet; cache the forwarded part
@@ -714,6 +717,21 @@ impl Client {
                 self.shards_mgr.insert_forwarded_chunk(forward);
                 return Err(Error::Chunk(near_chunks::Error::UnknownChunk));
             }
+            Err(near_chunks::Error::ChainError(chain_error)) => {
+                match chain_error.kind() {
+                    near_chain::ErrorKind::BlockMissing(_)
+                    | near_chain::ErrorKind::DBNotFoundErr(_) => {
+                        // We can't check if this chunk came from a valid chunk producer because
+                        // we don't know `prev_block`, however the signature is checked when
+                        // forwarded parts are later processed as partial encoded chunks, so we
+                        // can mark it as unknown for now.
+                        self.shards_mgr.insert_forwarded_chunk(forward);
+                        return Err(Error::Chunk(near_chunks::Error::UnknownChunk));
+                    }
+                    // Some other error occurred, we don't know how to handle it
+                    _ => Err(near_chunks::Error::ChainError(chain_error)),
+                }
+            }
             Err(err) => Err(err),
         }?;
         let partial_chunk =
@@ -1458,6 +1476,7 @@ mod test {
     use near_chunks::test_utils::ChunkForwardingTestFixture;
     use near_chunks::ProcessPartialEncodedChunkResult;
     use near_crypto::KeyType;
+    use near_network::types::PartialEncodedChunkForwardMsg;
     use near_primitives::block::{Approval, ApprovalInner};
     use near_primitives::hash::hash;
     use near_primitives::validator_signer::InMemoryValidatorSigner;
@@ -1502,6 +1521,10 @@ mod test {
         // change the prev_block to some unknown block
         mock_chunk.header.inner.prev_block_hash = hash(b"some_prev_block");
         mock_chunk.header.init();
+        let mock_forward = PartialEncodedChunkForwardMsg::from_header_and_parts(
+            &mock_chunk.header,
+            mock_chunk.parts.clone(),
+        );
 
         // process_partial_encoded_chunk should return Ok(NeedBlock) if the chunk is
         // based on a missing block.
@@ -1511,5 +1534,13 @@ mod test {
             &mut client.rs,
         );
         assert!(matches!(result, Ok(ProcessPartialEncodedChunkResult::NeedBlock)));
+
+        // process_partial_encoded_chunk_forward should return UnknownChunk if it is based on a
+        // a missing block.
+        let result = client.process_partial_encoded_chunk_forward(mock_forward);
+        assert!(matches!(
+            result,
+            Err(crate::types::Error::Chunk(near_chunks::Error::UnknownChunk))
+        ));
     }
 }