Fix to handle successful run with Uint64 overflow for multiple opcodes by silathdiir · Pull Request #1317 · privacy-ethereum/zkevm-circuits

silathdiir · 2023-03-13T10:14:15Z

Description

Fix successful run cases with Uint64 overflow for multiple opcodes.

Add WordByteRangeGadget to constrain if Word is within the specified byte range.
Add WordByteCapGadget to constrain if Word is within the specified byte range (implemented by WordByteRangeGadget) and less than a maximum cap (used to replace a WordByteRangeGadget and LtGadget).
Fix bus-mapping and zkevm-circuits to handle overflow cases. And add unit-tests for these cases.

TODO: will try to handle memory offset overflow with zero length in another PR (try to rebase for this local PR scroll-tech#393) and related issue #1301.

Rationale

Reference detailed code in go-etherum as:

. BLOCKHASH
. CALLDATALOAD
. CALLDATACOPY
. CODECOPY
. EXTCODECOPY
. JUMPI

Issue Link

Close #1276

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Add unit-test cases for Uint64 overflow values.

…t-params

ed255

First pass review, so far I've reviewed:

common_gadget
circuits: jumpi, extcodecopy, codecopy, error_invalid_jump

ed255

Review completed: In general everything looks correct! My only notes are about a redundant constraint, and a few IsZero gadget that seem to be unused. I think they should be used: the Lt check is only correct if the value uses the specified number of bytes, and that's what the IsZero gadget inside of WordByteRangeGadget can check. So probably the logic should be something like:

            let src_addr = select::expr(
                code_offset.overflow(), 
                code_size.expr(), 
                select::expr(
                   code_offset.lt_cap(),
                   code_offset.valid_value(),
                   code_size.expr()),
            );

silathdiir · 2023-04-18T01:27:48Z

Review completed ...

Thanks for review. I will try to resolve merge conflicts and review comments. Thanks.

…-cases-with-overflow-input-params

This reverts commit c17ba9d.

adria0 · 2023-04-22T09:51:22Z

LGTM

Amazing work @silathdiir

Also I see that this PR also activates EXTCODECOPY and there's a bunch of tests that are not passing, but nice to address them in another PRs.

e.g. cargo run --release -- --inspect "extcodehashEmpty_d1(storage)_g0_v0" dumps

error: constraint not satisfied

  Cell layout in region 'Execution step':
    | Offset |EVM_q_step|
    +--------+----------+
    |   345  |    x0    | <--{ Gate 'CALL_OP' applied here

  Constraint 'sum(addends_lo) == sum_lo + carry_lo ⋅ 2^128':
    (S9 * x0) * (( * ) * ((0 + 0 +  * 1 +  * 0x100 +  * 0x10000 +  * 0x1000000 +  * 0x100000000 +  * 0x10000000000 +  * 0x1000000000000 +  * 0x100000000000000 +  * 0x10000000000000000 +  * 0x1000000000000000000 +  * 0x100000000000000000000 +  * 0x10000000000000000000000 +  * 0x1000000000000000000000000 +  * 0x100000000000000000000000000 +  * 0x10000000000000000000000000000 +  * 0x1000000000000000000000000000000 + 0 +  * 1 +  * 0x100 +  * 0x10000 +  * 0x1000000 +  * 0x100000000 +  * 0x10000000000 +  * 0x1000000000000 +  * 0x100000000000000 +  * 0x10000000000000000 +  * 0x1000000000000000000 +  * 0x100000000000000000000 +  * 0x10000000000000000000000 +  * 0x1000000000000000000000000 +  * 0x100000000000000000000000000 +  * 0x10000000000000000000000000000 +  * 0x1000000000000000000000000000000 - (0 +  * 1 +  * 0x100 +  * 0x10000 +  * 0x1000000 +  * 0x100000000 +  * 0x10000000000 +  * 0x1000000000000 +  * 0x100000000000000 +  * 0x10000000000000000 +  * 0x1000000000000000000 +  * 0x100000000000000000000 +  * 0x10000000000000000000000 +  * 0x1000000000000000000000000 +  * 0x100000000000000000000000000 +  * 0x10000000000000000000000000000 +  * 0x1000000000000000000000000000000 +  * 0x100000000000000000000000000000000)) * )) = 0

  Assigned cell values:
    x0 = 1

error: constraint not satisfied

  Cell layout in region 'Execution step':
    | Offset |EVM_q_step|
    +--------+----------+
    |   345  |    x0    | <--{ Gate 'CALL_OP' applied here

  Constraint 'sum(addends_hi) + carry_lo == sum_hi':
    (S9 * x0) * (( * ) * ((0 + 0 +  * 1 +  * 0x100 +  * 0x10000 +  * 0x1000000 +  * 0x100000000 +  * 0x10000000000 +  * 0x1000000000000 +  * 0x100000000000000 +  * 0x10000000000000000 +  * 0x1000000000000000000 +  * 0x100000000000000000000 +  * 0x10000000000000000000000 +  * 0x1000000000000000000000000 +  * 0x100000000000000000000000000 +  * 0x10000000000000000000000000000 +  * 0x1000000000000000000000000000000 + 0 +  * 1 +  * 0x100 +  * 0x10000 +  * 0x1000000 +  * 0x100000000 +  * 0x10000000000 +  * 0x1000000000000 +  * 0x100000000000000 +  * 0x10000000000000000 +  * 0x1000000000000000000 +  * 0x100000000000000000000 +  * 0x10000000000000000000000 +  * 0x1000000000000000000000000 +  * 0x100000000000000000000000000 +  * 0x10000000000000000000000000000 +  * 0x1000000000000000000000000000000 +  - (0 +  * 1 +  * 0x100 +  * 0x10000 +  * 0x1000000 +  * 0x100000000 +  * 0x10000000000 +  * 0x1000000000000 +  * 0x100000000000000 +  * 0x10000000000000000 +  * 0x1000000000000000000 +  * 0x100000000000000000000 +  * 0x10000000000000000000000 +  * 0x1000000000000000000000000 +  * 0x100000000000000000000000000 +  * 0x10000000000000000000000000000 +  * 0x1000000000000000000000000000000)) * )) = 0

  Assigned cell values:
    x0 = 1

error: constraint not satisfied

  Cell layout in region 'Execution step':
    | Offset |EVM_q_step|
    +--------+----------+
    |   345  |    x0    | <--{ Gate 'CALL_OP' applied here

  Constraint 'sum(addends_lo) == sum_lo + carry_lo ⋅ 2^128':
    (S9 * x0) * (( * ) * ((0 + 0 +  * 1 +  * 0x100 +  * 0x10000 +  * 0x1000000 +  * 0x100000000 +  * 0x10000000000 +  * 0x1000000000000 +  * 0x100000000000000 +  * 0x10000000000000000 +  * 0x1000000000000000000 +  * 0x100000000000000000000 +  * 0x10000000000000000000000 +  * 0x1000000000000000000000000 +  * 0x100000000000000000000000000 +  * 0x10000000000000000000000000000 +  * 0x1000000000000000000000000000000 + 0 +  * 1 +  * 0x100 +  * 0x10000 +  * 0x1000000 +  * 0x100000000 +  * 0x10000000000 +  * 0x1000000000000 +  * 0x100000000000000 +  * 0x10000000000000000 +  * 0x1000000000000000000 +  * 0x100000000000000000000 +  * 0x10000000000000000000000 +  * 0x1000000000000000000000000 +  * 0x100000000000000000000000000 +  * 0x10000000000000000000000000000 +  * 0x1000000000000000000000000000000 - (0 +  * 1 +  * 0x100 +  * 0x10000 +  * 0x1000000 +  * 0x100000000 +  * 0x10000000000 +  * 0x1000000000000 +  * 0x100000000000000 +  * 0x10000000000000000 +  * 0x1000000000000000000 +  * 0x100000000000000000000 +  * 0x10000000000000000000000 +  * 0x1000000000000000000000000 +  * 0x100000000000000000000000000 +  * 0x10000000000000000000000000000 +  * 0x1000000000000000000000000000000 +  * 0x100000000000000000000000000000000)) * )) = 0

  Assigned cell values:
    x0 = 1

error: constraint not satisfied

  Cell layout in region 'Execution step':
    | Offset |EVM_q_step|
    +--------+----------+
    |   345  |    x0    | <--{ Gate 'CALL_OP' applied here

  Constraint 'State transition (delta) constraint of rw_counter':
    (S9 * x0) * (( * ) * ( * )) = 0

  Assigned cell values:
    x0 = 1

error: constraint not satisfied

  Cell layout in region 'Execution step':
    | Offset |EVM_q_step|
    +--------+----------+
    |   345  |    x0    | <--{ Gate 'CALL_OP' applied here

  Constraint 'State transition (delta) constraint of reversible_write_counter':
    (S9 * x0) * (( * ) * ( * )) = 0

  Assigned cell values:
    x0 = 1

error: lookup input does not exist in table
  (L0) ∉ ("A13" * C2 + "A12" * C2 + "A11" * C2 + "A10" * C2 + "A9" * C2 + "field_tag" * C2 + "address" * C2 + "id" * C2 + "tag" * C2 + "is_write" * C2 + "rw_counter")

  Lookup 'Rw' inputs:
    L0 = x0
    ^

    | Cell layout in region 'Execution step':
    |   | Offset |EVM_lookup_rw_0|
    |   +--------+---------------+
    |   |   348  |       x0      | <--{ Lookup 'Rw' inputs queried here
    |
    | Assigned cell values:
    |   x0 = 0x27f6a29ea0deafc1f53194885721c68d2d039966c07f5a0db4f23e49b662f264

error: lookup input does not exist in table
  (L0) ∉ ("A13" * C2 + "A12" * C2 + "A11" * C2 + "A10" * C2 + "A9" * C2 + "field_tag" * C2 + "address" * C2 + "id" * C2 + "tag" * C2 + "is_write" * C2 + "rw_counter")

  Lookup 'Rw' inputs:
    L0 = x0
    ^

    | Cell layout in region 'Execution step':
    |   | Offset |EVM_lookup_rw_2|
    |   +--------+---------------+
    |   |   348  |       x0      | <--{ Lookup 'Rw' inputs queried here
    |
    | Assigned cell values:
    |   x0 = 0x2c972172974c7a6810556b70c357a2743815b4c4fd8442fdd8eaf8bf5b8dad64

adria0 · 2023-04-21T10:20:23Z

+        from_bytes::expr(&self.original.cells[..VALID_BYTES])
+    }
+
+    pub(crate) fn within_range(&self) -> Expression<F> {


Nitpick: maybe is more readable if we name not_overflow() instead valid_range(), overflow/not_overflow is more dualistic over the same thing.

Rename within_range to not_overflow in commit 11f8eb3.

adria0 · 2023-04-21T10:30:30Z

+            for (i, byte) in calldata_bytes.iter_mut().enumerate() {
+                if call.is_root {
+                    // Fetch from tx call data.
+                    if src_addr.checked_add(i as u64).unwrap() < tx.call_data_length as u64 {


Maybe better just adding instead using checked_add with an unwrap later?

Replace check_add and unwrap with just adding in commit 594553d.

adria0 · 2023-04-21T10:31:53Z

+                    }
+                } else {
+                    // Fetch from memory.
+                    if src_addr.checked_add(i as u64).unwrap()


Replace check_add and unwrap with just adding in commit 594553d.

…-cases-with-overflow-input-params

…` function (only leave it in `construct`).

…t-params

… is Uint64 overflow` in `jumpi`.

silathdiir · 2023-04-27T08:05:42Z

Review completed: In general everything looks correct! My only notes are about a redundant constraint, and a few IsZero gadget that seem to be unused. I think they should be used: the Lt check is only correct if the value uses the specified number of bytes, and that's what the IsZero gadget inside of WordByteRangeGadget can check. So probably the logic should be something like:
            let src_addr = select::expr(
                code_offset.overflow(), 
                code_size.expr(), 
                select::expr(
                   code_offset.lt_cap(),
                   code_offset.valid_value(),
                   code_size.expr()),
            );

Seems that lt_cap contains the logic of within_range as with_range && offset < cap in WordByteCapGadget::construct. Could we use lt_cap to check directly?

            let src_addr = select::expr(
                code_offset.lt_cap(),
                code_offset.valid_value(),
                code_size.expr(),
            );

silathdiir · 2023-04-27T08:16:37Z

LGTM

Amazing work @silathdiir

Also I see that this PR also activates EXTCODECOPY and there's a bunch of tests that are not passing, but nice to address them in another PRs.

e.g. cargo run --release -- --inspect "extcodehashEmpty_d1(storage)_g0_v0"

It may be caused by memory_offset + memory_length should not be Uint64 overflow. I will check if it could be fixed by local PR scroll-tech#393. Thanks.

Fix to handle successful run with Uint64 overflow in multiple opcodes.

640b275

github-actions Bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-eth-types Issues related to the eth-types workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Mar 13, 2023

silathdiir added 2 commits March 13, 2023 18:23

Fix lint.

12669db

Fix failed cases.

78931f3

silathdiir changed the title ~~Bug: handle successful run with Uint64 overflow for multiple opcodes~~ Fix to handle successful run with Uint64 overflow for multiple opcodes Mar 13, 2023

Merge branch 'main' into bug/some-successful-cases-with-overflow-inpu…

a3226b4

…t-params

ed255 reviewed Apr 13, 2023

View reviewed changes

ed255 reviewed Apr 17, 2023

View reviewed changes

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

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

silathdiir added 4 commits April 18, 2023 16:29

Merge remote-tracking branch 'upstream/main' into bug/some-successful…

6fe356e

…-cases-with-overflow-input-params

Fix failed tests.

82505e2

Try to trigger CI for network failure.

c17ba9d

Revert "Try to trigger CI for network failure."

ee29e93

This reverts commit c17ba9d.

adria0 self-requested a review April 21, 2023 06:56

adria0 approved these changes Apr 22, 2023

View reviewed changes

silathdiir added 3 commits April 24, 2023 09:40

Merge remote-tracking branch 'upstream/main' into bug/some-successful…

c1d18a2

…-cases-with-overflow-input-params

Fix lint.

138378f

Merge remote-tracking branch 'upstream/main' into bug/some-successful…

2092dc2

…-cases-with-overflow-input-params

lispc approved these changes Apr 27, 2023

View reviewed changes

silathdiir and others added 3 commits April 27, 2023 14:43

Delete debug_assert!(VALID_BYTES < 32) in assign and `valid_value…

29cf64c

…` function (only leave it in `construct`).

Merge branch 'main' into bug/some-successful-cases-with-overflow-inpu…

4419f2b

…t-params

Delete redundant constraint `JUMPI condition must be 0 if destination…

787093f

… is Uint64 overflow` in `jumpi`.

silathdiir added 2 commits April 27, 2023 16:24

Rename within_range to not_overflow.

11f8eb3

Replace checked_add and unwrap with just adding.

594553d

silathdiir requested a review from ed255 April 27, 2023 09:03

lispc enabled auto-merge April 28, 2023 06:37

Update some comments to retry CI.

74a938b

auto-merge was automatically disabled April 28, 2023 11:19
Head branch was pushed to by a user without write access

lispc added this pull request to the merge queue Apr 28, 2023

Merged via the queue into privacy-ethereum:main with commit 22dd263 Apr 28, 2023

silathdiir deleted the bug/some-successful-cases-with-overflow-input-params branch April 29, 2023 06:33

silathdiir added a commit to scroll-tech/zkevm-circuits that referenced this pull request May 2, 2023

Merge [u64-overflow PR](privacy-ethereum#1317) to develop.

9d6579a

silathdiir mentioned this pull request May 2, 2023

Merge upstream u64-overflow PR back to develop scroll-tech/zkevm-circuits#496

Merged

1 task

Conversation

silathdiir commented Mar 13, 2023

Description

Rationale

Issue Link

Type of change

How Has This Been Tested?

Uh oh!

ed255 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ed255 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

silathdiir commented Apr 18, 2023

Uh oh!

adria0 commented Apr 22, 2023

Uh oh!

adria0 Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

silathdiir Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

adria0 Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

silathdiir Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

adria0 Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

silathdiir Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

silathdiir commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silathdiir commented Apr 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

silathdiir commented Apr 27, 2023 •

edited

Loading