From 474c1b44863927c588dd05ab2ac0f934298398e1 Mon Sep 17 00:00:00 2001 From: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 17 Jun 2024 17:05:24 +0200 Subject: [PATCH] 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_chain/src/blob_verification.rs | 13 +-- .../gossip_methods.rs | 1 - .../src/sync/network_context/requests.rs | 2 +- consensus/types/src/blob_sidecar.rs | 31 +++--- consensus/types/src/eth_spec.rs | 39 ++++++++ .../generate_random_block_and_blobs.rs | 96 +++++++++++++++++++ consensus/types/src/test_utils/mod.rs | 2 + 7 files changed, 153 insertions(+), 31 deletions(-) create mode 100644 consensus/types/src/test_utils/generate_random_block_and_blobs.rs diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index fdf8ee2b971..2b62a83194b 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/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 { /// 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( // Verify the inclusion proof in the sidecar let _timer = metrics::start_timer(&metrics::BLOB_SIDECAR_INCLUSION_PROOF_VERIFICATION); - if !blob_sidecar - .verify_blob_sidecar_inclusion_proof() - .map_err(GossipBlobError::InclusionProof)? - { + if !blob_sidecar.verify_blob_sidecar_inclusion_proof() { return Err(GossipBlobError::InvalidInclusionProof); } drop(_timer); diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index 374dca2a5ab..99622bfa306 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -690,7 +690,6 @@ impl NetworkBeaconProcessor { | GossipBlobError::InvalidSubnet { .. } | GossipBlobError::InvalidInclusionProof | GossipBlobError::KzgError(_) - | GossipBlobError::InclusionProof(_) | GossipBlobError::NotFinalizedDescendant { .. } => { warn!( self.log, diff --git a/beacon_node/network/src/sync/network_context/requests.rs b/beacon_node/network/src/sync/network_context/requests.rs index cd73b4bebab..6e4683701bd 100644 --- a/beacon_node/network/src/sync/network_context/requests.rs +++ b/beacon_node/network/src/sync/network_context/requests.rs @@ -120,7 +120,7 @@ impl ActiveBlobsByRootRequest { 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) { diff --git a/consensus/types/src/blob_sidecar.rs b/consensus/types/src/blob_sidecar.rs index e54bc2f4f97..50530543a56 100644 --- a/consensus/types/src/blob_sidecar.rs +++ b/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 BlobSidecar { } /// Verifies the kzg commitment inclusion merkle proof. - pub fn verify_blob_sidecar_inclusion_proof(&self) -> Result { - // 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 - let kzg_commitments_tree_depth = (E::max_blob_commitments_per_block() - .next_power_of_two() - .ilog2() - .safe_add(1))? as usize; + pub fn verify_blob_sidecar_inclusion_proof(&self) -> bool { + let kzg_commitments_tree_depth = E::kzg_commitments_tree_depth(); + + // EthSpec asserts that kzg_commitments_tree_depth is less than KzgCommitmentInclusionProofDepth + let (kzg_commitment_subtree_proof, kzg_commitments_proof) = self + .kzg_commitment_inclusion_proof + .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 - .get(0..kzg_commitments_tree_depth) - .ok_or(MerkleTreeError::PleaseNotifyTheDevs)?, + kzg_commitment_subtree_proof, 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 - .get(kzg_commitments_tree_depth..E::kzg_proof_inclusion_proof_depth()) - .ok_or(MerkleTreeError::PleaseNotifyTheDevs)?, - E::kzg_proof_inclusion_proof_depth().safe_sub(kzg_commitments_tree_depth)?, + kzg_commitments_proof, + E::block_body_tree_depth(), BLOB_KZG_COMMITMENTS_INDEX, self.signed_block_header.message.body_root, - )) + ) } pub fn random_valid(rng: &mut R, kzg: &Kzg) -> Result { diff --git a/consensus/types/src/eth_spec.rs b/consensus/types/src/eth_spec.rs index 62f7f1b8698..38a048d4690 100644 --- a/consensus/types/src/eth_spec.rs +++ b/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::kzg_commitments_tree_depth(); + E::block_body_tree_depth(); + } + + #[test] + fn mainnet_spec() { + assert_valid_spec::(); + } + #[test] + fn minimal_spec() { + assert_valid_spec::(); + } + #[test] + fn gnosis_spec() { + assert_valid_spec::(); + } +} diff --git a/consensus/types/src/test_utils/generate_random_block_and_blobs.rs b/consensus/types/src/test_utils/generate_random_block_and_blobs.rs new file mode 100644 index 00000000000..ab7ded04090 --- /dev/null +++ b/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 = (KzgCommitments, KzgProofs, BlobsList); + +pub fn generate_rand_block_and_blobs( + fork_name: ForkName, + num_blobs: usize, + rng: &mut impl Rng, +) -> (SignedBeaconBlock>, Vec>) { + 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::(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(n_blobs: usize) -> Result, String> { + let (mut commitments, mut proofs, mut blobs) = BlobsBundle::::default(); + + for blob_index in 0..n_blobs { + blobs + .push(Blob::::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::(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::(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()); + } + } +} diff --git a/consensus/types/src/test_utils/mod.rs b/consensus/types/src/test_utils/mod.rs index d172342ee64..9599bcd3641 100644 --- a/consensus/types/src/test_utils/mod.rs +++ b/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(v1: &T, v2: &U)