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

feat: bus mapping for sload and sstore#436

Merged
CPerezz merged 9 commits into
privacy-ethereum:mainfrom
scroll-tech:feat/sstore_busmapping
Apr 8, 2022
Merged

feat: bus mapping for sload and sstore#436
CPerezz merged 9 commits into
privacy-ethereum:mainfrom
scroll-tech:feat/sstore_busmapping

Conversation

@lispc
Copy link
Copy Markdown
Collaborator

@lispc lispc commented Apr 7, 2022

Some explanation:

  1. implement bus mapping for sload and sstore, and rewrite tests
  2. in sstore gas calculation, EVM needs "CommittedState", which is the storage state value before current transaction. So dirty_storage is added to statedb.rs, just like StateDB in go-ethereum.
  3. Some minor improvements for debugging experience: (1) enable env logger in tests (2) log::debug relationship mappings between global lookup index and lookup index of each rw type, so from circuit lookup error msg we can find which lookup is wrong more easily.
  4. Change docker of integration-tests from stable to nightly, since currently only nightly geth has refund in trace.

Some review comments from old PR #418

  1. As for refund and geth docker image upgrade, comments added in docker compose file in integration tests
  2. For those Rust struct which implements Copy trait, cargo clippy will suggest use *reference rather than reference.clone()

A questions:

in zkevm circuits, if the tests are already written in bus mapping, will it be reasonable to write very similar tests in bus mapping again?

@lispc lispc requested review from a team and ChihChengLiang and removed request for a team April 7, 2022 06:34
@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 Apr 7, 2022
@CPerezz
Copy link
Copy Markdown
Contributor

CPerezz commented Apr 7, 2022

Hey @ChihChengLiang I can take this as I already reviewed the previous PR related to this. So I have the background already.

As you prefer, but happy to do it! :)

Copy link
Copy Markdown
Contributor

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

Overall looks good!! Thanks for working on this!

Left some comments and questions! :)

Comment thread bus-mapping/src/evm/opcodes/sload.rs
Comment thread bus-mapping/src/evm/opcodes/sstore.rs
Comment thread bus-mapping/src/evm/opcodes/sstore.rs
Comment thread bus-mapping/src/evm/opcodes/sstore.rs
Comment thread external-tracer/src/lib.rs Outdated
Comment thread integration-tests/docker-compose.yml Outdated
Comment thread zkevm-circuits/src/evm_circuit/execution/sload.rs
Comment thread zkevm-circuits/src/evm_circuit/execution/sload.rs
Comment thread zkevm-circuits/src/evm_circuit/execution/sstore.rs
Comment thread zkevm-circuits/src/evm_circuit/execution/sstore.rs Outdated
@CPerezz CPerezz removed the request for review from ChihChengLiang April 7, 2022 10:08
Copy link
Copy Markdown
Contributor

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for this work @lispc 👍

@CPerezz
Copy link
Copy Markdown
Contributor

CPerezz commented Apr 8, 2022

in zkevm circuits, if the tests are already written in bus mapping, will it be reasonable to write very similar tests in bus mapping again?

Not sure I follow. But I think that it's safe to have tests in both sides even if they have some overlap TBH. As they might help to catch different kinds of errors as well as different missing API methods or similar stuff.

@CPerezz CPerezz merged commit 27320f8 into privacy-ethereum:main Apr 8, 2022
@0xmountaintop 0xmountaintop deleted the feat/sstore_busmapping branch April 8, 2022 15: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

Development

Successfully merging this pull request may close these issues.

2 participants