Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add memory checks for prover_input, as well as range_checks for prover_input, syscalls/exceptions #1168

Merged
merged 11 commits into from
Nov 7, 2023
2 changes: 2 additions & 0 deletions evm/src/all_stark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ fn ctl_arithmetic<F: Field>() -> CrossTableLookup<F> {
vec![
cpu_stark::ctl_arithmetic_base_rows(),
cpu_stark::ctl_arithmetic_shift_rows(),
cpu_stark::ctl_sys_exc_check_rows(),
cpu_stark::ctl_prover_input_check_rows(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Each of these cross-table lookups adds an extra column to the CPU table, so ideally we'd like to have as few of them as possible. Luckily, we can just use the first CTL (cpu_stark::ctl_arithmetic_base_rows()) for range checks.

],
arithmetic_stark::ctl_arithmetic_rows(),
)
Expand Down
5 changes: 3 additions & 2 deletions evm/src/arithmetic/arithmetic_stark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ fn cpu_arith_data_link<F: Field>(ops: &[usize], regs: &[Range<usize>]) -> Vec<Co
}

pub fn ctl_arithmetic_rows<F: Field>() -> TableWithColumns<F> {
const ARITH_OPS: [usize; 14] = [
const ARITH_OPS: [usize; 15] = [
columns::IS_ADD,
columns::IS_SUB,
columns::IS_MUL,
Expand All @@ -64,6 +64,7 @@ pub fn ctl_arithmetic_rows<F: Field>() -> TableWithColumns<F> {
columns::IS_DIV,
columns::IS_MOD,
columns::IS_BYTE,
columns::IS_RANGE_CHECK,
];

const REGISTER_MAP: [Range<usize>; 4] = [
Expand All @@ -90,7 +91,7 @@ pub struct ArithmeticStark<F, const D: usize> {
pub f: PhantomData<F>,
}

const RANGE_MAX: usize = 1usize << 16; // Range check strict upper bound
pub(crate) const RANGE_MAX: usize = 1usize << 16; // Range check strict upper bound

impl<F: RichField, const D: usize> ArithmeticStark<F, D> {
/// Expects input in *column*-major layout
Expand Down
4 changes: 3 additions & 1 deletion evm/src/arithmetic/columns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@ pub(crate) const IS_SUBMOD: usize = IS_SUBFP254 + 1;
pub(crate) const IS_LT: usize = IS_SUBMOD + 1;
pub(crate) const IS_GT: usize = IS_LT + 1;
pub(crate) const IS_BYTE: usize = IS_GT + 1;
pub(crate) const IS_RANGE_CHECK: usize = IS_BYTE + 1;

pub(crate) const START_SHARED_COLS: usize = IS_BYTE + 1;
pub(crate) const START_SHARED_COLS: usize = IS_RANGE_CHECK + 1;

/// Within the Arithmetic Unit, there are shared columns which can be
/// used by any arithmetic circuit, depending on which one is active
Expand Down Expand Up @@ -110,5 +111,6 @@ pub(crate) const MODULAR_DIV_DENOM_IS_ZERO: usize = AUX_REGISTER_2.end;
pub(crate) const NUM_RANGE_CHECK_COLS: usize = 1 + 2 * NUM_SHARED_COLS;
pub(crate) const RANGE_COUNTER: usize = START_SHARED_COLS + NUM_SHARED_COLS;
pub(crate) const RC_COLS: Range<usize> = RANGE_COUNTER + 1..RANGE_COUNTER + 1 + 2 * NUM_SHARED_COLS;
pub(crate) const NUM_CPU_LIMBS: usize = 8;

pub const NUM_ARITH_COLUMNS: usize = START_SHARED_COLS + NUM_SHARED_COLS + NUM_RANGE_CHECK_COLS;
27 changes: 27 additions & 0 deletions evm/src/arithmetic/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
use ethereum_types::U256;
use plonky2::field::types::PrimeField64;

use self::columns::NUM_CPU_LIMBS;
use crate::arithmetic::arithmetic_stark::RANGE_MAX;
use crate::arithmetic::columns::{IS_RANGE_CHECK, NUM_SHARED_COLS, START_SHARED_COLS};
use crate::extension_tower::BN_BASE;
use crate::util::{addmod, mulmod, submod};

Expand Down Expand Up @@ -107,6 +110,7 @@ impl TernaryOperator {
}
}

#[allow(clippy::enum_variant_names)]
#[derive(Debug)]
pub(crate) enum Operation {
BinaryOperation {
Expand All @@ -122,6 +126,9 @@ pub(crate) enum Operation {
input2: U256,
result: U256,
},
RangeCheckOperation {
values: [u32; NUM_CPU_LIMBS],
},
LindaGuiga marked this conversation as resolved.
Show resolved Hide resolved
}

impl Operation {
Expand Down Expand Up @@ -151,10 +158,15 @@ impl Operation {
}
}

pub(crate) fn range_check(values: [u32; NUM_CPU_LIMBS]) -> Self {
Self::RangeCheckOperation { values }
}

pub(crate) fn result(&self) -> U256 {
match self {
Operation::BinaryOperation { result, .. } => *result,
Operation::TernaryOperation { result, .. } => *result,
_ => panic!("This function should not be called for range checks."),
}
}

Expand All @@ -179,6 +191,7 @@ impl Operation {
input2,
result,
} => ternary_op_to_rows(operator.row_filter(), input0, input1, input2, result),
Operation::RangeCheckOperation { values } => range_check_to_rows(&values),
}
}
}
Expand Down Expand Up @@ -232,3 +245,17 @@ fn binary_op_to_rows<F: PrimeField64>(
}
}
}

fn range_check_to_rows<F: PrimeField64>(values: &[u32; NUM_CPU_LIMBS]) -> (Vec<F>, Option<Vec<F>>) {
// Each value is 32 bits long, so we split them into two 16-bit limbs.
assert!(2 * values.len() <= NUM_SHARED_COLS);
let mut row = vec![F::ZERO; columns::NUM_ARITH_COLUMNS];
row[IS_RANGE_CHECK] = F::ONE;
for i in 0..values.len() {
let low_limb = values[i] % RANGE_MAX as u32;
let high_limb = values[i] / RANGE_MAX as u32;
row[START_SHARED_COLS + 2 * i] = F::from_canonical_u32(low_limb);
row[START_SHARED_COLS + 2 * i + 1] = F::from_canonical_u32(high_limb);
}
(row, None)
}
98 changes: 95 additions & 3 deletions evm/src/cpu/cpu_stark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ use crate::memory::{NUM_CHANNELS, VALUE_LIMBS};
use crate::stark::Stark;
use crate::vars::{StarkEvaluationTargets, StarkEvaluationVars};

const EXTRA_OPS: [usize; 3] = [
COL_MAP.op.syscall,
COL_MAP.op.exception,
Nashtare marked this conversation as resolved.
Show resolved Hide resolved
COL_MAP.op.prover_input,
];

pub fn ctl_data_keccak_sponge<F: Field>() -> Vec<Column<F>> {
// When executing KECCAK_GENERAL, the GP memory channels are used as follows:
// GP channel 0: stack[-1] = context
Expand Down Expand Up @@ -70,9 +76,14 @@ fn ctl_data_binops<F: Field>(ops: &[usize]) -> Vec<Column<F>> {
/// case of shift operations, which will skip the first memory channel and use the
/// next three as ternary inputs. Because both `MUL` and `DIV` are binary operations,
/// the last memory channel used for the inputs will be safely ignored.
fn ctl_data_ternops<F: Field>(ops: &[usize], is_shift: bool) -> Vec<Column<F>> {
fn ctl_data_ternops<F: Field>(
ops: &[usize],
extra_ops: &[usize],
is_shift: bool,
) -> Vec<Column<F>> {
let offset = is_shift as usize;
let mut res = Column::singles(ops).collect_vec();
res.push(Column::sum(extra_ops));
res.extend(Column::singles(COL_MAP.mem_channels[offset].value));
res.extend(Column::singles(COL_MAP.mem_channels[offset + 1].value));
res.extend(Column::singles(COL_MAP.mem_channels[offset + 2].value));
Expand All @@ -82,6 +93,33 @@ fn ctl_data_ternops<F: Field>(ops: &[usize], is_shift: bool) -> Vec<Column<F>> {
res
}

fn ctl_data_sys_exc_range_check<F: Field>(ops: &[usize], extra_ops: &[usize]) -> Vec<Column<F>> {
// All arithmetic flags are 0 when the operation is either a syscall or an exception
let mut res = Column::singles(ops).collect_vec();
res.push(Column::sum(extra_ops));
res.push(Column::single(
COL_MAP.mem_channels[NUM_GP_CHANNELS - 1].value[6],
));
// Since we only need to check one limb, the rest of the columns are 0.
res.extend(vec![Column::zero(); VALUE_LIMBS * 4 - 1]);
res
}

fn ctl_data_prover_input_range_check<F: Field>(
ops: &[usize],
extra_ops: &[usize],
) -> Vec<Column<F>> {
// All arithmetic flags are 0 when the operation is prover_input
let mut res = Column::singles(ops).collect_vec();
res.push(Column::sum(extra_ops));
res.extend(Column::singles(
COL_MAP.mem_channels[NUM_GP_CHANNELS - 1].value,
));
// Since we only need to check one `U256`'s limbs, the rest of the columns are set to 0.
res.extend(vec![Column::zero(); VALUE_LIMBS * 3]);
res
}

pub fn ctl_data_logic<F: Field>() -> Vec<Column<F>> {
ctl_data_binops(&[COL_MAP.op.and, COL_MAP.op.or, COL_MAP.op.xor])
}
Expand All @@ -90,6 +128,60 @@ pub fn ctl_filter_logic<F: Field>() -> Column<F> {
Column::sum([COL_MAP.op.and, COL_MAP.op.or, COL_MAP.op.xor])
}

pub fn ctl_sys_exc_check_rows<F: Field>() -> TableWithColumns<F> {
const OPS: [usize; 14] = [
COL_MAP.op.add,
COL_MAP.op.sub,
COL_MAP.op.mul,
COL_MAP.op.lt,
COL_MAP.op.gt,
COL_MAP.op.addfp254,
COL_MAP.op.mulfp254,
COL_MAP.op.subfp254,
COL_MAP.op.addmod,
COL_MAP.op.mulmod,
COL_MAP.op.submod,
COL_MAP.op.div,
COL_MAP.op.mod_,
COL_MAP.op.byte,
];
// Create the CPU Table whose columns are the gas value and the rest of the columns are 0.
let filter = Some(Column::sum([COL_MAP.op.syscall, COL_MAP.op.exception]));

TableWithColumns::new(
Table::Cpu,
ctl_data_sys_exc_range_check(&OPS, &EXTRA_OPS),
filter,
)
}

pub fn ctl_prover_input_check_rows<F: Field>() -> TableWithColumns<F> {
const OPS: [usize; 14] = [
COL_MAP.op.add,
COL_MAP.op.sub,
COL_MAP.op.mul,
COL_MAP.op.lt,
COL_MAP.op.gt,
COL_MAP.op.addfp254,
COL_MAP.op.mulfp254,
COL_MAP.op.subfp254,
COL_MAP.op.addmod,
COL_MAP.op.mulmod,
COL_MAP.op.submod,
COL_MAP.op.div,
COL_MAP.op.mod_,
COL_MAP.op.byte,
];
// Create the CPU Table whose columns are the gas value and the rest of the columns are 0.
let filter = Some(Column::single(COL_MAP.op.prover_input));

TableWithColumns::new(
Table::Cpu,
ctl_data_prover_input_range_check(&OPS, &EXTRA_OPS),
filter,
)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

These can be deleted: we can reuse the base arithmetic CTL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nbgl The reason why I didn't reuse the base arithmetic CTL here was because I only selected the useful columns in the CPU for prover_input and syscalls/exceptions. For prover_input, the logic would still work with the base arithmetic CTL, but since we only wanted to range check output[6] for syscalls and exceptions, I thought we had to create an additional ctl. Am I missing something?
What I also thought we could do to avoid adding a CPU column is to range check handler_addr0-2 and syscall_info/exc_info, since they're already in the columns selected by the base CTL, instead of only range checking the gas. What do you think?

pub fn ctl_arithmetic_base_rows<F: Field>() -> TableWithColumns<F> {
const OPS: [usize; 14] = [
COL_MAP.op.add,
Expand All @@ -114,7 +206,7 @@ pub fn ctl_arithmetic_base_rows<F: Field>() -> TableWithColumns<F> {
// the third input.
TableWithColumns::new(
Table::Cpu,
ctl_data_ternops(&OPS, false),
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to add COL_MAP.op.syscall + COL_MAP.op.exception + COL_MAP.op.prover_input to the OPS list. This can be done with the Column::sum API, but will require a reworking of ctl_data_ternops.

This way we don't have to have a separate cross-table lookup just for range checks.

ctl_data_ternops(&OPS, &EXTRA_OPS, false),
Some(Column::sum(OPS)),
)
}
Expand Down Expand Up @@ -145,7 +237,7 @@ pub fn ctl_arithmetic_shift_rows<F: Field>() -> TableWithColumns<F> {
// the third input.
TableWithColumns::new(
Table::Cpu,
ctl_data_ternops(&OPS, true),
ctl_data_ternops(&OPS, &EXTRA_OPS, true),
Some(Column::sum([COL_MAP.op.shl, COL_MAP.op.shr])),
)
}
Expand Down
6 changes: 5 additions & 1 deletion evm/src/cpu/stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,11 @@ const STACK_BEHAVIORS: OpsColumnsView<Option<StackBehavior>> = OpsColumnsView {
pushes: true,
disable_other_channels: true,
}),
prover_input: None, // TODO
prover_input: Some(StackBehavior {
num_pops: 0,
pushes: true,
disable_other_channels: true,
}),
pop: Some(StackBehavior {
num_pops: 1,
pushes: false,
Expand Down
22 changes: 21 additions & 1 deletion evm/src/witness/operation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,14 @@ pub(crate) fn generate_prover_input<F: Field>(
let input = state.prover_input(input_fn);
let write = stack_push_log_and_fill(state, &mut row, input)?;

let input_limbs: [u64; 4] = input.0;
let mut values = [0_u32; 8];
for (i, limb) in input_limbs.into_iter().enumerate() {
values[2 * i] = limb as u32;
values[2 * i + 1] = (limb >> 32) as u32;
}
let range_check_op = arithmetic::Operation::range_check(values);
state.traces.push_arithmetic(range_check_op);
LindaGuiga marked this conversation as resolved.
Show resolved Hide resolved
state.traces.push_memory(write);
state.traces.push_cpu(row);
Ok(())
Expand Down Expand Up @@ -593,6 +601,12 @@ pub(crate) fn generate_syscall<F: Field>(
+ (U256::from(u64::from(state.registers.is_kernel)) << 32)
+ (U256::from(state.registers.gas_used) << 192);

let mut values = [0_u32; 8];
let gas = state.registers.gas_used;
values[0] = gas as u32;
values[1] = (gas >> 32) as u32;

let range_check_op = arithmetic::Operation::range_check(values);
// Set registers before pushing to the stack; in particular, we need to set kernel mode so we
// can't incorrectly trigger a stack overflow. However, note that we have to do it _after_ we
// make `syscall_info`, which should contain the old values.
Expand All @@ -604,6 +618,7 @@ pub(crate) fn generate_syscall<F: Field>(

log::debug!("Syscall to {}", KERNEL.offset_name(new_program_counter));

state.traces.push_arithmetic(range_check_op);
state.traces.push_memory(log_in0);
state.traces.push_memory(log_in1);
state.traces.push_memory(log_in2);
Expand Down Expand Up @@ -770,6 +785,11 @@ pub(crate) fn generate_exception<F: Field>(
let exc_info =
U256::from(state.registers.program_counter) + (U256::from(state.registers.gas_used) << 192);

let mut values = [0_u32; 8];
let gas = state.registers.gas_used;
values[0] = gas as u32;
values[1] = (gas >> 32) as u32;
let range_check_op = arithmetic::Operation::range_check(values);
// Set registers before pushing to the stack; in particular, we need to set kernel mode so we
// can't incorrectly trigger a stack overflow. However, note that we have to do it _after_ we
// make `exc_info`, which should contain the old values.
Expand All @@ -780,7 +800,7 @@ pub(crate) fn generate_exception<F: Field>(
let log_out = stack_push_log_and_fill(state, &mut row, exc_info)?;

log::debug!("Exception to {}", KERNEL.offset_name(new_program_counter));

state.traces.push_arithmetic(range_check_op);
state.traces.push_memory(log_in0);
state.traces.push_memory(log_in1);
state.traces.push_memory(log_in2);
Expand Down