From cb78f2f8df6b91bd63c7c0ad987e29384083d14e Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Mon, 28 Nov 2022 20:23:15 +0530 Subject: [PATCH] Add more kzg validations --- beacon_node/beacon_chain/src/beacon_chain.rs | 2 +- .../beacon_chain/src/blob_verification.rs | 87 +++++++++++++++---- .../beacon_chain/src/block_verification.rs | 24 ++++- beacon_node/beacon_chain/src/kzg_utils.rs | 2 +- .../per_block_processing/eip4844/eip4844.rs | 3 +- crypto/kzg/src/lib.rs | 1 + 6 files changed, 94 insertions(+), 25 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 15167bfa3..92b1e06c5 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -3967,7 +3967,7 @@ impl BeaconChain { slot, beacon_block_root, expected_kzg_commitments, - blobs_sidecar.clone(), + &blobs_sidecar, ) .map_err(BlockProductionError::KzgError)?; self.blob_cache.put(beacon_block_root, blobs_sidecar); diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index a4e7115d0..8756766c4 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -1,11 +1,9 @@ -use derivative::Derivative; use slot_clock::SlotClock; -use std::sync::Arc; use crate::beacon_chain::{BeaconChain, BeaconChainTypes, MAXIMUM_GOSSIP_CLOCK_DISPARITY}; -use crate::BeaconChainError; -use bls::PublicKey; -use types::{consts::eip4844::BLS_MODULUS, BeaconStateError, BlobsSidecar, Hash256, Slot}; +use crate::{kzg_utils, BeaconChainError}; +use state_processing::per_block_processing::eip4844::eip4844::verify_kzg_commitments_against_transactions; +use types::{BeaconStateError, BlobsSidecar, Hash256, KzgCommitment, Slot, Transactions}; #[derive(Debug)] pub enum BlobError { @@ -36,7 +34,9 @@ pub enum BlobError { /// ## Peer scoring /// /// The peer has sent an invalid message. - BlobOutOfRange { blob_index: usize }, + BlobOutOfRange { + blob_index: usize, + }, /// The blob sidecar contains a KZGCommitment that is not a valid G1 point on /// the bls curve. @@ -52,13 +52,31 @@ pub enum BlobError { /// The signature on the blob sidecar invalid and the peer is faulty. ProposalSignatureInvalid, + /// No kzg ccommitment associated with blob sidecar. + KzgCommitmentMissing, + + /// No transactions in block + TransactionsMissing, + + /// Blob transactions in the block do not correspond to the kzg commitments. + TransactionCommitmentMismatch, + + TrustedSetupNotInitialized, + + InvalidKzgProof, + + KzgError(String), + /// A blob sidecar for this proposer and slot has already been observed. /// /// ## Peer scoring /// /// The `proposer` has already proposed a sidecar at this slot. The existing sidecar may or may not /// be equal to the given sidecar. - RepeatSidecar { proposer: u64, slot: Slot }, + RepeatSidecar { + proposer: u64, + slot: Slot, + }, /// There was an error whilst processing the sync contribution. It is not known if it is valid or invalid. /// @@ -83,6 +101,10 @@ impl From for BlobError { pub fn validate_blob_for_gossip( blob_sidecar: &BlobsSidecar, + kzg_commitments: &[KzgCommitment], + transactions: &Transactions, + block_slot: Slot, + block_root: Hash256, chain: &BeaconChain, ) -> Result<(), BlobError> { let blob_slot = blob_sidecar.beacon_block_slot; @@ -109,19 +131,46 @@ pub fn validate_blob_for_gossip( }); } - // Verify that blobs are properly formatted - //TODO: add the check while constructing a Blob type from bytes instead of after - // for (i, blob) in blob_sidecar.blobs.iter().enumerate() { - // if blob.iter().any(|b| *b >= *BLS_MODULUS) { - // return Err(BlobError::BlobOutOfRange { blob_index: i }); - // } - // } - - // Verify that the KZG proof is a valid G1 point - if PublicKey::deserialize(&blob_sidecar.kzg_aggregated_proof.0).is_err() { - return Err(BlobError::InvalidKZGCommitment); + // Verify that kzg commitments in the block are valid BLS g1 points + for commitment in kzg_commitments { + if kzg::bytes_to_g1(&commitment.0).is_err() { + return Err(BlobError::InvalidKZGCommitment); + } } - // TODO: `validate_blobs_sidecar` + // Validate commitments agains transactions in the block. + if verify_kzg_commitments_against_transactions::(transactions, kzg_commitments) + .is_err() + { + return Err(BlobError::TransactionCommitmentMismatch); + } + + // Check that blobs are < BLS_MODULUS + // TODO(pawan): Add this check after there's some resolution of this + // issue https://github.com/ethereum/c-kzg-4844/issues/11 + // As of now, `bytes_to_bls_field` does not fail in the c-kzg library if blob >= BLS_MODULUS + + // Validate that kzg proof is a valid g1 point + if kzg::bytes_to_g1(&blob_sidecar.kzg_aggregated_proof.0).is_err() { + return Err(BlobError::InvalidKzgProof); + } + + // Validatate that the kzg proof is valid against the commitments and blobs + let kzg = chain + .kzg + .as_ref() + .ok_or(BlobError::TrustedSetupNotInitialized)?; + + if !kzg_utils::validate_blobs_sidecar( + kzg, + block_slot, + block_root, + kzg_commitments, + blob_sidecar, + ) + .map_err(BlobError::KzgError)? + { + return Err(BlobError::InvalidKzgProof); + } Ok(()) } diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index c311d5b54..71fbb4537 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -90,7 +90,7 @@ use types::{ EthSpec, ExecutionBlockHash, Hash256, InconsistentFork, PublicKey, PublicKeyBytes, RelativeEpoch, SignedBeaconBlock, SignedBeaconBlockHeader, Slot, }; -use types::{BlobsSidecar, ExecPayload, SignedBeaconBlockAndBlobsSidecar}; +use types::{BlobsSidecar, ExecPayload}; pub const POS_PANDA_BANNER: &str = r#" ,,, ,,, ,,, ,,, @@ -905,7 +905,27 @@ impl GossipVerifiedBlock { validate_execution_payload_for_gossip(&parent_block, block.message(), chain)?; if let Some(blobs_sidecar) = blobs.as_ref() { - validate_blob_for_gossip(blobs_sidecar, chain).map_err(BlobValidation)?; + let kzg_commitments = block + .message() + .body() + .blob_kzg_commitments() + .map_err(|_| BlockError::BlobValidation(BlobError::KzgCommitmentMissing))?; + let transactions = block + .message() + .body() + .execution_payload_eip4844() + .map(|payload| payload.transactions()) + .map_err(|_| BlockError::BlobValidation(BlobError::TransactionsMissing))? + .ok_or(BlockError::BlobValidation(BlobError::TransactionsMissing))?; + validate_blob_for_gossip( + blobs_sidecar, + kzg_commitments, + transactions, + block.slot(), + block_root, + chain, + ) + .map_err(BlobValidation)?; //FIXME(sean) validate blobs sidecar } diff --git a/beacon_node/beacon_chain/src/kzg_utils.rs b/beacon_node/beacon_chain/src/kzg_utils.rs index f26a441cd..3e181facf 100644 --- a/beacon_node/beacon_chain/src/kzg_utils.rs +++ b/beacon_node/beacon_chain/src/kzg_utils.rs @@ -17,7 +17,7 @@ pub fn validate_blobs_sidecar( slot: Slot, beacon_block_root: Hash256, expected_kzg_commitments: &[KzgCommitment], - blobs_sidecar: BlobsSidecar, + blobs_sidecar: &BlobsSidecar, ) -> Result { if slot != blobs_sidecar.beacon_block_slot || beacon_block_root != blobs_sidecar.beacon_block_root diff --git a/consensus/state_processing/src/per_block_processing/eip4844/eip4844.rs b/consensus/state_processing/src/per_block_processing/eip4844/eip4844.rs index 0998756fd..80f20660c 100644 --- a/consensus/state_processing/src/per_block_processing/eip4844/eip4844.rs +++ b/consensus/state_processing/src/per_block_processing/eip4844/eip4844.rs @@ -3,7 +3,6 @@ use eth2_hashing::hash_fixed; use itertools::{EitherOrBoth, Itertools}; use safe_arith::SafeArith; use ssz::Decode; -use ssz_types::VariableList; use types::consts::eip4844::{BLOB_TX_TYPE, VERSIONED_HASH_VERSION_KZG}; use types::{ AbstractExecPayload, BeaconBlockBodyRef, EthSpec, ExecPayload, KzgCommitment, Transaction, @@ -30,7 +29,7 @@ pub fn process_blob_kzg_commitments> pub fn verify_kzg_commitments_against_transactions( transactions: &Transactions, - kzg_commitments: &VariableList, + kzg_commitments: &[KzgCommitment], ) -> Result { let nested_iter = transactions .into_iter() diff --git a/crypto/kzg/src/lib.rs b/crypto/kzg/src/lib.rs index 8f068848c..40eee4711 100644 --- a/crypto/kzg/src/lib.rs +++ b/crypto/kzg/src/lib.rs @@ -2,6 +2,7 @@ mod kzg_commitment; mod kzg_proof; pub use crate::{kzg_commitment::KzgCommitment, kzg_proof::KzgProof}; +pub use c_kzg::bytes_to_g1; use c_kzg::{Error as CKzgError, KZGSettings, BYTES_PER_FIELD_ELEMENT, FIELD_ELEMENTS_PER_BLOB}; use std::path::PathBuf;