Handle contract creation tx by roynalnaruto · Pull Request #1140 · privacy-ethereum/zkevm-circuits

roynalnaruto · 2023-02-06T16:12:25Z

This PR handles "case 1" in the BeginTx gadget, i.e. a contract deployment transaction.

Major changes:

Bus Mapping:
- Add RLP([tx_caller, tx_nonce]) to the keccak table
- Add a reversible account op for the deployed contract's nonce
BeginTx gadget:
- Conditional lookups to the keccak table (depending on byte_size(tx.nonce))
- Appropriate step transition for contract creation case
- More tests
Others:
- Add a keccak_rlc method in constraint builder that computes a random linear combination using challenge.keccak_input
- Add a BinaryNumber gadget to evm_circuit::util::math_gadget

CPerezz · 2023-02-07T19:46:44Z

Starting the review tomorrow as my first task. Will try to do it ASAP!

CPerezz

Nice work done here! It's a quite big PR.
Have done a 1st pass but will need to do a 2nd definitely and check deeper.

CPerezz · 2023-02-08T15:35:01Z

+        keccak_lookup!(0, tx_nonce_lt_128.expr());
+        keccak_lookup!(
+            1,
+            not::expr(tx_nonce_lt_128.expr()),
+            tx_nonce_byte_size_cmp.value_equals(1usize)
+        );
+        keccak_lookup!(2, tx_nonce_byte_size_cmp.value_equals(2usize));
+        keccak_lookup!(3, tx_nonce_byte_size_cmp.value_equals(3usize));
+        keccak_lookup!(4, tx_nonce_byte_size_cmp.value_equals(4usize));
+        keccak_lookup!(5, tx_nonce_byte_size_cmp.value_equals(5usize));
+        keccak_lookup!(6, tx_nonce_byte_size_cmp.value_equals(6usize));
+        keccak_lookup!(7, tx_nonce_byte_size_cmp.value_equals(7usize));
+        keccak_lookup!(8, tx_nonce_byte_size_cmp.value_equals(8usize));


There has to be a better way to do this I think. It's fine to land this now but we should file a tech-debt issue.

With the RLP gadget, I believe it will be possible to remove this. Before merging this PR, I'll make it a point to create that issue and define the scope. Again, as I said above, I'm happy to implement the RLP gadget for contract creation address (CREATE/CREATE2)

@CPerezz I've ported over @z2trillion 's work with tests, which includes the gadget to calculate CREATE address is here:
https://github.com/scroll-tech/zkevm-circuits/blob/feat/contract-create-rlp/zkevm-circuits/src/evm_circuit/util/math_gadget/rlp.rs#L182

I'd suggest I can do the refactoring as part of a separate PR (to make the reviewing process more convenient). But if you think it's better to be wrapped up in this very PR, happy to make the changes :)

Sure if we file a tech-debt PR before merging with all of the things like this.

This part of multiple condition guarded keccak lookups has been replaced by a single keccak lookup, where the input rlc and input length is calculated from the ContractCreate gadget that we merged earlier.

CPerezz · 2023-02-08T17:01:05Z

+                        tx_call_data_length.expr(),
+                    ),
+                    (CallContextFieldTag::Value, tx_value.expr()),
+                    (CallContextFieldTag::IsStatic, 0.expr()),


How do we know that IsStatic = false? We aren't in a create Tx but we should still be able to be in a non-static call.

Nevermind. The root call always needs to be non-static right? @ed255 do you happen to know?

Yes, I forgot to respond to this. My understanding was that a contract creation transaction (is_create * is_root == true) MUST be non-static.

roynalnaruto · 2023-02-13T08:08:50Z

@CPerezz I have added this PR: #1164

We can review/merge the above, then I can update this PR to refactor it and use the ContractCreateGadget (and remove the macros and unnecessary extra conditional guards). Sounds good?

CPerezz · 2023-02-13T08:42:36Z

@roynalnaruto perfect! I'll take a look to the other one after 2 other pending reviews I have! :)

roynalnaruto · 2023-02-16T20:11:22Z

@CPerezz This PR is now ready for review after refactoring on #1164 . The keccak lookup looks much neater now.

### Description The `CREATE2` input length and `RLC(input)` were not tested during the #1164 merge. This PR adds and enables a test for CREATE2 specific `input_len` and `input_rlc` calculation. ### Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### How Has This Been Tested? The helper container can test both `CREATE` and `CREATE2` inputs (length and `RLC(input_bytes)`). These inputs are then used as inputs to do a lookup to the Keccak table. ### Related PRs - #1164 - #1140 (needs rebasing after this is merged) --------- Co-authored-by: adria0.eth <5526331+adria0@users.noreply.github.com>

CPerezz · 2023-02-21T12:16:19Z

Is this ready for review now that the other related PRs have been merged @roynalnaruto ?

roynalnaruto · 2023-02-22T01:17:09Z

@CPerezz Yes it's ready for review. I've also answered your questions and resolved most of the review comments!

CPerezz

Just one change required.
Aside from a couple nits, the rest looks good!

Nice work @roynalnaruto

CPerezz · 2023-02-23T09:47:10Z

+                        tx_call_data_length.expr(),
+                    ),
+                    (CallContextFieldTag::Value, tx_value.expr()),
+                    (CallContextFieldTag::IsStatic, 0.expr()),


Nevermind. The root call always needs to be non-static right? @ed255 do you happen to know?

CPerezz

LGTM!!!

* fix: Stale code comment * fix: Inconsistent structure constructor enables clippy::inconsistent_struct_constructor lint * fix: Incorrect documentation on the MinMaxGadget * fix: cargo audit * fix clippy

feat(BeginTx): handle contract creation tx

1c60395

roynalnaruto requested a review from ed255 February 6, 2023 16:12

github-actions Bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Feb 6, 2023

roynalnaruto requested review from CPerezz and lispc February 6, 2023 16:12

lispc approved these changes Feb 7, 2023

View reviewed changes

Merge branch 'main' into feat/begintx-contract-deploy

5716b52

CPerezz suggested changes Feb 8, 2023

View reviewed changes

lispc mentioned this pull request Feb 9, 2023

more account lookup consistency issues #1118

Open

andyguzmaneth mentioned this pull request Feb 9, 2023

Bug on account existance checking in bus-mapping #1132

Closed

han0110 mentioned this pull request Feb 10, 2023

CALL inside tx init code panics #1153

Closed

roynalnaruto added 2 commits February 11, 2023 05:42

Merge branch 'main' into feat/begintx-contract-deploy

e75216d

chore: review by @CPerezz

d125fc3

roynalnaruto requested a review from CPerezz February 11, 2023 06:01

Merge branch 'main' into feat/begintx-contract-deploy

bfb6e97

Merge branch 'main' into feat/begintx-contract-deploy

2441a61

roynalnaruto mentioned this pull request Feb 14, 2023

Contract CREATE/CREATE2 Helper (Util) Gadget #1164

Merged

roynalnaruto added 3 commits February 14, 2023 15:43

Merge branch 'main' into feat/begintx-contract-deploy

c37e1db

Merge branch 'main' into feat/begintx-contract-deploy

b1dd1ee

refactor and use contract create gadget

d512d2e

roynalnaruto mentioned this pull request Feb 19, 2023

[Tests] CREATE2 inputs in Math Utils Gadget #1226

Merged

4 tasks

Merge branch 'main' into feat/begintx-contract-deploy

160929a

roynalnaruto added 2 commits February 21, 2023 04:52

Merge branch 'main' into feat/begintx-contract-deploy

9367139

Merge branch 'main' into feat/begintx-contract-deploy

f7d9786

roynalnaruto added 2 commits February 21, 2023 15:41

Merge branch 'main' into feat/begintx-contract-deploy

3cefda2

Merge branch 'main' into feat/begintx-contract-deploy

8e08cd1

roynalnaruto added 6 commits February 22, 2023 08:19

Merge branch 'main' into feat/begintx-contract-deploy

d21d1ab

Merge branch 'main' into feat/begintx-contract-deploy

955a2d5

Merge branch 'main' into feat/begintx-contract-deploy

5aff8e5

Merge branch 'main' into feat/begintx-contract-deploy

aa8ba0b

Merge branch 'main' into feat/begintx-contract-deploy

700e773

fix: latest updates broke compilation

33789b7

CPerezz suggested changes Feb 23, 2023

View reviewed changes

fix: code review suggestions

d5cf4e9

roynalnaruto requested a review from CPerezz February 23, 2023 10:03

roynalnaruto added 5 commits February 23, 2023 16:09

Merge branch 'main' into feat/begintx-contract-deploy

82c791d

Merge branch 'main' into feat/begintx-contract-deploy

8fc4330

Merge branch 'main' into feat/begintx-contract-deploy

d45cbd4

Merge branch 'main' into feat/begintx-contract-deploy

067af9c

Merge branch 'main' into feat/begintx-contract-deploy

affa194

CPerezz approved these changes Feb 23, 2023

View reviewed changes

CPerezz added this pull request to the merge queue Feb 23, 2023

Merged via the queue into privacy-ethereum:main with commit 646cd32 Feb 23, 2023

roynalnaruto deleted the feat/begintx-contract-deploy branch February 23, 2023 18:36

Conversation

roynalnaruto commented Feb 6, 2023

Uh oh!

CPerezz commented Feb 7, 2023

Uh oh!

CPerezz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roynalnaruto Feb 11, 2023 • 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!

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!

roynalnaruto commented Feb 13, 2023

Uh oh!

CPerezz commented Feb 13, 2023

Uh oh!

roynalnaruto commented Feb 16, 2023

Uh oh!

CPerezz commented Feb 21, 2023

Uh oh!

roynalnaruto commented Feb 22, 2023

Uh oh!

CPerezz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CPerezz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

roynalnaruto Feb 11, 2023 •

edited

Loading