From 624d9d581b79d8e28da8693dd8fb04891c69eaf2 Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Mon, 11 Sep 2023 17:00:01 +0200 Subject: [PATCH] feat: enforce replacements dont conflict (#4539) --- crates/rpc/rpc/src/eth/error.rs | 9 +++++ crates/transaction-pool/src/error.rs | 9 +++++ crates/transaction-pool/src/pool/txpool.rs | 43 ++++++++++++++++++--- crates/transaction-pool/src/validate/mod.rs | 10 +++++ 4 files changed, 65 insertions(+), 6 deletions(-) diff --git a/crates/rpc/rpc/src/eth/error.rs b/crates/rpc/rpc/src/eth/error.rs index a2b0e38126..6062b00100 100644 --- a/crates/rpc/rpc/src/eth/error.rs +++ b/crates/rpc/rpc/src/eth/error.rs @@ -475,6 +475,12 @@ pub enum RpcPoolError { /// Eip-4844 related error #[error(transparent)] Eip4844(#[from] Eip4844PoolTransactionError), + /// Thrown if a conflicting transaction type is already in the pool + /// + /// In other words, thrown if a transaction with the same sender that violates the exclusivity + /// constraint (blob vs normal tx) + #[error("address already reserved")] + AddressAlreadyReserved, #[error(transparent)] Other(Box), } @@ -498,6 +504,9 @@ impl From for RpcPoolError { PoolError::InvalidTransaction(_, err) => err.into(), PoolError::Other(_, err) => RpcPoolError::Other(err), PoolError::AlreadyImported(_) => RpcPoolError::AlreadyKnown, + PoolError::ExistingConflictingTransactionType(_, _, _) => { + RpcPoolError::AddressAlreadyReserved + } } } } diff --git a/crates/transaction-pool/src/error.rs b/crates/transaction-pool/src/error.rs index 1f0442833d..f405f96301 100644 --- a/crates/transaction-pool/src/error.rs +++ b/crates/transaction-pool/src/error.rs @@ -39,6 +39,9 @@ pub enum PoolError { /// Thrown when the transaction is considered invalid. #[error("[{0:?}] {1:?}")] InvalidTransaction(TxHash, InvalidPoolTransactionError), + /// Thrown if the mutual exclusivity constraint (blob vs normal transaction) is violated. + #[error("[{1:?}] Transaction type {2} conflicts with existing transaction for {0:?}")] + ExistingConflictingTransactionType(Address, TxHash, u8), /// Any other error that occurred while inserting/validating a transaction. e.g. IO database /// error #[error("[{0:?}] {1:?}")] @@ -58,6 +61,7 @@ impl PoolError { PoolError::DiscardedOnInsert(hash) => hash, PoolError::InvalidTransaction(hash, _) => hash, PoolError::Other(hash, _) => hash, + PoolError::ExistingConflictingTransactionType(_, hash, _) => hash, } } @@ -110,6 +114,11 @@ impl PoolError { // internal error unrelated to the transaction false } + PoolError::ExistingConflictingTransactionType(_, _, _) => { + // this is not a protocol error but an implementation error since the pool enforces + // exclusivity (blob vs normal tx) for all senders + false + } } } } diff --git a/crates/transaction-pool/src/pool/txpool.rs b/crates/transaction-pool/src/pool/txpool.rs index d3e84ffafb..80e25d9636 100644 --- a/crates/transaction-pool/src/pool/txpool.rs +++ b/crates/transaction-pool/src/pool/txpool.rs @@ -430,6 +430,13 @@ impl TxPool { *transaction.hash(), InvalidPoolTransactionError::Overdraft, )), + InsertErr::TxTypeConflict { transaction } => { + Err(PoolError::ExistingConflictingTransactionType( + transaction.sender(), + *transaction.hash(), + transaction.tx_type(), + )) + } } } } @@ -1169,6 +1176,23 @@ impl AllTransactions { /// Note: For EIP-4844 blob transactions additional constraints are enforced: /// - new blob transactions must not have any nonce gaps /// - blob transactions cannot go into overdraft + /// + /// ## Transaction type Exclusivity + /// + /// The pool enforces exclusivity of eip-4844 blob vs non-blob transactions on a per sender + /// basis: + /// - If the pool already includes a blob transaction from the `transaction`'s sender, then + /// the `transaction` must also be a blob transaction + /// - If the pool already includes a non-blob transaction from the `transaction`'s sender, then + /// the `transaction` must _not_ be a blob transaction. + /// + /// In other words, the presence of blob transactions exclude non-blob transactions and vice + /// versa: + /// + /// ## Replacements + /// + /// The replacement candidate must satisfy given price bump constraints: replacement candidate + /// must not be underpriced pub(crate) fn insert_tx( &mut self, transaction: ValidPoolTransaction, @@ -1225,7 +1249,7 @@ impl AllTransactions { let mut replaced_tx = None; let pool_tx = PoolInternalTransaction { - transaction: transaction.clone(), + transaction: Arc::clone(&transaction), subpool: state.into(), state, cumulative_cost, @@ -1239,13 +1263,18 @@ impl AllTransactions { entry.insert(pool_tx); } Entry::Occupied(mut entry) => { + // Transaction with the same nonce already exists: replacement candidate + let existing_transaction = entry.get().transaction.as_ref(); + let maybe_replacement = transaction.as_ref(); + if existing_transaction.tx_type_conflicts_with(maybe_replacement) { + // blob vs non blob replacement + return Err(InsertErr::TxTypeConflict { transaction: pool_tx.transaction }) + } + // Transaction already exists // Ensure the new transaction is not underpriced - if Self::is_underpriced( - entry.get().transaction.as_ref(), - transaction.as_ref(), - &self.price_bumps, - ) { + if Self::is_underpriced(existing_transaction, maybe_replacement, &self.price_bumps) + { return Err(InsertErr::Underpriced { transaction: pool_tx.transaction, existing: *entry.get().transaction.hash(), @@ -1418,6 +1447,8 @@ pub(crate) enum InsertErr { block_gas_limit: u64, tx_gas_limit: u64, }, + /// Thrown if the mutual exclusivity constraint (blob vs normal transaction) is violated. + TxTypeConflict { transaction: Arc> }, } /// Transaction was successfully inserted into the pool diff --git a/crates/transaction-pool/src/validate/mod.rs b/crates/transaction-pool/src/validate/mod.rs index 1a302d14e6..e53828b7e9 100644 --- a/crates/transaction-pool/src/validate/mod.rs +++ b/crates/transaction-pool/src/validate/mod.rs @@ -295,6 +295,16 @@ impl ValidPoolTransaction { pub(crate) fn size(&self) -> usize { self.transaction.size() } + + /// EIP-4844 blob transactions and normal transactions are treated as mutually exclusive per + /// account. + /// + /// Returns true if the transaction is an EIP-4844 blob transaction and the other is not, or + /// vice versa. + #[inline] + pub(crate) fn tx_type_conflicts_with(&self, other: &Self) -> bool { + self.is_eip4844() != other.is_eip4844() + } } impl IntoRecoveredTransaction for ValidPoolTransaction {