From 94954593ef616c1ba6717d4638c09f0d056b26cb Mon Sep 17 00:00:00 2001
From: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
Date: Fri, 15 Mar 2024 11:46:12 -0500
Subject: [PATCH] Check that `excess_blob_gas` is a multiple of
 `data_gas_per_blob` (#7160)

---
 crates/blockchain-tree/src/chain.rs       |  4 ++--
 crates/consensus/beacon-core/src/lib.rs   |  2 +-
 crates/consensus/common/src/validation.rs | 21 ++++++++++++++-------
 crates/interfaces/src/consensus.rs        | 11 +++++++++++
 4 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/crates/blockchain-tree/src/chain.rs b/crates/blockchain-tree/src/chain.rs
index 7d60514b8..01522c048 100644
--- a/crates/blockchain-tree/src/chain.rs
+++ b/crates/blockchain-tree/src/chain.rs
@@ -28,8 +28,8 @@ use std::{
     time::Instant,
 };
 
-/// A chain if the blockchain tree, that has functionality to execute blocks and append them to the
-/// it self.
+/// A chain in the blockchain tree that has functionality to execute blocks and append them to
+/// itself.
 #[derive(Clone, Debug, Default, PartialEq, Eq)]
 pub struct AppendableChain {
     chain: Chain,
diff --git a/crates/consensus/beacon-core/src/lib.rs b/crates/consensus/beacon-core/src/lib.rs
index 230278c88..e9b4114a3 100644
--- a/crates/consensus/beacon-core/src/lib.rs
+++ b/crates/consensus/beacon-core/src/lib.rs
@@ -83,7 +83,7 @@ impl Consensus for BeaconConsensus {
             //  * difficulty, mix_hash & nonce aka PoW stuff
             // low priority as syncing is done in reverse order
 
-            // Check if timestamp is in future. Clock can drift but this can be consensus issue.
+            // Check if timestamp is in the future. Clock can drift but this can be consensus issue.
             let present_timestamp =
                 SystemTime::now().duration_since(SystemTime::UNIX_EPOCH).unwrap().as_secs();
 
diff --git a/crates/consensus/common/src/validation.rs b/crates/consensus/common/src/validation.rs
index 8d50a8d70..030e085a2 100644
--- a/crates/consensus/common/src/validation.rs
+++ b/crates/consensus/common/src/validation.rs
@@ -15,7 +15,7 @@ pub fn validate_header_standalone(
     header: &SealedHeader,
     chain_spec: &ChainSpec,
 ) -> Result<(), ConsensusError> {
-    // Gas used needs to be less then gas limit. Gas used is going to be check after execution.
+    // Gas used needs to be less than gas limit. Gas used is going to be checked after execution.
     if header.gas_used > header.gas_limit {
         return Err(ConsensusError::HeaderGasUsedExceedsGasLimit {
             gas_used: header.gas_used,
@@ -55,7 +55,7 @@ pub fn validate_header_standalone(
     Ok(())
 }
 
-/// Validate a transaction in regards to a block header.
+/// Validate a transaction with regard to a block header.
 ///
 /// The only parameter from the header that affects the transaction is `base_fee`.
 pub fn validate_transaction_regarding_header(
@@ -248,7 +248,7 @@ pub fn validate_block_standalone(
     Ok(())
 }
 
-/// Validate block in regards to chain (parent)
+/// Validate block with regard to chain (parent)
 ///
 /// Checks:
 ///  If we already know the block.
@@ -282,12 +282,10 @@ pub fn validate_block_regarding_chain<PROV: HeaderProvider + WithdrawalsProvider
 ///  * `parent_beacon_block_root` exists as a header field
 ///  * `blob_gas_used` is less than or equal to `MAX_DATA_GAS_PER_BLOCK`
 ///  * `blob_gas_used` is a multiple of `DATA_GAS_PER_BLOB`
+///  * `excess_blob_gas` is a multiple of `DATA_GAS_PER_BLOB`
 pub fn validate_4844_header_standalone(header: &SealedHeader) -> Result<(), ConsensusError> {
     let blob_gas_used = header.blob_gas_used.ok_or(ConsensusError::BlobGasUsedMissing)?;
-
-    if header.excess_blob_gas.is_none() {
-        return Err(ConsensusError::ExcessBlobGasMissing)
-    }
+    let excess_blob_gas = header.excess_blob_gas.ok_or(ConsensusError::ExcessBlobGasMissing)?;
 
     if header.parent_beacon_block_root.is_none() {
         return Err(ConsensusError::ParentBeaconBlockRootMissing)
@@ -307,6 +305,15 @@ pub fn validate_4844_header_standalone(header: &SealedHeader) -> Result<(), Cons
         })
     }
 
+    // `excess_blob_gas` must also be a multiple of `DATA_GAS_PER_BLOB`. This will be checked later
+    // (via `calculate_excess_blob_gas`), but it doesn't hurt to catch the problem sooner.
+    if excess_blob_gas % DATA_GAS_PER_BLOB != 0 {
+        return Err(ConsensusError::ExcessBlobGasNotMultipleOfBlobGasPerBlob {
+            excess_blob_gas,
+            blob_gas_per_blob: DATA_GAS_PER_BLOB,
+        })
+    }
+
     Ok(())
 }
 
diff --git a/crates/interfaces/src/consensus.rs b/crates/interfaces/src/consensus.rs
index 2bad200c6..5ee2cac0e 100644
--- a/crates/interfaces/src/consensus.rs
+++ b/crates/interfaces/src/consensus.rs
@@ -222,6 +222,17 @@ pub enum ConsensusError {
         blob_gas_per_blob: u64,
     },
 
+    /// Error when excess blob gas is not a multiple of blob gas per blob.
+    #[error(
+    "excess blob gas {excess_blob_gas} is not a multiple of blob gas per blob {blob_gas_per_blob}"
+    )]
+    ExcessBlobGasNotMultipleOfBlobGasPerBlob {
+        /// The actual excess blob gas.
+        excess_blob_gas: u64,
+        /// The blob gas per blob.
+        blob_gas_per_blob: u64,
+    },
+
     /// Error when the blob gas used in the header does not match the expected blob gas used.
     #[error("blob gas used mismatch: {0}")]
     BlobGasUsedDiff(GotExpected<u64>),