-
Notifications
You must be signed in to change notification settings - Fork 272
Conversation
…t our architecture
14624c1
to
52a8dd1
Compare
e0f42f3
to
9c6b839
Compare
4420adf
to
b59667c
Compare
b59667c
to
9690c3d
Compare
c8da278
to
1bf4565
Compare
aebea3e
to
e53104a
Compare
|
||
# 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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from eth_utils import keccak | ||
|
||
|
||
class Row: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This circuit comes from SignVerifyChip
in tx_circuit and added some columns for sig_table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 3ae72d0
Have created #498 for it. My plan is to implement verification logic first and then do integration part (the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! For me it's fine if the duplicated code gets removed in the follow up PRs.
specs/precompile/01ecRecover.md
Outdated
|
||
### 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: "and following" -> "and the following", "v is either" -> "The value v is either"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 86866f9
specs/precompile/01ecRecover.md
Outdated
|
||
### 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: "then return 0" -> "then the output is 0".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 86866f9
specs/sig-proof.md
Outdated
|
||
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Perhaps make "checking" upper case as there are two sentences in the bullet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 86866f9
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: "ref:" two times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 86866f9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Nice work!
closed #318