Skip to content

Commit

Permalink
fix TOB-SCROLL-14 (scroll-tech#661)
Browse files Browse the repository at this point in the history 
* fix TOB-SCROLL-14

* constrain zero for overflow condition

* refactor CommonReturnDataCopyGadget

* some updates

* fix clippy

---------

Co-authored-by: naure <[email protected]>
  • Loading branch information
@DreamWuGit @naure
DreamWuGit and naure authored Aug 14, 2023
1 parent 9beba68 commit 48172be
Show file tree
Hide file tree
Showing 3 changed files with 157 additions and 124 deletions.
89 changes: 12 additions & 77 deletions zkevm-circuits/src/evm_circuit/execution/error_return_data_oo_bound.rs
View file
Original file line number Diff line number Diff line change
@@ -1,35 +1,27 @@
use crate::{
evm_circuit::{
execution::ExecutionGadget,
param::N_BYTES_U64,
step::ExecutionState,
util::{
common_gadget::CommonErrorGadget,
common_gadget::{CommonErrorGadget, CommonReturnDataCopyGadget},
constraint_builder::{ConstrainBuilderCommon, EVMConstraintBuilder},
from_bytes,
math_gadget::{AddWordsGadget, IsZeroGadget, LtGadget},
not, or, sum, CachedRegion, Cell,
CachedRegion, Cell,
},
witness::{Block, Call, ExecStep, Transaction},
},
table::CallContextFieldTag,
util::Expr,
};
use eth_types::{evm_types::OpcodeId, Field, ToLittleEndian, ToScalar};
use eth_types::{evm_types::OpcodeId, Field, ToScalar};
use halo2_proofs::{circuit::Value, plonk::Error};

#[derive(Clone, Debug)]
pub(crate) struct ErrorReturnDataOutOfBoundGadget<F> {
opcode: Cell<F>,
memory_offset: Cell<F>,
sum: AddWordsGadget<F, 2, false>,
// Hold the size of the last callee return data.
return_data_length: Cell<F>,
is_data_offset_within_u64: IsZeroGadget<F>,
// remainder_end = (data_offset + size) mod U256
is_remainder_end_within_u64: IsZeroGadget<F>,
// when remainder end is within Uint64, check if it exceeds return data size.
is_remainder_end_exceed_len: LtGadget<F, N_BYTES_U64>,
overflow_gadget: CommonReturnDataCopyGadget<F>,
common_error_gadget: CommonErrorGadget<F>,
}

Expand All @@ -41,9 +33,6 @@ impl<F: Field> ExecutionGadget<F> for ErrorReturnDataOutOfBoundGadget<F> {
fn configure(cb: &mut EVMConstraintBuilder<F>) -> Self {
let opcode = cb.query_cell();
let memory_offset = cb.query_cell();
let data_offset = cb.query_word_rlc();
let size = cb.query_word_rlc();
let remainder_end = cb.query_word_rlc();
let return_data_length = cb.query_cell();

cb.require_equal(
Expand All @@ -52,10 +41,13 @@ impl<F: Field> ExecutionGadget<F> for ErrorReturnDataOutOfBoundGadget<F> {
OpcodeId::RETURNDATACOPY.expr(),
);

let overflow_gadget: CommonReturnDataCopyGadget<F> =
CommonReturnDataCopyGadget::construct(cb, return_data_length.expr(), 1.expr());

// Pop memory_offset, offset, size from stack
cb.stack_pop(memory_offset.expr());
cb.stack_pop(data_offset.expr());
cb.stack_pop(size.expr());
cb.stack_pop(overflow_gadget.data_offset().expr());
cb.stack_pop(overflow_gadget.size().expr());

// Read last callee return data length
cb.call_context_lookup(
Expand All @@ -66,49 +58,15 @@ impl<F: Field> ExecutionGadget<F> for ErrorReturnDataOutOfBoundGadget<F> {
);

// Check if `data_offset` is Uint64 overflow.
let data_offset_larger_u64 = sum::expr(&data_offset.cells[N_BYTES_U64..]);
let is_data_offset_within_u64 = IsZeroGadget::construct(cb, "", data_offset_larger_u64);

// Check if `remainder_end` is Uint64 overflow.
let sum = AddWordsGadget::construct(cb, [data_offset, size], remainder_end.clone());
let is_end_u256_overflow = sum.carry().as_ref().unwrap();

let remainder_end_larger_u64 = sum::expr(&remainder_end.cells[N_BYTES_U64..]);
let is_remainder_end_within_u64 = IsZeroGadget::construct(cb, "", remainder_end_larger_u64);

// check if `remainder_end` exceeds return data length.
let is_remainder_end_exceed_len = LtGadget::construct(
cb,
return_data_length.expr(),
from_bytes::expr(&remainder_end.cells[..N_BYTES_U64]),
);

// Need to check if `data_offset + size` is U256 overflow via `AddWordsGadget` carry. If
// yes, it should be also an error of return data out of bound.
cb.require_equal(
"Any of [data_offset > u64::MAX, data_offset + size > U256::MAX, remainder_end > u64::MAX, remainder_end > return_data_length] occurs",
or::expr([
// data_offset > u64::MAX
not::expr(is_data_offset_within_u64.expr()),
// data_offset + size > U256::MAX
is_end_u256_overflow.expr(),
// remainder_end > u64::MAX
not::expr(is_remainder_end_within_u64.expr()),
// remainder_end > return_data_length
is_remainder_end_exceed_len.expr(),
]),
1.expr(),
);

let common_error_gadget = CommonErrorGadget::construct(cb, opcode.expr(), 6.expr());

Self {
opcode,
memory_offset,
is_data_offset_within_u64,
is_remainder_end_within_u64,
is_remainder_end_exceed_len,
sum,
overflow_gadget,
return_data_length,
common_error_gadget,
}
Expand All @@ -134,10 +92,6 @@ impl<F: Field> ExecutionGadget<F> for ErrorReturnDataOutOfBoundGadget<F> {
self.memory_offset
.assign(region, offset, Value::known(F::from(dest_offset.as_u64())))?;

let remainder_end = data_offset.overflowing_add(size).0;
self.sum
.assign(region, offset, [data_offset, size], remainder_end)?;

let return_data_length = block.rws[step.rw_indices[3]].call_context_value();
self.return_data_length.assign(
region,
Expand All @@ -149,27 +103,8 @@ impl<F: Field> ExecutionGadget<F> for ErrorReturnDataOutOfBoundGadget<F> {
),
)?;

let data_offset_overflow = data_offset.to_le_bytes()[N_BYTES_U64..]
.iter()
.fold(0, |acc, val| acc + u64::from(*val));
self.is_data_offset_within_u64
.assign(region, offset, F::from(data_offset_overflow))?;

let remainder_end_overflow = remainder_end.to_le_bytes()[N_BYTES_U64..]
.iter()
.fold(0, |acc, val| acc + u64::from(*val));
self.is_remainder_end_within_u64
.assign(region, offset, F::from(remainder_end_overflow))?;

// check if it exceeds last callee return data length
let remainder_end_u64 = remainder_end.low_u64();
let return_length = return_data_length.to_scalar().unwrap();
self.is_remainder_end_exceed_len.assign(
region,
offset,
return_length,
F::from(remainder_end_u64),
)?;
self.overflow_gadget
.assign(region, offset, data_offset, size, return_data_length)?;

self.common_error_gadget
.assign(region, offset, block, call, step, 6)?;
Expand Down
73 changes: 27 additions & 46 deletions zkevm-circuits/src/evm_circuit/execution/returndatacopy.rs
View file
Original file line number Diff line number Diff line change
@@ -1,29 +1,28 @@
use crate::{
evm_circuit::{
execution::ExecutionGadget,
param::{N_BYTES_MEMORY_ADDRESS, N_BYTES_MEMORY_WORD_SIZE},
param::{N_BYTES_MEMORY_ADDRESS, N_BYTES_MEMORY_WORD_SIZE, N_BYTES_U64},
step::ExecutionState,
util::{
common_gadget::SameContextGadget,
common_gadget::{CommonReturnDataCopyGadget, SameContextGadget},
constraint_builder::{
ConstrainBuilderCommon, EVMConstraintBuilder, StepStateTransition,
Transition::{Delta, To},
},
from_bytes,
math_gadget::RangeCheckGadget,
memory_gadget::{
CommonMemoryAddressGadget, MemoryAddressGadget, MemoryCopierGasGadget,
MemoryExpansionGadget,
},
CachedRegion, Cell, MemoryAddress,
CachedRegion, Cell, RandomLinearCombination,
},
witness::{Block, Call, ExecStep, Transaction},
},
table::CallContextFieldTag,
util::Expr,
};
use bus_mapping::{circuit_input_builder::CopyDataType, evm::OpcodeId};
use eth_types::{evm_types::GasCost, Field, ToLittleEndian, ToScalar};
use eth_types::{evm_types::GasCost, Field, ToScalar};
use gadgets::util::not;
use halo2_proofs::{circuit::Value, plonk::Error};

Expand All @@ -39,9 +38,8 @@ pub(crate) struct ReturnDataCopyGadget<F> {
/// The data is copied to memory. To verify this
/// copy operation we need the MemoryAddressGadget.
dst_memory_addr: MemoryAddressGadget<F>,
/// Holds the memory address for the offset in return data from where we
/// read.
data_offset: MemoryAddress<F>,
/// check if overflow
check_overflow_gadget: CommonReturnDataCopyGadget<F>,
/// Opcode RETURNDATACOPY has a dynamic gas cost:
/// gas_code = static_gas * minimum_word_size + memory_expansion_cost
memory_expansion: MemoryExpansionGadget<F, 1, N_BYTES_MEMORY_WORD_SIZE>,
Expand All @@ -51,8 +49,6 @@ pub(crate) struct ReturnDataCopyGadget<F> {
/// RW inverse counter from the copy table at the start of related copy
/// steps.
copy_rwc_inc: Cell<F>,
/// Out of bound check circuit.
in_bound_check: RangeCheckGadget<F, N_BYTES_MEMORY_WORD_SIZE>,
}

impl<F: Field> ExecutionGadget<F> for ReturnDataCopyGadget<F> {
Expand All @@ -64,18 +60,27 @@ impl<F: Field> ExecutionGadget<F> for ReturnDataCopyGadget<F> {
let opcode = cb.query_cell();

let dest_offset = cb.query_cell_phase2();
let data_offset = cb.query_word_rlc();
let size = cb.query_word_rlc();

let return_data_size: Cell<F> = cb.query_cell();

let size: RandomLinearCombination<F, N_BYTES_MEMORY_ADDRESS> = cb.query_word_rlc();
// enusre no other out of bound errors occur, otherwise go to `ErrorReturnDataOutOfBound`
// state
let check_overflow_gadget =
CommonReturnDataCopyGadget::construct(cb, return_data_size.expr(), false.expr());
// in normal case, size = CommonReturnDataCopyGadget::size
cb.require_equal(
"size = CommonReturnDataCopyGadget::size",
size.expr(),
check_overflow_gadget.size().expr(),
);
// 1. Pop dest_offset, offset, length from stack
cb.stack_pop(dest_offset.expr());
cb.stack_pop(data_offset.expr());
cb.stack_pop(check_overflow_gadget.data_offset().expr());
cb.stack_pop(size.expr());

// 2. Add lookup constraint in the call context for the returndatacopy field.
let last_callee_id = cb.query_cell();
let return_data_offset = cb.query_cell();
let return_data_size = cb.query_cell();
cb.call_context_lookup(
false.expr(),
None,
Expand All @@ -95,18 +100,9 @@ impl<F: Field> ExecutionGadget<F> for ReturnDataCopyGadget<F> {
return_data_size.expr(),
);

// 3. contraints for copy: copy overflow check
// i.e., offset + size <= return_data_size
let in_bound_check = RangeCheckGadget::construct(
cb,
return_data_size.expr()
- (from_bytes::expr(&data_offset.cells) + from_bytes::expr(&size.cells)),
);

// 4. memory copy
// Construct memory address in the destination (memory) to which we copy memory.
let dst_memory_addr = MemoryAddressGadget::construct(cb, dest_offset, size);

// Calculate the next memory size and the gas cost for this memory
// access. This also accounts for the dynamic gas required to copy bytes to
// memory.
Expand All @@ -124,7 +120,8 @@ impl<F: Field> ExecutionGadget<F> for ReturnDataCopyGadget<F> {
CopyDataType::Memory.expr(),
cb.curr.state.call_id.expr(),
CopyDataType::Memory.expr(),
return_data_offset.expr() + from_bytes::expr(&data_offset.cells),
return_data_offset.expr()
+ from_bytes::expr(&check_overflow_gadget.data_offset().cells[..N_BYTES_U64]),
return_data_offset.expr() + return_data_size.expr(),
dst_memory_addr.offset(),
dst_memory_addr.length(),
Expand Down Expand Up @@ -159,11 +156,10 @@ impl<F: Field> ExecutionGadget<F> for ReturnDataCopyGadget<F> {
return_data_offset,
return_data_size,
dst_memory_addr,
data_offset,
check_overflow_gadget,
memory_expansion,
memory_copier_gas,
copy_rwc_inc,
in_bound_check,
}
}

Expand All @@ -181,16 +177,6 @@ impl<F: Field> ExecutionGadget<F> for ReturnDataCopyGadget<F> {
let [dest_offset, data_offset, size] =
[0, 1, 2].map(|i| block.rws[step.rw_indices[i as usize]].stack_value());

self.data_offset.assign(
region,
offset,
Some(
data_offset.to_le_bytes()[..N_BYTES_MEMORY_ADDRESS]
.try_into()
.unwrap(),
),
)?;

let [last_callee_id, return_data_offset, return_data_size] = [
(3, CallContextFieldTag::LastCalleeId),
(4, CallContextFieldTag::LastCalleeReturnDataOffset),
Expand Down Expand Up @@ -254,13 +240,8 @@ impl<F: Field> ExecutionGadget<F> for ReturnDataCopyGadget<F> {
),
)?;

self.in_bound_check.assign(
region,
offset,
(return_data_size - (data_offset + size))
.to_scalar()
.expect("unexpected U256 -> Scalar conversion failure"),
)?;
self.check_overflow_gadget
.assign(region, offset, data_offset, size, return_data_size)?;

Ok(())
}
Expand Down Expand Up @@ -370,7 +351,7 @@ mod test {
#[test]
fn returndatacopy_gadget_overflow_offset_and_zero_length() {
test_ok_internal(0, 0x20, 0, 0x20, Word::MAX);
test_ok_internal(0, 0x10, 0x10, 0x10, 0x20.into());
test_ok_internal(0, 0x10, 0x10, 0, 0x2000000.into());
}
// test_ok_internal(0, 0x10, 0x10, 0x10, 0x20.into());
// test_ok_internal(0, 0x10, 0x10, 0, 0x2000000.into());
}
Loading

0 comments on commit 48172be

Please sign in to comment.