Circuit for EXTCODEHASH opcode by z2trillion · Pull Request #353 · privacy-ethereum/zkevm-circuits

z2trillion · 2022-02-26T03:02:24Z

Spec PR: privacy-ethereum/zkevm-specs#139

z2trillion · 2022-02-26T04:28:24Z

@ed255, can you take a look at this when you get the chance?

roynalnaruto · 2022-02-28T11:09:52Z

+    external_address: RandomLinearCombination<F, N_BYTES_ACCOUNT_ADDRESS>,
+    tx_id: Cell<F>,
+    previously_accessed: Cell<F>,
+    external_code_hash: Cell<F>,


This should be Word<F>, right? As the code hash can be more than 254 bits

The code hash is 256 bits, but the constraints are all expressed in terms of its RLC encoding. The code hash is RLC encoded for the RW table here: https://github.com/scroll-tech/zkevm-circuits/blob/e2a746f632f67e905fde5390e604c491cbc367fc/zkevm-circuits/src/evm_circuit/witness.rs#L647 and self.external_code_hash is RLC encoded in the assign_exec_step step in this gadget as well.

roynalnaruto · 2022-02-28T11:12:00Z

+    #[test]
+    fn extcodehash_gadget_cold_account() {


I think we should also have a test case that asserts failure, when gas available is only sufficient for a warm account, whereas gas_cost for EXTCODEHASH should be more than that (for a cold account). In this scenario the prover should fail with insufficient gas available.

I wrote this test:

#[test] fn extcodehash_gadget_insufficient_gas() { let address = address!("0xaabbccddee000000000000000000000000000011"); let code = bytecode! { PUSH20(address.to_word()) #[start] EXTCODEHASH STOP }; assert_eq!(test_circuits_using_bytecode(code, BytecodeTestConfig { gas_limit: 100, ..Default::default() }), Ok(())); }

but it fails with

---- evm_circuit::execution::extcodehash::test::extcodehash_gadget_insufficient_gas stdout ---- thread 'evm_circuit::execution::extcodehash::test::extcodehash_gadget_insufficient_gas' panicked at 'called `Result::unwrap()` on an `Err` value: TracingError("Failed to trace tx, err: intrinsic gas too low: have 100, want 21000")', zkevm-circuits/src/test_util.rs:58:78

which is during the witness generation. I don't think it's possible (right now) to even try to generate a proof for a transaction that reverts because of insufficient gas.

The two existing warm/cold tests already ensure that the gas cost constraint in the state transition is correct.

We're working on enabling better test-case generation. See: #349
Other Opcodes like GASPRICE also do have issues to be implemented until we close it.
We can leave an issue open as tech-debt or something similar.

For now, we don't really have a way as when Geth gets an out-of-gas exception it doesn't return the trace.
We also need to update this behaviour. And we should file an issue for that.

Forget about what I said above.
I was wrong. In fact, a tx like the one you purpose here is not valid. And would not be included in a block as basically, there's no way to execute it.

The minimum gas needed to start a tx is 21000. Hence, if you use 21001 as gas argument this would end up in an out-of-gas and you'd get a trace back. But without enough funds to even kickstart the tx,

My suggestion is to increase the gas you provide to the tx.

CPerezz

LGTM

Brilliant job!!

CPerezz · 2022-02-28T22:13:33Z

+    external_address: RandomLinearCombination<F, N_BYTES_ACCOUNT_ADDRESS>,
+    tx_id: Cell<F>,
+    previously_accessed: Cell<F>,
+    external_code_hash: Cell<F>,


CPerezz · 2022-02-28T22:24:47Z

+    #[test]
+    fn extcodehash_gadget_cold_account() {


We're working on enabling better test-case generation. See: #349
Other Opcodes like GASPRICE also do have issues to be implemented until we close it.
We can leave an issue open as tech-debt or something similar.

For now, we don't really have a way as when Geth gets an out-of-gas exception it doesn't return the trace.
We also need to update this behaviour. And we should file an issue for that.

ed255 · 2022-03-07T16:15:08Z

@z2trillion The current approach followed to implement the EXTCODEHASH looks good to me! I think it would be good to now work on the spec of this opcode :) And once the spec is reviewed and merged we can conclude the work in this PR.

z2trillion · 2022-03-08T02:29:15Z

@ed255 i've made a PR for the specs here: privacy-ethereum/zkevm-specs#139. This PR is ready for another look as well.

z2trillion · 2022-03-16T02:40:25Z

@ed255 this is ready for review again

ed255

Overall looks good! I've just added a few comments about details on the bus-mapping implementation, and a typo I spotted in the circuit tests.

ed255 · 2022-03-17T10:38:52Z

+            from_bytes::expr(&external_address.cells),
+            1.expr(),
+            is_warm.expr(),
+            None,


I've reviewed the implementation of SSTORE and I realized I missed this before. For an op that can revert, this shouldn't be None. Take a look here:
https://github.com/appliedzkp/zkevm-circuits/blob/c5dda20e901951e61f751b89dd50607f83b98a95/zkevm-circuits/src/evm_circuit/execution/sstore.rs#L75-L81

This will require adding two read operations for CallContextFieldTag::RwCounterEndOfReversion, CallContextFieldTag::IsPersistent as seen here:
https://github.com/appliedzkp/zkevm-circuits/blob/c5dda20e901951e61f751b89dd50607f83b98a95/zkevm-circuits/src/evm_circuit/execution/sstore.rs#L50-L56

I've made these changes to the circuit and the changes needed to the bus mapping, but now the circuit tests fail.

Do you know if the witness generation supports RwCounterEndOfReversion and IsPersistent? I noticed that the sload bus mapping doesn't correspond to the circuit and that the sload and sstore circuits both manually construct the rw tables for their tests.

I've made these changes to the circuit and the changes needed to the bus mapping, but now the circuit tests fail.

Do you know if the witness generation supports RwCounterEndOfReversion and IsPersistent? I noticed that the sload bus mapping doesn't correspond to the circuit and that the sload and sstore circuits both manually construct the rw tables for their tests.

I will investigate this

Found the bug! After you added generating the operations for CallContextFieldTag::RwCounterEndOfReversion and CallContextFieldTag::RwCounterEndOfReversion you altered the positions of the other operations for that step in rw_indices. So when you query the values in the assign advice method, you need to update the indexes of rw_index to obtain the values for nonce, balance and code_hash. See #353 (comment)

…sts/empty

…lly test that

…t existence

Co-authored-by: Eduard S. <eduardsanou@posteo.net>

ed255

LGTM! Thanks for working on this :)

icemelon

LGTM. just one nitpick

icemelon · 2022-03-24T18:32:59Z

+                * (1.expr() - nonce.expr() * nonempty_witness.expr())
+                * (1.expr() - balance.expr() * nonempty_witness.expr())
+                * (1.expr() - (code_hash.expr() - empty_code_hash_rlc) * nonempty_witness.expr()),
+        );


this is smart. it took me a while to realize the constraint is right. 😄

Co-authored-by: Haichen Shen <shenhaichen@gmail.com>

* add test case for tx.to is zero * fix: handle the rlp of tx whose to = zero * add gates for is_create * clippy

github-actions Bot added crate-bus-mapping Issues related to the bus-mapping workspace member T-opcode Type: opcode-related and focused PR/Issue labels Feb 26, 2022

z2trillion force-pushed the feat/opcode-extcodehash branch 2 times, most recently from f3e186b to 14ec0d9 Compare February 26, 2022 03:24

roynalnaruto reviewed Feb 28, 2022

View reviewed changes

CPerezz approved these changes Feb 28, 2022

View reviewed changes

CPerezz mentioned this pull request Feb 28, 2022

external-tracer: Enable a way to test out-of-gas cases #358

Closed

ed255 suggested changes Mar 1, 2022

View reviewed changes

Comment thread bus-mapping/src/evm/opcodes/extcodehash.rs Outdated

Comment thread zkevm-circuits/src/evm_circuit/execution/extcodehash.rs

Comment thread bus-mapping/src/evm/opcodes/extcodehash.rs

Comment thread zkevm-circuits/src/evm_circuit/execution/extcodehash.rs Outdated

CPerezz mentioned this pull request Mar 4, 2022

Required opcodes for advanced l2 bridge contracts #370

Closed

10 tasks

ed255 reviewed Mar 7, 2022

View reviewed changes

Comment thread bus-mapping/src/evm/opcodes/extcodehash.rs

z2trillion force-pushed the feat/opcode-extcodehash branch from 0637e1a to 8d4ce09 Compare March 7, 2022 18:17

z2trillion requested a review from ed255 March 8, 2022 02:29

z2trillion force-pushed the feat/opcode-extcodehash branch from 595c329 to 3df8130 Compare March 14, 2022 22:38

ed255 suggested changes Mar 16, 2022

View reviewed changes

Comment thread bus-mapping/src/evm/opcodes/extcodehash.rs

Comment thread bus-mapping/src/evm/opcodes/extcodehash.rs Outdated

Comment thread zkevm-circuits/src/evm_circuit/execution/extcodehash.rs Outdated

z2trillion requested a review from ed255 March 16, 2022 19:41

ed255 suggested changes Mar 17, 2022

View reviewed changes

Mason Liang added 10 commits March 21, 2022 11:24

Add bus mapping for EXTCODEHASH

62874c1

Small fixes

b25b178

Add gadget for EXTCODEHASH

23f1f2d

cargo fix and fmt

5eaea33

Fix typos

ccada25

Fix bad rebase

6cd1d22

Update comments

8b0c484

Update bus mapping tests to cover all four cases of warm/cold and exi…

cef4a63

…sts/empty

Fix bus mapping and add stack push constraint

2dede30

Use is_warm instead of matching on gas cost

bf5bdfc

Mason Liang and others added 17 commits March 21, 2022 11:25

Fix bus mapping tests. Previously cold_existing_account did not actua…

15bae03

…lly test that

Update bus mapping for reads of balance and nonce to determine accoun…

842f0ba

…t existence

Add account lookups so circuit tests pass

a0bd75a

cleanup code

e8686cd

Add is_empty

bed9356

Add constraints for is_empty

e5a5bff

Add tests for more account states

1b95edd

clean up code

3495a00

Revert test changes

bb1aaa0

Rebase

95d6eb7

fmt

890084f

Fix bus-mapping for rebase

c7df8ff

cleanup

60dc8f8

set nonce instead of balance

8499d4d

Co-authored-by: Eduard S. <eduardsanou@posteo.net>

Update access list of sdb

d5f3e0e

Make tx access list write reversible

4f5ec5a

Update bus mapping and circuit for reversions. Circuit tests fail

33cf4f7

z2trillion force-pushed the feat/opcode-extcodehash branch from 9c760d0 to 33cf4f7 Compare March 21, 2022 17:25

z2trillion and others added 2 commits March 21, 2022 13:28

Merge branch 'main' into feat/opcode-extcodehash

2b498a0

Fix merge errors

e19f4ee

ed255 reviewed Mar 22, 2022

View reviewed changes

Comment thread zkevm-circuits/src/evm_circuit/execution/extcodehash.rs Outdated

update assignment for updated bus mapping

0a3d897

Co-authored-by: Eduard S. <eduardsanou@posteo.net>

ed255 approved these changes Mar 23, 2022

View reviewed changes

icemelon approved these changes Mar 24, 2022

View reviewed changes

Fix comment

4997ad1

Co-authored-by: Haichen Shen <shenhaichen@gmail.com>

icemelon merged commit 1ec38f2 into privacy-ethereum:main Mar 24, 2022

icemelon deleted the feat/opcode-extcodehash branch March 24, 2022 19:42

z2trillion mentioned this pull request Mar 25, 2022

Update EXTCODEHASH to handle reverts privacy-ethereum/zkevm-specs#163

Merged

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

Bug-fix: handle tx.to = 0x0 correctly (privacy-ethereum#353)

d8a2a6a

* add test case for tx.to is zero * fix: handle the rlp of tx whose to = zero * add gates for is_create * clippy

Conversation

z2trillion commented Feb 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

z2trillion commented Feb 26, 2022

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!

roynalnaruto Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ed255 commented Mar 7, 2022

Uh oh!

Uh oh!

z2trillion commented Mar 8, 2022

Uh oh!

z2trillion commented Mar 16, 2022

Uh oh!

ed255 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

ed255 left a comment

Choose a reason for hiding this comment

Uh oh!

icemelon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

z2trillion commented Feb 26, 2022 •

edited

Loading

roynalnaruto Feb 28, 2022 •

edited

Loading