Make `EngineValidator` more extensible

[meyer9]

Describe the feature

Our goal is to reuse transaction validation work from flashblocks during engine validation.

Currently, the engine validator is an add-on that can be specified through the Reth Node API:

reth/crates/optimism/node/src/node.rs

Line 254 in 2305c3e

BasicEngineValidatorBuilder<OpEngineValidatorBuilder>,

However, there are private fields and structs, so it's not possible to implement the existing BasicEngineValidator externally.

Here are the changes we made to BasicEngineValidator to reuse execution results: https://github.com/base/base/pull/509/files#diff-d89f69cddd687427998ca98935d9568f4787fc1831f9cd032f8787fb720851d4

Our question is:

• Should we submit a PR to make more fields public and slightly refactor BasicEngineValidator to make it easier to wrap? Maybe we can decompose it slightly so the executor is a trait that can be overridden...
  • Add trait to allow wrapping Executor?
  • Pass execute_block as a callback to allow overriding just that method?
  • Extract execute_block to a new trait like EngineExecutor that EngineValidator would call?
  • Or implement CachedExecutionProvider upstream so you can also reuse flashblock execution?
  • Something else?

We'd like to avoid copying the entire engine-tree crate into our repo and try to make as many methods just self.inner.whatever() to avoid duplicating validate_block_with_state logic as much as possible.

Additional context

No response

[meyer9]

Here's an example of non-public fields that are used by BasicEngineValidator: https://github.com/base/base/blob/6fe64a4c06dc2386a49db1e7e27071b5f042286b/crates/client/engine-tree/src/tree/mod.rs#L134

[emmajam]

cc: @emhane

[meyer9]

@mattsse suggested inserting the block after the last flashblock so that the engine validator won't run (the block will already be inserted), however I don't think this is optimal.

This means we must calculate the state root between the end of the last flashblock execution and the start of the NewPayload call to get any benefit from reuse. If we don't calculate the state root in time, the NewPayload call will be received before we can insert the full block.

Ideally, we can allow partial reuse: if we execute 9/10 flashblocks before NewPayload, then NewPayload should be able to use results from those 9/10 flashblocks and only execute the transactions from the final flashblock.

Basically, the reuse should be transaction-granular rather than block-granular. This is what I have implemented here: https://github.com/base/base/pull/509/files#diff-b7b451b852cb6ebd6e6d4efb32d7fde7b8766e6b23d2d961f656c3221b8f3f4dR55

To upstream this, we could either implement CachedExecutor from my PR upstream or modify TreeState to accept InsertPartiallyExecutedBlock (currently there is only InsertExecutedBlock) which would provide transactions and results that will probably be included in the next block (no state root should be needed).

[emhane]

I think you want to impl EngineValidatorBuilder for OpEngineValidatorBuilder explicitly and set EngineValidatorBuilder::EngineValidator to OpEngineValidator.

if you want to reuse BasicEngineValidator as inner type of OpEngineValidator for this, then we should make a new type BasicEngineValidatorInner, following the pattern used for EthApi, EthApiInner and OpEthApi

[github-actions]

This issue is stale because it has been open for 21 days with no activity.