[Epoch Sync] Verify merkle proofs for the first block in current epoc…

…h. (#12330)

* Verify the first block of the current epoch against the last final
block of the current epoch, using the merkle proof.
* Verify the last and second last headers before the first header.
* Verify the partial merkle tree (note: this is NOT the merkle *proof*!)
of the first block; add `is_well_formed()` to facilitate that
validation.
* Clearly document what every field of EpochSyncProof is validated
against.

chain/client/src/sync/epoch.rs

@@ -5,7 +5,7 @@ use near_async::futures::{AsyncComputationSpawner, AsyncComputationSpawnerExt};
 use near_async::messaging::{CanSend, Handler};
 use near_async::time::Clock;
 use near_chain::types::Tip;
-use near_chain::{BlockHeader, Chain, ChainStoreAccess, Error};
+use near_chain::{BlockHeader, Chain, ChainStoreAccess, Error, MerkleProofAccess};
 use near_chain_configs::EpochSyncConfig;
 use near_client_primitives::types::{EpochSyncStatus, SyncStatus};
 use near_crypto::Signature;
@@ -316,14 +316,11 @@ impl EpochSync {
             )?
             .ok_or_else(|| Error::Other("Could not find first block of next epoch".to_string()))?;
 
-        // TODO(#12255) That currently does not work because we might need some old block hashes
-        // in order to build the merkle proof.
-        // let merkle_proof_for_first_block_of_current_epoch = store
-        //     .compute_past_block_proof_in_merkle_tree_of_later_block(
-        //         first_block_of_current_epoch.hash(),
-        //         final_block_header_in_current_epoch.hash(),
-        //     )?;
-        let merkle_proof_for_first_block_of_current_epoch = Default::default();
+        let merkle_proof_for_first_block_of_current_epoch = store
+            .compute_past_block_proof_in_merkle_tree_of_later_block(
+                first_block_of_current_epoch.hash(),
+                last_final_block_header_in_current_epoch.hash(),
+            )?;
 
         let partial_merkle_tree_for_first_block_of_current_epoch = store
             .get_ser::<PartialMerkleTree>(
@@ -837,30 +834,25 @@ impl EpochSync {
 
     fn verify_current_epoch_data(
         current_epoch: &EpochSyncProofCurrentEpochData,
-        _current_epoch_final_block_header: &BlockHeader,
+        current_epoch_final_block_header: &BlockHeader,
     ) -> Result<(), Error> {
         // Verify first_block_header_in_epoch
         let first_block_header = &current_epoch.first_block_header_in_epoch;
-        // TODO(#12255) Uncomment the check below when `merkle_proof_for_first_block` is generated.
-        // if !merkle::verify_hash(
-        //     *current_epoch_final_block_header.block_merkle_root(),
-        //     &current_epoch.merkle_proof_for_first_block,
-        //     *first_block_header.hash(),
-        // ) {
-        //     return Err(Error::InvalidEpochSyncProof(
-        //         "invalid merkle_proof_for_first_block".to_string(),
-        //     ));
-        // }
-
-        // Verify partial_merkle_tree_for_first_block
-        if current_epoch.partial_merkle_tree_for_first_block.root()
-            != *first_block_header.block_merkle_root()
-        {
+        if !near_primitives::merkle::verify_hash(
+            *current_epoch_final_block_header.block_merkle_root(),
+            &current_epoch.merkle_proof_for_first_block,
+            *first_block_header.hash(),
+        ) {
             return Err(Error::InvalidEpochSyncProof(
-                "invalid path in partial_merkle_tree_for_first_block".to_string(),
+                "invalid merkle_proof_for_first_block".to_string(),
             ));
         }
-        // TODO(#12256) Investigate why "+1" was needed here, looks like it should not be there.
+
+        // Verify partial_merkle_tree_for_first_block. The size needs to match to ensure that
+        // the partial merkle tree is for the right block ordinal, and the partial tree itself
+        // needs to be valid and have the correct root.
+        //
+        // Note that the block_ordinal in the header is 1-based, so we need to add 1 to the size.
         if current_epoch.partial_merkle_tree_for_first_block.size() + 1
             != first_block_header.block_ordinal()
         {
@@ -869,6 +861,31 @@ impl EpochSync {
             ));
         }
 
+        if !current_epoch.partial_merkle_tree_for_first_block.is_well_formed()
+            || current_epoch.partial_merkle_tree_for_first_block.root()
+                != *first_block_header.block_merkle_root()
+        {
+            return Err(Error::InvalidEpochSyncProof(
+                "invalid path in partial_merkle_tree_for_first_block".to_string(),
+            ));
+        }
+
+        // Verify the two headers before the first block.
+        if current_epoch.last_block_header_in_prev_epoch.hash()
+            != current_epoch.first_block_header_in_epoch.prev_hash()
+        {
+            return Err(Error::InvalidEpochSyncProof(
+                "invalid last_block_header_in_prev_epoch".to_string(),
+            ));
+        }
+        if current_epoch.second_last_block_header_in_prev_epoch.hash()
+            != current_epoch.last_block_header_in_prev_epoch.prev_hash()
+        {
+            return Err(Error::InvalidEpochSyncProof(
+                "invalid second_last_block_header_in_prev_epoch".to_string(),
+            ));
+        }
+
         Ok(())
     }
 

core/primitives/src/epoch_sync.rs

@@ -49,6 +49,9 @@ pub struct EpochSyncProofV1 {
     /// epoch EpochId::default, and then the next epoch after genesis is fully determined by
     /// the genesis; after that would be the first epoch included here), to and including the
     /// current epoch, in that order.
+    ///
+    /// The first entry in this list is proven against the genesis. Then, each entry is proven
+    /// against the previous entry, thereby validating the entire list by induction.
     pub all_epochs: Vec<EpochSyncProofEpochData>,
     /// Some extra data for the last epoch before the current epoch.
     pub last_epoch: EpochSyncProofLastEpochData,
@@ -135,9 +138,10 @@ pub struct EpochSyncProofEpochData {
 /// Data needed to initialize the epoch sync boundary.
 #[derive(Debug, Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize, ProtocolSchema)]
 pub struct EpochSyncProofLastEpochData {
-    /// The following six fields are used to derive the epoch_sync_data_hash included in any
-    /// BlockHeaderV3. This is used to verify all the data we need around the epoch sync
-    /// boundary, against `last_final_block_header` in the second last epoch data.
+    /// The following six fields are used to derive the epoch_sync_data_hash included in the
+    /// first block of the epoch right after (assuming it is a BlockHeaderV3 or newer). This
+    /// is used to verify all the data we need around the epoch sync boundary, against
+    /// `first_block_header_in_epoch` in `current_epoch`.
     pub epoch_info: EpochInfo,
     pub next_epoch_info: EpochInfo,
     pub next_next_epoch_info: EpochInfo,
@@ -150,21 +154,21 @@ pub struct EpochSyncProofLastEpochData {
 #[derive(Debug, Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize, ProtocolSchema)]
 pub struct EpochSyncProofCurrentEpochData {
     /// The first block header that begins the epoch. It is proven using a merkle proof
-    /// against `last_final_block_header` in the current epoch data. Note that we cannot
-    /// use signatures to prove this like for the final block, because the first block
-    /// header may not have a consecutive height afterwards.
+    /// against `last_final_block_header` in the last entry of `all_epochs`. Note that we cannot
+    /// use signatures to prove this like for the final block, because the first block header may
+    /// not have a consecutive height afterwards.
     pub first_block_header_in_epoch: BlockHeader,
-    // The last two block headers are also needed for various purposes after epoch sync.
-    // TODO(#11931): do we really need these?
-    // TODO(#12259) These 2 fields are currently unverified.
+    /// The last two block headers are also needed for various purposes after epoch sync.
+    /// They are proven against the `first_block_header_in_epoch`.
     pub last_block_header_in_prev_epoch: BlockHeader,
     pub second_last_block_header_in_prev_epoch: BlockHeader,
-    // Used to prove the block against the merkle root
-    // included in the final block in this next epoch (included in LastEpochData).
-    // TODO(#12255) This field is currently ungenerated and unverified.
+    /// Used to prove `first_block_header_in_epoch` against the `last_final_block_header` of
+    /// the last entry of `all_epochs`.
     pub merkle_proof_for_first_block: Vec<MerklePathItem>,
-    // Partial merkle tree for the first block in this next epoch.
-    // It is necessary and sufficient to calculate next blocks merkle roots.
-    // It is proven using `first_block_header_in_epoch`.
+    /// Partial merkle tree for the first block in this epoch. It is needed to construct future
+    /// partial merkle trees for any blocks that follow.
+    /// This is proven against the merkle root and block ordinal in `first_block_header_in_epoch`
+    /// (as there is only one unique correct partial merkle tree for a specific root and a specific
+    /// block ordinal).
     pub partial_merkle_tree_for_first_block: PartialMerkleTree,
 }

core/primitives/src/merkle.rs

@@ -154,6 +154,22 @@ pub struct PartialMerkleTree {
 }
 
 impl PartialMerkleTree {
+    /// A PartialMerkleTree is well formed iff the path would be a valid proof for the next block
+    /// of ordinal `size`. This means that the path contains exactly `size.count_ones()` elements.
+    ///
+    /// The <= direction of this statement is easy to prove, as the subtrees whose roots are being
+    /// combined to form the overall root correspond to the binary 1s in the size.
+    ///
+    /// The => direction is proven by observing that the root is computed as
+    /// hash(path[0], hash(path[1], hash(path[2], ... hash(path[n-1], path[n]) ...))
+    /// and there is only one way to provide an array of paths of the exact same size that would
+    /// produce the same result when combined in this way. (This would not have been true if we
+    /// could provide a path of a different size, e.g. if we could provide just one hash, we could
+    /// provide only the root).
+    pub fn is_well_formed(&self) -> bool {
+        self.path.len() == self.size.count_ones() as usize
+    }
+
     pub fn root(&self) -> MerkleHash {
         if self.path.is_empty() {
             CryptoHash::default()
@@ -273,6 +289,17 @@ mod tests {
         let mut hashes = vec![];
         for i in 0..50 {
             assert_eq!(compute_root(&hashes), tree.root());
+            assert!(tree.is_well_formed());
+
+            let mut tree_copy = tree.clone();
+            tree_copy.path.push(CryptoHash::hash_bytes(&[i]));
+            assert!(!tree_copy.is_well_formed());
+            tree_copy.path.pop();
+            if !tree_copy.path.is_empty() {
+                tree_copy.path.pop();
+                assert!(!tree_copy.is_well_formed());
+            }
+
             let cur_hash = CryptoHash::hash_bytes(&[i]);
             hashes.push(cur_hash);
             tree.insert(cur_hash);