EVM Circuit and trait OpGadget #26

han0110 · 2021-08-10T11:05:18Z

This PR aims to provides:

A flexible design of evm circuit
A trait for op gadget for us to implmenent without caring about the actual circuit layout
An example AddGadget for reference (Closes [EVM] ADD example #9)

Todo:

Trait OpGadget
Check linear_combinations configured by OpGadget
Check lookups configured by OpGadget
- Word 8bit lookups
- Bus mapping lookups for stack and memory
AddGadget and PushGadget as examples.
~~Feed ExecutionStep by crate bus-mapping into circuit~~ -> Next PR

ChihChengLiang

First round of review. Added a nitpick

zkevm-circuits/src/evm_circuit.rs

zkevm-circuits/src/evm_circuit/op_execution.rs

gaswhat · 2021-08-27T04:56:44Z

zkevm-circuits/src/evm_circuit.rs

+}
+
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
+enum Case {


I think the name Case is a bit too general. Maybe use ErrorCode to be more specific?

zkevm-circuits/src/evm_circuit/op_execution.rs

zkevm-circuits/src/evm_circuit/op_execution/arithmetic.rs

zkevm-circuits/src/evm_circuit.rs

zkevm-circuits/src/evm_circuit/op_execution.rs

zkevm-circuits/src/evm_circuit.rs

CPerezz

Overall it looks good if what we plan is to have a Layout example.

But I have several questions:

Seems that we still need to integrate bus-mapping in here. Do we plan do do it in a follow-up PR?
The Opcode trait should (IMO) contain one or multiple ExecutionSteps so that we can basically have all of the CaseConfig data an all the other stuff extracted from there.
Case and CaseConfig as well as CallContext should probably be migrated to bus-mappign and obtained during the parsing process.
We should document more. Even if are not polished docs. But at least to help improve the general idea that the development path follows.
CaseConfig as said in the comments should probably not be an associated const and instead be the result of a fn or similar. The reason is that multiple instances of the same Opode trait object can result in different CaseConfig.

zkevm-circuits/src/evm_circuit.rs

zkevm-circuits/src/evm_circuit/op_execution.rs

zkevm-circuits/src/evm_circuit/op_execution/arithmetic.rs

CPerezz · 2021-09-01T11:33:26Z

zkevm-circuits/src/evm_circuit/op_execution/push.rs

+    const CASE_CONFIGS: &'static [CaseConfig] = &[
+        CaseConfig {
+            case: Case::Success,
+            num_word: 1,
+            num_cell: 32, // for PUSH selectors
+            will_resume: false,
+        },
+        CaseConfig {
+            case: Case::StackOverflow,
+            num_word: 0,
+            num_cell: 0,
+            will_resume: true,
+        },
+        CaseConfig {
+            case: Case::OutOfGas,
+            num_word: 0,
+            num_cell: 0,
+            will_resume: true,
+        },
+    ];


This kinda reforces my previous argument that it doesn't seem to fit really well to have CaseConfigs as a const associated to the gadget since it can change depending on the instance.

zkevm-circuits/src/evm_circuit.rs

zkevm-circuits/src/evm_circuit/op_execution/arithmetic.rs

CPerezz

LGTM.

We will need to address all of the issues that have been raised here BTW. Not sure if before or after the first opcode impls. But the more we add the bigger the refactor will become.

miha-stopar

The ideas used and the implementation both seem very nice to me. I was thinking about possible optimizations but nothing came to my mind that hasn't already been considered. I have one question though - would we need one additional constraint to make sure that execution_step.opcode and the opcode selector (talking about 80 cells) are in sync? Wouldn't be now possible to set execution_step.opcode to ADD and set the opcode selector to PUSH (the one of 80s that corresponds to PUSH) which would make some attacks possible?

zkevm-circuits/src/evm_circuit.rs

han0110 · 2021-09-14T16:53:05Z

I have one question though - would we need one additional constraint to make sure that execution_step.opcode and the opcode selector (talking about 80 cells) are in sync? Wouldn't be now possible to set execution_step.opcode to ADD and set the opcode selector to PUSH (the one of 80s that corresponds to PUSH) which would make some attacks possible?

@miha-stopar According to your question, currently in AddGadget and PushGadget we check if opcode == 1 or opcode == 3 and 0 <= opcode - PUSH1 <= 31 respectively to keep them in sync. If we set opcode to ADD, the constraint in PushGadget should not pass, right?

miha-stopar · 2021-09-14T18:49:32Z

@han0110 yes, that's ok. What I meant are these assignments: https://github.com/appliedzkp/zkevm-circuits/blob/f0bd8afe2607abfb428c5fb171730625448ddd25/zkevm-circuits/src/evm_circuit/op_execution.rs#L484. Not sure if there is a check that these assignments correspond to the actual opcode?

miha-stopar · 2021-09-15T06:23:57Z

@han0110 I can see it now - opcode number cell is used only as kind of a hint and the 80 cells determine the branching. Sorry, all good!

gaswhat

LGTM. Just a few minor comments

gaswhat · 2021-09-15T17:11:13Z

zkevm-circuits/src/evm_circuit.rs

+            meta.advice_column(), // global_counter
+            meta.advice_column(), // target
+            meta.advice_column(), // is_write
+            meta.advice_column(), // val1


Add comment for actual meaning.

Suggested change

meta.advice_column(), // val1

meta.advice_column(), // val1, call_id

These val* mean different things depends on the target, so I left it arbitrary instead of concrete meaning. For stack or memroy it would be call_id but for storage it would be account address cause the storage update is not scoped in a single call but in a block. Will try to document it in a better way to clarify.

zkevm-circuits/src/evm_circuit.rs

zkevm-circuits/src/evm_circuit/op_execution.rs

zkevm-circuits/src/evm_circuit/op_execution/arithmetic.rs

Implement LT & GT opcode

therealyingtong

I've not looked through this whole PR and am just submitting a few nits -- please don't block on my review.

therealyingtong · 2021-09-15T12:45:10Z

zkevm-circuits/src/evm_circuit.rs

+    // Value in wei of call.
+    Value,
+    // If call succeeds or not in the future.
+    IsPersistant,


Suggested change

IsPersistant,

IsPersistent,

therealyingtong · 2021-09-15T12:54:54Z

zkevm-circuits/src/evm_circuit.rs

+    // relative position to selector for synthesis
+    column: Column<Advice>,
+    rotation: usize,


Suggested change

// relative position to selector for synthesis

column: Column<Advice>,

rotation: usize,

column: Column<Advice>,

// relative position to selector for synthesis

rotation: usize,

therealyingtong · 2021-09-15T13:10:57Z

zkevm-circuits/src/evm_circuit/param.rs

+
+// Number of cells used for each purpose
+// TODO: pub const NUM_CELL_CALL_INITIALIZATION_STATE: usize = ;
+pub const NUM_CELL_OP_EXECTION_STATE: usize = 7;


Suggested change

pub const NUM_CELL_OP_EXECTION_STATE: usize = 7;

pub const NUM_CELL_OP_EXECUTION_STATE: usize = 7;

Fixed with all above suggestions in 702039d.

therealyingtong · 2021-09-15T13:39:26Z

zkevm-circuits/src/evm_circuit/op_execution.rs

+
+        construct_op_gadget!(add_gadget);
+        construct_op_gadget!(push_gadget);
+        let _ = qs_op_idx;


Suggested change

let _ = qs_op_idx;

This is intended for escaping the clippy warning since the macro increases qs_op_idx even it's the last one.

ChihChengLiang

LGTM. Added some nitpicks

zkevm-circuits/src/evm_circuit.rs

zkevm-circuits/src/evm_circuit/op_execution/push.rs

…es it

…#73

This was referenced Aug 10, 2021

Abstract circuit layout privacy-scaling-explorations/zkevm-specs#27

Closed

EVM gas privacy-scaling-explorations/zkevm-specs#28

Closed

EVM errors privacy-scaling-explorations/zkevm-specs#29

Closed

gaswhat mentioned this pull request Aug 13, 2021

[WIP] Implement SUB opcode #27

Closed

scroll-dev mentioned this pull request Aug 17, 2021

[WIP] Implement SUB opcode #29

Closed

han0110 force-pushed the evm_circuit branch 3 times, most recently from 2deada3 to 1767301 Compare August 24, 2021 10:12

han0110 marked this pull request as ready for review August 25, 2021 10:13

han0110 force-pushed the evm_circuit branch from 1767301 to cfea9e1 Compare August 25, 2021 10:37

ChihChengLiang reviewed Aug 26, 2021

View reviewed changes

zkevm-circuits/src/evm_circuit.rs Outdated Show resolved Hide resolved

DreamWuGit reviewed Aug 26, 2021

View reviewed changes

zkevm-circuits/src/evm_circuit/op_execution.rs Outdated Show resolved Hide resolved

DreamWuGit reviewed Aug 26, 2021

View reviewed changes

zkevm-circuits/src/evm_circuit/op_execution.rs Show resolved Hide resolved

gaswhat reviewed Aug 27, 2021

View reviewed changes

gaswhat reviewed Aug 30, 2021

View reviewed changes

zkevm-circuits/src/evm_circuit.rs Outdated Show resolved Hide resolved

DreamWuGit reviewed Aug 30, 2021

View reviewed changes

zkevm-circuits/src/evm_circuit/op_execution.rs Outdated Show resolved Hide resolved

ChihChengLiang mentioned this pull request Aug 30, 2021

Complete the rest of the evm proof privacy-scaling-explorations/zkevm-specs#37

Closed

gaswhat reviewed Aug 31, 2021

View reviewed changes

zkevm-circuits/src/evm_circuit.rs Show resolved Hide resolved

han0110 force-pushed the evm_circuit branch 2 times, most recently from 35586b4 to 52a55d6 Compare September 1, 2021 08:13

han0110 changed the title ~~[WIP] EVM Circuit and trait for op gadget~~ EVM Circuit and trait OpGadget Sep 1, 2021

han0110 requested review from ChihChengLiang, CPerezz and miha-stopar September 1, 2021 09:20

CPerezz reviewed Sep 1, 2021

View reviewed changes

therealyingtong mentioned this pull request Sep 2, 2021

[WIP] Implement ADD opcode. #15

Closed

3 tasks

han0110 commented Sep 2, 2021

View reviewed changes

zkevm-circuits/src/evm_circuit.rs Outdated Show resolved Hide resolved

gaswhat reviewed Sep 3, 2021

View reviewed changes

zkevm-circuits/src/evm_circuit/op_execution/arithmetic.rs Show resolved Hide resolved

DreamWuGit reviewed Sep 7, 2021

View reviewed changes

zkevm-circuits/src/evm_circuit/op_execution/arithmetic.rs Outdated Show resolved Hide resolved

han0110 mentioned this pull request Sep 7, 2021

Verify gas is indeed sufficient in success case #68

Closed

gaswhat added 6 commits September 8, 2021 22:13

fix

e333771

update test

bac2aa2

fix lt gadget

0bb40cf

update add/sub test case

e997305

eol

968df25

lint

6e8a53b

CPerezz approved these changes Sep 13, 2021

View reviewed changes

fix clippy

31e4db5

miha-stopar reviewed Sep 14, 2021

View reviewed changes

DreamWuGit reviewed Sep 14, 2021

View reviewed changes

zkevm-circuits/src/evm_circuit.rs Show resolved Hide resolved

gaswhat approved these changes Sep 15, 2021

View reviewed changes

han0110 added 2 commits September 16, 2021 02:41

fix: add AddGadget's carry range check (should be 0 or 1)

c7b32e2

fix: add constraint on qs_byte_lookups for cases to enable byte lookup

a8e4d6f

han0110 force-pushed the evm_circuit branch from ce345d5 to a8e4d6f Compare September 15, 2021 18:41

gaswhat and others added 3 commits September 15, 2021 23:09

fix clippy and address comments

b9d8ea1

fix more comments

5925fd3

Merge pull request #66 from scroll-dev/lt

f8924dc

Implement LT & GT opcode

therealyingtong reviewed Sep 16, 2021

View reviewed changes

ChihChengLiang approved these changes Sep 16, 2021

View reviewed changes

zkevm-circuits/src/evm_circuit.rs Outdated Show resolved Hide resolved

zkevm-circuits/src/evm_circuit/op_execution/push.rs Outdated Show resolved Hide resolved

han0110 added 3 commits September 16, 2021 17:13

fix: fix typo and comment

702039d

fix: remove witness selector boolean constraint cause OpGadget enforc…

8063d88

…es it

chore: remove commented implementation on PushGadget and move to issue …

26019ee

…#73

miha-stopar approved these changes Sep 17, 2021

View reviewed changes

han0110 merged commit 1e26882 into main Sep 17, 2021

han0110 deleted the evm_circuit branch September 17, 2021 06:27

ChihChengLiang added crate-zkevm-circuits Issues related to the zkevm-circuits workspace member T-opcode Type: opcode-related and focused PR/Issue labels Oct 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EVM Circuit and trait OpGadget #26

EVM Circuit and trait OpGadget #26

han0110 commented Aug 10, 2021 •

edited

ChihChengLiang left a comment

gaswhat Aug 27, 2021

CPerezz left a comment

CPerezz Sep 1, 2021

CPerezz left a comment

miha-stopar left a comment •

edited

han0110 commented Sep 14, 2021

miha-stopar commented Sep 14, 2021

miha-stopar commented Sep 15, 2021

gaswhat left a comment

gaswhat Sep 15, 2021

han0110 Sep 15, 2021 •

edited

therealyingtong left a comment

therealyingtong Sep 15, 2021

therealyingtong Sep 15, 2021

therealyingtong Sep 15, 2021

han0110 Sep 16, 2021

therealyingtong Sep 15, 2021

han0110 Sep 16, 2021

ChihChengLiang left a comment

	meta.advice_column(), // val1
	meta.advice_column(), // val1, call_id

	pub const NUM_CELL_OP_EXECTION_STATE: usize = 7;
	pub const NUM_CELL_OP_EXECUTION_STATE: usize = 7;

EVM Circuit and trait OpGadget #26

EVM Circuit and trait OpGadget #26

Conversation

han0110 commented Aug 10, 2021 • edited

ChihChengLiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPerezz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPerezz left a comment

Choose a reason for hiding this comment

miha-stopar left a comment • edited

Choose a reason for hiding this comment

han0110 commented Sep 14, 2021

miha-stopar commented Sep 14, 2021

miha-stopar commented Sep 15, 2021

gaswhat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

han0110 Sep 15, 2021 • edited

Choose a reason for hiding this comment

therealyingtong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChihChengLiang left a comment

Choose a reason for hiding this comment

han0110 commented Aug 10, 2021 •

edited

miha-stopar left a comment •

edited

han0110 Sep 15, 2021 •

edited