diff --git a/crates/net/network/src/transactions/mod.rs b/crates/net/network/src/transactions/mod.rs index f9ff128062..cc6e7d11c5 100644 --- a/crates/net/network/src/transactions/mod.rs +++ b/crates/net/network/src/transactions/mod.rs @@ -473,8 +473,14 @@ impl TransactionsManager { self.transactions_by_peers.remove(&hash); } - /// Penalize the peers that intentionally sent the bad transaction, and cache it to avoid - /// fetching or importing it again. + /// Handles a failed transaction import. + /// + /// Blob sidecar errors (e.g. invalid proof, missing sidecar) are penalized via + /// `report_peer_bad_transactions` but NOT cached in `bad_imports` — the transaction itself + /// may be valid when fetched from another peer with correct sidecar data. + /// + /// Other bad transactions are penalized and cached in `bad_imports` to avoid fetching or + /// importing them again. /// /// Errors that count as bad transactions are: /// @@ -499,6 +505,18 @@ impl TransactionsManager { fn on_bad_import(&mut self, err: PoolError) { let peers = self.transactions_by_peers.remove(&err.hash); + if err.is_bad_blob_sidecar() { + // Blob sidecar errors: penalize but do NOT cache the hash as bad. + // The transaction may be valid — only the sidecar from this peer was wrong. + // Using regular penalties means repeated offenders still get disconnected. + if let Some(peers) = peers { + for peer_id in peers { + self.report_peer_bad_transactions(peer_id); + } + } + return + } + // if we're _currently_ syncing, we ignore a bad transaction if !err.is_bad_transaction() || self.network.is_syncing() { return @@ -2169,6 +2187,7 @@ mod tests { NetworkConfigBuilder, NetworkManager, }; use alloy_consensus::{TxEip1559, TxLegacy}; + use alloy_eips::eip4844::BlobTransactionValidationError; use alloy_primitives::{hex, Signature, TxKind, B256, U256}; use alloy_rlp::Decodable; use futures::FutureExt; @@ -2181,11 +2200,13 @@ mod tests { }; use reth_storage_api::noop::NoopProvider; use reth_tasks::Runtime; - use reth_transaction_pool::test_utils::{ - testing_pool, MockTransaction, MockTransactionFactory, TestPool, + use reth_transaction_pool::{ + error::{Eip4844PoolTransactionError, InvalidPoolTransactionError, PoolError}, + test_utils::{testing_pool, MockTransaction, MockTransactionFactory, TestPool}, }; use secp256k1::SecretKey; use std::{ + collections::HashSet, future::poll_fn, net::{IpAddr, Ipv4Addr, SocketAddr}, str::FromStr, @@ -2553,6 +2574,67 @@ mod tests { ); } + #[tokio::test(flavor = "multi_thread")] + async fn test_bad_blob_sidecar_not_cached_as_bad_import() { + let (mut tx_manager, _network) = new_tx_manager().await; + let peer_id = PeerId::new([1; 64]); + let hash = B256::from_slice(&[1; 32]); + + tx_manager.network.update_sync_state(SyncState::Idle); + tx_manager.transactions_by_peers.insert(hash, HashSet::from([peer_id])); + + let err = PoolError::new( + hash, + InvalidPoolTransactionError::Eip4844(Eip4844PoolTransactionError::InvalidEip4844Blob( + BlobTransactionValidationError::InvalidProof, + )), + ); + + tx_manager.on_bad_import(err); + + assert!(!tx_manager.bad_imports.contains(&hash)); + } + + #[tokio::test(flavor = "multi_thread")] + async fn test_missing_blob_sidecar_not_cached_as_bad_import() { + let (mut tx_manager, _network) = new_tx_manager().await; + let peer_id = PeerId::new([1; 64]); + let hash = B256::from_slice(&[3; 32]); + + tx_manager.network.update_sync_state(SyncState::Idle); + tx_manager.transactions_by_peers.insert(hash, HashSet::from([peer_id])); + + let err = PoolError::new( + hash, + InvalidPoolTransactionError::Eip4844( + Eip4844PoolTransactionError::MissingEip4844BlobSidecar, + ), + ); + + tx_manager.on_bad_import(err); + + assert!(!tx_manager.bad_imports.contains(&hash)); + } + + #[tokio::test(flavor = "multi_thread")] + async fn test_non_blob_sidecar_error_still_cached_as_bad_import() { + let (mut tx_manager, _network) = new_tx_manager().await; + let peer_id = PeerId::new([1; 64]); + let hash = B256::from_slice(&[2; 32]); + + tx_manager.network.update_sync_state(SyncState::Idle); + tx_manager.transactions_by_peers.insert(hash, HashSet::from([peer_id])); + + let err = PoolError::new( + hash, + InvalidPoolTransactionError::Eip4844(Eip4844PoolTransactionError::NoEip4844Blobs), + ); + + tx_manager.on_bad_import(err); + + assert!(tx_manager.bad_imports.contains(&hash)); + } + #[tokio::test(flavor = "multi_thread")] async fn test_on_get_pooled_transactions_network() { reth_tracing::init_test_tracing(); diff --git a/crates/transaction-pool/src/error.rs b/crates/transaction-pool/src/error.rs index a835e37d54..b54cbbed2b 100644 --- a/crates/transaction-pool/src/error.rs +++ b/crates/transaction-pool/src/error.rs @@ -145,6 +145,18 @@ impl PoolError { } } } + + /// Returns `true` if this is a blob sidecar error that should NOT be cached as a bad import. + /// + /// The transaction hash may be valid — the issue is peer-specific (e.g. malformed sidecar + /// data), so we penalize the peer but allow re-fetching from other peers. + #[inline] + pub const fn is_bad_blob_sidecar(&self) -> bool { + match &self.kind { + PoolErrorKind::InvalidTransaction(err) => err.is_bad_blob_sidecar(), + _ => false, + } + } } /// Represents all errors that can happen when validating transactions for the pool for EIP-4844 @@ -400,6 +412,24 @@ impl InvalidPoolTransactionError { } } + /// Returns `true` if this is a blob sidecar error (e.g. invalid proof, missing sidecar). + /// + /// These errors indicate the sidecar data from a specific peer was bad, but the transaction + /// hash itself may be valid when fetched from another peer. + #[inline] + pub const fn is_bad_blob_sidecar(&self) -> bool { + matches!( + self, + Self::Eip4844( + Eip4844PoolTransactionError::MissingEip4844BlobSidecar | + Eip4844PoolTransactionError::InvalidEip4844Blob(_) | + Eip4844PoolTransactionError::UnexpectedEip7594SidecarBeforeOsaka | + Eip4844PoolTransactionError::UnexpectedEip4844SidecarAfterOsaka | + Eip4844PoolTransactionError::Eip7594SidecarDisallowed + ) + ) + } + /// Returns true if this is a [`Self::Consensus`] variant. pub const fn as_consensus(&self) -> Option<&InvalidTransactionError> { match self { @@ -475,4 +505,32 @@ mod tests { assert!(err.downcast_other_ref::().is_some()); } + + #[test] + fn bad_blob_sidecar_detection() { + let err = PoolError::new( + TxHash::ZERO, + InvalidPoolTransactionError::Eip4844(Eip4844PoolTransactionError::InvalidEip4844Blob( + BlobTransactionValidationError::InvalidProof, + )), + ); + + assert!(err.is_bad_blob_sidecar()); + + let err = PoolError::new( + TxHash::ZERO, + InvalidPoolTransactionError::Eip4844( + Eip4844PoolTransactionError::MissingEip4844BlobSidecar, + ), + ); + + assert!(err.is_bad_blob_sidecar()); + + let err = PoolError::new( + TxHash::ZERO, + InvalidPoolTransactionError::Eip4844(Eip4844PoolTransactionError::NoEip4844Blobs), + ); + + assert!(!err.is_bad_blob_sidecar()); + } }