-
Notifications
You must be signed in to change notification settings - Fork 856
Conversation
…circuits into tmp/challenge-evm-circuit
b5ee253
to
5ba2cf1
Compare
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.
First round pass. Overall looks good to me! Only left some small questions.
#[test] | ||
fn callop_base() { | ||
test_ok( | ||
caller(&OpcodeId::CALL, Stack::default(), true), | ||
callee(bytecode! {}), | ||
); | ||
// use crate::evm_circuit::util::print_op_count; | ||
// print_op_count(); | ||
} | ||
|
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.
leftover?
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.
oops! fixed in 8aa678d
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 to be still here 🤣
zkevm-circuits/src/evm_circuit/util/math_gadget/batched_is_zero.rs
Outdated
Show resolved
Hide resolved
pub(crate) fn storage_for_inv<F: FieldExt>(value: &Expression<F>) -> CellType { | ||
match Self::expr_phase(value) { | ||
0 => CellType::StoragePhase1, | ||
1 => CellType::StoragePhase3, |
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 can't think about the reason to require phase 3 here? Would you like to explain it more?
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, the idea is is that which is the "storage requiered to store the inverse of an expression", and this function is really only used in IsZeroGadget
.
IIRC, we need an extra phase when computing the inverse of an expression (this is why is_zero in the IsZeroGadget) when it was not in the first phase.
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.
hmm but the expr_phase
returns max_phase_challenge_usable_after
, which seems to be "this expression will be evaluateable after such phase", so the inverse should also be in max_phase_challenge_usable_after + 1
, right?
For example, an expression advice_at_phase_1 + challenge_usable_after_phase_1
, we can store its inverse in phase 2.
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.
👍
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.
Done in 8bfda72
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.
hmm sorry I think I'm confused about this function now, so what's the difference between storage_for
and storage_for_inv
? Inverse of a value should be able to live in the same phase as the value, right?
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.
The idea was:
storage_for
-> storage for an expressionstorage_for_inv
-> storage for the inverse of an expression
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 understand, my question is more like, what makes inverse storage need one extra phase to be stored? For example advice_at_phase_1 + challenge_usable_after_phase_1
we should be able to store its inverse in phase 2 right? Because this expression is evaluateable in phase 2.
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.
Changed in 61710e8
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.
First pass
e69e704
to
75149e6
Compare
75149e6
to
c755ee2
Compare
…circuits into feat/challenge-evm-circuit
…ng-explorations/zkevm-circuits into feat/challenge-evm-circuit
…circuits into feat/challenge-evm-circuit
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.
Finished my second review, only left some nitpicks. Also I think now it might be a good time to try to run integration test for this PR before being merged.
#[test] | ||
fn callop_base() { | ||
test_ok( | ||
caller(&OpcodeId::CALL, Stack::default(), true), | ||
callee(bytecode! {}), | ||
); | ||
// use crate::evm_circuit::util::print_op_count; | ||
// print_op_count(); | ||
} | ||
|
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 to be still here 🤣
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.
Nice work!
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. I added some nitpicks.
…circuits into feat/challenge-evm-circuit
The original PR was #990 but after renaming the branch to
feat/challenge-evm-circuit
github automatically closed it.Depends on privacy-scaling-explorations/halo2#115