Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

[CREATE Part A] Modification to Copy Circuit #1356

Closed
wants to merge 3 commits into from
Closed

[CREATE Part A] Modification to Copy Circuit #1356

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f747962
feat: copy circuit maintains accumulator value and value RLC for memo…
roynalnaruto Apr 14, 2023
92bcef7
Merge branch 'main' into feat/create-part-a
roynalnaruto Apr 14, 2023
5e13f2b
Merge branch 'main' into feat/create-part-a
roynalnaruto Apr 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion mock/src/test_ctx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ pub use external_tracer::LoggerConfig;
/// // Now we can start generating the traces and items we need to inspect
/// // the behaviour of the generated env.
/// ```
#[derive(Debug)]
#[derive(Debug, Clone)]
pub struct TestContext<const NACC: usize, const NTX: usize> {
/// chain id
pub chain_id: Word,
Expand Down
123 changes: 77 additions & 46 deletions zkevm-circuits/src/copy_circuit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ pub struct CopyCircuitConfig<F> {
pub is_last: Column<Advice>,
/// The value copied in this copy step.
pub value: Column<Advice>,
/// Random linear combination accumulator value.
pub value_acc: Column<Advice>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename value_acc into value_keccak_acc for more self-explanation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to rename to value_acc_rlc. Most of our code and docs are using _rlc as suffix to represent rlc variables and it looks good to me without keyword, keccak.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 55aebf9

/// Whether the row is padding.
pub is_pad: Column<Advice>,
/// In case of a bytecode tag, this denotes whether or not the copied byte
Expand Down Expand Up @@ -105,6 +107,7 @@ impl<F: Field> SubCircuitConfig<F> for CopyCircuitConfig<F> {
let q_step = meta.complex_selector();
let is_last = meta.advice_column();
let value = meta.advice_column_in(SecondPhase);
let value_acc = meta.advice_column_in(SecondPhase);
let is_code = meta.advice_column();
let is_pad = meta.advice_column();
let is_first = copy_table.is_first;
Expand Down Expand Up @@ -222,23 +225,48 @@ impl<F: Field> SubCircuitConfig<F> for CopyCircuitConfig<F> {
rw_diff,
);
});
cb.condition(
and::expr([
meta.query_advice(is_last, Rotation::cur()),
tag.value_equals(CopyDataType::RlcAcc, Rotation::cur())(meta),
]),
|cb| {
cb.require_equal(
"value == rlc_acc at the last row for RlcAcc",
meta.query_advice(value, Rotation::cur()),
meta.query_advice(rlc_acc, Rotation::cur()),
);
},
);

cb.gate(meta.query_fixed(q_enable, Rotation::cur()))
});

meta.create_gate(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this gate and next gate can be combine into just 1 gate ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 55aebf9

"Last Step (check value accumulator) Memory => Bytecode",
|meta| {
let mut cb = BaseConstraintBuilder::default();

cb.require_equal(
"value_acc == rlc_acc on the last row",
meta.query_advice(value_acc, Rotation::next()),
meta.query_advice(rlc_acc, Rotation::next()),
);

cb.gate(and::expr([
meta.query_fixed(q_enable, Rotation::cur()),
meta.query_advice(is_last, Rotation::next()),
and::expr([
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here nested and::expr seems unnessarary, is it for readibility?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, I don't think we need it, but it has better readability for me. I prefer to keep the way it is. How do you think?

tag.value_equals(CopyDataType::Memory, Rotation::cur())(meta),
tag.value_equals(CopyDataType::Bytecode, Rotation::next())(meta),
]),
]))
},
);

meta.create_gate("Last Step (check value accumulator) RlcAcc", |meta| {
let mut cb = BaseConstraintBuilder::default();

cb.require_equal(
"value_acc == rlc_acc on the last row",
meta.query_advice(value_acc, Rotation::next()),
meta.query_advice(rlc_acc, Rotation::next()),
);

cb.gate(and::expr([
meta.query_fixed(q_enable, Rotation::cur()),
meta.query_advice(is_last, Rotation::next()),
tag.value_equals(CopyDataType::RlcAcc, Rotation::next())(meta),
]))
});

meta.create_gate("verify step (q_step == 1)", |meta| {
let mut cb = BaseConstraintBuilder::default();

Expand All @@ -262,25 +290,30 @@ impl<F: Field> SubCircuitConfig<F> for CopyCircuitConfig<F> {
);
},
);
cb.require_equal(
"write value == read value",
meta.query_advice(value, Rotation::cur()),
meta.query_advice(value, Rotation::next()),
);
cb.require_equal(
"value_acc is same for read-write rows",
meta.query_advice(value_acc, Rotation::cur()),
meta.query_advice(value_acc, Rotation::next()),
);
cb.condition(
not::expr(tag.value_equals(CopyDataType::RlcAcc, Rotation::next())(
meta,
)),
and::expr([
not::expr(meta.query_advice(is_last, Rotation::next())),
not::expr(meta.query_advice(is_pad, Rotation::cur())),
]),
|cb| {
cb.require_equal(
"write value == read value (if not rlc acc)",
meta.query_advice(value, Rotation::cur()),
meta.query_advice(value, Rotation::next()),
"value_acc(2) == value_acc(0) * r + value(2)",
meta.query_advice(value_acc, Rotation(2)),
meta.query_advice(value_acc, Rotation::cur()) * challenges.keccak_input()
+ meta.query_advice(value, Rotation(2)),
);
},
);
cb.condition(meta.query_advice(is_first, Rotation::cur()), |cb| {
cb.require_equal(
"write value == read value (is_first == 1)",
meta.query_advice(value, Rotation::cur()),
meta.query_advice(value, Rotation::next()),
);
});
cb.require_zero(
"value == 0 when is_pad == 1 for read",
and::expr([
Expand All @@ -298,25 +331,9 @@ impl<F: Field> SubCircuitConfig<F> for CopyCircuitConfig<F> {
meta.query_advice(is_pad, Rotation::next()),
);

cb.gate(meta.query_selector(q_step))
});

meta.create_gate("verify_step (q_step == 0)", |meta| {
let mut cb = BaseConstraintBuilder::default();

cb.require_equal(
"rows[2].value == rows[0].value * r + rows[1].value",
meta.query_advice(value, Rotation(2)),
meta.query_advice(value, Rotation::cur()) * challenges.keccak_input()
+ meta.query_advice(value, Rotation::next()),
);

cb.gate(and::expr([
meta.query_fixed(q_enable, Rotation::cur()),
not::expr(meta.query_selector(q_step)),
not::expr(meta.query_advice(is_last, Rotation::cur())),
tag.value_equals(CopyDataType::RlcAcc, Rotation::cur())(meta),
not::expr(meta.query_advice(is_pad, Rotation::cur())),
meta.query_selector(q_step),
]))
});

Expand Down Expand Up @@ -402,6 +419,7 @@ impl<F: Field> SubCircuitConfig<F> for CopyCircuitConfig<F> {
q_step,
is_last,
value,
value_acc,
is_pad,
is_code,
q_enable,
Expand Down Expand Up @@ -463,9 +481,15 @@ impl<F: Field> CopyCircuitConfig<F> {
)?;

// is_last, value, is_pad, is_code
for (column, &(value, label)) in [self.is_last, self.value, self.is_pad, self.is_code]
.iter()
.zip_eq(circuit_row)
for (column, &(value, label)) in [
self.is_last,
self.value,
self.value_acc,
self.is_pad,
self.is_code,
]
.iter()
.zip_eq(circuit_row)
{
region.assign_advice(
|| format!("{} at row: {}", label, *offset),
Expand Down Expand Up @@ -616,6 +640,13 @@ impl<F: Field> CopyCircuitConfig<F> {
*offset,
|| Value::known(F::zero()),
)?;
// value_acc
region.assign_advice(
|| format!("assign value_acc {}", *offset),
self.value_acc,
*offset,
|| Value::known(F::zero()),
)?;
// rlc_acc
region.assign_advice(
|| format!("assign rlc_acc {}", *offset),
Expand Down
23 changes: 14 additions & 9 deletions zkevm-circuits/src/evm_circuit/execution/return_revert.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ pub(crate) struct ReturnRevertGadget<F> {
opcode: Cell<F>,

range: MemoryAddressGadget<F>,
init_code_rlc: Cell<F>,

is_success: Cell<F>,
restore_context: RestoreContextGadget<F>,
Expand Down Expand Up @@ -94,11 +95,12 @@ impl<F: Field> ExecutionGadget<F> for ReturnRevertGadget<F> {

let is_contract_deployment =
is_create.clone() * is_success.expr() * not::expr(copy_rw_increase_is_zero.expr());
let (caller_id, address, reversion_info, code_hash) =
let (caller_id, address, reversion_info, code_hash, init_code_rlc) =
han0110 marked this conversation as resolved.
Show resolved Hide resolved
cb.condition(is_contract_deployment.clone(), |cb| {
// We don't need to place any additional constraints on code_hash because the
// copy circuit enforces that it is the hash of the bytes in the copy lookup.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since intruduce init_code_rlc i think the comment also need to adjust a bit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what comment I should add. Could you provide an example?

let code_hash = cb.query_cell_phase2();
let init_code_rlc = cb.query_cell_phase2();
cb.copy_table_lookup(
cb.curr.state.call_id.expr(),
CopyDataType::Memory.expr(),
Expand All @@ -108,7 +110,7 @@ impl<F: Field> ExecutionGadget<F> for ReturnRevertGadget<F> {
range.address(),
0.expr(),
range.length(),
0.expr(),
init_code_rlc.expr(),
copy_rw_increase.expr(),
);

Expand All @@ -127,7 +129,7 @@ impl<F: Field> ExecutionGadget<F> for ReturnRevertGadget<F> {
Some(&mut reversion_info),
);

(caller_id, address, reversion_info, code_hash)
(caller_id, address, reversion_info, code_hash, init_code_rlc)
});

// Case B in the specs.
Expand Down Expand Up @@ -218,6 +220,7 @@ impl<F: Field> ExecutionGadget<F> for ReturnRevertGadget<F> {
Self {
opcode,
range,
init_code_rlc,
is_success,
copy_length,
copy_rw_increase,
Expand Down Expand Up @@ -276,6 +279,11 @@ impl<F: Field> ExecutionGadget<F> for ReturnRevertGadget<F> {
let values: Vec<_> = (3..3 + length.as_usize())
.map(|i| block.rws[step.rw_indices[i]].memory_value())
.collect();
self.init_code_rlc.assign(
region,
offset,
region.keccak_rlc(&values.iter().rev().cloned().collect::<Vec<u8>>()),
)?;
let mut code_hash = CodeDB::hash(&values).to_fixed_bytes();
code_hash.reverse();
self.code_hash.assign(
Expand Down Expand Up @@ -617,10 +625,8 @@ mod test {
PUSH1(0) // dest offset
RETURNDATACOPY
});

let block: GethData = TestContext::<2, 1>::simple_ctx_with_bytecode(bytecode.clone())
.unwrap()
.into();
let test_ctx = TestContext::<2, 1>::simple_ctx_with_bytecode(bytecode.clone()).unwrap();
let block: GethData = test_ctx.clone().into();

// collect return opcode, retrieve next step, assure both contract create
// successfully
Expand Down Expand Up @@ -651,7 +657,6 @@ mod test {
.iter()
.for_each(|size| assert_eq!(size, &Word::zero()));

let text_ctx = TestContext::<2, 1>::simple_ctx_with_bytecode(bytecode).unwrap();
CircuitTestBuilder::new_from_test_ctx(text_ctx).run();
CircuitTestBuilder::new_from_test_ctx(test_ctx).run();
}
}
13 changes: 13 additions & 0 deletions zkevm-circuits/src/evm_circuit/util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,10 +195,23 @@ impl<'r, 'b, F: FieldExt> CachedRegion<'r, 'b, F> {
.evm_word()
.map(|r| rlc::value(&n.to_le_bytes(), r))
}

pub fn keccak_rlc(&self, le_bytes: &[u8]) -> Value<F> {
self.challenges
.keccak_input()
.map(|r| rlc::value(le_bytes, r))
}

pub fn empty_code_hash_rlc(&self) -> Value<F> {
self.word_rlc(CodeDB::empty_code_hash().to_word())
}

pub fn code_hash(&self, n: U256) -> Value<F> {
self.challenges
.evm_word()
.map(|r| rlc::value(&n.to_le_bytes(), r))
}

/// Constrains a cell to have a constant value.
///
/// Returns an error if the cell is in a column where equality has not been
Expand Down
32 changes: 16 additions & 16 deletions zkevm-circuits/src/table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1044,7 +1044,7 @@ pub struct CopyTable {
}

type CopyTableRow<F> = [(Value<F>, &'static str); 8];
type CopyCircuitRow<F> = [(Value<F>, &'static str); 4];
type CopyCircuitRow<F> = [(Value<F>, &'static str); 5];

impl CopyTable {
/// Construct a new CopyTable
Expand All @@ -1069,7 +1069,7 @@ impl CopyTable {
) -> Vec<(CopyDataType, CopyTableRow<F>, CopyCircuitRow<F>)> {
let mut assignments = Vec::new();
// rlc_acc
let rlc_acc = if copy_event.dst_type == CopyDataType::RlcAcc {
let rlc_acc = {
let values = copy_event
.bytes
.iter()
Expand All @@ -1078,8 +1078,6 @@ impl CopyTable {
challenges
.keccak_input()
.map(|keccak_input| rlc::value(values.iter().rev(), keccak_input))
} else {
Value::known(F::zero())
};
let mut value_acc = Value::known(F::zero());
for (step_idx, (is_read_step, copy_step)) in copy_event
Expand Down Expand Up @@ -1154,17 +1152,11 @@ impl CopyTable {
// bytes_left
let bytes_left = u64::try_from(copy_event.bytes.len() * 2 - step_idx).unwrap() / 2;
// value
let value = if copy_event.dst_type == CopyDataType::RlcAcc {
if is_read_step {
Value::known(F::from(copy_step.value as u64))
} else {
value_acc = value_acc * challenges.keccak_input()
+ Value::known(F::from(copy_step.value as u64));
value_acc
}
} else {
Value::known(F::from(copy_step.value as u64))
};
let value = Value::known(F::from(copy_step.value as u64));
// value_acc
if is_read_step {
value_acc = value_acc * challenges.keccak_input() + value;
}
// is_pad
let is_pad = Value::known(F::from(
is_read_step && copy_step_addr >= copy_event.src_addr_end,
Expand All @@ -1184,7 +1176,14 @@ impl CopyTable {
"src_addr_end",
),
(Value::known(F::from(bytes_left)), "bytes_left"),
(rlc_acc, "rlc_acc"),
(
match (copy_event.src_type, copy_event.dst_type) {
(CopyDataType::Memory, CopyDataType::Bytecode) => rlc_acc,
(_, CopyDataType::RlcAcc) => rlc_acc,
_ => Value::known(F::zero()),
},
"rlc_acc",
),
(
Value::known(F::from(copy_event.rw_counter(step_idx))),
"rw_counter",
Expand All @@ -1197,6 +1196,7 @@ impl CopyTable {
[
(is_last, "is_last"),
(value, "value"),
(value_acc, "value_acc"),
(is_pad, "is_pad"),
(is_code, "is_code"),
],
Expand Down