Finish block building process even when cancel request is found (#606)

## 📝 Summary

This PR introduces three changes:
- It refactors some of the common utilities shared among the tests into
a single `TestHarness` struct which spins the test framework with all
the components. Note that it does not update the tests to use this new
utility.
- It fixes an issue with the block builder that would stop the block
building process and not return any block if a cancel request was found.
This happens when an FCU and a getPayload request are called to close to
each other, the getPayload cancels the block building process, and
getPayload waits forever for a block that will never be built. Now, the
block building finishes.
- It adds an integration test to cover this use case with the new
utility.

## 💡 Motivation and Context

<!--- (Optional) Why is this change required? What problem does it
solve? Remove this section if not applicable. -->

---

## ✅ I have completed the following steps:

* [ ] Run `make lint`
* [ ] Run `make test`
* [ ] Added tests (if applicable)

Author: ferranbt

Cargo.lock

@@ -80,6 +80,7 @@ metrics.workspace = true
 serde_json.workspace = true
 tokio-util.workspace = true
 thiserror.workspace = true
+parking_lot.workspace = true
 
 time = { version = "0.3.36", features = ["macros", "formatting", "parsing"] }
 chrono = "0.4"

crates/op-rbuilder/Cargo.toml

@@ -1,7 +1,9 @@
 #[cfg(all(test, feature = "integration"))]
 mod tests {
     use crate::{
-        integration::{op_rbuilder::OpRbuilderConfig, op_reth::OpRethConfig, IntegrationFramework},
+        integration::{
+            op_rbuilder::OpRbuilderConfig, op_reth::OpRethConfig, IntegrationFramework, TestHarness,
+        },
         tester::{BlockGenerator, EngineApi},
         tx_signer::Signer,
     };
@@ -66,7 +68,7 @@ mod tests {
         let engine_api = EngineApi::new("http://localhost:1234").unwrap();
         let validation_api = EngineApi::new("http://localhost:1236").unwrap();
 
-        let mut generator = BlockGenerator::new(&engine_api, Some(&validation_api), false, 1, None);
+        let mut generator = BlockGenerator::new(engine_api, Some(validation_api), false, 1, None);
         generator.init().await?;
 
         let provider = ProviderBuilder::<Identity, Identity, Optimism>::default()
@@ -138,7 +140,7 @@ mod tests {
         let engine_api = EngineApi::new("http://localhost:1244").unwrap();
         let validation_api = EngineApi::new("http://localhost:1246").unwrap();
 
-        let mut generator = BlockGenerator::new(&engine_api, Some(&validation_api), false, 1, None);
+        let mut generator = BlockGenerator::new(engine_api, Some(validation_api), false, 1, None);
         let latest_block = generator.init().await?;
 
         let provider = ProviderBuilder::<Identity, Identity, Optimism>::default()
@@ -263,7 +265,7 @@ mod tests {
         let engine_api = EngineApi::new("http://localhost:1264").unwrap();
         let validation_api = EngineApi::new("http://localhost:1266").unwrap();
 
-        let mut generator = BlockGenerator::new(&engine_api, Some(&validation_api), false, 1, None);
+        let mut generator = BlockGenerator::new(engine_api, Some(validation_api), false, 1, None);
         let latest_block = generator.init().await?;
 
         let provider = ProviderBuilder::<Identity, Identity, Optimism>::default()
@@ -349,6 +351,25 @@ mod tests {
         Ok(())
     }
 
+    #[tokio::test]
+    #[cfg(not(feature = "flashblocks"))]
+    async fn integration_test_get_payload_close_to_fcu() -> eyre::Result<()> {
+        let test_harness = TestHarness::new("integration_test_get_payload_close_to_fcu").await;
+        let mut block_generator = test_harness.block_generator().await?;
+
+        // add some transactions to the pool so that the builder is busy when we send the fcu/getPayload requests
+        for _ in 0..10 {
+            // Note, for this test it is okay if they are not valid
+            test_harness.send_valid_transaction().await?;
+        }
+
+        // TODO: In the fail case scenario, this hangs forever, but it should return an error
+        // Figure out how to do timeout (i.e. 1s) on the engine api.
+        block_generator.submit_payload(None, 0, true).await?;
+
+        Ok(())
+    }
+
     #[tokio::test]
     #[cfg(feature = "flashblocks")]
     async fn integration_test_chain_produces_blocks() -> eyre::Result<()> {
@@ -410,7 +431,7 @@ mod tests {
         let engine_api = EngineApi::new("http://localhost:1234").unwrap();
         let validation_api = EngineApi::new("http://localhost:1236").unwrap();
 
-        let mut generator = BlockGenerator::new(&engine_api, Some(&validation_api), false, 2, None);
+        let mut generator = BlockGenerator::new(engine_api, Some(validation_api), false, 2, None);
         generator.init().await?;
 
         let provider = ProviderBuilder::<Identity, Identity, Optimism>::default()
@@ -539,8 +560,8 @@ mod tests {
         let validation_api = EngineApi::new("http://localhost:1246").unwrap();
 
         let mut generator = BlockGenerator::new(
-            &engine_api,
-            Some(&validation_api),
+            engine_api,
+            Some(validation_api),
             false,
             block_time_ms / 1000,
             None,

crates/op-rbuilder/src/integration/integration_test.rs

@@ -1,5 +1,18 @@
+use alloy_consensus::TxEip1559;
+use alloy_eips::BlockNumberOrTag;
+use alloy_eips::{eip1559::MIN_PROTOCOL_BASE_FEE, eip2718::Encodable2718};
+use alloy_provider::{Identity, Provider, ProviderBuilder};
+use op_alloy_consensus::OpTypedTransaction;
+use op_alloy_network::Optimism;
+use op_rbuilder::OpRbuilderConfig;
+use op_reth::OpRethConfig;
+use parking_lot::Mutex;
+use std::cmp::max;
+use std::collections::HashSet;
 use std::future::Future;
+use std::net::TcpListener;
 use std::path::Path;
+use std::sync::LazyLock;
 use std::{
     fs::{File, OpenOptions},
     io,
@@ -10,6 +23,10 @@ use std::{
 };
 use time::{format_description, OffsetDateTime};
 use tokio::time::sleep;
+use uuid::Uuid;
+
+use crate::tester::{BlockGenerator, EngineApi};
+use crate::tx_signer::Signer;
 
 /// Default JWT token for testing purposes
 pub const DEFAULT_JWT_TOKEN: &str =
@@ -195,3 +212,122 @@ impl Drop for IntegrationFramework {
         }
     }
 }
+
+const BUILDER_PRIVATE_KEY: &str =
+    "0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d";
+
+pub struct TestHarness {
+    _framework: IntegrationFramework,
+    builder_auth_rpc_port: u16,
+    builder_http_port: u16,
+    validator_auth_rpc_port: u16,
+}
+
+impl TestHarness {
+    pub async fn new(name: &str) -> Self {
+        let mut framework = IntegrationFramework::new(name).unwrap();
+
+        // we are going to use a genesis file pre-generated before the test
+        let mut genesis_path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
+        genesis_path.push("../../genesis.json");
+        assert!(genesis_path.exists());
+
+        // create the builder
+        let builder_data_dir = std::env::temp_dir().join(Uuid::new_v4().to_string());
+        let builder_auth_rpc_port = get_available_port();
+        let builder_http_port = get_available_port();
+        let op_rbuilder_config = OpRbuilderConfig::new()
+            .chain_config_path(genesis_path.clone())
+            .data_dir(builder_data_dir)
+            .auth_rpc_port(builder_auth_rpc_port)
+            .network_port(get_available_port())
+            .http_port(builder_http_port)
+            .with_builder_private_key(BUILDER_PRIVATE_KEY);
+
+        // create the validation reth node
+
+        let reth_data_dir = std::env::temp_dir().join(Uuid::new_v4().to_string());
+        let validator_auth_rpc_port = get_available_port();
+        let reth = OpRethConfig::new()
+            .chain_config_path(genesis_path)
+            .data_dir(reth_data_dir)
+            .auth_rpc_port(validator_auth_rpc_port)
+            .network_port(get_available_port());
+
+        framework.start("op-reth", &reth).await.unwrap();
+
+        let _ = framework
+            .start("op-rbuilder", &op_rbuilder_config)
+            .await
+            .unwrap();
+
+        Self {
+            _framework: framework,
+            builder_auth_rpc_port,
+            builder_http_port,
+            validator_auth_rpc_port,
+        }
+    }
+
+    pub async fn send_valid_transaction(&self) -> eyre::Result<()> {
+        // Get builder's address
+        let known_wallet = Signer::try_from_secret(BUILDER_PRIVATE_KEY.parse()?)?;
+        let builder_address = known_wallet.address;
+
+        let url = format!("http://localhost:{}", self.builder_http_port);
+        let provider =
+            ProviderBuilder::<Identity, Identity, Optimism>::default().on_http(url.parse()?);
+
+        // Get current nonce includeing the ones from the txpool
+        let nonce = provider
+            .get_transaction_count(builder_address)
+            .pending()
+            .await?;
+
+        let latest_block = provider
+            .get_block_by_number(BlockNumberOrTag::Latest)
+            .await?
+            .unwrap();
+
+        let base_fee = max(
+            latest_block.header.base_fee_per_gas.unwrap(),
+            MIN_PROTOCOL_BASE_FEE,
+        );
+
+        // Transaction from builder should succeed
+        let tx_request = OpTypedTransaction::Eip1559(TxEip1559 {
+            chain_id: 901,
+            nonce,
+            gas_limit: 210000,
+            max_fee_per_gas: base_fee.into(),
+            ..Default::default()
+        });
+        let signed_tx = known_wallet.sign_tx(tx_request)?;
+        let _ = provider
+            .send_raw_transaction(signed_tx.encoded_2718().as_slice())
+            .await?;
+
+        Ok(())
+    }
+
+    pub async fn block_generator(&self) -> eyre::Result<BlockGenerator> {
+        let engine_api = EngineApi::new_with_port(self.builder_auth_rpc_port).unwrap();
+        let validation_api = Some(EngineApi::new_with_port(self.validator_auth_rpc_port).unwrap());
+
+        let mut generator = BlockGenerator::new(engine_api, validation_api, false, 1, None);
+        generator.init().await?;
+
+        Ok(generator)
+    }
+}
+
+pub fn get_available_port() -> u16 {
+    static CLAIMED_PORTS: LazyLock<Mutex<HashSet<u16>>> =
+        LazyLock::new(|| Mutex::new(HashSet::new()));
+    loop {
+        let port: u16 = rand::random_range(1000..20000);
+        if TcpListener::bind(("127.0.0.1", port)).is_ok() && CLAIMED_PORTS.lock().insert(port) {
+            return port;
+        }
+    }
+}

crates/op-rbuilder/src/integration/mod.rs

@@ -514,18 +514,13 @@ impl<Txs> OpBuilder<'_, Txs> {
             ctx.metrics
                 .transaction_pool_fetch_duration
                 .record(best_txs_start_time.elapsed());
-            if ctx
-                .execute_best_transactions(
-                    &mut info,
-                    state,
-                    best_txs,
-                    block_gas_limit,
-                    block_da_limit,
-                )?
-                .is_some()
-            {
-                return Ok(BuildOutcomeKind::Cancelled);
-            }
+            ctx.execute_best_transactions(
+                &mut info,
+                state,
+                best_txs,
+                block_gas_limit,
+                block_da_limit,
+            )?;
         }
 
         // Add builder tx to the block

crates/op-rbuilder/src/payload_builder_vanilla.rs

@@ -62,7 +62,7 @@ async fn main() -> eyre::Result<()> {
         }
         Commands::Deposit { address, amount } => {
             let engine_api = EngineApi::builder().build().unwrap();
-            let mut generator = BlockGenerator::new(&engine_api, None, false, 1, None);
+            let mut generator = BlockGenerator::new(engine_api, None, false, 1, None);
 
             generator.init().await?;