Merge pull request #34 from flashbots/e2e/monitor-txn

Add e2e test to check that the builder emits a log when a transaction is discarded

Author: ferranbt

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

@@ -2,7 +2,8 @@
 mod tests {
     use crate::{
         integration::{
-            op_rbuilder::OpRbuilderConfig, op_reth::OpRethConfig, IntegrationFramework, TestHarness,
+            op_rbuilder::OpRbuilderConfig, op_reth::OpRethConfig, IntegrationFramework,
+            TestHarness, TestHarnessBuilder,
         },
         tester::{BlockGenerator, EngineApi},
         tx_signer::Signer,
@@ -94,10 +95,66 @@ mod tests {
         Ok(())
     }
 
+    #[tokio::test]
+    #[cfg(not(feature = "flashblocks"))]
+    async fn integration_test_monitor_transaction_drops() -> eyre::Result<()> {
+        // This test ensures that the transactions that get reverted an not included in the block
+        // are emitted as a log on the builder.
+        let harness = TestHarnessBuilder::new("integration_test_monitor_transaction_drops")
+            .with_revert_protection()
+            .build()
+            .await?;
+
+        let mut generator = harness.block_generator().await?;
+
+        // send 10 reverting transactions
+        let mut pending_txn = Vec::new();
+        for _ in 0..10 {
+            let txn = harness.send_revert_transaction().await?;
+            pending_txn.push(txn);
+        }
+
+        // generate 10 blocks
+        for _ in 0..10 {
+            let block_hash = generator.generate_block().await?;
+
+            // query the block and the transactions inside the block
+            let block = harness
+                .provider()?
+                .get_block_by_hash(block_hash)
+                .await?
+                .expect("block");
+
+            // blocks should only include two transactions (deposit + builder)
+            assert_eq!(block.transactions.len(), 2);
+        }
+
+        // check that the builder emitted logs for the reverted transactions
+        // with the monitoring logic
+        // TODO: this is not ideal, lets find a different way to detect this
+        // Each time a transaction is dropped, it emits a log like this
+        // 'Transaction event received target="monitoring" tx_hash="<tx_hash>" kind="discarded"'
+        let builder_logs = std::fs::read_to_string(harness.builder_log_path)?;
+
+        for txn in pending_txn {
+            let txn_log = format!(
+                "Transaction event received target=\"monitoring\" tx_hash=\"{}\" kind=\"discarded\"",
+                txn.tx_hash()
+            );
+
+            assert!(builder_logs.contains(txn_log.as_str()));
+        }
+
+        Ok(())
+    }
+
     #[tokio::test]
     #[cfg(not(feature = "flashblocks"))]
     async fn integration_test_revert_protection_disabled() -> eyre::Result<()> {
-        let harness = TestHarness::new("integration_test_revert_protection_disabled").await;
+        let harness = TestHarnessBuilder::new("integration_test_revert_protection_disabled")
+            .build()
+            .await?;
+
         let mut generator = harness.block_generator().await?;
 
         let txn1 = harness.send_valid_transaction().await?;
@@ -386,13 +443,15 @@ mod tests {
     #[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 test_harness = TestHarnessBuilder::new("integration_test_get_payload_close_to_fcu")
+            .build()
+            .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?;
+            let _ = test_harness.send_valid_transaction().await?;
         }
 
         // TODO: In the fail case scenario, this hangs forever, but it should return an error

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

@@ -219,16 +219,26 @@ 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,
+pub struct TestHarnessBuilder {
+    name: String,
+    use_revert_protection: bool,
 }
 
-impl TestHarness {
-    pub async fn new(name: &str) -> Self {
-        let mut framework = IntegrationFramework::new(name).unwrap();
+impl TestHarnessBuilder {
+    pub fn new(name: &str) -> Self {
+        Self {
+            name: name.to_string(),
+            use_revert_protection: false,
+        }
+    }
+
+    pub fn with_revert_protection(mut self) -> Self {
+        self.use_revert_protection = true;
+        self
+    }
+
+    pub async fn build(self) -> eyre::Result<TestHarness> {
+        let mut framework = IntegrationFramework::new(&self.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"));
@@ -245,7 +255,8 @@ impl TestHarness {
             .auth_rpc_port(builder_auth_rpc_port)
             .network_port(get_available_port())
             .http_port(builder_http_port)
-            .with_builder_private_key(BUILDER_PRIVATE_KEY);
+            .with_builder_private_key(BUILDER_PRIVATE_KEY)
+            .with_revert_protection(self.use_revert_protection);
 
         // create the validation reth node
 
@@ -259,19 +270,33 @@ impl TestHarness {
 
         framework.start("op-reth", &reth).await.unwrap();
 
-        let _ = framework
+        let builder = framework
             .start("op-rbuilder", &op_rbuilder_config)
             .await
             .unwrap();
 
-        Self {
+        let builder_log_path = builder.log_path.clone();
+
+        Ok(TestHarness {
             _framework: framework,
             builder_auth_rpc_port,
             builder_http_port,
             validator_auth_rpc_port,
-        }
+            builder_log_path,
+        })
     }
+}
+
+pub struct TestHarness {
+    _framework: IntegrationFramework,
+    builder_auth_rpc_port: u16,
+    builder_http_port: u16,
+    validator_auth_rpc_port: u16,
+    #[allow(dead_code)] // I think this is due to some feature flag conflicts
+    builder_log_path: PathBuf,
+}
 
+impl TestHarness {
     pub async fn send_valid_transaction(
         &self,
     ) -> eyre::Result<PendingTransactionBuilder<Optimism>> {

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

@@ -116,6 +116,9 @@ impl Service for OpRbuilderConfig {
             .arg("--datadir")
             .arg(self.data_dir.as_ref().expect("data_dir not set"))
             .arg("--disable-discovery")
+            .arg("--color")
+            .arg("never")
+            .arg("--builder.log-pool-transactions")
             .arg("--port")
             .arg(self.network_port.expect("network_port not set").to_string())
             .arg("--ipcdisable");

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

@@ -80,6 +80,8 @@ impl Service for OpRethConfig {
             .arg("--datadir")
             .arg(self.data_dir.as_ref().expect("data_dir not set"))
             .arg("--disable-discovery")
+            .arg("--color")
+            .arg("never")
             .arg("--port")
             .arg(self.network_port.expect("network_port not set").to_string())
             .arg("--ipcdisable");