From 2550170337da697bdb92eae472d1fede32ff23e7 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 6 Sep 2023 05:50:57 +1000 Subject: [PATCH] Deneb review suggestions (3) (#4694) * Fix typos * Avoid consuming/cloning blob * Tidy comments --- beacon_node/beacon_chain/src/beacon_chain.rs | 3 +++ beacon_node/beacon_chain/src/blob_verification.rs | 9 +++++---- beacon_node/beacon_chain/src/block_verification.rs | 2 +- .../beacon_chain/src/block_verification_types.rs | 4 ++-- beacon_node/beacon_chain/src/kzg_utils.rs | 14 +++++++------- .../src/cases/kzg_blob_to_kzg_commitment.rs | 2 +- .../ef_tests/src/cases/kzg_compute_kzg_proof.rs | 2 +- 7 files changed, 20 insertions(+), 16 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 03d816db22..45acab478a 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -475,7 +475,10 @@ pub struct BeaconChain { pub validator_monitor: RwLock>, /// The slot at which blocks are downloaded back to. pub genesis_backfill_slot: Slot, + // Provides a KZG verification and temporary storage for blocks and blobs as + // they are collected and combined. pub data_availability_checker: Arc>, + /// The KZG trusted setup used by this chain. pub kzg: Option::Kzg>>>, } diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index ae66b093be..ed07e9176a 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -35,12 +35,13 @@ pub enum GossipBlobError { latest_permissible_slot: Slot, }, - /// There was an error whilst processing the sync contribution. It is not known if it is valid or invalid. + /// There was an error whilst processing the blob. It is not known if it is + /// valid or invalid. /// /// ## Peer scoring /// - /// We were unable to process this sync committee message due to an internal error. It's unclear if the - /// sync committee message is valid. + /// We were unable to process this blob due to an internal error. It's + /// unclear if the blob is valid. BeaconChainError(BeaconChainError), /// The `BlobSidecar` was gossiped over an incorrect subnet. @@ -80,7 +81,7 @@ pub enum GossipBlobError { /// /// ## Peer scoring /// - /// The block is invalid and the peer is faulty. + /// The blob is invalid and the peer is faulty. UnknownValidator(u64), /// The provided blob is not from a later slot than its parent. diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 615e0c472d..cb7219ea4f 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -758,7 +758,7 @@ impl GossipVerifiedBlock { /// Instantiates `Self`, a wrapper that indicates the given `block` is safe to be re-gossiped /// on the p2p network. /// - /// Returns an error if the block is invalid, or i8f the block was unable to be verified. + /// Returns an error if the block is invalid, or if the block was unable to be verified. pub fn new( block: Arc>, chain: &BeaconChain, diff --git a/beacon_node/beacon_chain/src/block_verification_types.rs b/beacon_node/beacon_chain/src/block_verification_types.rs index 587fd2d1c5..b20bb4f0ef 100644 --- a/beacon_node/beacon_chain/src/block_verification_types.rs +++ b/beacon_node/beacon_chain/src/block_verification_types.rs @@ -24,7 +24,7 @@ use types::{ /// 1. `BlockAndBlobs`: A fully available post deneb block with all the blobs available. This variant /// is only constructed after making consistency checks between blocks and blobs. /// Hence, it is fully self contained w.r.t verification. i.e. this block has all the required -/// data to get verfied and imported into fork choice. +/// data to get verified and imported into fork choice. /// /// 2. `Block`: This can be a fully available pre-deneb block **or** a post-deneb block that may or may /// not require blobs to be considered fully available. @@ -118,7 +118,7 @@ impl From> for RpcBlock { } /// A block that has gone through all pre-deneb block processing checks including block processing -/// and execution by an EL client. This block hasn't completed data availability checks. +/// and execution by an EL client. This block hasn't necessarily completed data availability checks. /// /// /// It contains 2 variants: diff --git a/beacon_node/beacon_chain/src/kzg_utils.rs b/beacon_node/beacon_chain/src/kzg_utils.rs index c4a05ee666..8898ab83b4 100644 --- a/beacon_node/beacon_chain/src/kzg_utils.rs +++ b/beacon_node/beacon_chain/src/kzg_utils.rs @@ -4,9 +4,9 @@ use types::{Blob, EthSpec, Hash256, KzgCommitment, KzgProof}; /// Converts a blob ssz List object to an array to be used with the kzg /// crypto library. fn ssz_blob_to_crypto_blob( - blob: Blob, + blob: &Blob, ) -> Result<<::Kzg as KzgPreset>::Blob, KzgError> { - T::blob_from_bytes(blob.to_vec().as_slice()) + T::blob_from_bytes(blob.as_ref()) } /// Validate a single blob-commitment-proof triplet from a `BlobSidecar`. @@ -17,7 +17,7 @@ pub fn validate_blob( kzg_proof: KzgProof, ) -> Result { kzg.verify_blob_kzg_proof( - ssz_blob_to_crypto_blob::(blob)?, + ssz_blob_to_crypto_blob::(&blob)?, kzg_commitment, kzg_proof, ) @@ -32,7 +32,7 @@ pub fn validate_blobs( ) -> Result { let blobs = blobs .iter() - .map(|blob| ssz_blob_to_crypto_blob::(blob.clone())) // Avoid this clone + .map(|blob| ssz_blob_to_crypto_blob::(blob)) // Avoid this clone .collect::, KzgError>>()?; kzg.verify_blob_kzg_proof_batch(&blobs, expected_kzg_commitments, kzg_proofs) @@ -45,13 +45,13 @@ pub fn compute_blob_kzg_proof( kzg_commitment: KzgCommitment, ) -> Result { // Avoid this blob clone - kzg.compute_blob_kzg_proof(ssz_blob_to_crypto_blob::(blob.clone())?, kzg_commitment) + kzg.compute_blob_kzg_proof(ssz_blob_to_crypto_blob::(blob)?, kzg_commitment) } /// Compute the kzg commitment for a given blob. pub fn blob_to_kzg_commitment( kzg: &Kzg, - blob: Blob, + blob: &Blob, ) -> Result { kzg.blob_to_kzg_commitment(ssz_blob_to_crypto_blob::(blob)?) } @@ -59,7 +59,7 @@ pub fn blob_to_kzg_commitment( /// Compute the kzg proof for a given blob and an evaluation point z. pub fn compute_kzg_proof( kzg: &Kzg, - blob: Blob, + blob: &Blob, z: Hash256, ) -> Result<(KzgProof, Hash256), KzgError> { let z = z.0.into(); diff --git a/testing/ef_tests/src/cases/kzg_blob_to_kzg_commitment.rs b/testing/ef_tests/src/cases/kzg_blob_to_kzg_commitment.rs index bb37b47903..ec2f1b1694 100644 --- a/testing/ef_tests/src/cases/kzg_blob_to_kzg_commitment.rs +++ b/testing/ef_tests/src/cases/kzg_blob_to_kzg_commitment.rs @@ -35,7 +35,7 @@ impl Case for KZGBlobToKZGCommitment { let kzg = get_kzg::()?; let commitment = parse_blob::(&self.input.blob).and_then(|blob| { - blob_to_kzg_commitment::(&kzg, blob).map_err(|e| { + blob_to_kzg_commitment::(&kzg, &blob).map_err(|e| { Error::InternalError(format!("Failed to compute kzg commitment: {:?}", e)) }) }); diff --git a/testing/ef_tests/src/cases/kzg_compute_kzg_proof.rs b/testing/ef_tests/src/cases/kzg_compute_kzg_proof.rs index 1c9ca6a588..49cd2d0b88 100644 --- a/testing/ef_tests/src/cases/kzg_compute_kzg_proof.rs +++ b/testing/ef_tests/src/cases/kzg_compute_kzg_proof.rs @@ -48,7 +48,7 @@ impl Case for KZGComputeKZGProof { let kzg = get_kzg::()?; let proof = parse_input(&self.input).and_then(|(blob, z)| { - compute_kzg_proof::(&kzg, blob, z) + compute_kzg_proof::(&kzg, &blob, z) .map_err(|e| Error::InternalError(format!("Failed to compute kzg proof: {:?}", e))) });