Skip to content

Commit

Permalink
Merge upstream u64-overflow PR back to develop (scroll-tech#496)
Browse files Browse the repository at this point in the history 
* Fix to handle successful run with Uint64 overflow in multiple opcodes.

* Fix lint.

* Fix failed cases.

* Fix failed tests.

* Try to trigger CI for network failure.

* Revert "Try to trigger CI for network failure."

This reverts commit c17ba9d.

* Fix lint.

* Delete `debug_assert!(VALID_BYTES < 32)` in `assign` and `valid_value` function (only leave it in `construct`).

* Delete redundant constraint `JUMPI condition must be 0 if destination is Uint64 overflow` in `jumpi`.

* Rename `within_range` to `not_overflow`.

* Replace `checked_add` and `unwrap` with just adding.

* Update some comments to retry CI.

---------

Co-authored-by: Zhang Zhuo <[email protected]>
  • Loading branch information
@silathdiir @lispc
silathdiir and lispc authored May 2, 2023
1 parent 7e456d0 commit d0cc1e9
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 43 deletions.
22 changes: 10 additions & 12 deletions zkevm-circuits/src/evm_circuit/execution/calldataload.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ impl<F: Field> ExecutionGadget<F> for CallDataLoadGadget<F> {
cb.stack_pop(data_offset.original_word());

cb.condition(
and::expr([data_offset.within_range(), cb.curr.state.is_root.expr()]),
and::expr([data_offset.not_overflow(), cb.curr.state.is_root.expr()]),
|cb| {
cb.call_context_lookup(
false.expr(),
Expand All @@ -92,7 +92,7 @@ impl<F: Field> ExecutionGadget<F> for CallDataLoadGadget<F> {

cb.condition(
and::expr([
data_offset.within_range(),
data_offset.not_overflow(),
not::expr(cb.curr.state.is_root.expr()),
]),
|cb| {
Expand Down Expand Up @@ -134,7 +134,7 @@ impl<F: Field> ExecutionGadget<F> for CallDataLoadGadget<F> {
// For a root call, the call data comes from tx's data field.
cb.condition(
and::expr([
data_offset.within_range(),
data_offset.not_overflow(),
buffer_reader.read_flag(idx),
cb.curr.state.is_root.expr(),
]),
Expand All @@ -150,7 +150,7 @@ impl<F: Field> ExecutionGadget<F> for CallDataLoadGadget<F> {
// For an internal call, the call data comes from memory.
cb.condition(
and::expr([
data_offset.within_range(),
data_offset.not_overflow(),
buffer_reader.read_flag(idx),
not::expr(cb.curr.state.is_root.expr()),
]),
Expand Down Expand Up @@ -227,34 +227,32 @@ impl<F: Field> ExecutionGadget<F> for CallDataLoadGadget<F> {
.assign(region, offset, Value::known(F::from(call_data_offset)))?;

let data_offset = block.rws[step.rw_indices[0]].stack_value();
let offset_within_range =
let offset_not_overflow =
self.data_offset
.assign(region, offset, data_offset, F::from(call_data_length))?;

let data_offset = if offset_within_range {
let data_offset = if offset_not_overflow {
data_offset.as_u64()
} else {
call_data_length
};
let src_addr_end = call_data_offset.checked_add(call_data_length).unwrap();
let src_addr_end = call_data_offset + call_data_length;
let src_addr = call_data_offset
.checked_add(data_offset)
.unwrap_or(src_addr_end)
.min(src_addr_end);

let mut calldata_bytes = vec![0u8; N_BYTES_WORD];
if offset_within_range {
if offset_not_overflow {
for (i, byte) in calldata_bytes.iter_mut().enumerate() {
if call.is_root {
// Fetch from tx call data.
if src_addr.checked_add(i as u64).unwrap() < tx.call_data_length as u64 {
if src_addr + (i as u64) < tx.call_data_length as u64 {
*byte = tx.call_data[src_addr as usize + i];
}
} else {
// Fetch from memory.
if src_addr.checked_add(i as u64).unwrap()
< call.call_data_offset + call.call_data_length
{
if src_addr + (i as u64) < call.call_data_offset + call.call_data_length {
*byte =
block.rws[step.rw_indices[OFFSET_RW_MEMORY_INDICES + i]].memory_value();
}
Expand Down
16 changes: 8 additions & 8 deletions zkevm-circuits/src/evm_circuit/execution/jumpi.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use halo2_proofs::plonk::Error;
#[derive(Clone, Debug)]
pub(crate) struct JumpiGadget<F> {
same_context: SameContextGadget<F>,
dest_word: WordByteRangeGadget<F, N_BYTES_PROGRAM_COUNTER>,
dest: WordByteRangeGadget<F, N_BYTES_PROGRAM_COUNTER>,
phase2_condition: Cell<F>,
is_condition_zero: IsZeroGadget<F>,
}
Expand All @@ -33,11 +33,11 @@ impl<F: Field> ExecutionGadget<F> for JumpiGadget<F> {
const EXECUTION_STATE: ExecutionState = ExecutionState::JUMPI;

fn configure(cb: &mut EVMConstraintBuilder<F>) -> Self {
let dest_word = WordByteRangeGadget::construct(cb);
let dest = WordByteRangeGadget::construct(cb);
let phase2_condition = cb.query_cell_phase2();

// Pop the value from the stack
cb.stack_pop(dest_word.original_word());
cb.stack_pop(dest.original_word());
cb.stack_pop(phase2_condition.expr());

// Determine if the jump condition is met
Expand All @@ -48,18 +48,18 @@ impl<F: Field> ExecutionGadget<F> for JumpiGadget<F> {
cb.condition(should_jump.clone(), |cb| {
cb.require_equal(
"JUMPI destination must be within range if condition is non-zero",
dest_word.within_range(),
dest.not_overflow(),
1.expr(),
);

cb.opcode_lookup_at(dest_word.valid_value(), OpcodeId::JUMPDEST.expr(), 1.expr());
cb.opcode_lookup_at(dest.valid_value(), OpcodeId::JUMPDEST.expr(), 1.expr());
});

// Transit program_counter to destination when should_jump, otherwise by
// delta 1.
let next_program_counter = select::expr(
should_jump,
dest_word.valid_value(),
dest.valid_value(),
cb.curr.state.program_counter.expr() + 1.expr(),
);

Expand All @@ -76,7 +76,7 @@ impl<F: Field> ExecutionGadget<F> for JumpiGadget<F> {

Self {
same_context,
dest_word,
dest,
phase2_condition,
is_condition_zero,
}
Expand All @@ -97,7 +97,7 @@ impl<F: Field> ExecutionGadget<F> for JumpiGadget<F> {
[step.rw_indices[0], step.rw_indices[1]].map(|idx| block.rws[idx].stack_value());
let condition = region.word_rlc(condition);

self.dest_word.assign(region, offset, destination)?;
self.dest.assign(region, offset, destination)?;
self.phase2_condition.assign(region, offset, condition)?;
self.is_condition_zero
.assign_value(region, offset, condition)?;
Expand Down
40 changes: 18 additions & 22 deletions zkevm-circuits/src/evm_circuit/util/common_gadget.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1105,8 +1105,8 @@ impl<F: Field> CommonErrorGadget<F> {
}
}

/// Check if the passed in word is within the specified byte range and less than
/// a maximum cap.
/// Check if the passed in word is within the specified byte range
/// (not overflow) and less than a maximum cap.
#[derive(Clone, Debug)]
pub(crate) struct WordByteCapGadget<F, const VALID_BYTES: usize> {
word: WordByteRangeGadget<F, VALID_BYTES>,
Expand All @@ -1116,24 +1116,24 @@ pub(crate) struct WordByteCapGadget<F, const VALID_BYTES: usize> {
impl<F: Field, const VALID_BYTES: usize> WordByteCapGadget<F, VALID_BYTES> {
pub(crate) fn construct(cb: &mut EVMConstraintBuilder<F>, cap: Expression<F>) -> Self {
let word = WordByteRangeGadget::construct(cb);
let value = select::expr(word.within_range(), word.valid_value(), cap.expr());
let value = select::expr(word.overflow(), cap.expr(), word.valid_value());
let lt_cap = LtGadget::construct(cb, value, cap);

Self { word, lt_cap }
}

/// Return true if within the specified byte range, false if overflow. No
/// matter whether it is less than the cap.
/// Return true if within the specified byte range (not overflow), false if
/// overflow. No matter whether it is less than the cap.
pub(crate) fn assign(
&self,
region: &mut CachedRegion<'_, '_, F>,
offset: usize,
original: U256,
cap: F,
) -> Result<bool, Error> {
let within_range = self.word.assign(region, offset, original)?;
let not_overflow = self.word.assign(region, offset, original)?;

let value = if within_range {
let value = if not_overflow {
let mut bytes = [0; 32];
bytes[0..VALID_BYTES].copy_from_slice(&original.to_le_bytes()[0..VALID_BYTES]);
F::from_repr(bytes).unwrap()
Expand All @@ -1143,7 +1143,7 @@ impl<F: Field, const VALID_BYTES: usize> WordByteCapGadget<F, VALID_BYTES> {

self.lt_cap.assign(region, offset, value, cap)?;

Ok(within_range)
Ok(not_overflow)
}

pub(crate) fn lt_cap(&self) -> Expression<F> {
Expand All @@ -1162,28 +1162,28 @@ impl<F: Field, const VALID_BYTES: usize> WordByteCapGadget<F, VALID_BYTES> {
self.word.valid_value()
}

pub(crate) fn within_range(&self) -> Expression<F> {
self.word.within_range()
pub(crate) fn not_overflow(&self) -> Expression<F> {
self.word.not_overflow()
}
}

/// Check if the passed in word is within the specified byte range.
/// Check if the passed in word is within the specified byte range (not overflow).
#[derive(Clone, Debug)]
pub(crate) struct WordByteRangeGadget<F, const VALID_BYTES: usize> {
original: Word<F>,
within_range: IsZeroGadget<F>,
not_overflow: IsZeroGadget<F>,
}

impl<F: Field, const VALID_BYTES: usize> WordByteRangeGadget<F, VALID_BYTES> {
pub(crate) fn construct(cb: &mut EVMConstraintBuilder<F>) -> Self {
debug_assert!(VALID_BYTES < 32);

let original = cb.query_word_rlc();
let within_range = IsZeroGadget::construct(cb, sum::expr(&original.cells[VALID_BYTES..]));
let not_overflow = IsZeroGadget::construct(cb, sum::expr(&original.cells[VALID_BYTES..]));

Self {
original,
within_range,
not_overflow,
}
}

Expand All @@ -1194,15 +1194,13 @@ impl<F: Field, const VALID_BYTES: usize> WordByteRangeGadget<F, VALID_BYTES> {
offset: usize,
original: U256,
) -> Result<bool, Error> {
debug_assert!(VALID_BYTES < 32);

self.original
.assign(region, offset, Some(original.to_le_bytes()))?;

let overflow_hi = original.to_le_bytes()[VALID_BYTES..]
.iter()
.fold(0, |acc, val| acc + u64::from(*val));
self.within_range
self.not_overflow
.assign(region, offset, F::from(overflow_hi))?;

Ok(overflow_hi == 0)
Expand All @@ -1213,16 +1211,14 @@ impl<F: Field, const VALID_BYTES: usize> WordByteRangeGadget<F, VALID_BYTES> {
}

pub(crate) fn overflow(&self) -> Expression<F> {
not::expr(self.within_range())
not::expr(self.not_overflow())
}

pub(crate) fn valid_value(&self) -> Expression<F> {
debug_assert!(VALID_BYTES < 32);

from_bytes::expr(&self.original.cells[..VALID_BYTES])
}

pub(crate) fn within_range(&self) -> Expression<F> {
self.within_range.expr()
pub(crate) fn not_overflow(&self) -> Expression<F> {
self.not_overflow.expr()
}
}
2 changes: 1 addition & 1 deletion zkevm-circuits/src/witness/step.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,12 +204,12 @@ impl From<&circuit_input_builder::ExecStep> for ExecutionState {
OpcodeId::CODECOPY => ExecutionState::CODECOPY,
OpcodeId::CALLDATALOAD => ExecutionState::CALLDATALOAD,
OpcodeId::CODESIZE => ExecutionState::CODESIZE,
OpcodeId::EXTCODECOPY => ExecutionState::EXTCODECOPY,
OpcodeId::RETURN | OpcodeId::REVERT => ExecutionState::RETURN_REVERT,
OpcodeId::RETURNDATASIZE => ExecutionState::RETURNDATASIZE,
OpcodeId::RETURNDATACOPY => ExecutionState::RETURNDATACOPY,
OpcodeId::CREATE => ExecutionState::CREATE,
OpcodeId::CREATE2 => ExecutionState::CREATE2,
OpcodeId::EXTCODECOPY => ExecutionState::EXTCODECOPY,
// dummy ops
OpcodeId::SELFDESTRUCT => dummy!(ExecutionState::SELFDESTRUCT),
_ => unimplemented!("unimplemented opcode {:?}", op),
Expand Down

0 comments on commit d0cc1e9

Please sign in to comment.