Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Update geth-utils and bus-mapping to build more witness #292

Conversation

han0110
Copy link
Contributor

@han0110 han0110 commented Jan 20, 2022

This PR is once part of #278, and aims to fulfill all required witness when parsing.

It first update geth-utils to better match how geth trace API works, and parse all fields of call for further usage.

The most tricky field of call is rw_counter_end_of_reversion, which takes 2 rounds of iteration on the traces to infer. Briefly speaking, the first round is for inspecting if a call ends up success (e.g. STOP or RETURN) or failure (REVERT or all kind of Exceptions), the second round is for recording the correct amount of read/write, with proper reversions if needed, then knowing the correct rw_counter_end_of_reversion. For more details on state write reversion, please refer to this note with illustration.

Also due to the interpreter changes of geth, error parsing of InvalidOpcode and WriteProtection are handled by get_step_err instead of directly from error string (see ethereum/go-ethereum#24031 and ethereum/go-ethereum#23970)

Resolves #174
Resolves #188
Resolves #225
Resolves #250
Resolves #289

@han0110 han0110 requested a review from ed255 January 20, 2022 06:29
@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member T-opcode Type: opcode-related and focused PR/Issue crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Jan 20, 2022
@han0110 han0110 mentioned this pull request Jan 20, 2022
6 tasks
@han0110 han0110 force-pushed the feature/bus-mapping-enhancement branch 2 times, most recently from db2ab33 to 253e337 Compare January 20, 2022 16:13
geth-utils/lib/lib.go Outdated Show resolved Hide resolved
geth-utils/gethutil/trace.go Outdated Show resolved Hide resolved
// TODO: Embed error from result into TracingError if possible
true => Err(Error::TracingError),
match result.is_empty() || result.starts_with("Failed") {
true => Err(Error::TracingError(result)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉
This resolves some of the tasks defined here #165

bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
@han0110 han0110 force-pushed the feature/bus-mapping-enhancement branch from 253e337 to 7c3d514 Compare January 21, 2022 10:30
@han0110 han0110 force-pushed the feature/bus-mapping-enhancement branch from 7c3d514 to 316fc50 Compare January 21, 2022 14:07
@ed255
Copy link
Member

ed255 commented Jan 27, 2022

This PR also resolves #174

@han0110 han0110 force-pushed the feature/bus-mapping-enhancement branch from 0c16aa0 to 07c3e31 Compare January 30, 2022 09:50
bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
bus-mapping/src/circuit_input_builder.rs Show resolved Hide resolved
bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some comments and notes. I plan to review the circuit input builder changes in more detail after this first review.

bus-mapping/src/circuit_input_builder.rs Show resolved Hide resolved
bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
bus-mapping/src/circuit_input_builder.rs Show resolved Hide resolved
bus-mapping/src/evm/opcodes/stop.rs Show resolved Hide resolved
bus-mapping/src/evm/opcodes/gas.rs Outdated Show resolved Hide resolved
@@ -59,12 +59,22 @@ impl Account {
code_hash: *CODE_HASH_ZERO,
}
}

/// Return if account is empty or not.
pub fn is_empty(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How/Where will this method be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is used here in #278 and I forgot to remove it. Should I remove it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! No need to remove it, I just wanted to understand where it was needed because I was thinking of a possible edge case but it may not apply here. The edge case I was thinking about is that an Account has a Storage which we model as a HashMap. But keys can be deleted in EVM by setting the value to 0, but our HashMap will not be empty in that case. Maybe this edge case never occurs, because to write to the Storage you would need a code hash != empty.

Nevertheless let me confirm this: is_empty should return true when either the account never existed, or it has been destroyed, and no other case right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, to put something in storage of an account, its nonce must be non-zero (being created or already created), so I guess this edge case would not happen (actually contract could be have some storage without code, we can do this by store something and return nothing when creation).

is_empty should return true when either the account never existed, or it has been destroyed, and no other case right?

Not really. You just reminds me, for destroyed one, it depends on if we have finalized or not (tx ends or not), because before finalizing, destroyed account is not empty. We might need to add a tag to record if this account is destroyed or not, and finalize to actual remove it after each tx ends.

eth-types/src/geth_types.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/memory.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/timestamp.rs Outdated Show resolved Hide resolved
@han0110 han0110 force-pushed the feature/bus-mapping-enhancement branch from 0b44a56 to 1406ff6 Compare February 16, 2022 16:04
bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
bus-mapping/src/circuit_input_builder.rs Show resolved Hide resolved
@ed255
Copy link
Member

ed255 commented Feb 16, 2022

Overall the PR looks good to me! Quite a lot of changes!
I really like the introduction of ReversionGroup. I think it makes the code more readable and easier to follow what's going on. I want to add that I think diagrams are extremely useful to understand the concepts and details of how reversion works, and the possible cases that can happen. @han0110 you've already made some diagrams and documentation about this in hackmd; I think we should move them to the zkevm-specs repository so that they are easily accessible.

@han0110
Copy link
Contributor Author

han0110 commented Feb 16, 2022

@ed255 Thanks a lot for reviewing and suggestion of docs! All suggested docs are applied in 20ed665.

@han0110 han0110 force-pushed the feature/bus-mapping-enhancement branch from 20ed665 to 8639ab2 Compare February 17, 2022 10:25
@han0110 han0110 merged commit c221f9b into privacy-scaling-explorations:main Feb 17, 2022
@han0110 han0110 deleted the feature/bus-mapping-enhancement branch February 17, 2022 12:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member T-opcode Type: opcode-related and focused PR/Issue
Projects
None yet
2 participants