-
Notifications
You must be signed in to change notification settings - Fork 146
feat(l1): generate execution witnesses during payload execution #5708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Lines of code reportTotal lines added: Detailed view |
There was a problem hiding this 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 implements caching for execution witnesses to improve response times for debug_executionWitness requests. The implementation stores the last 128 execution witnesses in the database after successful block execution via the Engine API.
Key changes:
- Adds a
--generate-witnessCLI flag to enable witness generation and caching - Stores execution witnesses after
newPayloadexecution when the flag is enabled - Implements cache lookup in
debug_executionWitnessas a fast path before generating witnesses on-demand
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/CLI.md | Documents the new --generate-witness flag and fixes whitespace formatting |
| crates/storage/api/tables.rs | Adds EXECUTION_WITNESSES table definition to store cached witnesses |
| crates/storage/store.rs | Implements witness storage, retrieval, and cleanup logic with MAX_WITNESSES=128 limit |
| crates/networking/rpc/rpc.rs | Adds generate_witness field to RpcApiContext to propagate the flag |
| crates/networking/rpc/engine/payload.rs | Generates and stores witness after successful payload execution |
| crates/networking/rpc/debug/execution_witness.rs | Checks cache before generating witness for single-block requests |
| crates/networking/rpc/test_utils.rs | Adds generate_witness=false to test contexts |
| crates/l2/networking/rpc/rpc.rs | Propagates generate_witness parameter in L2 RPC initialization |
| cmd/ethrex/l2/initializers.rs | Passes generate_witness flag to L2 RPC API |
| cmd/ethrex/initializers.rs | Passes generate_witness flag to L1 RPC API |
| cmd/ethrex/cli.rs | Defines --generate-witness CLI argument with default false |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
crates/storage/store.rs
Outdated
|
|
||
| let threshold = latest_block_number - MAX_WITNESSES; | ||
|
|
||
| self.get_oldest_witness_number()? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be an if-let
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 263e0e5!
JereSalo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
We should specify in the CLI flag more accurately that its purpose is to generate witnesses for blocks after being synced with the network and to also cache them.
suggested help = "After syncing blocks from the network, generate execution witnesses for the synced blocks and store them in the local witness cache." -
A better name for the flag in my opinion would be
cache-witnessesinstead. -
generate_witness_from_account_updatesandgenerate_witness_for_blocks_with_fee_configsare very untidy. We should at least open an issue because they are a pain to read IMO and they can be improved.
I have yet to review other things but I will submit this review for now.
|
|
||
| let vm_db = StoreVmDatabase::new(self.storage.clone(), parent_header.clone())?; | ||
| let vm = self.new_evm(vm_db)?; | ||
| let (mut vm, logger) = if self.options.generate_witness && self.is_synced() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a quick comment here saying what the purpose of this code is. Like just to execute with the database logger in order to generate the witness in one execution instead of executing more than once for this goal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done aa754b7!
crates/storage/api/tables.rs
Outdated
| /// Oldest witness block column family: [`Vec<u8>`] => [`Vec<u8>`] | ||
| /// - [`Vec<u8>`] = `b"oldest_witness_block"` | ||
| /// - [`Vec<u8>`] = `oldest_block_number.to_le_bytes()` | ||
| pub const OLDEST_WITNESS_BLOCK: &str = "oldest_witness_block"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is okay but it'd be good to specify that it's a number, like OLDEST_WITNESS_BLOCK_NUMBER
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 14173ad!
| pub current_accounts_state: CacheDB, | ||
| pub initial_accounts_state: CacheDB, | ||
| pub previous_tx_accounts_state: CacheDB, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add comments to each because it may be confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done aa754b7!
Motivation
To provide a quicker response to
debug_executionWitnessrequests.Description
This PR generates execution witnesses during payload execution and stores them for a period of time (128 blocks).
generate-witness.newPayloadmessage, if thegenerate-witnessflag is enabled, the node will generate and store theExecutionWitnessfor that block.debug_executionWitnessrequest, the node first checks if it has the witness in storage and only generates it as a fallback.Measurements
execution_witness_measurements.md contains a comparative analysis of
debug_executionWitnesslatency for pre-generated vs on-demand execution witnesses.Checklist
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync.