From fdfeeb42dcd1a039a5f5d5f223095eeb4887cfa7 Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Fri, 10 Mar 2023 15:57:12 +0100 Subject: [PATCH] refactor(txpool): use transaction error type (#1698) --- Cargo.lock | 1 - crates/rpc/rpc/src/eth/error.rs | 32 ++--- crates/transaction-pool/Cargo.toml | 1 - crates/transaction-pool/src/error.rs | 57 +++++---- crates/transaction-pool/src/lib.rs | 36 +++--- crates/transaction-pool/src/pool/mod.rs | 7 +- crates/transaction-pool/src/pool/txpool.rs | 9 +- crates/transaction-pool/src/test_utils/mod.rs | 7 +- crates/transaction-pool/src/validate.rs | 118 ++++++++++++------ 9 files changed, 158 insertions(+), 110 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d66d9ac35c..6417ea64c8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5149,7 +5149,6 @@ dependencies = [ "parking_lot 0.12.1", "paste", "rand 0.8.5", - "reth-interfaces", "reth-metrics-derive", "reth-primitives", "reth-provider", diff --git a/crates/rpc/rpc/src/eth/error.rs b/crates/rpc/rpc/src/eth/error.rs index 8fe9eff1f4..2d5466417c 100644 --- a/crates/rpc/rpc/src/eth/error.rs +++ b/crates/rpc/rpc/src/eth/error.rs @@ -4,7 +4,7 @@ use crate::result::{internal_rpc_err, rpc_err}; use jsonrpsee::{core::Error as RpcError, types::error::INVALID_PARAMS_CODE}; use reth_primitives::{constants::SELECTOR_LEN, Address, U128, U256}; use reth_rpc_types::{error::EthRpcErrorCode, BlockError}; -use reth_transaction_pool::error::PoolError; +use reth_transaction_pool::error::{InvalidPoolTransactionError, PoolError}; use revm::primitives::{EVMError, Halt}; /// Result alias @@ -22,7 +22,7 @@ pub(crate) enum EthApiError { #[error("Invalid transaction signature")] InvalidTransactionSignature, #[error(transparent)] - PoolError(GethTxPoolError), + PoolError(RpcPoolError), #[error("Unknown block number")] UnknownBlockNumber, #[error("Invalid block range")] @@ -266,9 +266,9 @@ impl std::fmt::Display for RevertError { impl std::error::Error for RevertError {} -/// A helper error type that mirrors `geth` Txpool's error messages +/// A helper error type that's mainly used to mirror `geth` Txpool's error messages #[derive(Debug, thiserror::Error)] -pub(crate) enum GethTxPoolError { +pub(crate) enum RpcPoolError { #[error("already known")] AlreadyKnown, #[error("invalid sender")] @@ -285,26 +285,28 @@ pub(crate) enum GethTxPoolError { NegativeValue, #[error("oversized data")] OversizedData, + #[error(transparent)] + Invalid(#[from] InvalidPoolTransactionError), + #[error(transparent)] + Other(Box), } -impl From for GethTxPoolError { - fn from(err: PoolError) -> GethTxPoolError { +impl From for RpcPoolError { + fn from(err: PoolError) -> RpcPoolError { match err { - PoolError::ReplacementUnderpriced(_) => GethTxPoolError::ReplaceUnderpriced, - PoolError::ProtocolFeeCapTooLow(_, _) => GethTxPoolError::Underpriced, - PoolError::SpammerExceededCapacity(_, _) => GethTxPoolError::TxPoolOverflow, - PoolError::DiscardedOnInsert(_) => GethTxPoolError::TxPoolOverflow, - PoolError::TxExceedsGasLimit(_, _, _) => GethTxPoolError::GasLimit, - PoolError::TxExceedsMaxInitCodeSize(_, _, _) => GethTxPoolError::OversizedData, - PoolError::AccountNotFound(_) => GethTxPoolError::InvalidSender, - PoolError::OversizedData(_, _, _) => GethTxPoolError::OversizedData, + PoolError::ReplacementUnderpriced(_) => RpcPoolError::ReplaceUnderpriced, + PoolError::ProtocolFeeCapTooLow(_, _) => RpcPoolError::Underpriced, + PoolError::SpammerExceededCapacity(_, _) => RpcPoolError::TxPoolOverflow, + PoolError::DiscardedOnInsert(_) => RpcPoolError::TxPoolOverflow, + PoolError::InvalidTransaction(_, err) => err.into(), + PoolError::Other(_, err) => RpcPoolError::Other(err), } } } impl From for EthApiError { fn from(err: PoolError) -> Self { - EthApiError::PoolError(GethTxPoolError::from(err)) + EthApiError::PoolError(RpcPoolError::from(err)) } } diff --git a/crates/transaction-pool/Cargo.toml b/crates/transaction-pool/Cargo.toml index bde4c1da72..e04c502ec8 100644 --- a/crates/transaction-pool/Cargo.toml +++ b/crates/transaction-pool/Cargo.toml @@ -20,7 +20,6 @@ normal = [ # reth reth-primitives = { path = "../primitives" } reth-provider = { path = "../storage/provider" } -reth-interfaces = { path = "../interfaces" } reth-rlp = { path = "../rlp" } # async/futures diff --git a/crates/transaction-pool/src/error.rs b/crates/transaction-pool/src/error.rs index 4048a37c41..1a9a1e6b04 100644 --- a/crates/transaction-pool/src/error.rs +++ b/crates/transaction-pool/src/error.rs @@ -1,6 +1,6 @@ //! Transaction pool errors -use reth_primitives::{Address, TxHash}; +use reth_primitives::{Address, InvalidTransactionError, TxHash}; /// Transaction pool result type. pub type PoolResult = Result; @@ -21,22 +21,13 @@ pub enum PoolError { /// respect the size limits of the pool. #[error("[{0:?}] Transaction discarded outright due to pool size constraints.")] DiscardedOnInsert(TxHash), - /// Thrown when a new transaction is added to the pool, but then immediately discarded to - /// respect the size limits of the pool. - #[error("[{0:?}] Transaction's gas limit {1} exceeds block's gas limit {2}.")] - TxExceedsGasLimit(TxHash, u64, u64), - /// Thrown when a new transaction is added to the pool, but then immediately discarded to - /// respect the max_init_code_size. - #[error("[{0:?}] Transaction's size {1} exceeds max_init_code_size {2}.")] - TxExceedsMaxInitCodeSize(TxHash, usize, usize), - /// Thrown if the transaction contains an invalid signature - #[error("[{0:?}]: Invalid sender")] - AccountNotFound(TxHash), - /// Thrown if the input data of a transaction is greater - /// than some meaningful limit a user might use. This is not a consensus error - /// making the transaction invalid, rather a DOS protection. - #[error("[{0:?}]: Input data too large")] - OversizedData(TxHash, usize, usize), + /// Thrown when the transaction is considered invalid. + #[error("[{0:?}] {1:?}")] + InvalidTransaction(TxHash, InvalidPoolTransactionError), + /// Any other error that occurred while inserting/validating a transaction. e.g. IO database + /// error + #[error("[{0:?}] {1:?}")] + Other(TxHash, Box), } // === impl PoolError === @@ -49,10 +40,34 @@ impl PoolError { PoolError::ProtocolFeeCapTooLow(hash, _) => hash, PoolError::SpammerExceededCapacity(_, hash) => hash, PoolError::DiscardedOnInsert(hash) => hash, - PoolError::TxExceedsGasLimit(hash, _, _) => hash, - PoolError::TxExceedsMaxInitCodeSize(hash, _, _) => hash, - PoolError::AccountNotFound(hash) => hash, - PoolError::OversizedData(hash, _, _) => hash, + PoolError::InvalidTransaction(hash, _) => hash, + PoolError::Other(hash, _) => hash, } } } + +/// Represents errors that can happen when validating transactions for the pool +/// +/// See [TransactionValidator](crate::TransactionValidator). +#[derive(Debug, Clone, thiserror::Error)] +pub enum InvalidPoolTransactionError { + /// Hard consensus errors + #[error(transparent)] + Consensus(#[from] InvalidTransactionError), + /// Thrown when a new transaction is added to the pool, but then immediately discarded to + /// respect the size limits of the pool. + #[error("Transaction's gas limit {0} exceeds block's gas limit {1}.")] + ExceedsGasLimit(u64, u64), + /// Thrown when a new transaction is added to the pool, but then immediately discarded to + /// respect the max_init_code_size. + #[error("Transaction's size {0} exceeds max_init_code_size {1}.")] + ExceedsMaxInitCodeSize(usize, usize), + /// Thrown if the transaction contains an invalid signature + #[error("Invalid sender")] + AccountNotFound, + /// Thrown if the input data of a transaction is greater + /// than some meaningful limit a user might use. This is not a consensus error + /// making the transaction invalid, rather a DOS protection. + #[error("Input data too large")] + OversizedData(usize, usize), +} diff --git a/crates/transaction-pool/src/lib.rs b/crates/transaction-pool/src/lib.rs index 1304d607a2..d355e34d24 100644 --- a/crates/transaction-pool/src/lib.rs +++ b/crates/transaction-pool/src/lib.rs @@ -86,7 +86,10 @@ pub use crate::{ BestTransactions, OnNewBlockEvent, PoolTransaction, PooledTransaction, PropagateKind, PropagatedTransactions, TransactionOrigin, TransactionPool, }, - validate::{TransactionValidationOutcome, TransactionValidator, ValidPoolTransaction}, + validate::{ + EthTransactionValidator, TransactionValidationOutcome, TransactionValidator, + ValidPoolTransaction, + }, }; use crate::{ error::PoolResult, @@ -94,7 +97,7 @@ use crate::{ traits::{NewTransactionEvent, PoolSize}, }; -use reth_interfaces::consensus::ConsensusError; +use crate::error::PoolError; use reth_primitives::{TxHash, U256}; use std::{collections::HashMap, sync::Arc}; use tokio::sync::mpsc::Receiver; @@ -164,9 +167,7 @@ where &self, origin: TransactionOrigin, transactions: impl IntoIterator, - ) -> PoolResult< - HashMap, ConsensusError>>, - > { + ) -> PoolResult>> { let outcome = futures_util::future::join_all( transactions.into_iter().map(|tx| self.validate(origin, tx)), ) @@ -182,7 +183,7 @@ where &self, origin: TransactionOrigin, transaction: V::Transaction, - ) -> (TxHash, Result, ConsensusError>) { + ) -> (TxHash, TransactionValidationOutcome) { let hash = *transaction.hash(); // TODO(mattsse): this is where additional validate checks would go, like banned senders @@ -229,18 +230,14 @@ where let (_, tx) = self.validate(origin, transaction).await; match tx { - Ok(TransactionValidationOutcome::Valid { - balance: _, - state_nonce: _, - transaction: _, - }) => self - .pool - .add_transactions(origin, std::iter::once(tx.unwrap())) - .pop() - .expect("exists; qed"), - Ok(TransactionValidationOutcome::Invalid(_transaction, error)) => Err(error), - Err(_err) => { - unimplemented!() + TransactionValidationOutcome::Valid { .. } => { + self.pool.add_transactions(origin, std::iter::once(tx)).pop().expect("exists; qed") + } + TransactionValidationOutcome::Invalid(transaction, error) => { + Err(PoolError::InvalidTransaction(*transaction.hash(), error)) + } + TransactionValidationOutcome::Error(transaction, error) => { + Err(PoolError::Other(*transaction.hash(), error)) } } } @@ -252,8 +249,7 @@ where ) -> PoolResult>> { let validated = self.validate_all(origin, transactions).await?; - let transactions = - self.pool.add_transactions(origin, validated.into_values().map(|x| x.unwrap())); + let transactions = self.pool.add_transactions(origin, validated.into_values()); Ok(transactions) } diff --git a/crates/transaction-pool/src/pool/mod.rs b/crates/transaction-pool/src/pool/mod.rs index 6ef8fc5312..7ae9ee48af 100644 --- a/crates/transaction-pool/src/pool/mod.rs +++ b/crates/transaction-pool/src/pool/mod.rs @@ -232,7 +232,12 @@ where TransactionValidationOutcome::Invalid(tx, err) => { let mut listener = self.event_listener.write(); listener.discarded(tx.hash()); - Err(err) + Err(PoolError::InvalidTransaction(*tx.hash(), err)) + } + TransactionValidationOutcome::Error(tx, err) => { + let mut listener = self.event_listener.write(); + listener.discarded(tx.hash()); + Err(PoolError::Other(*tx.hash(), err)) } } } diff --git a/crates/transaction-pool/src/pool/txpool.rs b/crates/transaction-pool/src/pool/txpool.rs index 2dcf6f5d5b..9f43dafacd 100644 --- a/crates/transaction-pool/src/pool/txpool.rs +++ b/crates/transaction-pool/src/pool/txpool.rs @@ -1,7 +1,7 @@ //! The internal transaction pool implementation. use crate::{ config::MAX_ACCOUNT_SLOTS_PER_SENDER, - error::PoolError, + error::{InvalidPoolTransactionError, PoolError}, identifier::{SenderId, TransactionId}, metrics::TxPoolMetrics, pool::{ @@ -220,8 +220,6 @@ impl TxPool { .or_default() .update(on_chain_nonce, on_chain_balance); - let _hash = *tx.hash(); - match self.all_transactions.insert_tx(tx, on_chain_balance, on_chain_nonce) { Ok(InsertOk { transaction, move_to, replaced_tx, updates, .. }) => { self.add_new_transaction(transaction.clone(), replaced_tx, move_to); @@ -263,10 +261,9 @@ impl TxPool { transaction, block_gas_limit, tx_gas_limit, - } => Err(PoolError::TxExceedsGasLimit( + } => Err(PoolError::InvalidTransaction( *transaction.hash(), - block_gas_limit, - tx_gas_limit, + InvalidPoolTransactionError::ExceedsGasLimit(block_gas_limit, tx_gas_limit), )), } } diff --git a/crates/transaction-pool/src/test_utils/mod.rs b/crates/transaction-pool/src/test_utils/mod.rs index a20cf98dcd..7fd1da50bb 100644 --- a/crates/transaction-pool/src/test_utils/mod.rs +++ b/crates/transaction-pool/src/test_utils/mod.rs @@ -9,7 +9,6 @@ use crate::{ }; use async_trait::async_trait; pub use mock::*; -use reth_interfaces::consensus::ConsensusError; use std::{marker::PhantomData, sync::Arc}; /// A [Pool] used for testing @@ -37,12 +36,12 @@ impl TransactionValidator for NoopTransactionValidator { &self, origin: TransactionOrigin, transaction: Self::Transaction, - ) -> Result, ConsensusError> { - Ok(TransactionValidationOutcome::Valid { + ) -> TransactionValidationOutcome { + TransactionValidationOutcome::Valid { balance: Default::default(), state_nonce: 0, transaction, - }) + } } } diff --git a/crates/transaction-pool/src/validate.rs b/crates/transaction-pool/src/validate.rs index 5a5f9a553e..cafab6aaf7 100644 --- a/crates/transaction-pool/src/validate.rs +++ b/crates/transaction-pool/src/validate.rs @@ -1,12 +1,11 @@ //! Transaction validation abstractions. use crate::{ - error::PoolError, + error::InvalidPoolTransactionError, identifier::{SenderId, TransactionId}, traits::{PoolTransaction, TransactionOrigin}, MAX_INIT_CODE_SIZE, TX_MAX_SIZE, }; -use reth_interfaces::consensus::ConsensusError; use reth_primitives::{ Address, InvalidTransactionError, TransactionKind, TxHash, EIP1559_TX_TYPE_ID, EIP2930_TX_TYPE_ID, LEGACY_TX_TYPE_ID, U256, @@ -28,7 +27,9 @@ pub enum TransactionValidationOutcome { }, /// The transaction is considered invalid indefinitely: It violates constraints that prevent /// this transaction from ever becoming valid. - Invalid(T, PoolError), + Invalid(T, InvalidPoolTransactionError), + /// An error occurred while trying to validate the transaction + Error(T, Box), } /// Provides support for validating transaction at any given state of the chain @@ -59,7 +60,7 @@ pub trait TransactionValidator: Send + Sync { &self, origin: TransactionOrigin, transaction: Self::Transaction, - ) -> Result, ConsensusError>; + ) -> TransactionValidationOutcome; /// Ensure that the code size is not greater than `max_init_code_size`. /// `max_init_code_size` should be configurable so this will take it as an argument. @@ -67,11 +68,10 @@ pub trait TransactionValidator: Send + Sync { &self, transaction: Self::Transaction, max_init_code_size: usize, - ) -> Result<(), PoolError> { + ) -> Result<(), InvalidPoolTransactionError> { if *transaction.kind() == TransactionKind::Create && transaction.size() > max_init_code_size { - Err(PoolError::TxExceedsMaxInitCodeSize( - *transaction.hash(), + Err(InvalidPoolTransactionError::ExceedsMaxInitCodeSize( transaction.size(), max_init_code_size, )) @@ -81,8 +81,9 @@ pub trait TransactionValidator: Send + Sync { } } -/// TODO: Add docs and make this public -pub(crate) struct EthTransactionValidatorConfig { +/// A [TransactionValidator] implementation that validates ethereum transaction. +#[derive(Debug, Clone)] +pub struct EthTransactionValidator { /// Chain id chain_id: u64, /// This type fetches account info from the db @@ -101,7 +102,7 @@ pub(crate) struct EthTransactionValidatorConfig { #[async_trait::async_trait] impl TransactionValidator - for EthTransactionValidatorConfig + for EthTransactionValidator { type Transaction = T; @@ -109,7 +110,7 @@ impl TransactionValidator &self, origin: TransactionOrigin, transaction: Self::Transaction, - ) -> Result, ConsensusError> { + ) -> TransactionValidationOutcome { // Checks for tx_type match transaction.tx_type() { LEGACY_TX_TYPE_ID => { @@ -119,101 +120,136 @@ impl TransactionValidator EIP2930_TX_TYPE_ID => { // Accept only legacy transactions until EIP-2718/2930 activates if !self.eip2718 { - return Err(InvalidTransactionError::Eip2930Disabled.into()) + return TransactionValidationOutcome::Invalid( + transaction, + InvalidTransactionError::Eip1559Disabled.into(), + ) } } EIP1559_TX_TYPE_ID => { // Reject dynamic fee transactions until EIP-1559 activates. if !self.eip1559 { - return Err(InvalidTransactionError::Eip1559Disabled.into()) + return TransactionValidationOutcome::Invalid( + transaction, + InvalidTransactionError::Eip1559Disabled.into(), + ) } } - _ => return Err(InvalidTransactionError::TxTypeNotSupported.into()), + _ => { + return TransactionValidationOutcome::Invalid( + transaction, + InvalidTransactionError::TxTypeNotSupported.into(), + ) + } }; // Reject transactions over defined size to prevent DOS attacks if transaction.size() > TX_MAX_SIZE { - return Ok(TransactionValidationOutcome::Invalid( + return TransactionValidationOutcome::Invalid( transaction.clone(), - PoolError::OversizedData(*transaction.hash(), transaction.size(), TX_MAX_SIZE), - )) + InvalidPoolTransactionError::OversizedData(transaction.size(), TX_MAX_SIZE), + ) } // Check whether the init code size has been exceeded. if self.shanghai { - match self.ensure_max_init_code_size(transaction.clone(), MAX_INIT_CODE_SIZE) { - Ok(_) => {} - Err(e) => return Ok(TransactionValidationOutcome::Invalid(transaction, e)), + if let Err(err) = + self.ensure_max_init_code_size(transaction.clone(), MAX_INIT_CODE_SIZE) + { + return TransactionValidationOutcome::Invalid(transaction, err) } } // Checks for gas limit if transaction.gas_limit() > self.current_max_gas_limit { - return Ok(TransactionValidationOutcome::Invalid( + return TransactionValidationOutcome::Invalid( transaction.clone(), - PoolError::TxExceedsGasLimit( - *transaction.hash(), + InvalidPoolTransactionError::ExceedsGasLimit( transaction.gas_limit(), self.current_max_gas_limit, ), - )) + ) } // Ensure max_fee_per_gas is greater than or equal to max_priority_fee_per_gas. if transaction.max_fee_per_gas() <= transaction.max_priority_fee_per_gas() { - return Err(InvalidTransactionError::TipAboveFeeCap.into()) + return TransactionValidationOutcome::Invalid( + transaction, + InvalidTransactionError::TipAboveFeeCap.into(), + ) } // Drop non-local transactions under our own minimal accepted gas price or tip if !origin.is_local() && transaction.max_fee_per_gas() < self.gas_price { - return Err(InvalidTransactionError::MaxFeeLessThenBaseFee.into()) + return TransactionValidationOutcome::Invalid( + transaction, + InvalidTransactionError::MaxFeeLessThenBaseFee.into(), + ) } // Checks for chainid if transaction.chain_id() != Some(self.chain_id) { - return Err(InvalidTransactionError::ChainIdMismatch.into()) + return TransactionValidationOutcome::Invalid( + transaction, + InvalidTransactionError::ChainIdMismatch.into(), + ) } - let account = match self.client.basic_account(transaction.sender()).unwrap() { + let account = match self.client.basic_account(transaction.sender()) { + Ok(account) => account, + Err(err) => return TransactionValidationOutcome::Error(transaction, Box::new(err)), + }; + + let account = match account { Some(account) => { // Signer account shouldn't have bytecode. Presence of bytecode means this is a // smartcontract. if account.has_bytecode() { - return Err(InvalidTransactionError::SignerAccountHasBytecode.into()) + return TransactionValidationOutcome::Invalid( + transaction, + InvalidTransactionError::SignerAccountHasBytecode.into(), + ) } else { account } } None => { - return Ok(TransactionValidationOutcome::Invalid( - transaction.clone(), - PoolError::AccountNotFound(*transaction.hash()), - )) + return TransactionValidationOutcome::Invalid( + transaction, + InvalidPoolTransactionError::AccountNotFound, + ) } }; // Checks for nonce if transaction.nonce() < account.nonce { - return Err(InvalidTransactionError::NonceNotConsistent.into()) + return TransactionValidationOutcome::Invalid( + transaction, + InvalidTransactionError::NonceNotConsistent.into(), + ) } // Checks for max cost if transaction.cost() > account.balance { - return Err(InvalidTransactionError::InsufficientFunds { - max_fee: transaction.max_fee_per_gas().unwrap_or_default(), - available_funds: account.balance, - } - .into()) + let max_fee = transaction.max_fee_per_gas().unwrap_or_default(); + return TransactionValidationOutcome::Invalid( + transaction, + InvalidTransactionError::InsufficientFunds { + max_fee, + available_funds: account.balance, + } + .into(), + ) } // Return the valid transaction - Ok(TransactionValidationOutcome::Valid { + TransactionValidationOutcome::Valid { balance: account.balance, state_nonce: account.nonce, transaction, - }) + } } }