Skip to content

Conversation

karim-agha
Copy link
Contributor

@karim-agha karim-agha commented Jun 6, 2025

📝 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)

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

@karim-agha karim-agha self-assigned this Jun 10, 2025
@karim-agha karim-agha merged commit 046a730 into main Jun 11, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants