Add reverting hashes + min block number to Bundle (#115)

* Implement bundle reverting hashes

* Merge main

* Fix

* Add min_block_number

* fix litn

* Fix

* Fix comment

* Mark invalid

* Rename reverting_hashes

* Remove Option

* Revert "Remove Option"

This reverts commit 4087d35.

Author: ferranbt

crates/op-rbuilder/src/builders/context.rs

@@ -1,4 +1,6 @@
-use alloy_consensus::{Eip658Value, Transaction, TxEip1559};
+use alloy_consensus::{
+    conditional::BlockConditionalAttributes, Eip658Value, Transaction, TxEip1559,
+};
 use alloy_eips::{eip7623::TOTAL_COST_FLOOR_PER_TOKEN, Encodable2718, Typed2718};
 use alloy_op_evm::block::receipt_builder::OpReceiptBuilder;
 use alloy_primitives::{Address, Bytes, TxKind, U256};
@@ -20,6 +22,7 @@ use reth_optimism_node::OpPayloadBuilderAttributes;
 use reth_optimism_payload_builder::{config::OpDAConfig, error::OpPayloadBuilderError};
 use reth_optimism_primitives::{OpReceipt, OpTransactionSigned};
 use reth_optimism_txpool::{
+    conditional::MaybeConditionalTransaction,
     estimated_da_size::DataAvailabilitySized,
     interop::{is_valid_interop, MaybeInteropTransaction},
 };
@@ -342,17 +345,37 @@ impl OpPayloadBuilderCtx {
             .da_tx_size_limit
             .set(tx_da_limit.map_or(-1.0, |v| v as f64));
 
+        let block_attr = BlockConditionalAttributes {
+            number: self.block_number(),
+            timestamp: self.attributes().timestamp(),
+        };
+
         while let Some(tx) = best_txs.next(()) {
             let interop = tx.interop_deadline();
-            let exclude_reverting_txs = tx.exclude_reverting_txs();
+            let reverted_hashes = tx.reverted_hashes().clone();
+            let conditional = tx.conditional().cloned();
+
             let tx_da_size = tx.estimated_da_size();
             let tx = tx.into_consensus();
             let tx_hash = tx.tx_hash();
 
+            // exclude reverting transaction if:
+            // - the transaction comes from a bundle and the hash **is not** in reverted hashes
+            let exclude_reverting_txs =
+                reverted_hashes.is_some() && !reverted_hashes.unwrap().contains(&tx_hash);
+
             let log_txn = |result: TxnExecutionResult| {
                 info!(target: "payload_builder", tx_hash = ?tx_hash, tx_da_size = ?tx_da_size, exclude_reverting_txs = ?exclude_reverting_txs, result = %result, "Considering transaction");
             };
 
+            if let Some(conditional) = conditional {
+                // TODO: ideally we should get this from the txpool stream
+                if !conditional.matches_block_attributes(&block_attr) {
+                    best_txs.mark_invalid(tx.signer(), tx.nonce());
+                    continue;
+                }
+            }
+
             // TODO: remove this condition and feature once we are comfortable enabling interop for everything
             if cfg!(feature = "interop") {
                 // We skip invalid cross chain txs, they would be removed on the next block update in

crates/op-rbuilder/src/primitives/bundle.rs

@@ -9,19 +9,30 @@ pub struct Bundle {
     #[serde(rename = "txs")]
     pub transactions: Vec<Bytes>,
 
+    #[serde(rename = "reverting_tx_hashes")]
+    pub reverting_hashes: Vec<B256>,
+
     #[serde(
         default,
         rename = "maxBlockNumber",
         with = "alloy_serde::quantity::opt",
         skip_serializing_if = "Option::is_none"
     )]
     pub block_number_max: Option<u64>,
+
+    #[serde(
+        default,
+        rename = "minBlockNumber",
+        with = "alloy_serde::quantity::opt",
+        skip_serializing_if = "Option::is_none"
+    )]
+    pub block_number_min: Option<u64>,
 }
 
 impl Bundle {
     pub fn conditional(&self) -> TransactionConditional {
         TransactionConditional {
-            block_number_min: None,
+            block_number_min: self.block_number_min,
             block_number_max: self.block_number_max,
             known_accounts: Default::default(),
             timestamp_max: None,

crates/op-rbuilder/src/revert_protection.rs

@@ -106,6 +106,15 @@ 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(
@@ -128,7 +137,7 @@ where
         let mut pool_transaction: FBPooledTransaction =
             OpPooledTransaction::from_pooled(recovered).into();
 
-        pool_transaction.set_exclude_reverting_txs(true);
+        pool_transaction.set_reverted_hashes(bundle.reverting_hashes.clone());
         pool_transaction.set_conditional(bundle.conditional());
 
         let hash = self

crates/op-rbuilder/src/tests/framework/txs.rs

@@ -18,6 +18,7 @@ use super::FUNDED_PRIVATE_KEYS;
 #[derive(Clone, Copy, Default)]
 pub struct BundleOpts {
     pub block_number_max: Option<u64>,
+    pub block_number_min: Option<u64>,
 }
 
 #[derive(Clone)]
@@ -28,6 +29,7 @@ pub struct TransactionBuilder {
     base_fee: Option<u128>,
     tx: TxEip1559,
     bundle_opts: Option<BundleOpts>,
+    with_reverted_hash: bool,
     key: Option<u64>,
 }
 
@@ -44,6 +46,7 @@ impl TransactionBuilder {
                 ..Default::default()
             },
             bundle_opts: None,
+            with_reverted_hash: false,
             key: None,
         }
     }
@@ -93,6 +96,11 @@ impl TransactionBuilder {
         self
     }
 
+    pub fn with_reverted_hash(mut self) -> Self {
+        self.with_reverted_hash = true;
+        self
+    }
+
     pub fn with_revert(mut self) -> Self {
         self.tx.input = hex!("60006000fd").into();
         self
@@ -145,16 +153,24 @@ impl TransactionBuilder {
     }
 
     pub async fn send(self) -> eyre::Result<PendingTransactionBuilder<Optimism>> {
+        let with_reverted_hash = self.with_reverted_hash;
         let bundle_opts = self.bundle_opts;
         let provider = self.provider.clone();
         let transaction = self.build().await;
+        let txn_hash = transaction.tx_hash();
         let transaction_encoded = transaction.encoded_2718();
 
         if let Some(bundle_opts) = bundle_opts {
             // Send the transaction as a bundle with the bundle options
             let bundle = Bundle {
                 transactions: vec![transaction_encoded.into()],
+                reverting_hashes: if with_reverted_hash {
+                    vec![txn_hash]
+                } else {
+                    vec![]
+                },
                 block_number_max: bundle_opts.block_number_max,
+                block_number_min: bundle_opts.block_number_min,
             };
 
             let result: BundleResult = provider

crates/op-rbuilder/src/tests/vanilla/revert.rs

@@ -27,6 +27,7 @@ async fn revert_protection_monitor_transaction_gc() -> eyre::Result<()> {
             .with_revert()
             .with_bundle(BundleOpts {
                 block_number_max: Some(i),
+                block_number_min: None,
             })
             .send()
             .await?;
@@ -123,42 +124,106 @@ async fn revert_protection_bundle() -> eyre::Result<()> {
     assert!(block_generated.includes(*valid_bundle.tx_hash()));
 
     let bundle_opts = BundleOpts {
-        block_number_max: Some(4),
+        block_number_max: Some(5),
+        block_number_min: None,
     };
 
-    let reverted_bundle = harness
+    let reverted_bundle_0 = harness
         .create_transaction()
         .with_revert()
+        .with_reverted_hash()
         .with_bundle(bundle_opts)
         .send()
         .await?;
 
-    // Test 2: Bundle reverts. It is not included in the block
+    // Test 2: Bundle reverts. It is included in the block since the transaction
+    // includes the reverted_hashes field
     let block_generated = generator.generate_block().await?; // Block 3
-    assert!(block_generated.not_includes(*reverted_bundle.tx_hash()));
+    assert!(block_generated.includes(*reverted_bundle_0.tx_hash()));
+
+    let reverted_bundle_1 = harness
+        .create_transaction()
+        .with_revert()
+        .with_bundle(bundle_opts)
+        .send()
+        .await?;
+
+    // Test 3: Bundle reverts. It is not included in the block since it reverts
+    // and the hash is not in the reverted_hashes field.
+    let block_generated = generator.generate_block().await?; // Block 4
+    assert!(block_generated.not_includes(*reverted_bundle_1.tx_hash()));
 
     // After the block the transaction is still pending in the pool
     assert!(harness
-        .check_tx_in_pool(*reverted_bundle.tx_hash())
+        .check_tx_in_pool(*reverted_bundle_1.tx_hash())
         .await?
         .is_pending());
 
     // Test 3: Chain progresses beyond the bundle range. The transaction is dropped from the pool
-    generator.generate_block().await?; // Block 4
+    generator.generate_block().await?; // Block 5
     assert!(harness
-        .check_tx_in_pool(*reverted_bundle.tx_hash())
+        .check_tx_in_pool(*reverted_bundle_1.tx_hash())
         .await?
         .is_pending());
 
-    generator.generate_block().await?; // Block 5
+    generator.generate_block().await?; // Block 6
     assert!(harness
-        .check_tx_in_pool(*reverted_bundle.tx_hash())
+        .check_tx_in_pool(*reverted_bundle_1.tx_hash())
         .await?
         .is_dropped());
 
     Ok(())
 }
 
+/// Test the behaviour of the revert protection bundle with a min block number.
+#[tokio::test]
+async fn revert_protection_bundle_min_block_number() -> eyre::Result<()> {
+    let harness = TestHarnessBuilder::new("revert_protection_bundle_min_block_number")
+        .with_revert_protection()
+        .build()
+        .await?;
+
+    let mut generator = harness.block_generator().await?;
+
+    // The bundle is valid when the min block number is equal to the current block
+    let bundle_with_min_block = harness
+        .create_transaction()
+        .with_revert() // the transaction reverts but it is included in the block
+        .with_reverted_hash()
+        .with_bundle(BundleOpts {
+            block_number_max: None,
+            block_number_min: Some(2),
+        })
+        .send()
+        .await?;
+
+    let block = generator.generate_block().await?; // Block 1, bundle still not valid
+    assert!(block.not_includes(*bundle_with_min_block.tx_hash()));
+
+    let block = generator.generate_block().await?; // Block 2, bundle is valid
+    assert!(block.includes(*bundle_with_min_block.tx_hash()));
+
+    // Send a bundle with a match of min and max block number
+    let bundle_with_min_and_max_block = harness
+        .create_transaction()
+        .with_revert()
+        .with_reverted_hash()
+        .with_bundle(BundleOpts {
+            block_number_max: Some(4),
+            block_number_min: Some(4),
+        })
+        .send()
+        .await?;
+
+    let block = generator.generate_block().await?; // Block 3, bundle still not valid
+    assert!(block.not_includes(*bundle_with_min_and_max_block.tx_hash()));
+
+    let block = generator.generate_block().await?; // Block 4, bundle is valid
+    assert!(block.includes(*bundle_with_min_and_max_block.tx_hash()));
+
+    Ok(())
+}
+
 /// Test the range limits for the revert protection bundle.
 #[tokio::test]
 async fn revert_protection_bundle_range_limits() -> eyre::Result<()> {
@@ -176,31 +241,54 @@ async fn revert_protection_bundle_range_limits() -> eyre::Result<()> {
     async fn send_bundle(
         harness: &TestHarness,
         block_number_max: u64,
+        block_number_min: Option<u64>,
     ) -> eyre::Result<PendingTransactionBuilder<Optimism>> {
         harness
             .create_transaction()
             .with_bundle(BundleOpts {
                 block_number_max: Some(block_number_max),
+                block_number_min: block_number_min,
             })
             .send()
             .await
     }
 
     // Max block cannot be a past block
-    assert!(send_bundle(&harness, 1).await.is_err());
+    assert!(send_bundle(&harness, 1, None).await.is_err());
 
     // Bundles are valid if their max block in in between the current block and the max block range
-    let next_valid_block = 3;
+    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).await.is_ok());
+        assert!(send_bundle(&harness, 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,
+        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());
+
+    // 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))
+        .await
+        .is_ok());
+
+    // A bundle with a min block equal to max block is valid
     assert!(
-        send_bundle(&harness, next_valid_block + MAX_BLOCK_RANGE_BLOCKS + 1)
+        send_bundle(&harness, next_valid_block, Some(next_valid_block))
             .await
-            .is_err()
+            .is_ok()
     );
 
     Ok(())
@@ -249,6 +337,7 @@ async fn revert_protection_check_transaction_receipt_status_message() -> eyre::R
         .with_revert()
         .with_bundle(BundleOpts {
             block_number_max: Some(3),
+            block_number_min: None,
         })
         .send()
         .await?;