diff --git a/crates/op-rbuilder/src/primitives/bundle.rs b/crates/op-rbuilder/src/primitives/bundle.rs index c686530c7..251e28b93 100644 --- a/crates/op-rbuilder/src/primitives/bundle.rs +++ b/crates/op-rbuilder/src/primitives/bundle.rs @@ -1,5 +1,6 @@ use alloy_primitives::{Bytes, B256}; use alloy_rpc_types_eth::erc4337::TransactionConditional; +use reth_rpc_eth_types::EthApiError; use serde::{Deserialize, Serialize}; pub const MAX_BLOCK_RANGE_BLOCKS: u64 = 10; @@ -29,15 +30,89 @@ pub struct Bundle { pub block_number_min: Option, } +impl From for EthApiError { + fn from(err: BundleConditionalError) -> Self { + EthApiError::InvalidParams(err.to_string()) + } +} + +#[derive(Debug, thiserror::Error)] +pub enum BundleConditionalError { + #[error("block_number_min ({min}) is greater than block_number_max ({max})")] + MinGreaterThanMax { min: u64, max: u64 }, + #[error("block_number_max ({max}) is a past block (current: {current})")] + MaxBlockInPast { max: u64, current: u64 }, + #[error( + "block_number_max ({max}) is too high (current: {current}, max allowed: {max_allowed})" + )] + MaxBlockTooHigh { + max: u64, + current: u64, + max_allowed: u64, + }, + #[error( + "block_number_min ({min}) is too high with default max range (max allowed: {max_allowed})" + )] + MinTooHighForDefaultRange { min: u64, max_allowed: u64 }, +} + impl Bundle { - pub fn conditional(&self) -> TransactionConditional { - TransactionConditional { - block_number_min: self.block_number_min, - block_number_max: self.block_number_max, + pub fn conditional( + &self, + last_block_number: u64, + ) -> Result { + let mut block_number_max = self.block_number_max; + let block_number_min = self.block_number_min; + + // Validate block number ranges + if let Some(max) = block_number_max { + // Check if min > max + if let Some(min) = block_number_min { + if min > max { + return Err(BundleConditionalError::MinGreaterThanMax { min, max }); + } + } + + // The max block cannot be a past block + if max <= last_block_number { + return Err(BundleConditionalError::MaxBlockInPast { + max, + current: last_block_number, + }); + } + + // Validate that it is not greater than the max_block_range + let max_allowed = last_block_number + MAX_BLOCK_RANGE_BLOCKS; + if max > max_allowed { + return Err(BundleConditionalError::MaxBlockTooHigh { + max, + current: last_block_number, + max_allowed, + }); + } + } else { + // If no upper bound is set, use the maximum block range + let default_max = last_block_number + MAX_BLOCK_RANGE_BLOCKS; + block_number_max = Some(default_max); + + // Ensure that the new max is not smaller than the min + if let Some(min) = block_number_min { + if min > default_max { + return Err(BundleConditionalError::MinTooHighForDefaultRange { + min, + max_allowed: default_max, + }); + } + } + } + + Ok(TransactionConditional { + block_number_min, + block_number_max, known_accounts: Default::default(), timestamp_max: None, timestamp_min: None, - } + }) } } @@ -46,3 +121,176 @@ pub struct BundleResult { #[serde(rename = "bundleHash")] pub bundle_hash: B256, } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_bundle_conditional_no_bounds() { + let bundle = Bundle { + transactions: vec![], + reverting_hashes: None, + block_number_max: None, + block_number_min: None, + }; + + let last_block = 1000; + let result = bundle.conditional(last_block).unwrap(); + + assert_eq!(result.block_number_min, None); + assert_eq!( + result.block_number_max, + Some(last_block + MAX_BLOCK_RANGE_BLOCKS) + ); + } + + #[test] + fn test_bundle_conditional_with_valid_bounds() { + let bundle = Bundle { + transactions: vec![], + reverting_hashes: None, + block_number_max: Some(1005), + block_number_min: Some(1002), + }; + + let last_block = 1000; + let result = bundle.conditional(last_block).unwrap(); + + assert_eq!(result.block_number_min, Some(1002)); + assert_eq!(result.block_number_max, Some(1005)); + } + + #[test] + fn test_bundle_conditional_min_greater_than_max() { + let bundle = Bundle { + transactions: vec![], + reverting_hashes: None, + block_number_max: Some(1005), + block_number_min: Some(1010), + }; + + let last_block = 1000; + let result = bundle.conditional(last_block); + + assert!(matches!( + result, + Err(BundleConditionalError::MinGreaterThanMax { + min: 1010, + max: 1005 + }) + )); + } + + #[test] + fn test_bundle_conditional_max_in_past() { + let bundle = Bundle { + transactions: vec![], + reverting_hashes: None, + block_number_max: Some(999), + block_number_min: None, + }; + + let last_block = 1000; + let result = bundle.conditional(last_block); + + assert!(matches!( + result, + Err(BundleConditionalError::MaxBlockInPast { + max: 999, + current: 1000 + }) + )); + } + + #[test] + fn test_bundle_conditional_max_too_high() { + let bundle = Bundle { + transactions: vec![], + reverting_hashes: None, + block_number_max: Some(1020), + block_number_min: None, + }; + + let last_block = 1000; + let result = bundle.conditional(last_block); + + assert!(matches!( + result, + Err(BundleConditionalError::MaxBlockTooHigh { + max: 1020, + current: 1000, + max_allowed: 1010 + }) + )); + } + + #[test] + fn test_bundle_conditional_min_too_high_for_default_range() { + let bundle = Bundle { + transactions: vec![], + reverting_hashes: None, + block_number_max: None, + block_number_min: Some(1015), + }; + + let last_block = 1000; + let result = bundle.conditional(last_block); + + assert!(matches!( + result, + Err(BundleConditionalError::MinTooHighForDefaultRange { + min: 1015, + max_allowed: 1010 + }) + )); + } + + #[test] + fn test_bundle_conditional_with_only_min() { + let bundle = Bundle { + transactions: vec![], + reverting_hashes: None, + block_number_max: None, + block_number_min: Some(1005), + }; + + let last_block = 1000; + let result = bundle.conditional(last_block).unwrap(); + + assert_eq!(result.block_number_min, Some(1005)); + assert_eq!(result.block_number_max, Some(1010)); // last_block + MAX_BLOCK_RANGE_BLOCKS + } + + #[test] + fn test_bundle_conditional_with_only_max() { + let bundle = Bundle { + transactions: vec![], + reverting_hashes: None, + block_number_max: Some(1008), + block_number_min: None, + }; + + let last_block = 1000; + let result = bundle.conditional(last_block).unwrap(); + + assert_eq!(result.block_number_min, None); + assert_eq!(result.block_number_max, Some(1008)); + } + + #[test] + fn test_bundle_conditional_min_lower_than_last_block() { + let bundle = Bundle { + transactions: vec![], + reverting_hashes: None, + block_number_max: None, + block_number_min: Some(999), + }; + + let last_block = 1000; + let result = bundle.conditional(last_block).unwrap(); + + assert_eq!(result.block_number_min, Some(999)); + assert_eq!(result.block_number_max, Some(1010)); + } +} diff --git a/crates/op-rbuilder/src/revert_protection.rs b/crates/op-rbuilder/src/revert_protection.rs index 5989a62f9..d9f7a9ed8 100644 --- a/crates/op-rbuilder/src/revert_protection.rs +++ b/crates/op-rbuilder/src/revert_protection.rs @@ -1,5 +1,5 @@ use crate::{ - primitives::bundle::{Bundle, BundleResult, MAX_BLOCK_RANGE_BLOCKS}, + primitives::bundle::{Bundle, BundleResult}, tx::{FBPooledTransaction, MaybeRevertingTransaction}, }; use alloy_json_rpc::RpcObject; @@ -82,7 +82,7 @@ where Pool: TransactionPool + Clone + 'static, Provider: StateProviderFactory + Send + Sync + Clone + 'static, { - async fn send_bundle(&self, mut bundle: Bundle) -> RpcResult { + async fn send_bundle(&self, bundle: Bundle) -> RpcResult { let last_block_number = self .provider .best_block_number() @@ -105,48 +105,16 @@ where } }; - if let Some(block_number_max) = bundle.block_number_max { - if let Some(block_number_min) = bundle.block_number_min { - if block_number_min > block_number_max { - return Err(EthApiError::InvalidParams( - "block_number_min is greater than block_number_max".into(), - ) - .into()); - } - } - - // The max block cannot be a past block - if block_number_max <= last_block_number { - return Err( - EthApiError::InvalidParams("block_number_max is a past block".into()).into(), - ); - } - - // Validate that it is not greater than the max_block_range - if block_number_max > last_block_number + MAX_BLOCK_RANGE_BLOCKS { - return Err( - EthApiError::InvalidParams("block_number_max is too high".into()).into(), - ); - } - } else { - // If no upper bound is set, use the maximum block range - bundle.block_number_max = Some(last_block_number + MAX_BLOCK_RANGE_BLOCKS); - // Ensure that the new max is not smaller than the min - if let Some(block_number_min) = bundle.block_number_min { - if block_number_min > bundle.block_number_max.unwrap() { - return Err( - EthApiError::InvalidParams("block_number_min is too high".into()).into(), - ); - } - } - } + let conditional = bundle + .conditional(last_block_number) + .map_err(EthApiError::from)?; let recovered = recover_raw_transaction(&bundle_transaction)?; let mut pool_transaction: FBPooledTransaction = OpPooledTransaction::from_pooled(recovered).into(); pool_transaction.set_reverted_hashes(bundle.reverting_hashes.clone().unwrap_or_default()); - pool_transaction.set_conditional(bundle.conditional()); + pool_transaction.set_conditional(conditional); let hash = self .pool diff --git a/crates/op-rbuilder/src/tests/vanilla/revert.rs b/crates/op-rbuilder/src/tests/vanilla/revert.rs index 5f44204d0..be0d139e2 100644 --- a/crates/op-rbuilder/src/tests/vanilla/revert.rs +++ b/crates/op-rbuilder/src/tests/vanilla/revert.rs @@ -240,57 +240,75 @@ async fn revert_protection_bundle_range_limits() -> eyre::Result<()> { async fn send_bundle( harness: &TestHarness, - block_number_max: u64, + block_number_max: Option, block_number_min: Option, ) -> eyre::Result> { harness .create_transaction() .with_bundle(BundleOpts { - block_number_max: Some(block_number_max), - block_number_min: block_number_min, + block_number_max, + block_number_min, }) .send() .await } // Max block cannot be a past block - assert!(send_bundle(&harness, 1, None).await.is_err()); + assert!(send_bundle(&harness, Some(1), None).await.is_err()); // Bundles are valid if their max block in in between the current block and the max block range let current_block = 2; let next_valid_block = current_block + 1; for i in next_valid_block..next_valid_block + MAX_BLOCK_RANGE_BLOCKS { - assert!(send_bundle(&harness, i, None).await.is_ok()); + assert!(send_bundle(&harness, Some(i), None).await.is_ok()); } // A bundle with a block out of range is invalid assert!(send_bundle( &harness, - next_valid_block + MAX_BLOCK_RANGE_BLOCKS + 1, + Some(next_valid_block + MAX_BLOCK_RANGE_BLOCKS + 1), None ) .await .is_err()); // A bundle with a min block number higher than the max block is invalid - assert!(send_bundle(&harness, 1, Some(2)).await.is_err()); + assert!(send_bundle(&harness, Some(1), Some(2)).await.is_err()); // A bundle with a min block number lower or equal to the current block is valid - assert!(send_bundle(&harness, next_valid_block, Some(current_block)) - .await - .is_ok()); - assert!(send_bundle(&harness, next_valid_block, Some(0)) + assert!( + send_bundle(&harness, Some(next_valid_block), Some(current_block)) + .await + .is_ok() + ); + assert!(send_bundle(&harness, Some(next_valid_block), Some(0)) .await .is_ok()); // A bundle with a min block equal to max block is valid assert!( - send_bundle(&harness, next_valid_block, Some(next_valid_block)) + send_bundle(&harness, Some(next_valid_block), Some(next_valid_block)) .await .is_ok() ); + // Test min-only cases (no max specified) + // A bundle with only min block that's within the default range is valid + let default_max = current_block + MAX_BLOCK_RANGE_BLOCKS; + assert!(send_bundle(&harness, None, Some(current_block)) + .await + .is_ok()); + assert!(send_bundle(&harness, None, Some(default_max - 1)) + .await + .is_ok()); + assert!(send_bundle(&harness, None, Some(default_max)).await.is_ok()); + + // A bundle with only min block that exceeds the default max range is invalid + assert!(send_bundle(&harness, None, Some(default_max + 1)) + .await + .is_err()); + Ok(()) }