Merge pull request #130 from chainbound/feat/invalidate-state

chore: add commitment validity tests

bolt-sidecar/bin/sidecar.rs

@@ -73,19 +73,16 @@ async fn main() -> eyre::Result<()> {
                 let validator_index = match consensus_state.validate_request(&request) {
                     Ok(index) => index,
                     Err(e) => {
-                        tracing::error!("Failed to validate request: {:?}", e);
+                        tracing::error!(err = ?e, "Failed to validate request");
                         let _ = response_tx.send(Err(ApiError::Custom(e.to_string())));
                         continue;
                     }
                 };
 
-                let sender = match execution_state
-                    .validate_commitment_request(&request)
-                .await
-                {
+                let sender = match execution_state.validate_commitment_request(&request).await {
                     Ok(sender) => sender,
                     Err(e) => {
-                        tracing::error!("Failed to commit request: {:?}", e);
+                        tracing::error!(err = ?e, "Failed to commit request");
                         let _ = response_tx.send(Err(ApiError::Custom(e.to_string())));
                         continue;
                     }

bolt-sidecar/src/client/rpc.rs

@@ -10,7 +10,7 @@ use std::{
 
 use alloy::ClientBuilder;
 use alloy_eips::BlockNumberOrTag;
-use alloy_primitives::{Address, B256, U256, U64};
+use alloy_primitives::{Address, Bytes, B256, U256, U64};
 use alloy_rpc_client::{self as alloy, Waiter};
 use alloy_rpc_types::{Block, EIP1186AccountProofResponse, FeeHistory, TransactionRequest};
 use alloy_rpc_types_trace::parity::{TraceResults, TraceType};
@@ -100,16 +100,22 @@ impl RpcClient {
             .add_call("eth_getTransactionCount", &(address, tag))
             .expect("Correct parameters");
 
+        let code = batch
+            .add_call("eth_getCode", &(address, tag))
+            .expect("Correct parameters");
+
         // After the batch is complete, we can get the results.
         // Note that requests may error separately!
         batch.send().await?;
 
         let tx_count: U64 = tx_count.await?;
         let balance: U256 = balance.await?;
+        let code: Bytes = code.await?;
 
         Ok(AccountState {
             balance,
             transaction_count: tx_count.to(),
+            has_code: !code.is_empty(),
         })
     }
 
@@ -260,4 +266,18 @@ mod tests {
 
         Ok(())
     }
+
+    #[tokio::test]
+    async fn test_smart_contract_code() -> eyre::Result<()> {
+        let rpc_url = Url::parse("https://cloudflare-eth.com")?;
+        let rpc_client = RpcClient::new(rpc_url);
+
+        // random deployed smart contract address
+        let addr = Address::from_str("0xBA12222222228d8Ba445958a75a0704d566BF2C8")?;
+        let account = rpc_client.get_account_state(&addr, None).await?;
+
+        assert!(account.has_code);
+
+        Ok(())
+    }
 }

bolt-sidecar/src/common.rs

@@ -59,6 +59,11 @@ pub fn validate_transaction(
         return Err(ValidationError::InsufficientBalance);
     }
 
+    // Check if the account has code (i.e. is a smart contract)
+    if account_state.has_code {
+        return Err(ValidationError::AccountHasCode);
+    }
+
     Ok(())
 }
 

bolt-sidecar/src/primitives/commitment.rs

@@ -14,6 +14,18 @@ pub enum CommitmentRequest {
     Inclusion(InclusionRequest),
 }
 
+impl CommitmentRequest {
+    /// Returns a reference to the inner request if this is an inclusion request, otherwise `None`.
+    pub fn as_inclusion_request(&self) -> Option<&InclusionRequest> {
+        match self {
+            CommitmentRequest::Inclusion(req) => Some(req),
+            // TODO: remove this when we have more request types
+            #[allow(unreachable_patterns)]
+            _ => None,
+        }
+    }
+}
+
 /// Request to include a transaction at a specific slot.
 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
 pub struct InclusionRequest {

bolt-sidecar/src/primitives/mod.rs

@@ -17,7 +17,9 @@ use ethereum_consensus::{
     types::mainnet::ExecutionPayload,
     Fork,
 };
-use reth_primitives::{BlobTransactionSidecar, PooledTransactionsElement, TxType};
+use reth_primitives::{
+    BlobTransactionSidecar, Bytes, PooledTransactionsElement, TransactionKind, TxType,
+};
 use tokio::sync::{mpsc, oneshot};
 
 /// Commitment types, received by users wishing to receive preconfirmations.
@@ -37,7 +39,10 @@ pub type Slot = u64;
 pub struct AccountState {
     /// The nonce of the account. This is the number of transactions sent from this account
     pub transaction_count: u64,
+    /// The balance of the account in wei
     pub balance: U256,
+    /// Flag to indicate if the account is a smart contract or an EOA
+    pub has_code: bool,
 }
 
 #[derive(Debug, Default, Clone, SimpleSerialize, serde::Serialize, serde::Deserialize)]
@@ -227,8 +232,11 @@ pub trait TransactionExt {
     fn gas_limit(&self) -> u64;
     fn value(&self) -> U256;
     fn tx_type(&self) -> TxType;
+    fn tx_kind(&self) -> TransactionKind;
+    fn input(&self) -> &Bytes;
     fn chain_id(&self) -> Option<u64>;
     fn blob_sidecar(&self) -> Option<&BlobTransactionSidecar>;
+    fn size(&self) -> usize;
 }
 
 impl TransactionExt for PooledTransactionsElement {
@@ -259,6 +267,24 @@ impl TransactionExt for PooledTransactionsElement {
         }
     }
 
+    fn tx_kind(&self) -> TransactionKind {
+        match self {
+            PooledTransactionsElement::Legacy { transaction, .. } => transaction.to,
+            PooledTransactionsElement::Eip2930 { transaction, .. } => transaction.to,
+            PooledTransactionsElement::Eip1559 { transaction, .. } => transaction.to,
+            PooledTransactionsElement::BlobTransaction(blob_tx) => blob_tx.transaction.to,
+        }
+    }
+
+    fn input(&self) -> &Bytes {
+        match self {
+            PooledTransactionsElement::Legacy { transaction, .. } => &transaction.input,
+            PooledTransactionsElement::Eip2930 { transaction, .. } => &transaction.input,
+            PooledTransactionsElement::Eip1559 { transaction, .. } => &transaction.input,
+            PooledTransactionsElement::BlobTransaction(blob_tx) => &blob_tx.transaction.input,
+        }
+    }
+
     fn chain_id(&self) -> Option<u64> {
         match self {
             PooledTransactionsElement::Legacy { transaction, .. } => transaction.chain_id,
@@ -276,4 +302,13 @@ impl TransactionExt for PooledTransactionsElement {
             _ => None,
         }
     }
+
+    fn size(&self) -> usize {
+        match self {
+            PooledTransactionsElement::Legacy { transaction, .. } => transaction.size(),
+            PooledTransactionsElement::Eip2930 { transaction, .. } => transaction.size(),
+            PooledTransactionsElement::Eip1559 { transaction, .. } => transaction.size(),
+            PooledTransactionsElement::BlobTransaction(blob_tx) => blob_tx.transaction.size(),
+        }
+    }
 }

bolt-sidecar/src/state/execution.rs

@@ -10,7 +10,7 @@ use thiserror::Error;
 use crate::{
     builder::BlockTemplate,
     common::{calculate_max_basefee, validate_transaction},
-    primitives::{AccountState, CommitmentRequest, SignedConstraints, Slot},
+    primitives::{AccountState, CommitmentRequest, SignedConstraints, Slot, TransactionExt},
 };
 
 use super::fetcher::StateFetcher;
@@ -27,12 +27,27 @@ pub enum ValidationError {
     /// The transaction blob is invalid.
     #[error(transparent)]
     BlobValidation(#[from] BlobTransactionValidationError),
+    /// The max basefee calculation incurred an overflow error.
+    #[error("Invalid max basefee calculation: overflow")]
+    MaxBaseFeeCalcOverflow,
     /// The transaction nonce is too low.
     #[error("Transaction nonce too low")]
     NonceTooLow,
     /// The transaction nonce is too high.
     #[error("Transaction nonce too high")]
     NonceTooHigh,
+    /// The sender account is a smart contract and has code.
+    #[error("Account has code")]
+    AccountHasCode,
+    /// The gas limit is too high.
+    #[error("Gas limit too high")]
+    GasLimitTooHigh,
+    /// The transaction input size is too high.
+    #[error("Transaction input size too high")]
+    TransactionSizeTooHigh,
+    /// Max priority fee per gas is greater than max fee per gas.
+    #[error("Max priority fee per gas is greater than max fee per gas")]
+    MaxPriorityFeePerGasTooHigh,
     /// The sender does not have enough balance to pay for the transaction.
     #[error("Not enough balance to pay for value + maximum fee")]
     InsufficientBalance,
@@ -87,7 +102,6 @@ pub struct ExecutionState<C> {
     /// These only contain the canonical account states at the head block,
     /// not the intermediate states.
     account_states: HashMap<Address, AccountState>,
-
     /// The block templates by target SLOT NUMBER.
     /// We have multiple block templates because in rare cases we might have multiple
     /// proposal duties for a single lookahead.
@@ -100,6 +114,26 @@ pub struct ExecutionState<C> {
     kzg_settings: EnvKzgSettings,
     /// The state fetcher client.
     client: C,
+    /// Other values used for validation
+    validation_params: ValidationParams,
+}
+
+/// Other values used for validation.
+#[derive(Debug)]
+pub struct ValidationParams {
+    block_gas_limit: u64,
+    max_tx_input_bytes: usize,
+    max_init_code_byte_size: usize,
+}
+
+impl Default for ValidationParams {
+    fn default() -> Self {
+        Self {
+            block_gas_limit: 30_000_000,
+            max_tx_input_bytes: 4 * 32 * 1024,
+            max_init_code_byte_size: 2 * 24576,
+        }
+    }
 }
 
 impl<C: StateFetcher> ExecutionState<C> {
@@ -109,21 +143,27 @@ impl<C: StateFetcher> ExecutionState<C> {
         client: C,
         max_commitments_per_slot: NonZero<usize>,
     ) -> Result<Self, TransportError> {
-        let (basefee, blob_basefee) =
-            tokio::try_join!(client.get_basefee(None), client.get_blob_basefee(None))?;
+        let (basefee, blob_basefee, block_number, chain_id) = tokio::try_join!(
+            client.get_basefee(None),
+            client.get_blob_basefee(None),
+            client.get_head(),
+            client.get_chain_id()
+        )?;
 
         Ok(Self {
             basefee,
             blob_basefee,
-            block_number: client.get_head().await?,
+            block_number,
+            chain_id,
+            max_commitments_per_slot,
+            client,
             slot: 0,
             account_states: HashMap::new(),
             block_templates: HashMap::new(),
-            chain_id: client.get_chain_id().await?,
             // Load the default KZG settings
             kzg_settings: EnvKzgSettings::default(),
-            max_commitments_per_slot,
-            client,
+            // TODO: add a way to configure these values from CLI
+            validation_params: ValidationParams::default(),
         })
     }
 
@@ -138,6 +178,7 @@ impl<C: StateFetcher> ExecutionState<C> {
     }
 
     /// Validates the commitment request against state (historical + intermediate).
+    ///
     /// NOTE: This function only simulates against execution state, it does not consider
     /// timing or proposer slot targets.
     ///
@@ -168,22 +209,49 @@ impl<C: StateFetcher> ExecutionState<C> {
             }
         }
 
-        let sender = req.tx.recover_signer().ok_or(ValidationError::Internal(
-            "Failed to recover signer from transaction".to_string(),
-        ))?;
+        // Check if the transaction size exceeds the maximum
+        if req.tx.size() > self.validation_params.max_tx_input_bytes {
+            return Err(ValidationError::TransactionSizeTooHigh);
+        }
+
+        // Check if the transaction is a contract creation and the init code size exceeds the maximum
+        if req.tx.tx_kind().is_create()
+            && req.tx.input().len() > self.validation_params.max_init_code_byte_size
+        {
+            return Err(ValidationError::TransactionSizeTooHigh);
+        }
+
+        // Check if the gas limit is higher than the maximum block gas limit
+        if req.tx.gas_limit() > self.validation_params.block_gas_limit {
+            return Err(ValidationError::GasLimitTooHigh);
+        }
+
+        // Ensure max_priority_fee_per_gas is less than max_fee_per_gas, if any
+        if req
+            .tx
+            .max_priority_fee_per_gas()
+            .is_some_and(|max_priority_fee| max_priority_fee > req.tx.max_fee_per_gas())
+        {
+            return Err(ValidationError::MaxPriorityFeePerGasTooHigh);
+        }
+
+        let sender = req
+            .tx
+            .recover_signer()
+            .ok_or(ValidationError::RecoverSigner)?;
 
         tracing::debug!(%sender, target_slot = req.slot, "Trying to commit inclusion request to block template");
 
         // Check if the max_fee_per_gas would cover the maximum possible basefee.
-        let slot_diff = req.slot - self.slot;
+        let slot_diff = req.slot.saturating_sub(self.slot);
 
         // Calculate the max possible basefee given the slot diff
         let max_basefee = calculate_max_basefee(self.basefee, slot_diff)
-            .ok_or(reject_internal("Overflow calculating max basefee"))?;
+            .ok_or(ValidationError::MaxBaseFeeCalcOverflow)?;
 
         // Validate the base fee
         if !req.validate_basefee(max_basefee) {
-            return Err(ValidationError::BaseFeeTooLow(max_basefee as u128));
+            return Err(ValidationError::BaseFeeTooLow(max_basefee));
         }
 
         // If we have the account state, use it here
@@ -194,11 +262,13 @@ impl<C: StateFetcher> ExecutionState<C> {
         } else {
             tracing::debug!(address = %sender, "Unknown account state");
             // If we don't have the account state, we need to fetch it
-            let account_state = self
-                .client
-                .get_account_state(&sender, None)
-                .await
-                .map_err(|e| reject_internal(&e.to_string()))?;
+            let account_state =
+                self.client
+                    .get_account_state(&sender, None)
+                    .await
+                    .map_err(|e| {
+                        ValidationError::Internal(format!("Failed to fetch account state: {:?}", e))
+                    })?;
 
             tracing::debug!(address = %sender, "Fetched account state: {account_state:?}");
 
@@ -223,7 +293,7 @@ impl<C: StateFetcher> ExecutionState<C> {
 
             // Calculate max possible increase in blob basefee
             let max_blob_basefee = calculate_max_basefee(self.blob_basefee, slot_diff)
-                .ok_or(reject_internal("Overflow calculating max blob basefee"))?;
+                .ok_or(ValidationError::MaxBaseFeeCalcOverflow)?;
 
             if blob_transaction.transaction.max_fee_per_blob_gas < max_blob_basefee {
                 return Err(ValidationError::BlobBaseFeeTooLow(max_blob_basefee));
@@ -259,7 +329,6 @@ impl<C: StateFetcher> ExecutionState<C> {
     ) -> Result<(), TransportError> {
         self.slot = slot;
 
-        // TODO: invalidate any state that we don't need anymore (will be based on block template)
         let accounts = self.account_states.keys().collect::<Vec<_>>();
         let update = self.client.get_state_update(accounts, block_number).await?;
 
@@ -338,7 +407,3 @@ pub struct StateUpdate {
     pub min_blob_basefee: u128,
     pub block_number: u64,
 }
-
-fn reject_internal(reason: &str) -> ValidationError {
-    ValidationError::Internal(reason.to_string())
-}