Verify inclusion proof should not be fallible (#5787)

* Verify inclusion proof should not be fallible

* Add blob sidecar inclusion test (#33)

* Add blob sidecar inclusion test.

* Fix lint

beacon_node/beacon_chain/src/blob_verification.rs

@@ -10,7 +10,6 @@ use crate::block_verification::{
 use crate::kzg_utils::{validate_blob, validate_blobs};
 use crate::{metrics, BeaconChainError};
 use kzg::{Error as KzgError, Kzg, KzgCommitment};
-use merkle_proof::MerkleTreeError;
 use slog::debug;
 use ssz_derive::{Decode, Encode};
 use ssz_types::VariableList;
@@ -128,13 +127,6 @@ pub enum GossipBlobError<E: EthSpec> {
     /// The blob sidecar is invalid and the peer is faulty.
     KzgError(kzg::Error),
 
-    /// The kzg commitment inclusion proof failed.
-    ///
-    /// ## Peer scoring
-    ///
-    /// The blob sidecar is invalid
-    InclusionProof(MerkleTreeError),
-
     /// The pubkey cache timed out.
     ///
     /// ## Peer scoring
@@ -459,10 +451,7 @@ pub fn validate_blob_sidecar_for_gossip<T: BeaconChainTypes>(
 
     // Verify the inclusion proof in the sidecar
     let _timer = metrics::start_timer(&metrics::BLOB_SIDECAR_INCLUSION_PROOF_VERIFICATION);
-    if !blob_sidecar
+    if !blob_sidecar.verify_blob_sidecar_inclusion_proof() {
-        .verify_blob_sidecar_inclusion_proof()
-        .map_err(GossipBlobError::InclusionProof)?
-    {
         return Err(GossipBlobError::InvalidInclusionProof);
     }
     drop(_timer);

beacon_node/network/src/network_beacon_processor/gossip_methods.rs

@@ -690,7 +690,6 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
                     | GossipBlobError::InvalidSubnet { .. }
                     | GossipBlobError::InvalidInclusionProof
                     | GossipBlobError::KzgError(_)
-                    | GossipBlobError::InclusionProof(_)
                     | GossipBlobError::NotFinalizedDescendant { .. } => {
                         warn!(
                             self.log,

beacon_node/network/src/sync/network_context/requests.rs

@@ -120,7 +120,7 @@ impl<E: EthSpec> ActiveBlobsByRootRequest<E> {
         if self.request.block_root != block_root {
             return Err(LookupVerifyError::UnrequestedBlockRoot(block_root));
         }
-        if !blob.verify_blob_sidecar_inclusion_proof().unwrap_or(false) {
+        if !blob.verify_blob_sidecar_inclusion_proof() {
             return Err(LookupVerifyError::InvalidInclusionProof);
         }
         if !self.request.indices.contains(&blob.index) {

consensus/types/src/blob_sidecar.rs

@@ -12,7 +12,7 @@ use kzg::{
 };
 use merkle_proof::{merkle_root_from_branch, verify_merkle_proof, MerkleTreeError};
 use rand::Rng;
-use safe_arith::{ArithError, SafeArith};
+use safe_arith::ArithError;
 use serde::{Deserialize, Serialize};
 use ssz::Encode;
 use ssz_derive::{Decode, Encode};
@@ -190,33 +190,30 @@ impl<E: EthSpec> BlobSidecar<E> {
     }
 
     /// Verifies the kzg commitment inclusion merkle proof.
-    pub fn verify_blob_sidecar_inclusion_proof(&self) -> Result<bool, MerkleTreeError> {
+    pub fn verify_blob_sidecar_inclusion_proof(&self) -> bool {
-        // Depth of the subtree rooted at `blob_kzg_commitments` in the `BeaconBlockBody`
+        let kzg_commitments_tree_depth = E::kzg_commitments_tree_depth();
-        // is equal to depth of the ssz List max size + 1 for the length mixin
+
-        let kzg_commitments_tree_depth = (E::max_blob_commitments_per_block()
+        // EthSpec asserts that kzg_commitments_tree_depth is less than KzgCommitmentInclusionProofDepth
-            .next_power_of_two()
+        let (kzg_commitment_subtree_proof, kzg_commitments_proof) = self
-            .ilog2()
+            .kzg_commitment_inclusion_proof
-            .safe_add(1))? as usize;
+            .split_at(kzg_commitments_tree_depth);
+
         // Compute the `tree_hash_root` of the `blob_kzg_commitments` subtree using the
         // inclusion proof branches
         let blob_kzg_commitments_root = merkle_root_from_branch(
             self.kzg_commitment.tree_hash_root(),
-            self.kzg_commitment_inclusion_proof
+            kzg_commitment_subtree_proof,
-                .get(0..kzg_commitments_tree_depth)
-                .ok_or(MerkleTreeError::PleaseNotifyTheDevs)?,
             kzg_commitments_tree_depth,
             self.index as usize,
         );
         // The remaining inclusion proof branches are for the top level `BeaconBlockBody` tree
-        Ok(verify_merkle_proof(
+        verify_merkle_proof(
             blob_kzg_commitments_root,
-            self.kzg_commitment_inclusion_proof
+            kzg_commitments_proof,
-                .get(kzg_commitments_tree_depth..E::kzg_proof_inclusion_proof_depth())
+            E::block_body_tree_depth(),
-                .ok_or(MerkleTreeError::PleaseNotifyTheDevs)?,
-            E::kzg_proof_inclusion_proof_depth().safe_sub(kzg_commitments_tree_depth)?,
             BLOB_KZG_COMMITMENTS_INDEX,
             self.signed_block_header.message.body_root,
-        ))
+        )
     }
 
     pub fn random_valid<R: Rng>(rng: &mut R, kzg: &Kzg) -> Result<Self, String> {

consensus/types/src/eth_spec.rs

@@ -292,6 +292,22 @@ pub trait EthSpec:
         Self::KzgCommitmentInclusionProofDepth::to_usize()
     }
 
+    fn kzg_commitments_tree_depth() -> usize {
+        // Depth of the subtree rooted at `blob_kzg_commitments` in the `BeaconBlockBody`
+        // is equal to depth of the ssz List max size + 1 for the length mixin
+        Self::max_blob_commitments_per_block()
+            .next_power_of_two()
+            .ilog2()
+            .safe_add(1)
+            .expect("The log of max_blob_commitments_per_block can not overflow") as usize
+    }
+
+    fn block_body_tree_depth() -> usize {
+        Self::kzg_proof_inclusion_proof_depth()
+            .safe_sub(Self::kzg_commitments_tree_depth())
+            .expect("Preset values are not configurable and never result in non-positive block body depth")
+    }
+
     /// Returns the `PENDING_BALANCE_DEPOSITS_LIMIT` constant for this specification.
     fn pending_balance_deposits_limit() -> usize {
         Self::PendingBalanceDepositsLimit::to_usize()
@@ -517,3 +533,26 @@ impl EthSpec for GnosisEthSpec {
         EthSpecId::Gnosis
     }
 }
+
+#[cfg(test)]
+mod test {
+    use crate::{EthSpec, GnosisEthSpec, MainnetEthSpec, MinimalEthSpec};
+
+    fn assert_valid_spec<E: EthSpec>() {
+        E::kzg_commitments_tree_depth();
+        E::block_body_tree_depth();
+    }
+
+    #[test]
+    fn mainnet_spec() {
+        assert_valid_spec::<MainnetEthSpec>();
+    }
+    #[test]
+    fn minimal_spec() {
+        assert_valid_spec::<MinimalEthSpec>();
+    }
+    #[test]
+    fn gnosis_spec() {
+        assert_valid_spec::<GnosisEthSpec>();
+    }
+}

consensus/types/src/test_utils/generate_random_block_and_blobs.rs

@@ -0,0 +1,96 @@
+use rand::Rng;
+
+use kzg::{KzgCommitment, KzgProof};
+
+use crate::beacon_block_body::KzgCommitments;
+use crate::*;
+
+use super::*;
+
+type BlobsBundle<E> = (KzgCommitments<E>, KzgProofs<E>, BlobsList<E>);
+
+pub fn generate_rand_block_and_blobs<E: EthSpec>(
+    fork_name: ForkName,
+    num_blobs: usize,
+    rng: &mut impl Rng,
+) -> (SignedBeaconBlock<E, FullPayload<E>>, Vec<BlobSidecar<E>>) {
+    let inner = map_fork_name!(fork_name, BeaconBlock, <_>::random_for_test(rng));
+    let mut block = SignedBeaconBlock::from_block(inner, Signature::random_for_test(rng));
+    let mut blob_sidecars = vec![];
+
+    if block.fork_name_unchecked() < ForkName::Deneb {
+        return (block, blob_sidecars);
+    }
+
+    let (commitments, proofs, blobs) = generate_blobs::<E>(num_blobs).unwrap();
+    *block
+        .message_mut()
+        .body_mut()
+        .blob_kzg_commitments_mut()
+        .expect("kzg commitment expected from Deneb") = commitments.clone();
+
+    for (index, ((blob, kzg_commitment), kzg_proof)) in blobs
+        .into_iter()
+        .zip(commitments.into_iter())
+        .zip(proofs.into_iter())
+        .enumerate()
+    {
+        blob_sidecars.push(BlobSidecar {
+            index: index as u64,
+            blob: blob.clone(),
+            kzg_commitment,
+            kzg_proof,
+            signed_block_header: block.signed_block_header(),
+            kzg_commitment_inclusion_proof: block
+                .message()
+                .body()
+                .kzg_commitment_merkle_proof(index)
+                .unwrap(),
+        });
+    }
+    (block, blob_sidecars)
+}
+
+pub fn generate_blobs<E: EthSpec>(n_blobs: usize) -> Result<BlobsBundle<E>, String> {
+    let (mut commitments, mut proofs, mut blobs) = BlobsBundle::<E>::default();
+
+    for blob_index in 0..n_blobs {
+        blobs
+            .push(Blob::<E>::default())
+            .map_err(|_| format!("blobs are full, blob index: {:?}", blob_index))?;
+        commitments
+            .push(KzgCommitment::empty_for_testing())
+            .map_err(|_| format!("blobs are full, blob index: {:?}", blob_index))?;
+        proofs
+            .push(KzgProof::empty())
+            .map_err(|_| format!("blobs are full, blob index: {:?}", blob_index))?;
+    }
+
+    Ok((commitments, proofs, blobs))
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use rand::thread_rng;
+
+    #[test]
+    fn test_verify_blob_inclusion_proof() {
+        let (_block, blobs) =
+            generate_rand_block_and_blobs::<MainnetEthSpec>(ForkName::Deneb, 6, &mut thread_rng());
+        for blob in blobs {
+            assert!(blob.verify_blob_sidecar_inclusion_proof());
+        }
+    }
+
+    #[test]
+    fn test_verify_blob_inclusion_proof_invalid() {
+        let (_block, blobs) =
+            generate_rand_block_and_blobs::<MainnetEthSpec>(ForkName::Deneb, 6, &mut thread_rng());
+
+        for mut blob in blobs {
+            blob.kzg_commitment_inclusion_proof = FixedVector::random_for_test(&mut thread_rng());
+            assert!(!blob.verify_blob_sidecar_inclusion_proof());
+        }
+    }
+}

consensus/types/src/test_utils/mod.rs

@@ -15,6 +15,8 @@ use tree_hash::TreeHash;
 #[macro_use]
 mod macros;
 mod generate_deterministic_keypairs;
+#[cfg(test)]
+mod generate_random_block_and_blobs;
 mod test_random;
 
 pub fn test_ssz_tree_hash_pair<T, U>(v1: &T, v2: &U)