-
Notifications
You must be signed in to change notification settings - Fork 856
[word-lo-hi] evm circuit #1411
[word-lo-hi] evm circuit #1411
Conversation
974c04e
to
7be1ccd
Compare
a8f84d8
to
a601c89
Compare
related to #1415 |
56c13ed
to
a18b8d6
Compare
a18b8d6
to
9083bf5
Compare
9083bf5
to
ec8f365
Compare
Update: fix |
This is a big PR and while reviewing I think I can categorize my comments into:
I think the most important one to resolve is For |
@ed255 Good point. I'll focus on finding issues in |
It make senses! since evm circuit touch many change, I think first huge pr we can focus |
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 after addressing all issues.
3574d57
to
1a6f6a9
Compare
1a6f6a9
to
6bc88fe
Compare
Updated pr description to compare main/word-lo-hi cell utilization via command |
@@ -1473,29 +1241,14 @@ impl<'a, F: Field> EVMConstraintBuilder<'a, F> { | |||
self.reversion_info(call_id, false) | |||
} | |||
|
|||
pub(crate) fn reversion_info_write( | |||
pub(crate) fn reversion_info_write_unchecked( |
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.
Discussion: I noticed in begin_tx.rs and ReversionInfo main branch there is no range check, does it safe?
9ad99e7
to
6d82c49
Compare
6d82c49
to
87370f8
Compare
@@ -114,8 +118,7 @@ impl<F: Field> ExecutionGadget<F> for BalanceGadget<F> { | |||
self.same_context.assign_exec_step(region, offset, step)?; | |||
|
|||
let address = block.get_rws(step, 0).stack_value(); | |||
self.address_word | |||
.assign(region, offset, Some(address.to_le_bytes()))?; | |||
self.address.assign_u256(region, offset, address)?; |
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.
Shouldn't this be assign_h160(..., address.to_address())
?
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.
assign_u256
is correct because here address already in u256 little endian 20 bytes.
of course assign_h160(..., address.to_address())
is also equivalent, just we will convert to big endian then convert back to little endian immediately. I think this is just coding style, and assign_u256
looks still ok and efficiently save some conversion, so i prefer to keep assign_u256
quotient | ||
.to_word() | ||
.mul_selector(is_shl.expr()) | ||
.add_unchecked(dividend.to_word().mul_selector(is_shr.expr())), |
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 we could find a better way to write these kind of expressions (I also saw them in another opcode).
So originally we had:
v = is_shl.expr() * dividend.expr() + is_shr.expr() * quotient.expr())
* (1.expr() - divisor_is_zero.expr()
The logic for this is the following:
v = if (not (divisor_is_zero) {
if (is_shl) {
dividend
} else if (is_shr) {
quotient
} else {
0
}
} else {
0
};
So maybe we could have some helpers to write these if/else where the conditions are booleans and the values are words. But let's leave this for a future PR! I just wanted to share my thoughts 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.
Yes, agree current unchecked style looks very ugly and hard to read 🥲
It will be good to left it as a future PR and check whether can leverage macro!
features to make it looks confortable!
@ed255 I create another issue #1487 for recording future optimisation and housekeeping related to word-lo-hi, if you thought any related to |
pub fn expr_unchecked(&self) -> Expression<F> { | ||
self.lo() + self.hi() * (1 << (N_BYTES_HALF_WORD * 8)).expr() | ||
/// No overflow check on lo/hi limbs | ||
pub fn mul_unchecked(self, rhs: Self) -> Self { |
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 a future PR I think we should try to remove add_unchecked
, sub_unchecked
and mul_unchecked
and rewrite the gadgets that use these functionalities in a more clear way.
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've finished reviewing all the files, phew!
There are still comments from me marked as unresolved but I think we can discuss them / figure them out in a future PR. The only comment that I would like to really discuss before merging the PR is this one #1411 (comment)
Aside from that everything looks good to me! This was a massive work! Awesome job @hero78119 !!! 🎊
I'm approving the PR already so that you can merge when you see fit!
Appreciate @ChihChengLiang and @ed255 great effort on reviewing 🔥 👏 🙏 Will merge it sooner :) |
f245af8
to
ad221d8
Compare
Description
This PR is based on #1394
Need to merge #1394 first before review this.
Issue Link
#1379
Type of change
Contents
callop.rs
andbegin_tx.rs
.callop.rs
andbegin_tx.rs
construct_new
under evm circuitcargo test --features warn-unimplemented --features test --package zkevm-circuits --lib -- evm_circuit::execution::
word
gadgets generics to takeWord<T>
instead ofT
to restrict it flexibility, since it's non sense to put type not related to worddeprecated
api under evm circuitsCell utilization on main branch vs on word-lo-hi branch
Storage_1
Storage_2
Byte_lookup