-
Notifications
You must be signed in to change notification settings - Fork 858
fix(zkevm-circuits/begin_tx): add missing constraints #1776
Changes from all commits
8c231e1
4390f88
4f77fa2
1ab9f99
8996f37
13d9cba
c40d3da
bc6715a
757dbf1
496829a
2291146
3af8f8f
f69053e
b6494f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -12,21 +12,22 @@ use crate::{ | |||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
is_precompiled, | ||||||||||||||||||||||||||||
math_gadget::{ | ||||||||||||||||||||||||||||
ContractCreateGadget, IsEqualWordGadget, IsZeroWordGadget, RangeCheckGadget, | ||||||||||||||||||||||||||||
ContractCreateGadget, IsEqualWordGadget, IsZeroGadget, IsZeroWordGadget, | ||||||||||||||||||||||||||||
RangeCheckGadget, | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
not, | ||||||||||||||||||||||||||||
not, rlc, | ||||||||||||||||||||||||||||
tx::{BeginTxHelperGadget, TxDataGadget}, | ||||||||||||||||||||||||||||
AccountAddress, CachedRegion, Cell, StepRws, | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
witness::{Block, Call, ExecStep, Transaction}, | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
table::{AccountFieldTag, BlockContextFieldTag, CallContextFieldTag}, | ||||||||||||||||||||||||||||
table::{AccountFieldTag, BlockContextFieldTag, CallContextFieldTag, TxContextFieldTag}, | ||||||||||||||||||||||||||||
util::{ | ||||||||||||||||||||||||||||
word::{Word32Cell, WordExpr, WordLoHi, WordLoHiCell}, | ||||||||||||||||||||||||||||
Expr, | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||
use bus_mapping::state_db::CodeDB; | ||||||||||||||||||||||||||||
use bus_mapping::{circuit_input_builder::CopyDataType, state_db::CodeDB}; | ||||||||||||||||||||||||||||
use eth_types::{evm_types::PRECOMPILE_COUNT, keccak256, Field, OpsIdentity, ToWord, U256}; | ||||||||||||||||||||||||||||
use halo2_proofs::{ | ||||||||||||||||||||||||||||
circuit::Value, | ||||||||||||||||||||||||||||
|
@@ -45,6 +46,9 @@ pub(crate) struct BeginTxGadget<F> { | |||||||||||||||||||||||||||
code_hash: WordLoHiCell<F>, | ||||||||||||||||||||||||||||
is_empty_code_hash: IsEqualWordGadget<F, WordLoHi<Expression<F>>, WordLoHi<Expression<F>>>, | ||||||||||||||||||||||||||||
caller_nonce_hash_bytes: Word32Cell<F>, | ||||||||||||||||||||||||||||
calldata_length: Cell<F>, | ||||||||||||||||||||||||||||
calldata_length_is_zero: IsZeroGadget<F>, | ||||||||||||||||||||||||||||
calldata_rlc: Cell<F>, | ||||||||||||||||||||||||||||
create: ContractCreateGadget<F, false>, | ||||||||||||||||||||||||||||
callee_not_exists: IsZeroWordGadget<F, WordLoHiCell<F>>, | ||||||||||||||||||||||||||||
is_caller_callee_equal: Cell<F>, | ||||||||||||||||||||||||||||
|
@@ -183,12 +187,23 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> { | |||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
let caller_nonce_hash_bytes = cb.query_word32(); | ||||||||||||||||||||||||||||
let calldata_length = cb.query_cell(); | ||||||||||||||||||||||||||||
let calldata_length_is_zero = cb.is_zero(calldata_length.expr()); | ||||||||||||||||||||||||||||
let calldata_rlc = cb.query_cell_phase2(); | ||||||||||||||||||||||||||||
let create = ContractCreateGadget::construct(cb); | ||||||||||||||||||||||||||||
cb.require_equal_word( | ||||||||||||||||||||||||||||
"tx caller address equivalence", | ||||||||||||||||||||||||||||
tx.caller_address.to_word(), | ||||||||||||||||||||||||||||
create.caller_address(), | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
cb.require_equal( | ||||||||||||||||||||||||||||
"tx nonce equivalence", | ||||||||||||||||||||||||||||
tx.nonce.expr(), | ||||||||||||||||||||||||||||
create.caller_nonce(), | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// 1. Handle contract creation transaction. | ||||||||||||||||||||||||||||
cb.condition(tx.is_create.expr(), |cb| { | ||||||||||||||||||||||||||||
cb.require_equal_word( | ||||||||||||||||||||||||||||
"call callee address equivalence", | ||||||||||||||||||||||||||||
|
@@ -201,21 +216,45 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> { | |||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
.to_word(), | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
cb.require_equal( | ||||||||||||||||||||||||||||
"tx nonce equivalence", | ||||||||||||||||||||||||||||
tx.nonce.expr(), | ||||||||||||||||||||||||||||
create.caller_nonce(), | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// 1. Handle contract creation transaction. | ||||||||||||||||||||||||||||
cb.condition(tx.is_create.expr(), |cb| { | ||||||||||||||||||||||||||||
cb.keccak_table_lookup( | ||||||||||||||||||||||||||||
create.input_rlc(cb), | ||||||||||||||||||||||||||||
create.input_length(), | ||||||||||||||||||||||||||||
caller_nonce_hash_bytes.to_word(), | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
cb.tx_context_lookup( | ||||||||||||||||||||||||||||
tx_id.expr(), | ||||||||||||||||||||||||||||
TxContextFieldTag::CallDataLength, | ||||||||||||||||||||||||||||
None, | ||||||||||||||||||||||||||||
WordLoHi::from_lo_unchecked(calldata_length.expr()), | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
// If calldata_length > 0 we use the copy circuit to calculate the calldata_rlc for the | ||||||||||||||||||||||||||||
// keccack input. | ||||||||||||||||||||||||||||
cb.condition(not::expr(calldata_length_is_zero.expr()), |cb| { | ||||||||||||||||||||||||||||
cb.copy_table_lookup( | ||||||||||||||||||||||||||||
WordLoHi::from_lo_unchecked(tx_id.expr()), | ||||||||||||||||||||||||||||
CopyDataType::TxCalldata.expr(), | ||||||||||||||||||||||||||||
WordLoHi::zero(), | ||||||||||||||||||||||||||||
CopyDataType::RlcAcc.expr(), | ||||||||||||||||||||||||||||
0.expr(), | ||||||||||||||||||||||||||||
calldata_length.expr(), | ||||||||||||||||||||||||||||
0.expr(), | ||||||||||||||||||||||||||||
calldata_length.expr(), | ||||||||||||||||||||||||||||
calldata_rlc.expr(), | ||||||||||||||||||||||||||||
0.expr(), | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
// If calldata_length == 0, the copy circuit will not contain any entry, so we skip the | ||||||||||||||||||||||||||||
// lookup and instead force calldata_rlc to be 0 for the keccack input. | ||||||||||||||||||||||||||||
cb.condition(calldata_length_is_zero.expr(), |cb| { | ||||||||||||||||||||||||||||
cb.require_equal("calldata_rlc = 0", calldata_rlc.expr(), 0.expr()); | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
cb.keccak_table_lookup( | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this how we should add a constraint for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, I don't think this is correct.
The input should be the rlc of the TxCalldata, which is dynamic length. Take a look at the SHA3 opcode to see how it calculates the keccak of memory: zkevm-circuits/zkevm-circuits/src/evm_circuit/execution/sha3.rs Lines 61 to 72 in 3bbc757
An then CALLDATACOPY to see how it uses the copy circuit with TxCalldata as input:
And from understanding those you should make a copy event that has TxCalldata as input and RLC as ouput. Something like this: let copy_rwc_inc = cb.query_cell();
let rlc_acc = cb.query_cell_phase2();
cb.copy_table_lookup(
WordLoHi::from_lo_unchecked(tx_id.expr()),
CopyDataType::TxCalldata.expr(),
WordLoHi::zero(),
CopyDataType::RlcAcc.expr(),
zero,
length, // The TxCalldata length calculated previously
0.expr(), // dst_addr for CopyDataType::RlcAcc is 0.
length, // The TxCalldata length calculated previously
rlc_acc.expr(),
copy_rwc_inc.expr(),
); Now you can use |
||||||||||||||||||||||||||||
calldata_rlc.expr(), | ||||||||||||||||||||||||||||
calldata_length.expr(), | ||||||||||||||||||||||||||||
cb.curr.state.code_hash.to_word(), | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
cb.account_write( | ||||||||||||||||||||||||||||
call_callee_address.to_word(), | ||||||||||||||||||||||||||||
AccountFieldTag::Nonce, | ||||||||||||||||||||||||||||
|
@@ -434,6 +473,9 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> { | |||||||||||||||||||||||||||
code_hash, | ||||||||||||||||||||||||||||
is_empty_code_hash, | ||||||||||||||||||||||||||||
caller_nonce_hash_bytes, | ||||||||||||||||||||||||||||
calldata_length, | ||||||||||||||||||||||||||||
calldata_length_is_zero, | ||||||||||||||||||||||||||||
calldata_rlc, | ||||||||||||||||||||||||||||
create, | ||||||||||||||||||||||||||||
callee_not_exists, | ||||||||||||||||||||||||||||
is_caller_callee_equal, | ||||||||||||||||||||||||||||
|
@@ -522,6 +564,18 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> { | |||||||||||||||||||||||||||
offset, | ||||||||||||||||||||||||||||
U256::from_big_endian(&untrimmed_contract_addr), | ||||||||||||||||||||||||||||
)?; | ||||||||||||||||||||||||||||
self.calldata_length.assign( | ||||||||||||||||||||||||||||
region, | ||||||||||||||||||||||||||||
offset, | ||||||||||||||||||||||||||||
Value::known(F::from(tx.call_data.len() as u64)), | ||||||||||||||||||||||||||||
)?; | ||||||||||||||||||||||||||||
self.calldata_length_is_zero | ||||||||||||||||||||||||||||
.assign(region, offset, F::from(tx.call_data.len() as u64))?; | ||||||||||||||||||||||||||||
let calldata_rlc = region | ||||||||||||||||||||||||||||
.challenges() | ||||||||||||||||||||||||||||
.keccak_input() | ||||||||||||||||||||||||||||
.map(|randomness| rlc::value(tx.call_data.iter().rev(), randomness)); | ||||||||||||||||||||||||||||
self.calldata_rlc.assign(region, offset, calldata_rlc)?; | ||||||||||||||||||||||||||||
self.create.assign( | ||||||||||||||||||||||||||||
region, | ||||||||||||||||||||||||||||
offset, | ||||||||||||||||||||||||||||
|
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 removed this; before there were two separate
cb.condition
with the same condition (that tx.is_create). I think we can merge them.And actually in the #1475 it is stated that we need to check if callee address is equal to
keccak(rlp(sender, nonce))
- but we actually have this constraint already, right? -call callee address equivalence
constraint and keccak table lookup forcaller_nonce_hash_bytes
seems to be enough. Am I missing something?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 this is needed. Notice that here we're checking against
call_callee_address
:zkevm-circuits/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs
Lines 202 to 212 in c40d3da
And
caller_nonce_hash_bytes
is constrained via:zkevm-circuits/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs
Lines 213 to 217 in c40d3da
which calls:
zkevm-circuits/zkevm-circuits/src/evm_circuit/util/math_gadget/rlp.rs
Line 327 in 3bbc757
Now notice how
input_rlc
uses:self.caller_address
(viaself.caller_address_rlc()
)self.nonce
Which are unconstrained at the construction of the gadget:
zkevm-circuits/zkevm-circuits/src/evm_circuit/util/math_gadget/rlp.rs
Lines 219 to 220 in 3bbc757
So we must check that they are correct.
This checks the nonce (your deleted code):
And this checks the address:
zkevm-circuits/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs
Lines 188 to 192 in c40d3da
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.
Sorry I just saw that the removed nonce check was moved to
zkevm-circuits/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs
Lines 194 to 198 in c40d3da
So I'm a bit confused.
Are you talking about the removal of the nonce check? Or something else?
I think it would be cleaner if all the
create
fields equality checks are inside the condition:zkevm-circuits/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs
Lines 200 to 201 in c40d3da
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 were two
cb.condition(tx.is_create.expr())
:zkevm-circuits/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs
Line 193 in 3bbc757
and the other one:
zkevm-circuits/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs
Line 213 in 3bbc757
But the second thing I asked is do we still need the constraint from here: #1475 (comment)
I think we already have that, right?
cc. @ed255
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.
Ah, thanks for the clarification! On the constraint level it's the same, but I think in the code level it looks cleaner.
Ah yes, I see now that this point was already done. I think I just missed this
https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/3bbc757a20d9d9f7759db47c096d5395361bee74/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs#L193C1-L205C12
Due to it being apart from all the logic of the
is_create
case. So yeah, all is good regarding this point :)