This repository was archived by the owner on Jul 5, 2024. It is now read-only.
feat: panic on invalid account lookups#1092
Merged
Merged
Conversation
0fabc6d to
2108aac
Compare
84f4b43 to
c74896e
Compare
3edb370 to
89dd62d
Compare
lispc
reviewed
Jan 24, 2023
lispc
approved these changes
Jan 24, 2023
ChihChengLiang
approved these changes
Jan 24, 2023
Collaborator
ChihChengLiang
left a comment
There was a problem hiding this comment.
LGTM. I added some feedback.
- Account field lookups for fields different than CodeHash are invalid if the account doesn't exist. - All account field lookups should contain a value_prev that matches with the field value found in the StateDB for that account. If any of these two points don't hold, the corresponding chain of MPT proofs can't be generated. Test with: `cargo test --profile release evm_circuit --all-features -- --nocapture 2>&1 | tee /tmp/zkevm-tests.log` from `zkevm-circuits`.
89dd62d to
1ccc071
Compare
1ccc071 to
4853f08
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After this was merged privacy-ethereum/zkevm-specs#354 the rule for opcodes that can access non-existing accounts is to first read the CodeHash, and only if it's not 0 (meaning that the account exists), proceed with other account field reads. If a field read is performed to a non-existing account, this may end up mapping to an MPT account field proof for a non-existing account (which are not supported); so the MPT proof would be impossible to generate. We can add some checks to make sure we don't implement any circuit that generates this situation; but we can even add more sanity checks to detect possible bugs early. In particular we can check for each account field lookup (either read or write):
account (only CodeHash reads with value=0 can be done to non-existing
accounts, which the State Circuit translates to MPT
AccountNonExisting proofs lookups).
If any of these two points don't hold, the corresponding chain of MPT proofs
can't be generated.
In order to perform this checks in a single place, I've moved the logic to update the accounts in the StateDB performed by the circuit input builder in a single place, after the check (instead of requiring each bus-mapping opcode implementation to do it by themselves).
With these checks I found some bugs in the
callopanderror_oog_call, which I have fixed.As a bonus I added the method
debug_print_txs_steps_rw_opsto thewitness::Blockwhich prints all the rw operations (with their content) of each step in each tx, which was very useful for debugging.Resolve #1084
Depends on #1069