refactor(tx-pool): add enum InvalidKind to mark_invalid (#12845)

This commit is contained in:
Léa Narzis
2024-11-28 17:23:27 +01:00
committed by GitHub
parent cca6372e87
commit 9f20ebc29a
5 changed files with 94 additions and 33 deletions

View File

@ -27,13 +27,13 @@ use reth_payload_builder_primitives::PayloadBuilderError;
use reth_payload_primitives::PayloadBuilderAttributes; use reth_payload_primitives::PayloadBuilderAttributes;
use reth_primitives::{ use reth_primitives::{
proofs::{self}, proofs::{self},
Block, BlockBody, BlockExt, EthereumHardforks, Receipt, Block, BlockBody, BlockExt, EthereumHardforks, InvalidTransactionError, Receipt,
}; };
use reth_provider::{ChainSpecProvider, StateProviderFactory}; use reth_provider::{ChainSpecProvider, StateProviderFactory};
use reth_revm::database::StateProviderDatabase; use reth_revm::database::StateProviderDatabase;
use reth_transaction_pool::{ use reth_transaction_pool::{
noop::NoopTransactionPool, BestTransactions, BestTransactionsAttributes, TransactionPool, error::InvalidPoolTransactionError, noop::NoopTransactionPool, BestTransactions,
ValidPoolTransaction, BestTransactionsAttributes, TransactionPool, ValidPoolTransaction,
}; };
use reth_trie::HashedPostState; use reth_trie::HashedPostState;
use revm::{ use revm::{
@ -228,7 +228,10 @@ where
// we can't fit this transaction into the block, so we need to mark it as invalid // we can't fit this transaction into the block, so we need to mark it as invalid
// which also removes all dependent transaction from the iterator before we can // which also removes all dependent transaction from the iterator before we can
// continue // continue
best_txs.mark_invalid(&pool_tx); best_txs.mark_invalid(
&pool_tx,
InvalidPoolTransactionError::ExceedsGasLimit(pool_tx.gas_limit(), block_gas_limit),
);
continue continue
} }
@ -250,7 +253,13 @@ where
// the iterator. This is similar to the gas limit condition // the iterator. This is similar to the gas limit condition
// for regular transactions above. // for regular transactions above.
trace!(target: "payload_builder", tx=?tx.hash, ?sum_blob_gas_used, ?tx_blob_gas, "skipping blob transaction because it would exceed the max data gas per block"); trace!(target: "payload_builder", tx=?tx.hash, ?sum_blob_gas_used, ?tx_blob_gas, "skipping blob transaction because it would exceed the max data gas per block");
best_txs.mark_invalid(&pool_tx); best_txs.mark_invalid(
&pool_tx,
InvalidPoolTransactionError::ExceedsGasLimit(
tx_blob_gas,
MAX_DATA_GAS_PER_BLOCK,
),
);
continue continue
} }
} }
@ -270,7 +279,12 @@ where
// if the transaction is invalid, we can skip it and all of its // if the transaction is invalid, we can skip it and all of its
// descendants // descendants
trace!(target: "payload_builder", %err, ?tx, "skipping invalid transaction and its descendants"); trace!(target: "payload_builder", %err, ?tx, "skipping invalid transaction and its descendants");
best_txs.mark_invalid(&pool_tx); best_txs.mark_invalid(
&pool_tx,
InvalidPoolTransactionError::Consensus(
InvalidTransactionError::TxTypeNotSupported,
),
);
} }
continue continue

View File

@ -17,8 +17,8 @@ use reth_evm::{
}; };
use reth_execution_types::ExecutionOutcome; use reth_execution_types::ExecutionOutcome;
use reth_primitives::{ use reth_primitives::{
proofs::calculate_transaction_root, Block, BlockBody, BlockExt, Receipt, proofs::calculate_transaction_root, Block, BlockBody, BlockExt, InvalidTransactionError,
SealedBlockWithSenders, SealedHeader, TransactionSignedEcRecovered, Receipt, SealedBlockWithSenders, SealedHeader, TransactionSignedEcRecovered,
}; };
use reth_provider::{ use reth_provider::{
BlockReader, BlockReaderIdExt, ChainSpecProvider, EvmEnvProvider, ProviderError, BlockReader, BlockReaderIdExt, ChainSpecProvider, EvmEnvProvider, ProviderError,
@ -32,7 +32,9 @@ use reth_revm::{
}, },
}; };
use reth_rpc_eth_types::{EthApiError, PendingBlock, PendingBlockEnv, PendingBlockEnvOrigin}; use reth_rpc_eth_types::{EthApiError, PendingBlock, PendingBlockEnv, PendingBlockEnvOrigin};
use reth_transaction_pool::{BestTransactionsAttributes, TransactionPool}; use reth_transaction_pool::{
error::InvalidPoolTransactionError, BestTransactionsAttributes, TransactionPool,
};
use reth_trie::HashedPostState; use reth_trie::HashedPostState;
use revm::{db::states::bundle_state::BundleRetention, DatabaseCommit, State}; use revm::{db::states::bundle_state::BundleRetention, DatabaseCommit, State};
use std::time::{Duration, Instant}; use std::time::{Duration, Instant};
@ -292,7 +294,13 @@ pub trait LoadPendingBlock:
// we can't fit this transaction into the block, so we need to mark it as invalid // we can't fit this transaction into the block, so we need to mark it as invalid
// which also removes all dependent transaction from the iterator before we can // which also removes all dependent transaction from the iterator before we can
// continue // continue
best_txs.mark_invalid(&pool_tx); best_txs.mark_invalid(
&pool_tx,
InvalidPoolTransactionError::ExceedsGasLimit(
pool_tx.gas_limit(),
block_gas_limit,
),
);
continue continue
} }
@ -300,7 +308,12 @@ pub trait LoadPendingBlock:
// we don't want to leak any state changes made by private transactions, so we mark // we don't want to leak any state changes made by private transactions, so we mark
// them as invalid here which removes all dependent transactions from the iterator // them as invalid here which removes all dependent transactions from the iterator
// before we can continue // before we can continue
best_txs.mark_invalid(&pool_tx); best_txs.mark_invalid(
&pool_tx,
InvalidPoolTransactionError::Consensus(
InvalidTransactionError::TxTypeNotSupported,
),
);
continue continue
} }
@ -316,7 +329,13 @@ pub trait LoadPendingBlock:
// invalid, which removes its dependent transactions from // invalid, which removes its dependent transactions from
// the iterator. This is similar to the gas limit condition // the iterator. This is similar to the gas limit condition
// for regular transactions above. // for regular transactions above.
best_txs.mark_invalid(&pool_tx); best_txs.mark_invalid(
&pool_tx,
InvalidPoolTransactionError::ExceedsGasLimit(
tx_blob_gas,
MAX_DATA_GAS_PER_BLOCK,
),
);
continue continue
} }
} }
@ -340,7 +359,12 @@ pub trait LoadPendingBlock:
} else { } else {
// if the transaction is invalid, we can skip it and all of its // if the transaction is invalid, we can skip it and all of its
// descendants // descendants
best_txs.mark_invalid(&pool_tx); best_txs.mark_invalid(
&pool_tx,
InvalidPoolTransactionError::Consensus(
InvalidTransactionError::TxTypeNotSupported,
),
);
} }
continue continue
} }

View File

@ -52,7 +52,6 @@ bitflags.workspace = true
auto_impl.workspace = true auto_impl.workspace = true
smallvec.workspace = true smallvec.workspace = true
# testing # testing
rand = { workspace = true, optional = true } rand = { workspace = true, optional = true }
paste = { workspace = true, optional = true } paste = { workspace = true, optional = true }

View File

@ -1,4 +1,5 @@
use crate::{ use crate::{
error::{Eip4844PoolTransactionError, InvalidPoolTransactionError},
identifier::{SenderId, TransactionId}, identifier::{SenderId, TransactionId},
pool::pending::PendingTransaction, pool::pending::PendingTransaction,
PoolTransaction, TransactionOrdering, ValidPoolTransaction, PoolTransaction, TransactionOrdering, ValidPoolTransaction,
@ -6,7 +7,7 @@ use crate::{
use alloy_primitives::Address; use alloy_primitives::Address;
use core::fmt; use core::fmt;
use reth_payload_util::PayloadTransactions; use reth_payload_util::PayloadTransactions;
use reth_primitives::TransactionSignedEcRecovered; use reth_primitives::{InvalidTransactionError, TransactionSignedEcRecovered};
use std::{ use std::{
collections::{BTreeMap, BTreeSet, HashSet, VecDeque}, collections::{BTreeMap, BTreeSet, HashSet, VecDeque},
sync::Arc, sync::Arc,
@ -27,8 +28,8 @@ pub(crate) struct BestTransactionsWithFees<T: TransactionOrdering> {
} }
impl<T: TransactionOrdering> crate::traits::BestTransactions for BestTransactionsWithFees<T> { impl<T: TransactionOrdering> crate::traits::BestTransactions for BestTransactionsWithFees<T> {
fn mark_invalid(&mut self, tx: &Self::Item) { fn mark_invalid(&mut self, tx: &Self::Item, kind: InvalidPoolTransactionError) {
BestTransactions::mark_invalid(&mut self.best, tx) BestTransactions::mark_invalid(&mut self.best, tx, kind)
} }
fn no_updates(&mut self) { fn no_updates(&mut self) {
@ -60,7 +61,11 @@ impl<T: TransactionOrdering> Iterator for BestTransactionsWithFees<T> {
{ {
return Some(best); return Some(best);
} }
crate::traits::BestTransactions::mark_invalid(self, &best); crate::traits::BestTransactions::mark_invalid(
self,
&best,
InvalidPoolTransactionError::Underpriced,
);
} }
} }
} }
@ -95,7 +100,11 @@ pub(crate) struct BestTransactions<T: TransactionOrdering> {
impl<T: TransactionOrdering> BestTransactions<T> { impl<T: TransactionOrdering> BestTransactions<T> {
/// Mark the transaction and it's descendants as invalid. /// Mark the transaction and it's descendants as invalid.
pub(crate) fn mark_invalid(&mut self, tx: &Arc<ValidPoolTransaction<T::Transaction>>) { pub(crate) fn mark_invalid(
&mut self,
tx: &Arc<ValidPoolTransaction<T::Transaction>>,
_kind: InvalidPoolTransactionError,
) {
self.invalid.insert(tx.sender_id()); self.invalid.insert(tx.sender_id());
} }
@ -154,8 +163,8 @@ impl<T: TransactionOrdering> BestTransactions<T> {
} }
impl<T: TransactionOrdering> crate::traits::BestTransactions for BestTransactions<T> { impl<T: TransactionOrdering> crate::traits::BestTransactions for BestTransactions<T> {
fn mark_invalid(&mut self, tx: &Self::Item) { fn mark_invalid(&mut self, tx: &Self::Item, kind: InvalidPoolTransactionError) {
Self::mark_invalid(self, tx) Self::mark_invalid(self, tx, kind)
} }
fn no_updates(&mut self) { fn no_updates(&mut self) {
@ -199,7 +208,12 @@ impl<T: TransactionOrdering> Iterator for BestTransactions<T> {
if self.skip_blobs && best.transaction.transaction.is_eip4844() { if self.skip_blobs && best.transaction.transaction.is_eip4844() {
// blobs should be skipped, marking them as invalid will ensure that no dependent // blobs should be skipped, marking them as invalid will ensure that no dependent
// transactions are returned // transactions are returned
self.mark_invalid(&best.transaction) self.mark_invalid(
&best.transaction,
InvalidPoolTransactionError::Eip4844(
Eip4844PoolTransactionError::NoEip4844Blobs,
),
)
} else { } else {
return Some(best.transaction) return Some(best.transaction)
} }
@ -280,7 +294,10 @@ where
if (self.predicate)(&best) { if (self.predicate)(&best) {
return Some(best) return Some(best)
} }
self.best.mark_invalid(&best); self.best.mark_invalid(
&best,
InvalidPoolTransactionError::Consensus(InvalidTransactionError::TxTypeNotSupported),
);
} }
} }
} }
@ -290,8 +307,8 @@ where
I: crate::traits::BestTransactions, I: crate::traits::BestTransactions,
P: FnMut(&<I as Iterator>::Item) -> bool + Send, P: FnMut(&<I as Iterator>::Item) -> bool + Send,
{ {
fn mark_invalid(&mut self, tx: &Self::Item) { fn mark_invalid(&mut self, tx: &Self::Item, kind: InvalidPoolTransactionError) {
crate::traits::BestTransactions::mark_invalid(&mut self.best, tx) crate::traits::BestTransactions::mark_invalid(&mut self.best, tx, kind)
} }
fn no_updates(&mut self) { fn no_updates(&mut self) {
@ -379,8 +396,8 @@ where
I: crate::traits::BestTransactions<Item = Arc<ValidPoolTransaction<T>>>, I: crate::traits::BestTransactions<Item = Arc<ValidPoolTransaction<T>>>,
T: PoolTransaction, T: PoolTransaction,
{ {
fn mark_invalid(&mut self, tx: &Self::Item) { fn mark_invalid(&mut self, tx: &Self::Item, kind: InvalidPoolTransactionError) {
self.inner.mark_invalid(tx) self.inner.mark_invalid(tx, kind)
} }
fn no_updates(&mut self) { fn no_updates(&mut self) {
@ -450,7 +467,10 @@ mod tests {
// mark the first tx as invalid // mark the first tx as invalid
let invalid = best.independent.iter().next().unwrap(); let invalid = best.independent.iter().next().unwrap();
best.mark_invalid(&invalid.transaction.clone()); best.mark_invalid(
&invalid.transaction.clone(),
InvalidPoolTransactionError::Consensus(InvalidTransactionError::TxTypeNotSupported),
);
// iterator is empty // iterator is empty
assert!(best.next().is_none()); assert!(best.next().is_none());
@ -475,7 +495,11 @@ mod tests {
> = Box::new(pool.best()); > = Box::new(pool.best());
let tx = Iterator::next(&mut best).unwrap(); let tx = Iterator::next(&mut best).unwrap();
crate::traits::BestTransactions::mark_invalid(&mut *best, &tx); crate::traits::BestTransactions::mark_invalid(
&mut *best,
&tx,
InvalidPoolTransactionError::Consensus(InvalidTransactionError::TxTypeNotSupported),
);
assert!(Iterator::next(&mut best).is_none()); assert!(Iterator::next(&mut best).is_none());
} }

View File

@ -806,7 +806,7 @@ pub trait BestTransactions: Iterator + Send {
/// Implementers must ensure all subsequent transaction _don't_ depend on this transaction. /// Implementers must ensure all subsequent transaction _don't_ depend on this transaction.
/// In other words, this must remove the given transaction _and_ drain all transaction that /// In other words, this must remove the given transaction _and_ drain all transaction that
/// depend on it. /// depend on it.
fn mark_invalid(&mut self, transaction: &Self::Item); fn mark_invalid(&mut self, transaction: &Self::Item, kind: InvalidPoolTransactionError);
/// An iterator may be able to receive additional pending transactions that weren't present it /// An iterator may be able to receive additional pending transactions that weren't present it
/// the pool when it was created. /// the pool when it was created.
@ -868,8 +868,8 @@ impl<T> BestTransactions for Box<T>
where where
T: BestTransactions + ?Sized, T: BestTransactions + ?Sized,
{ {
fn mark_invalid(&mut self, transaction: &Self::Item) { fn mark_invalid(&mut self, transaction: &Self::Item, kind: InvalidPoolTransactionError) {
(**self).mark_invalid(transaction); (**self).mark_invalid(transaction, kind)
} }
fn no_updates(&mut self) { fn no_updates(&mut self) {
@ -887,7 +887,7 @@ where
/// A no-op implementation that yields no transactions. /// A no-op implementation that yields no transactions.
impl<T> BestTransactions for std::iter::Empty<T> { impl<T> BestTransactions for std::iter::Empty<T> {
fn mark_invalid(&mut self, _tx: &T) {} fn mark_invalid(&mut self, _tx: &T, _kind: InvalidPoolTransactionError) {}
fn no_updates(&mut self) {} fn no_updates(&mut self) {}