Skip to content
This repository was archived by the owner on Jan 16, 2026. It is now read-only.

Conversation

@yoshidan
Copy link
Contributor

Summary

  • Add InspectorFactory trait to enable custom EVM inspector injection during block execution
  • Extend StatelessL2Builder and KonaExecutor with optional inspector factory support
  • Use create_evm_with_inspector when a custom inspector is provided

Motivation

This change enables external tooling to observe and trace EVM execution during block building. The InspectorFactory pattern allows inspectors to be created fresh for each block execution, ensuring clean state and proper scoping.

This is useful for investigating derivation errors like the one seen in #3108.

Changes

  • New InspectorFactory trait (kona-executor): Defines a factory pattern for creating EVM inspectors with full documentation on implementation requirements
  • StatelessL2Builder: Added IF type parameter and inspector_factory field
  • KonaExecutor: Extended to propagate inspector factory through the execution pipeline
  • Call sites: Updated to pass None::<()> for default no-op inspector behavior

API

// Without custom inspector (default behavior)
let executor = KonaExecutor::new(config, provider, hinter, evm_factory, None, None::<()>);

// With custom inspector factory
let executor = KonaExecutor::new(config, provider, hinter, evm_factory, None, Some(my_factory));

Copilot AI review requested due to automatic review settings December 27, 2025 08:45
Copy link
Contributor

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 adds an InspectorFactory trait to enable custom EVM inspector injection during block execution, supporting external tooling for observing and tracing EVM execution. This is particularly useful for investigating derivation errors like the one described in #3108.

  • Introduces an InspectorFactory trait pattern for creating fresh inspectors per block execution
  • Extends StatelessL2Builder and KonaExecutor with optional inspector factory support via a new type parameter IF
  • Uses create_evm_with_inspector when a custom inspector is provided, falling back to create_evm otherwise

Reviewed changes

Copilot reviewed 8 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
crates/proof/executor/src/builder/core.rs Adds InspectorFactory trait with comprehensive documentation and implements it for (). Adds IF type parameter to StatelessL2Builder and implements conditional inspector usage with unsafe code
crates/proof/executor/src/builder/mod.rs Re-exports InspectorFactory trait
crates/proof/executor/src/builder/env.rs Adds IF: InspectorFactory bound to impl block
crates/proof/executor/src/builder/assemble.rs Adds IF: InspectorFactory bound to impl block
crates/proof/executor/src/lib.rs Re-exports InspectorFactory trait
crates/proof/executor/src/test_utils.rs Updates test fixtures to pass None::<()> for default inspector
crates/proof/proof/src/executor.rs Adds IF type parameter to KonaExecutor and propagates inspector factory through execution pipeline
crates/proof/proof-interop/src/consolidation.rs Updates SuperchainConsolidator with 'static bound on C and passes None::<()> at call site
bin/client/src/single.rs Adds inspector_factory parameter to run() function with comprehensive trait bounds
bin/client/src/kona.rs Passes None::<()> to run() function
bin/client/src/interop/transition.rs Adds 'static bounds to generic parameters and passes None::<()>
bin/client/src/interop/consolidate.rs Adds 'static bounds to generic parameters
bin/host/src/single/cfg.rs Passes None::<()> to client run() function
bin/host/src/interop/handler.rs Passes None::<()> to KonaExecutor::new()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 54 to 63
pub const fn new(
rollup_config: &'a RollupConfig,
trie_provider: P,
trie_hinter: H,
evm_factory: Evm,
inner: Option<StatelessL2Builder<'a, P, H, Evm>>,
inner: Option<StatelessL2Builder<'a, P, H, Evm, IF>>,
inspector_factory: Option<IF>,
) -> Self {
Self { rollup_config, trie_provider, trie_hinter, evm_factory, inner }
Self { rollup_config, trie_provider, trie_hinter, evm_factory, inner, inspector_factory }
}
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

Similar to the StatelessL2Builder::new() breaking change, KonaExecutor::new() now requires an additional inspector_factory parameter. This is a breaking API change for any external code.

The same backwards-compatibility approach suggested for StatelessL2Builder would apply here - consider providing both new() (without inspector) and new_with_inspector() methods to maintain backward compatibility.

Copilot uses AI. Check for mistakes.
P: TrieDBProvider + Send + Sync + Clone,
H: TrieHinter + Send + Sync + Clone,
Evm: EvmFactory + Send + Sync + Clone,
IF: InspectorFactory,
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

The KonaExecutor struct is marked with #[derive(Debug)] but the IF type parameter only requires InspectorFactory bound in the struct definition (line 30). This could cause a compilation error if InspectorFactory implementors don't implement Debug.

Consider adding Debug to the bounds on line 30:

IF: InspectorFactory + Debug,

This ensures the derived Debug implementation will work correctly.

Suggested change
IF: InspectorFactory,
IF: InspectorFactory + Debug,

Copilot uses AI. Check for mistakes.
P: TrieDBProvider,
H: TrieHinter,
Evm: EvmFactory,
IF: InspectorFactory,
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

Similar to KonaExecutor, the StatelessL2Builder struct is marked with #[derive(Debug)] but the IF type parameter only requires InspectorFactory bound (line 159). This could cause a compilation error if InspectorFactory implementors don't implement Debug.

Consider adding Debug to the bounds:

IF: InspectorFactory + Debug,

This ensures the derived Debug implementation will work correctly.

Suggested change
IF: InspectorFactory,
IF: InspectorFactory + core::fmt::Debug,

Copilot uses AI. Check for mistakes.
Comment on lines +372 to +407
let ex_result = match &self.inspector_factory {
// When no custom inspector factory is provided, use the standard create_evm().
None => {
// Step 2. Create the executor, using the trie database.
let evm = self.factory.evm_factory().create_evm(&mut state, evm_env);

// Step 3. Execute the block containing the transactions within the payload attributes.
let executor = self.factory.create_executor(evm, ctx);
executor.execute_block(transactions.iter())?
}
// When a custom inspector factory is provided, we need to use raw pointers
// to work around the borrow checker's limitations with generic types.
//
// NOTE: With a generic IF::Inspector, Rust cannot prove that the mutable
// borrow of state ends when the EVM is dropped. We use raw pointers to work
// around this. This is safe because:
// 1. The state is valid for the entire duration of EVM usage
// 2. The EVM is consumed by execute_block before we access state again
// 3. No other references to state exist during EVM usage
Some(factory) => {
let state_ptr = &mut state as *mut _;
let inspector = factory.create();

// Step 2. Create the executor, using the trie database.
// SAFETY: state is valid and not accessed elsewhere during EVM usage.
// The EVM is consumed by execute_block, ending the borrow.
let evm = self.factory.evm_factory().create_evm_with_inspector(
unsafe { &mut *state_ptr },
evm_env,
inspector,
);

// Step 3. Execute the block containing the transactions within the payload attributes.
let executor = self.factory.create_executor(evm, ctx);
executor.execute_block(transactions.iter())?
}
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

The new InspectorFactory feature lacks test coverage. While the existing code path with None::<()> uses the no-op inspector, there are no tests that verify:

  1. That a custom inspector factory actually gets called during block execution
  2. That the inspector receives the expected callbacks from the EVM
  3. That the inspector's state is properly scoped to a single block execution
  4. Edge cases like inspector errors or panics

Consider adding at least one test that uses a simple custom inspector (e.g., a call counter) to verify the integration works correctly. This would catch issues like the inspector not being created or not being passed to the EVM correctly.

Copilot uses AI. Check for mistakes.
Comment on lines +382 to +407
// When a custom inspector factory is provided, we need to use raw pointers
// to work around the borrow checker's limitations with generic types.
//
// NOTE: With a generic IF::Inspector, Rust cannot prove that the mutable
// borrow of state ends when the EVM is dropped. We use raw pointers to work
// around this. This is safe because:
// 1. The state is valid for the entire duration of EVM usage
// 2. The EVM is consumed by execute_block before we access state again
// 3. No other references to state exist during EVM usage
Some(factory) => {
let state_ptr = &mut state as *mut _;
let inspector = factory.create();

// Step 2. Create the executor, using the trie database.
// SAFETY: state is valid and not accessed elsewhere during EVM usage.
// The EVM is consumed by execute_block, ending the borrow.
let evm = self.factory.evm_factory().create_evm_with_inspector(
unsafe { &mut *state_ptr },
evm_env,
inspector,
);

// Step 3. Execute the block containing the transactions within the payload attributes.
let executor = self.factory.create_executor(evm, ctx);
executor.execute_block(transactions.iter())?
}
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

This use of unsafe and raw pointers appears to be working around a borrow checker issue, but this pattern is concerning for maintainability and safety.

The comment claims this is "safe because... The EVM is consumed by execute_block before we access state again", but there's a potential issue: if execute_block panics or returns early via ?, the raw pointer could leave state in an invalid borrowed state, violating Rust's safety guarantees.

Consider refactoring to avoid unsafe code entirely. One approach would be to factor out the common code between the two branches and use a trait object or enum to abstract over the inspector type. For example:

enum MaybeInspector<I> {
    None,
    Some(I),
}

Or use a wrapper that implements Inspector and conditionally delegates. This would eliminate the unsafe code while maintaining the same functionality.

Copilot uses AI. Check for mistakes.
@codecov
Copy link

codecov bot commented Dec 27, 2025

Codecov Report

❌ Patch coverage is 34.92063% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.1%. Comparing base (fe6dfcf) to head (def4628).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
bin/client/src/single.rs 0.0% 17 Missing ⚠️
crates/proof/executor/src/builder/core.rs 64.7% 12 Missing ⚠️
crates/proof/proof/src/executor.rs 0.0% 4 Missing ⚠️
bin/client/src/interop/transition.rs 0.0% 3 Missing ⚠️
bin/client/src/interop/consolidate.rs 0.0% 2 Missing ⚠️
bin/host/src/interop/handler.rs 0.0% 1 Missing ⚠️
bin/host/src/single/cfg.rs 0.0% 1 Missing ⚠️
crates/proof/proof-interop/src/consolidation.rs 0.0% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (fe6dfcf) and HEAD (def4628). Click for more details.

HEAD has 23 uploads less than BASE
Flag BASE (fe6dfcf) HEAD (def4628)
proof 11 0
e2e 11 0
unit 2 1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant