In-process tests, optional dockerized validation node #132

karim-agha · 2025-06-06T14:34:10Z

📝 Summary

This PR closes the following issues:

Few things happened here:

RBuilder running in-process

Now we have a new type called LocalInstance that runs an entire RBuilder instance in-process. It uses IPC transport instead of HTTP for RPC and EngineAPI. This LocalInstance represents op-rbuilder node and can be configured by giving it an optional NodeConfig or RBuilderArgs structures. Examples:

let fb_node = LocalInstance::flashblocks().await?;
let standard_node = LocalInstance::standard().await?;
let fb_with_args = LocalInstance::new::<FlashblocksBuilder>(OpRBuilderArgs { ... }).await?;
let standard_with_args_and_node_config = LocalInstance::new_with_config::<StandardBuilder>(
   OpRBuilderArgs { ... }, 
   NodeConfig<OpChainSpec> {...}
).await?;

Built-in validation node management

An external op-reth node can be attached to any test using the following snippet:

let rbuilder = LocalInstance::standard().await?;
let driver = rbuilder
    .driver()
    .await?
    .with_validation_node(ExternalNode::reth().await?)
    .await?;

When an external node is attached to a chain driver, it will automatically sync with the LocalInstance that is running and then ingest every newly built block. Only if the external node validates and accepts a block as its canonical chain the block building process is considered successful.

We support attaching multiple validation node to one local instance:

let rbuilder = LocalInstance::standard().await?;
let driver = rbuilder
    .driver()
    .await?
    .with_validation_node(ExternalNode::reth().await?)
    .with_validation_node(ExternalNode::geth().await?)
    .await?;

Better `OpRbuilderArgs` defaults

The Default implementation of the cli arguments type and all its descendants will use the defaults specified by clap attributes instead of rust default values.

Chain Driver

When interacting with a LocalInstance we have now ChainDriver that is used to trigger production of new blocks, get latest blocks, etc. It can be either instantiated by ChainDriver::new(&local_instance) or local_instance.driver().

Better transaction pool events inspector

In LocalInstance all transaction pool events are recorded in a strongly typed manner inside TransactionPoolObserver. We record their entire state history, so we can later validate the correct order of transitions. The public API for this is:

driver.build_new_block().await?;
assert!(rbuilder.pool().is_dropped(*reverted_bundle.tx_hash()));
assert!(rbuilder.pool().is_pending(*reverted_bundle.tx_hash()));
// ...
rbuilder.pool().history(tx_hash).unwrap().contains(...)
rbuilder.pool().tx_status(tx_hash).

Transactions have states that are instance of TransactionEvent from the transaction-pool crate in reth. Namely:

pub enum TransactionEvent {
    Pending,
    Queued,
    Mined(B256),
    Replaced(TxHash),
    Discarded,
    Invalid,
    Propagated(Arc<Vec<PropagateKind>>),
}

Test tracing

You can now see detailed logs of your unit tests by setting the TEST_TRACE env variable. That will print out all reth and op-rbuilder logs to console when they're running. Usage:

$ TEST_TRACE=on cargo test
$ TEST_TRACE=error cargo test
$ TEST_TRACE=info cargo test
$ TEST_TRACE=debug cargo test
$ TEST_TRACE=trace cargo test

✅ I have completed the following steps:

Run make lint
Run make test
Added tests (if applicable)

Copilot

Pull Request Overview

This PR refactors the in-test infrastructure to use a new in-process LocalInstance and ChainDriver, replaces the previous TestHarness and related service modules, extends the test framework with utilities and transaction-pool observers, and updates dependencies and CI settings for test support.

Introduce LocalInstance and ChainDriver APIs and remove the old TestHarness/service modules
Update all vanilla and flashblocks tests to use the new driver-based interface
Add framework/utils.rs, transaction-pool observation, test logging init, and new Cargo features for testing

Reviewed Changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
crates/op-rbuilder/src/tests/vanilla/ordering.rs	Switched from `TestHarnessBuilder` to `LocalInstance` & `ChainDriver`
crates/op-rbuilder/src/tests/vanilla/data_availability.rs	Updated size-limit tests to use new driver API
crates/op-rbuilder/src/tests/framework/utils.rs	Added testing helper traits (`ChainDriverExt`, `BlockTransactionsExt`, etc.)
crates/op-rbuilder/src/tests/framework/instance.rs	Added `LocalInstance` implementation
crates/op-rbuilder/Cargo.toml	Added testing dependencies & features (duplicate entries)
.github/workflows/checks.yaml	Inject `TESTS_TEMP_DIR` env var for tests

Comments suppressed due to low confidence (6)

crates/op-rbuilder/Cargo.toml:119

There are duplicate dashmap entries in this Cargo.toml (once as optional, once unqualified). Remove or consolidate one declaration to avoid cargo manifest parsing errors.

dashmap = { version = "6.1", optional = true }

crates/op-rbuilder/Cargo.toml:120

Duplicate nanoid declaration—listed both as optional and non-optional further down. Consolidate these entries to avoid conflicts.

nanoid = { version = "0.4", optional = true }

crates/op-rbuilder/Cargo.toml:121

You have two reth-ipc definitions (optional and non-optional). Remove the redundant entry to prevent cargo errors.

reth-ipc = { workspace = true, optional = true }

crates/op-rbuilder/Cargo.toml:122

Duplicate bollard entries exist. Consolidate to a single declaration to keep the manifest valid.

bollard = { version = "0.19", optional = true }

crates/op-rbuilder/Cargo.toml:123

[nitpick] The tar crate is only declared once, but please verify if it should also be gated under the testing feature rather than listed unconditionally.

tar = { version = "0.4", optional = true }

.github/workflows/checks.yaml:76

The TESTS_TEMP_DIR environment variable is set here but not referenced in the test code. Either remove it or update the test framework to honor this variable.

TESTS_TEMP_DIR: ${{ github.workspace }}

karim-agha · 2025-06-06T15:24:37Z

Will follow up with another PR after this goes through for #133

.github/workflows/checks.yaml

SozinM · 2025-06-09T09:48:44Z

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

    } else {
-        println!("{}", serde_json::to_string_pretty(&genesis)?);
+        debug!("{}", serde_json::to_string_pretty(&genesis)?);


i think we want println here so we could easily copy genesis

As part of this PR I have added support for printing all reth and op-rbuilder logs into console during tests. You can do this:

$ TEST_TRACE=on cargo test $ TEST_TRACE=error cargo test $ TEST_TRACE=info cargo test $ TEST_TRACE=debug cargo test $ TEST_TRACE=trace cargo test

And it will print out to console everything that happens during the test you need to debug at the debug level you need.

SozinM · 2025-06-09T09:57:57Z

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

+            .duration_since(std::time::UNIX_EPOCH)
+            .map_err(|_| eyre::eyre!("Failed to get current system time"))?;
+
+        // block timestamp will be the max of the actual timestamp and the latest block


Were we using this logic anywhere before? Is there a need for this logic instead of spec-compliant logic?

We are calling build_new_block at various intervals but the chain supports 1s time resolution. This is here to ensure that if we build blocks in close enough intervals, that the assumed invariants still hold. Open to discussion whether we want to keep this logic or remove it.

SozinM · 2025-06-09T10:06:48Z

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

+            .with_da_config(da_config)
+            .build();
+
+        let node_builder = NodeBuilder::<_, OpChainSpec>::new(config.clone())


Would we need to keep this in sync with the main?

I will do that in a follow-up PR. The logic is slightly different between main and LocalInstance but I agree that the common bits need to be kept in sync in one place.

SozinM · 2025-06-09T10:10:15Z

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

+    }
+}
+
+impl Drop for ExternalNode {


Big point to check:
i'm not sure that Drop is invoked when you ctrl + c from cargo test

Added a handler that will clean up the test on ctrl-c.

That is great!
I was struggling sometime with left op-reth instances, thanks a lot!

SozinM · 2025-06-09T10:14:36Z

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

        .raw_request::<(i32, i32), bool>("miner_setMaxDASize".into(), (0, 1))
        .await?;
    assert!(call, "miner_setMaxDASize should be executed successfully");

-    let unfit_tx = harness.send_valid_transaction().await?;
-    let block = generator.generate_block().await?;
+    let unfit_tx = driver


loved it when we needed to just call send_valid_transaction and it was doing everything under the hood

Added a helper method that does it.

now this tests is:

#[tokio::test] async fn block_size_limit() -> eyre::Result<()> { let rbuilder = LocalInstance::standard().await?; let driver = rbuilder.driver().await?; // Set block da size to be small, so it won't include tx let call = driver .provider() .raw_request::<(i32, i32), bool>("miner_setMaxDASize".into(), (0, 1)) .await?; assert!(call, "miner_setMaxDASize should be executed successfully"); let (unfit_tx, block) = driver.build_new_block_with_valid_transaction().await?; // tx should not be included because we set the tx_size_limit to 1 assert!( !block.includes(&unfit_tx), "transaction should not be included in the block" ); Ok(()) }

SozinM · 2025-06-09T10:16:06Z

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

+            .await?;
+        let block = driver.build_new_block().await?;
+
+        assert!(block


Why have we removed includes? :(

it was not removed just not used in this test. Updated the test to use:

assert!(block.includes(valid_tx.tx_hash())); assert!(block.includes(reverting_tx.tx_hash()));

SozinM · 2025-06-09T10:19:46Z

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

        "Pending pool must contain at max 50 txs {:?}",
-        pool
+        rbuilder.pool().pending_count()


also want to see queue count in here

you can already do that, in txs.rs you have:

pub fn count(&self, status: TransactionEvent) -> usize { self.observations .iter() .filter(|tx| tx.value().back() == Some(&status)) .count() } pub fn pending_count(&self) -> usize { self.count(TransactionEvent::Pending) } pub fn queued_count(&self) -> usize { self.count(TransactionEvent::Queued) } pub fn dropped_count(&self) -> usize { self.count(TransactionEvent::Discarded) }

SozinM · 2025-06-09T10:20:07Z

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


    // Ensure that 10 extra txs got included
-    assert!(block.includes_vec(txs));
+    assert!(txs.into_iter().all(|tx| block.includes(&tx)));


QOL decrease

Now includes supports individual txs and collections of txs.

This snippet is now:

// Ensure that 10 extra txs got included assert!(block.includes(&txs));

karim-agha added 8 commits June 6, 2025 11:27

Validation node works and tests are passing using the new test infra

47f5a55

merge with main

7b36026

revert_protection_monitor_transaction_gc passes

de69af7

revert_protection_disabled passes

c8c04a1

ordering tests are passing

1433286

all tests are passing

5f18e65

ready

0833fed

merge with main

e1459b1

karim-agha requested review from ferranbt, SozinM and avalonche as code owners June 6, 2025 14:34

karim-agha mentioned this pull request Jun 6, 2025

In-process tests and automatic builder variant tests #91

Closed

3 tasks

karim-agha requested review from Copilot, ferranbt and SozinM and removed request for ferranbt, avalonche and SozinM June 6, 2025 14:35

Copilot AI reviewed Jun 6, 2025

View reviewed changes

karim-agha added 4 commits June 6, 2025 14:40

lint

213b7ff

Github Action login to GHCR.io

de85b60

testing Github Actions ghcr access

4417ae1

testing Github Actions ghcr access

0b8938e

karim-agha added 2 commits June 9, 2025 08:33

merge with main

8314713

lint

ba56dba

SozinM reviewed Jun 9, 2025

View reviewed changes

Closes issue 133

320b2d6

karim-agha added 7 commits June 10, 2025 08:53

review feedback

dd046c0

review feedback

600b119

Review feedback - ctrl-c

8bb45d2

Review feedback

0cfa9db

review feedback

0216e3b

Review feedback

39d99b2

merged with main

42e96d4

SozinM approved these changes Jun 10, 2025

View reviewed changes

review feedback

dae1716

In-process tests, optional dockerized validation node #132

Are you sure you want to change the base?

In-process tests, optional dockerized validation node #132

Conversation

karim-agha commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Summary

RBuilder running in-process

Built-in validation node management

Better OpRbuilderArgs defaults

Chain Driver

Better transaction pool events inspector

Test tracing

✅ I have completed the following steps:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

karim-agha commented Jun 6, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

karim-agha commented Jun 6, 2025 •

edited

Loading

Better `OpRbuilderArgs` defaults