feat: bus mapping for sload and sstore by lispc · Pull Request #418 · privacy-ethereum/zkevm-circuits

lispc · 2022-04-01T11:56:10Z

Some explanation:

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.
add a test_util.rs in bus mapping to remove some boilerplate codes
In fact with sstore & sload in this PR, multiple end-to-end contract calls of ERC20 transfer including both successful and failure cases can be proved using evm circuit. Details e2e erc20 transfer proving scroll-tech/zkevm-circuits#127, Edu's openzeppelin ERC20 test cases used.
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.
Change docker of integration-tests from stable to nightly, since currently only nightly geth has refund in trace

CPerezz

I just reviewed SSTORE opcode implementation in bus-mapping and some other stuff you added but stopped due to:

The PR is marked as Ready for Review but is not up to date with main. That basically causes that you haven't synced with #349 and therefore, you've implemented a test-utils module which should most probably be removed.
Also, the tests for SSTORE are missing a lot of different cases which are only testeable with the additions to mock crate in main.

My suggestion:

Remove all of the test-utils you've introduced in this PR (makes the review much more complex as there are a lot of files and different topics to review/address) when in theory the PR just wants to implement SSTORE and SLOAD bus-mapping opcodes.
The circuit impl is only done for SSTORE. Either you add both or none and do it in another follow-up PR.

Before introducing test-suit/mock changes or new utils, please, file an issue before so that this does not happen again and work is lost or unusable.
Also, make sure that the PRs are more atomic. As otherways, the review time that consumes is massive and not so productive.

Thanks. Carlos.

CPerezz · 2022-04-05T14:20:22Z


        for (index, geth_step) in geth_trace.struct_logs.iter().enumerate() {
            let mut state_ref = self.state_ref(&mut tx, &mut tx_ctx);
+            log::trace!("handle {}th opcode {:?} ", index, geth_step.op);


I prefer tokio/tracing for these things. But maybe it's not the best option.

Maybe @ed255 has some suggestions

I'm happy with using the log crate here, it's very lightweight and if logs aren't activated they don't add any performance penalty as far as I know. I don't know about tokio/tracing so I can't comment on that.

CPerezz · 2022-04-05T14:23:17Z

    // Fields with transaction lifespan, will be clear in `clear_access_list_and_refund`.
    access_list_account: HashSet<Address>,
    access_list_account_storage: HashSet<(Address, U256)>,
+    // `dirty_storage` constains writes during current transaction.


Suggested change

// `dirty_storage` constains writes during current transaction.

// `dirty_storage` contains writes during current transaction.

CPerezz · 2022-04-05T14:33:12Z

+    #[serde(default)]
+    refund: Gas,


Maybe add a comment to highlight that this does not pertain to the original GethExecStepInternal (alias StructLogs in Geth) and that it has been only added to allow compilation. As the only place where this is needed is in GethExecStep.

Also, maybe an Option<Gas> type makes everything easier IDK. And you just need to add it in GethExecStep without touching the internal one.

BTW, how was this working before that PR if we did not have this field in the struct??

Has it appeared after the change to latest for the geth version used??

CPerezz · 2022-04-05T14:42:33Z

 services:
  geth0:
-    image: "ethereum/client-go:stable"
+    image: "ethereum/client-go:latest"


Are we sure about that? Note that Geth is now with the integration of the merge and ETH2 so I'm not sure if we want all that stuff inside in our tests now.

Is there any reason specific to that PR that requires the latest version?

CPerezz · 2022-04-05T14:48:05Z

+#[cfg(test)]
+#[ctor::ctor]
+fn init_env_logger() {
+    // Enable RUST_LOG during tests
+    env_logger::init();
+}


2 concerns about this:

Can we make it feature-dependant? I'm not sure if we will always want to print all the logs for all the tests as we will not see anything else than logs and the stdout will explode really quickly.

Why ctor as opposite to lazy_static + initialization function? Is it a common practice?

I understand the concerns here!
For the first point, this would be a dev feature right? I don't think rust supports dev features though (I don't think we should expose a public feature that starts the env_logger, as that's a decision that must be made by the consumer of the library). Nevertheless, for the concern about having lots of outputs by default, we could mitigate this by setting the default log level to error, and then via the RUST_LOG env var you can dynamically enable lower levels.

For the second point, I find the usage of ctor interesting, because it guarantees that env_logger::init() is called once always in tests at the beginning, without requiring updating the tests themselves. Using lazy_static would require that every test function calls a setup function (or use a crate that handles test setup, but in any case we still need to add 1 line for each test function, which is cumbersome).

CPerezz · 2022-04-05T14:51:54Z

+        let geth_step = &geth_steps[0];
+        let mut exec_step = state.new_step(geth_step)?;
+
+        let _call_id = state.call()?.call_id;


If not used, remove this.

CPerezz · 2022-04-05T15:12:26Z

+        let value_prev = *value_prev;
+        let (_, committed_value) = state.sdb.get_committed_storage(&contract_addr, &key);
+        let committed_value = *committed_value;


Suggested change

let value_prev = *value_prev;

let (_, committed_value) = state.sdb.get_committed_storage(&contract_addr, &key);

let committed_value = *committed_value;

let value_prev = value_prev.clone();

let (_, committed_value) = state.sdb.get_committed_storage(&contract_addr, &key);

let committed_value = committed_value.clone();

Make the clones explicit please.

CPerezz · 2022-04-05T15:28:53Z

+                value: true,
+                value_prev: is_warm,


We should maybe consider to rename these two fields for something much closer to the actual values passed. value instead of true should contain a meaningful number/string IMO.

WDYT @han0110 @ed255

CPerezz · 2022-04-05T15:35:46Z

+    use pretty_assertions::assert_eq;
+
+    #[test]
+    fn sstore_opcode_impl() {


This test is missing several things IMO:

Refund logic was added to the crate but is never tested to be working correctly. I think SSTORE should have a testcase where it does not only write but also plays with the refund.

We should also add testcases for COLD & WARM situations.

This should be using the mock::TestContext helpers added in Modularize mock crate for easier and more customizable testing setup generation #349.

Please take a look to it and how to use it and it will allow you to perform all the testing you need.

lispc · 2022-04-07T04:47:51Z

So we cannot reopen this.... sorry .. will open a pr

…merged into `ErrorOutOfGasMemoryCopy`. (privacy-ethereum#418)

pick

e47613b

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 1, 2022

change geth to nightly in integration-tests

1079392

ed255 self-requested a review April 1, 2022 13:12

0xmountaintop reviewed Apr 1, 2022

View reviewed changes

Comment thread bus-mapping/src/circuit_input_builder.rs Outdated

Merge remote-tracking branch 'origin/main' into feat/sstore_busmapping

a891164

han0110 mentioned this pull request Apr 2, 2022

Implementation ExecutionState::STOP #419

Merged

6 tasks

lispc added 3 commits April 4, 2022 15:15

Merge remote-tracking branch 'origin/main' into feat/sstore_busmapping

3419920

fix conflict

2077c0e

minor

2ba341d

CPerezz self-requested a review April 4, 2022 10:51

lispc added 3 commits April 5, 2022 11:32

Merge branch 'main' into feat/sstore_busmapping

c03ac9c

fix conflict

11534d4

Merge branch 'main' into feat/sstore_busmapping

6778b3a

CPerezz suggested changes Apr 5, 2022

View reviewed changes

lispc closed this Apr 7, 2022

lispc deleted the feat/sstore_busmapping branch April 7, 2022 04:46

lispc mentioned this pull request Apr 7, 2022

feat: bus mapping for sload and sstore #436

Merged

zemse pushed a commit to zemse/zkevm-circuits that referenced this pull request Mar 22, 2023

Delete execution state ErrorOutOfGasEXTCODECOPY. Since it has been …

88fb389

…merged into `ErrorOutOfGasMemoryCopy`. (privacy-ethereum#418)

	// `dirty_storage` constains writes during current transaction.
	// `dirty_storage` contains writes during current transaction.

Conversation

lispc commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

CPerezz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lispc commented Apr 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lispc commented Apr 1, 2022 •

edited

Loading