Feat/#318 precompile ecrecover #495

KimiWu123 · 2023-10-16T05:41:25Z

closed #318

…t our architecture

KimiWu123 · 2023-10-23T05:59:06Z

src/zkevm_specs/util/ec.py

+
+# TODO: There is another one used in tx_circuit, try to merge into one.
+#       Reminder: endianness of public key is differ with the one in tx_circuit
+class ECDSAVerifyChip:


This was copied from tx_circuit and only change the endianness of public key (here and here)

KimiWu123 · 2023-10-23T06:07:25Z

src/zkevm_specs/sig_circuit.py

+from eth_utils import keccak
+
+
+class Row:


This circuit comes from SignVerifyChip in tx_circuit and added some columns for sig_table

han0110

Overall LGTM, the inputs (aux_data) are currently just pure witness, but in scroll it's constraint from the previous step (by the PrecompileGadget), so we'd also need to address this in the future.

src/zkevm_specs/sig_circuit.py

src/zkevm_specs/util/ec.py

src/zkevm_specs/util/tables.py

src/zkevm_specs/sig_circuit.py

han0110 · 2023-10-26T04:12:23Z

src/zkevm_specs/evm_circuit/execution/precompiles/ecrecover.py

+    is_recovered = FQ(instruction.is_zero(recovered_addr) != FQ(1))
+
+    # if the address is recovered, this call should be success either
+    instruction.constrain_equal(is_success, is_recovered)


It seems is_success will alwasy be true when gas is sufficient (scroll handles precompile insufficient gas in another execution state) even the input is malformed (ref: https://github.com/ethereum/execution-specs/blob/master/src/ethereum/shanghai/vm/precompiled_contracts/ecrecover.py)

fixed in 3ae72d0

src/zkevm_specs/evm_circuit/execution/precompiles/ecrecover.py

tests/evm/precompiles/test_ecRecover.py

KimiWu123 · 2023-10-26T07:08:14Z

Overall LGTM, the inputs (aux_data) are currently just pure witness, but in scroll it's constraint from the previous step (by the PrecompileGadget), so we'd also need to address this in the future.

Have created #498 for it. My plan is to implement verification logic first and then do integration part (the PrecompileGadget you mentioned)

miha-stopar

LGTM! For me it's fine if the duplicated code gets removed in the follow up PRs.

miha-stopar · 2023-10-26T11:28:09Z

specs/precompile/01ecRecover.md

+
+### Inputs
+
+The length of inputs is 128 bytes. The first 32 bytes is keccak hash of the message, and following 96 bytes are v, r, s values. v is either 27 or 28.


Nitpick: "and following" -> "and the following", "v is either" -> "The value v is either"

fixed in 86866f9

miha-stopar · 2023-10-26T11:30:42Z

specs/precompile/01ecRecover.md

+
+### Output
+
+The recovered 20-byte address right aligned to 32 byte. If an address can't be recovered or not enough gas was given, then return 0.


Nitpick: "then return 0" -> "then the output is 0".

fixed in 86866f9

miha-stopar · 2023-10-26T11:34:27Z

specs/sig-proof.md

+
+The Sig Circuit aims at proving the correctness of SigTable. This mainly includes the following type of constraints:
+- checking that the signature is obtained correctly. This is done by the ECDSA chip, and the correctness of `v` is checked separately;
+- checking that `msg_hash` is obtained correctly from Keccak hash function. This is done by lookup to Keccak table;


Nitpick: Perhaps make "checking" upper case as there are two sentences in the bullet.

fixed in 86866f9

miha-stopar · 2023-10-26T11:38:10Z

src/zkevm_specs/evm_circuit/execution/precompiles/ecrecover.py

+    is_recovered = FQ(instruction.is_zero(recovered_addr) != FQ(1))
+
+    # is_success is always true
+    # ref: ref: https://github.com/ethereum/execution-specs/blob/master/src/ethereum/shanghai/vm/precompiled_contracts/ecrecover.py


Nitpick: "ref:" two times

fixed in 86866f9

han0110

LGTM! Nice work!

doc: ecRecover.md spec file

0ff6fd4

KimiWu123 self-assigned this Oct 16, 2023

doc: signature circuit, copied from Sroll's design and revisted to fi…

52a8dd1

…t our architecture

KimiWu123 force-pushed the feat/#318-precompile-ecrecover branch from 14624c1 to 52a8dd1 Compare October 16, 2023 05:44

feat: impl. sig_circuit

9c6b839

KimiWu123 force-pushed the feat/#318-precompile-ecrecover branch from e0f42f3 to 9c6b839 Compare October 18, 2023 08:22

KimiWu123 added 2 commits October 18, 2023 17:10

test: add more cases

b2f3d2c

feat: add sig_table

e53546d

KimiWu123 force-pushed the feat/#318-precompile-ecrecover branch from 4420adf to b59667c Compare October 19, 2023 01:54

doc: complete constraints desc.

9690c3d

KimiWu123 force-pushed the feat/#318-precompile-ecrecover branch from b59667c to 9690c3d Compare October 19, 2023 03:24

feat: impl. ecRecover

1bf4565

KimiWu123 force-pushed the feat/#318-precompile-ecrecover branch from c8da278 to 1bf4565 Compare October 19, 2023 04:16

han0110 self-requested a review October 20, 2023 03:40

KimiWu123 added 2 commits October 20, 2023 13:02

test: add a normal case

8e29537

test: complete testing

e53104a

KimiWu123 force-pushed the feat/#318-precompile-ecrecover branch from aebea3e to e53104a Compare October 23, 2023 02:54

feat: remove rlc usage in sig circuit

c0e0a22

KimiWu123 changed the title ~~[WIP] Feat/#318 precompile ecrecover~~ Feat/#318 precompile ecrecover Oct 23, 2023

KimiWu123 commented Oct 23, 2023

View reviewed changes

KimiWu123 marked this pull request as ready for review October 23, 2023 06:08

han0110 reviewed Oct 26, 2023

View reviewed changes

KimiWu123 mentioned this pull request Oct 26, 2023

Refactor: clean up duplicated keccak_table #499

Closed

KimiWu123 added 4 commits October 26, 2023 15:39

feat: correct public key to little-endian

80e97bd

fix: is_success is always true and using iz_zero gadget for sig_r/v

3ae72d0

test: fix testing data

b517a95

fix return data length when the addr is not recoverable

a1b878d

KimiWu123 requested a review from han0110 October 26, 2023 08:48

miha-stopar self-requested a review October 26, 2023 10:07

miha-stopar approved these changes Oct 26, 2023

View reviewed changes

doc: refinement

86866f9

han0110 approved these changes Oct 27, 2023

View reviewed changes

KimiWu123 merged commit 6a9b04c into master Oct 27, 2023
1 check passed

KimiWu123 deleted the feat/#318-precompile-ecrecover branch November 25, 2023 07:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/#318 precompile ecrecover #495

Feat/#318 precompile ecrecover #495

KimiWu123 commented Oct 16, 2023

KimiWu123 Oct 23, 2023

KimiWu123 Oct 23, 2023

han0110 left a comment

han0110 Oct 26, 2023

KimiWu123 Oct 26, 2023

KimiWu123 commented Oct 26, 2023

miha-stopar left a comment

miha-stopar Oct 26, 2023

KimiWu123 Oct 27, 2023

miha-stopar Oct 26, 2023

KimiWu123 Oct 27, 2023

miha-stopar Oct 26, 2023

KimiWu123 Oct 27, 2023

miha-stopar Oct 26, 2023

KimiWu123 Oct 27, 2023

han0110 left a comment


		### Inputs

		The length of inputs is 128 bytes. The first 32 bytes is keccak hash of the message, and following 96 bytes are v, r, s values. v is either 27 or 28.


		### Output

		The recovered 20-byte address right aligned to 32 byte. If an address can't be recovered or not enough gas was given, then return 0.

Feat/#318 precompile ecrecover #495

Feat/#318 precompile ecrecover #495

Conversation

KimiWu123 commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

han0110 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KimiWu123 commented Oct 26, 2023

miha-stopar 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

han0110 left a comment

Choose a reason for hiding this comment