This repository has been archived by the owner on Jul 5, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 272
Feat/#498 integrate precompiles into callop #508
Merged
KimiWu123
merged 13 commits into
master
from
feat/#498-integrate-precompiles-into-callop
Feb 21, 2024
Merged
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
ed1cb05
feat: callop and PrecompileGadget (draft)
KimiWu123 df05ae2
feat: precompile oog
KimiWu123 9ddd4a0
feat: remove rlc in callop for PrecompileGadget
KimiWu123 7e6e2ad
test: refactor callop test for precompiles
KimiWu123 f130ab8
test: complete testing for precompiles in callop
KimiWu123 f8afa4e
chore: add comments
KimiWu123 17c326c
feat: complete precompile checks in precompile_gadget
KimiWu123 bedd91c
Update src/zkevm_specs/evm_circuit/execution/callop.py
KimiWu123 46f951e
fix: incorrect caller/callee call context
KimiWu123 3cf679c
fix: ecRecover allows input len is not 128 bytes
KimiWu123 b5816af
feat: add copy rlc for precompiles input/output
KimiWu123 f3eee6f
feat: add input/output rlc data check in ecRecover (PoC)
KimiWu123 2e5f926
refactor ecrecover rlc input
KimiWu123 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Aren't these two values actually the same (I mean not equal, but the same)? Because
rlc_data.input_rlc
is computed when instantiating the auxiliary data andinput_rlc
is computed accessing the same values inaux_data
?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.
For me, data assignment in testing is like what
assign_exec_step
does at proving time in our circuit. So, yes, it looks the same (but not always the same).At proving time, a prover assigns
input_rlc
here, this field is to verify data consistency between calls)In verification logic here, we have to gaurantee msg_hash, sig_v, sig_r and sig_s are the same pairs (sig_v, sig_r and sig_s are coming from the signature of msg_hash). However, a malious signer could sign msg_hash2 and have sig_v2, sig_r2 and sig_s2. It still can pass all the constraints if we don't have calculate input_rlc here. I'm assuming
rlc_data.input_rlc
is coming from previous step (we can pass data around between previous step and next step in our circuit, but it seems not doable in our spec), so that's whyrlc_data.input_rlc
andinput_rlc
look "the same".Does it make sense to you? Or do you think we need to copy
rlc_data.input_rlc
from copy_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.
I think I am missing something and would like to fix my understanding :). My understanding was that we need to ensure that
instruction.curr.aux_data
is the same as used incallop.py
and that this should be done by checking the RLC, so to have a lookup call intocopy_table
(RLC field) here inecrecover.py
. Maybe this is not the case?I agree on the assignment part, putting a link to the assignment was probably misleading from my side. What it seems to me here is that we have
aux_data: PrecompileAuxData = instruction.curr.aux_data[0]
and then both values,aux_data.input_rlc
andinput_rlc
, are computed usingaux_data
. Or is itaux_data.input_rlc
checked somewhere else?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.
Yes, that's exactly what I want to do.
No,
aux_data.input_rlc
was not check in other place.In my implementation, I treated
aux_data
as witness inputs, a place I can assign my witnesses. Those data (e.g. sig_r, sig_v...etc) can't be retrieved from stack or memory like what other opcode gagdets implemented.I might not explain it very well, but you can check Scroll's impl. The
msg_hash_raw
was converted into rlc formate and compared withmsg_hash_keccak_rlc
. In the assignment, you can see both of them come from the same source, here and here.