-
Couldn't load subscription status.
- Fork 1
Allow creating separate execution-only and storage proofs #28
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: kw/zkevm-benchmark-workload-repo
Are you sure you want to change the base?
Changes from all commits
78b38e3
bfbfbfa
4e5c3ae
8f898db
a9d8c60
adf0a58
4930f05
c9316c3
34fb8bd
de7b24d
c864358
a7df8ae
ea5023d
8d1b404
29f7c10
c655f1a
7a8c7cd
036c910
e386456
1ccd4b3
e9404fb
eca4e8b
404193d
8b215ad
5296ca0
1d35925
5329128
6d869a8
2996b4e
1cb8207
74d5b99
dc53328
8151362
c69de5d
46bb898
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| //! Witness recording types for EVM execution. | ||
|
|
||
| use alloy_primitives::{keccak256, Address, B256, U256}; | ||
| use revm::{ | ||
| database::{AccountStatus, DbAccount, State}, | ||
| primitives::{HashMap, HashSet}, | ||
| state::Bytecode, | ||
| }; | ||
|
|
||
| /// Records pre-state data for witness generation. | ||
| #[derive(Debug, Clone, Default)] | ||
| pub struct FlatPreState { | ||
| /// Accounts accessed during execution. | ||
| pub accounts: HashMap<Address, DbAccount>, | ||
| /// Bytecode accessed during execution. | ||
| pub contracts: HashMap<B256, Bytecode>, | ||
| /// The set of addresses that have been self-destructed in the execution. | ||
| pub destructed_addresses: HashSet<Address>, | ||
| } | ||
|
Comment on lines
+10
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is basically the "state witness" used for the execution-only STF. It is the most important field of the new FlatExecutionWitness. The need for |
||
|
|
||
| /// Records pre-state accesses that occurred during execution. | ||
| #[derive(Debug, Clone, Default)] | ||
| pub struct FlatWitnessRecord { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The I'll create a comment in that file later in the review. |
||
| /// Accounts accessed during execution. | ||
| pub accounts: HashMap<Address, AccessedAccount>, | ||
| /// Bytecode accessed during execution. | ||
| pub contracts: HashMap<B256, Option<Bytecode>>, | ||
| } | ||
|
|
||
| /// Represents an accessed account during execution. | ||
| #[derive(Debug, Clone)] | ||
| pub enum AccessedAccount { | ||
| /// Indicates if the account was destroyed during execution. | ||
| Destroyed, | ||
| /// Storage keys accessed during execution. | ||
| StorageKeys(HashSet<U256>), | ||
| } | ||
|
Comment on lines
+32
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good time to explain a quirk about how things work today in Reth regarding witness generation. When the The way this is used for the usual If an account was destroyed during execution, This means that for destroyed accounts, we lost information on which storage slots were accessed before the account was SELFDESTROYed. This has somewhat nasty implications for witness generation, since now we can't know which storage slots for this contract we must put in the witness pre-state. When I realized this through a failing EEST test, I was surprised since wasn't obvious to me how to solve it. But.. in theory this should be already solved in the The answer is that Reth includes all storage slots of destroyed accounts. If that feels surprising, see here. In that link, we see the Now, here's an important practical detail why this isn't a problem today:
This means that for post-Cancun blocks, the "In short", this "include all storage slots" is a bloating that can only happen for pre-Cancun blocks. While doesn't sound like an "urgent" problem, I think will be a problem whenever we want to standarize the witness generation (if the standard also covers older forks)... since clearly adding more storage slots as needed isn't required. Sorry for the length, but I wanted to explain this correctly. This situation of SELFDESTRUCT having to add all the storage slots, means a decent amount of complexity in the I've been thinking if there's a reason why they though would be needed to include all the slots, but the only reason I can think of is to workaround the problem that To be honest, I think for correct witness generation, ideally we want the For this PR, I didn't want to change revm, etc -- but I think doing proper pre-state capturing in revm is probably the best thing to do in the long run. But before doing that, probably is worth discussing with them. |
||
|
|
||
| impl FlatWitnessRecord { | ||
| /// Records the accessed state after execution. | ||
| pub fn record_executed_state<DB>(&mut self, statedb: &State<DB>) { | ||
| self.contracts = statedb | ||
| .cache | ||
| .contracts | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's another thing that I believe is a problem today (both for As I mentioned before, But I think that If that sounds suprising, see here (this is In any case, even if we remove that So... this also means that unless I'm not mistaken, there is probably extra bloating today of bytecodes in the execution witness. I'm thinking of creating some EEST test to triple-check this, but I feel confident about it (plus, that last link comment I think sounds wrong). |
||
| .values() | ||
| .map(|code| (keccak256(code.original_bytes()), Some(code.clone()))) | ||
| .collect(); | ||
|
|
||
| for (address, account) in &statedb.cache.accounts { | ||
| let account = match account.status { | ||
| AccountStatus::Destroyed => AccessedAccount::Destroyed, | ||
| _ => { | ||
| let storage_keys = account | ||
| .account | ||
| .as_ref() | ||
| .map_or_else(HashSet::default, |a| a.storage.keys().copied().collect()); | ||
| AccessedAccount::StorageKeys(storage_keys) | ||
| } | ||
| }; | ||
| self.accounts.insert(*address, account); | ||
| } | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The two above comments are probably teh only "hairy" part of what was discovered while doing this PR -- the rest should feel more natural. |
||
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.
flat_witnessis a new method added to theStateProofProvider. Analogous to the existingwitnessone. I'll ignore commenting on mock/test-utils impl ofStateProofProvider, and only in the relevant ones.