Allow creating separate execution-only and storage proofs #28
Conversation
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
| Ok(Vec::default()) | ||
| } | ||
|
|
||
| fn flat_witness(&self, _record: FlatWitnessRecord) -> ProviderResult<FlatPreState> { |
There was a problem hiding this comment.
flat_witness is a new method added to the StateProofProvider. Analogous to the existing witness one. I'll ignore commenting on mock/test-utils impl of StateProofProvider, and only in the relevant ones.
| /// 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>, | ||
| } |
There was a problem hiding this comment.
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 destructed_addresses is a rabbit hole that I'll explain later. But for now feel safe to not pollute understanding.
|
|
||
| /// Records pre-state accesses that occurred during execution. | ||
| #[derive(Debug, Clone, Default)] | ||
| pub struct FlatWitnessRecord { |
There was a problem hiding this comment.
The FlatWitnessRecord is analogous to WitnessRecord.
You can refresh how this worked for WitnessRecord in the blockchain_test.rs file, but the TL:DR is that when we execute a block, we can pass a closure that returns a State<DB> that we can use to capture which state was accessed.
I'll create a comment in that file later in the review.
| pub enum AccessedAccount { | ||
| /// Indicates if the account was destroyed during execution. | ||
| Destroyed, | ||
| /// Storage keys accessed during execution. | ||
| StorageKeys(HashSet<U256>), | ||
| } |
There was a problem hiding this comment.
This is a good time to explain a quirk about how things work today in Reth regarding witness generation.
When the State<DB> is passed in the closure after the block execution. State<DB> basically has the in-memory cache of all the state accessed/written during the execution.
The way this is used for the usual ExecutionWitness is the same as for FlatExecutionWitness, but there's a catch.
If an account was destroyed during execution, State<DB> set the account as None, since it was destroyed. A priori that is okay to give that signal, but unfortunately that means that it wiped out all the storage slots captured during the 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 ExecutionWitness, so... how is this solved today then?
The answer is that Reth includes all storage slots of destroyed accounts. If that feels surprising, see here.
In that link, we see the get_proof_targets() used when building the MPT proof for the usual ExecutionWitness. For wiped (i.e., destroyed) storage tries, it includes all storage slots! And not only the storage slots used during execution, which means that probably the witness might be bloated.
Now, here's an important practical detail why this isn't a problem today:
- Pre-cancun, SELFDESTROY indeed deletes the storage trie of a contract, thus situation can arise (i.e., witness bloating due to including all storage slots)
- Post-cancun, there was an EIP that if you SELFDESTROY an account that exists, it won't destroy the storage slots anymore.
This means that for post-Cancun blocks, the wiped flag won't happen since the storage trie isn't really destroyed thus the bloating won't happen.
"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 Provider traits implementations, that we'll see later... but after you understand the problem, you can understand the "why" of those.
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 State<DB> lost information of accessed storage slots in the case the account is destroyed. For destroying a storage trie, we don't really need all the values since the account will be nuked from the account trie, so we don't need to know their values. I think at some point would be nice to ask Roman or someone else about this.
To be honest, I think for correct witness generation, ideally we want the State<DB> to properly register the pre-state more cleanly, insetad of only registering the addresses and storage slots (since State contains post-state values) and having to use Providers after to get the pre-state.
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.
| pub fn record_executed_state<DB>(&mut self, statedb: &State<DB>) { | ||
| self.contracts = statedb | ||
| .cache | ||
| .contracts |
There was a problem hiding this comment.
Here's another thing that I believe is a problem today (both for FlatExecutionWitness and to the existing ExecutionWitness).
As I mentioned before, statedb.cache contains all hte state that was read/written during block execution. As said before, that's nice since we can know which accounts and storage slots where touched so we can get the pre-state.
But I think that statedb.cache.contracts also has newly created contracts, which we shouldn't include in the witness.
If that sounds suprising, see here (this is master code). I honestly, don't know why new contracts needs to be in the witness. New bytecode will be generated during the re-execution, so there's no need to include it.
In any case, even if we remove that .chain(...), I checked and new contracts will also be in statedb.cache.contracts (i.e., revm only goes to the database if doesn't have any contract bytecode cached there, which also includes new contracts (which makes sense)).
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).
|
|
||
| // Now validate using the stateless client if everything else passes | ||
| for (recovered_block, execution_witness) in &program_inputs { | ||
| for (recovered_block, execution_witnesses) in &program_inputs { |
There was a problem hiding this comment.
Here comes the stateless EEST runs.
| public_keys, | ||
| execution_witness.clone(), | ||
| // Validate stateless execution using a sparse trie for the storage access. | ||
| let (_, trie_post_state) = stateless_validation_with_trie::<StatelessSparseTrie, _, _>( |
There was a problem hiding this comment.
We keep doing hte usual stateless execution using a sparse merkle trie that runs both execution and storage; nothing new.
| // Validate stateless execution using a flatdb for the storage access. | ||
| let (_, flatdb_post_state) = stateless_validation_with_flatdb::<_, _>( | ||
| block.clone(), | ||
| public_keys.clone(), | ||
| execution_witnesses.flatdb.clone(), | ||
| chain_spec.clone(), | ||
| EthEvmConfig::new(chain_spec.clone()), | ||
| ) | ||
| .expect("stateless validation with flatdb failed"); |
There was a problem hiding this comment.
Now we check the execution-only run.
| if trie_post_state != flatdb_post_state { | ||
| return Err(Error::Assertion( | ||
| "Post state mismatch between trie and flatdb implementations".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
We do a sanity check that both post-states of the normal execution+storage run, and the execution-only generated the same post-state.
If that isn't the case, something is wrong.
| // Validate that the flatdb used as pre-state can be proven using a sparse trie (i.e., in a | ||
| // different proof). Also checks that the post-state diff generated during flatdb | ||
| // execution results in the expected post-state root. | ||
| stateless_validation_flatdb_storage_check::<StatelessSparseTrie>( | ||
| block, | ||
| execution_witnesses.trie.clone(), | ||
| execution_witnesses.flatdb.state.clone(), | ||
| flatdb_post_state, | ||
| ) | ||
| .expect("stateless flatdb state check failed"); |
There was a problem hiding this comment.
We validate that the storage proof STF, using the generated flatdb post state in the previous execution only, validates correctly, i.e., i) the flatdb matches the spase merkle trie, ii) the post-state calculation using flatdb_post_state ends up validating the header.state_root.
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
| let mut db = WitnessDatabase::new(&trie, Default::default(), ancestor_hashes); | ||
| for (address, flatdb_account) in flatdb_pre_state.accounts { | ||
| let trie_account = db | ||
| .basic(address) | ||
| .map_err(|_| StatelessValidationError::GetAccountFromWitnessDatabase)?; | ||
| let flatdb_account_info = (flatdb_account.account_state != AccountState::NotExisting) | ||
| .then_some(flatdb_account.info); | ||
|
|
||
| if trie_account != flatdb_account_info { | ||
| return Err(StatelessValidationError::FlatdbAccountStateMismatch { address }); | ||
| } | ||
| for (slot, value) in flatdb_account.storage { | ||
| let trie_value = db | ||
| .storage(address, slot) | ||
| .map_err(|_| StatelessValidationError::GetStorageSlotFromWitnessDatabase)?; | ||
| if trie_value != value { | ||
| return Err(StatelessValidationError::FlatdbStorageSlotStateMismatch { | ||
| address, | ||
| slot, | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Here is the critical check that if we query all the information in the flatdb pre-state, it exactly matches what we have in the (cryptographically validated) sparse merkle trie.
Said differently, the flatdb pre-state is a fair representation of the sparse merkle tree. The sparse merkle tree could have more information, which it is okay... but anything in the flatdb MUST be in the trie. This is what validates the execution-only proof used state that is correct.
1 participant
Note: I doubt we might merge this PR, and might get seen mostly as an experiment to run some benchmarks without merging into our fork until we fully process all I discovered here
Currently, Reth has a
stateless_validationfunction, which we use as the STF to generate L1 proofs. This method proves:Said differently, the proof proves both block execution and storage.
This PR adds the capability of proving things independently:
stateless_validation_with_flatdbwhich, instead of a usualExecutionWitnessit receives aFlatExecutionWitness. ThisFlatExecutionWitnessinstead of having a list of RLP encoded MPT nodes to rebuild a sparse MPT, it has a “flat database” i.e., ~a hashmap of accounts and storage trie values directly (no MPT nodes). This flat database is used for doing only block execution.stateless_validation_flatdb_storage_checkwhich receives:ExecutionWitness, which has the sparse MPT nodesThe idea of 1. is that the proof of this STF does not involve any MPT deserialization nor lots of keccak-ing verifying it. It goes directly to the block validation logic using a flat database (thus accessing state should be faster than using the underlying sparse MPT).
The idea of 2. is that we check multiple things:
ExecutionWitnessis correct. As in, this is our cryptographically verified source of truth for state since we verify the pre-state “as usual”.The EEST fixture runs today are run in stateful and stateless mode (usual execution + storage). But I also know added verifying the two new styles of runs passing the tests: i) execution-only, and ii) storage only. Said differently, if we do the execution-only STF, and use the pre-state+generated-post-state into the storage-only STF, then the pre-state validation against the sparse MPT plus the post-state root must check. (Basically is a whole consitency check — I’ll point this in a PR comment where can be seen more clearly). All EEST tests pass (this wasn't easy, failed ones actually helped to discover many rabbit holes)
Also, a new RPC is created, analogous to
debug_executionWitnesssto get the new type of witnessFlatExecutionWitness. For now this RPC is meant to have the witness for the execution-only proof, but prob we’ll need to make it return both witnesses since that will be needed for the “storage proof”.This is the TL;DR of the PR — but there’re other details to mention, which I’ll dive a bit deeper in PR comments