Fix/ecdsa err Fix #344 #345

zhenfeizhang · 2023-02-20T14:19:59Z

Description

[PR description]

#344

[link issue here]

Type of change

Bug fix (non-breaking change which fixes an issue)
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

Rationale

[design decisions and extended information]

How Has This Been Tested?

cargo test -p zkevm-circuits --features "onephase, enable-sign-verify, reject-eip2718" --release tx_cir  -- --nocapture

test tx_circuit::tx_circuit_tests::tx_circuit_2tx_2max_tx ... FAILED
test tx_circuit::tx_circuit_tests::tx_circuit_bad_address ... FAILED
test tx_circuit::tx_circuit_tests::tx_circuit_0tx_1max_tx ... ok
test tx_circuit::tx_circuit_tests::tx_circuit_1tx_1max_tx ... ok
test tx_circuit::tx_circuit_tests::tx_circuit_1tx_2max_tx ... ok
test tx_circuit::sign_verify::sign_verify_tests::sign_verify ... ok

How to fill a PR description

Please give a concise description of your PR.

The target readers could be future developers, reviewers, and auditors. By reading your description, they should easily understand the changes proposed in this pull request.

MUST: Reference the issue to resolve

Single responsability

Is RECOMMENDED to create single responsibility commits, but not mandatory.

Anyway, you MUST enumerate the changes in a unitary way, e.g.

This PR contains:
- Cleanup of xxxx, yyyy
- Changed xxxx to yyyy in order to bla bla
- Added xxxx function to ...
- Refactored ....

Design choices

RECOMMENDED to:

What types of design choices did you face?
What decisions you have made?
Any valuable information that could help reviewers to think critically

lispc · 2023-02-20T14:30:35Z

tx_circuit_2tx_2max_tx failed? is it expected?

lispc · 2023-02-20T14:33:59Z

circuit-benchmarks/src/bench_params.rs

@@ -0,0 +1 @@
+pub(crate) const DEGREE: usize = 19;


is this change needed?

fixed with 89fafcd

lispc · 2023-02-20T14:34:49Z

zkevm-circuits/src/tx_circuit.rs

+            let pk_hash = keccak(&sign_data);
+            let address = pk_hash.to_address();
+            assert_eq!(address, tx.caller_address);
+        }


suggest change to log::error! so the test tx_circuit_bad_address will not panic

fixed with 89fafcd

zhenfeizhang · 2023-02-20T14:35:22Z

test tx_circuit::tx_circuit_tests::tx_circuit_2tx_2max_tx ... FAILED
test tx_circuit::tx_circuit_tests::tx_circuit_bad_address ... FAILED

because of the assertion

        // assert tx.caller_address == recovered_pk
        for (sign_data, tx) in keccak_inputs_sign_verify(&sign_datas)
            .into_iter()
            .zip(self.txs.iter())
        {
            let pk_hash = keccak(&sign_data);
            let address = pk_hash.to_address();
            assert_eq!(address, tx.caller_address);
        }


---- tx_circuit::tx_circuit_tests::tx_circuit_2tx_2max_tx stdout ----
thread 'tx_circuit::tx_circuit_tests::tx_circuit_2tx_2max_tx' panicked at 'assertion failed: `(left == right)`
  left: `0x2e67890492c05eebdb86a6a9d2e0dbabd9629623`,
 right: `0x1c4fab018e8d4d88f0812c40651462c8fe689c7c`', zkevm-circuits/src/tx_circuit.rs:1686:13

---- tx_circuit::tx_circuit_tests::tx_circuit_bad_address stdout ----
thread 'tx_circuit::tx_circuit_tests::tx_circuit_bad_address' panicked at 'assertion failed: `(left == right)`
  left: `0xeefca179f40d3b8b3d941e6a13e48835a3af8241`,
 right: `0x1230000000000000000000000000000000000456`', zkevm-circuits/src/tx_circuit.rs:1686:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

shall we keep this check and return an error instead, or remove this check? @kunxian-xia

lispc · 2023-02-20T14:43:23Z

@kunxian-xia can you have another check of tx_circuit_2tx_2max_tx? strange..

zhenfeizhang · 2023-02-20T15:11:05Z

before padding
address from sign data 0xeefca179f40d3b8b3d941e6a13e48835a3af8241
address from sign data 0xf77b585877dcc1b7a1e71cb81898081b6d421ad0
address from sign data 0xd0377f5f15ede557b4b69cf49b2f6a25748f9010
address from sign data 0x2e67890492c05eebdb86a6a9d2e0dbabd9629623
address from sign data 0x7e5f4552091a69125d5dfcb7b8c2659029395bdf
===
tx.caller_address: 0xeefca179f40d3b8b3d941e6a13e48835a3af8241 
address from sign data 0xeefca179f40d3b8b3d941e6a13e48835a3af8241
tx.caller_address: 0xd0377f5f15ede557b4b69cf49b2f6a25748f9010 
address from sign data 0xf77b585877dcc1b7a1e71cb81898081b6d421ad0
[2023-02-20T15:05:32Z ERROR zkevm_circuits::tx_circuit] pk address from sign data 0xf77b585877dcc1b7a1e71cb81898081b6d421ad0 does not match the one from tx address 0xd0377f5f15ede557b4b69cf49b2f6a25748f9010
===
after padding
address from sign data 0xeefca179f40d3b8b3d941e6a13e48835a3af8241
address from sign data 0xf77b585877dcc1b7a1e71cb81898081b6d421ad0
address from sign data 0xd0377f5f15ede557b4b69cf49b2f6a25748f9010
address from sign data 0x2e67890492c05eebdb86a6a9d2e0dbabd9629623
address from sign data 0x7e5f4552091a69125d5dfcb7b8c2659029395bdf

seems like sign_datas is already padded and 0xf77b585877dcc1b7a1e71cb81898081b6d421ad0 is get inserted somehow

kunxian-xia · 2023-02-21T03:21:06Z

zkevm-circuits/src/tx_circuit/sign_verify.rs

        let (_pk, _, address) = ecdsa_chip.range.gate.inner_product(
            ctx,
-            &powers_of_256_cells[0..20].to_vec(),
-            &pk_hash_cells[12..].to_vec(),


pk_hash_cells is in little endian order?

zhenfeizhang · 2023-02-21T03:40:07Z

zkevm-circuits/src/tx_circuit.rs

+            .into_iter()
+            .enumerate()
+            .filter(|(idx, _)| {
+                // each sign_data produce two inputs for hashing


cool! and LGTM!

kunxian-xia and others added 2 commits February 20, 2023 18:41

add assertion that sign_data is valid

58e27c8

fix pk endianess bug

ab1befd

github-actions bot added crate-circuit-benchmarks crate-zkevm-circuits T-bench labels Feb 20, 2023

zhenfeizhang requested review from kunxian-xia and lispc February 20, 2023 14:20

clean up

7a8b263

zhenfeizhang self-assigned this Feb 20, 2023

lispc reviewed Feb 20, 2023

View reviewed changes

address comments

89fafcd

fix errors in the sign_data check

ce93161

kunxian-xia marked this pull request as ready for review February 21, 2023 03:11

Delete bench_params.rs

0f70fcc

github-actions bot removed crate-circuit-benchmarks T-bench labels Feb 21, 2023

lispc approved these changes Feb 21, 2023

View reviewed changes

Merge branch 'scroll-stable' into fix/ecdsa_err

2b9cf3f

kunxian-xia reviewed Feb 21, 2023

View reviewed changes

kunxian-xia approved these changes Feb 21, 2023

View reviewed changes

zhenfeizhang commented Feb 21, 2023

View reviewed changes

lispc merged commit 06e9d64 into scroll-stable Feb 21, 2023

lispc deleted the fix/ecdsa_err branch February 21, 2023 05:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix/ecdsa err Fix #344 #345

Fix/ecdsa err Fix #344 #345

Uh oh!

zhenfeizhang commented Feb 20, 2023 •

edited

Loading

Uh oh!

lispc commented Feb 20, 2023

Uh oh!

lispc Feb 20, 2023

Uh oh!

zhenfeizhang Feb 20, 2023

Uh oh!

lispc Feb 20, 2023

Uh oh!

zhenfeizhang Feb 20, 2023

Uh oh!

zhenfeizhang commented Feb 20, 2023 •

edited

Loading

Uh oh!

lispc commented Feb 20, 2023

Uh oh!

zhenfeizhang commented Feb 20, 2023

Uh oh!

kunxian-xia Feb 21, 2023

Uh oh!

zhenfeizhang Feb 21, 2023

Uh oh!

Uh oh!

Fix/ecdsa err Fix #344 #345

Fix/ecdsa err Fix #344 #345

Uh oh!

Conversation

zhenfeizhang commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

#344

Type of change

Contents

Rationale

How Has This Been Tested?

How to fill a PR description

Single responsability

Design choices

Uh oh!

lispc commented Feb 20, 2023

Uh oh!

lispc Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

zhenfeizhang Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

lispc Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

zhenfeizhang Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

zhenfeizhang commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lispc commented Feb 20, 2023

Uh oh!

zhenfeizhang commented Feb 20, 2023

Uh oh!

kunxian-xia Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

zhenfeizhang Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zhenfeizhang commented Feb 20, 2023 •

edited

Loading

zhenfeizhang commented Feb 20, 2023 •

edited

Loading