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

Combine mload_32bytes and mstore_32bytes flags #1252

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
16 changes: 4 additions & 12 deletions evm/src/all_stark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,25 +125,17 @@ fn ctl_arithmetic<F: Field>() -> CrossTableLookup<F> {
}

fn ctl_byte_packing<F: Field>() -> CrossTableLookup<F> {
let cpu_packing_looking = TableWithColumns::new(
let cpu_byte32_looking = TableWithColumns::new(
Table::Cpu,
cpu_stark::ctl_data_byte_packing(),
Some(cpu_stark::ctl_filter_byte_packing()),
);
let cpu_unpacking_looking = TableWithColumns::new(
Table::Cpu,
cpu_stark::ctl_data_byte_unpacking(),
Some(cpu_stark::ctl_filter_byte_unpacking()),
cpu_stark::ctl_data_byte32(),
Some(cpu_stark::ctl_filter_byte32()),
);
let byte_packing_looked = TableWithColumns::new(
Table::BytePacking,
byte_packing_stark::ctl_looked_data(),
Some(byte_packing_stark::ctl_looked_filter()),
);
CrossTableLookup::new(
vec![cpu_packing_looking, cpu_unpacking_looking],
byte_packing_looked,
)
CrossTableLookup::new(vec![cpu_byte32_looking], byte_packing_looked)
}

fn ctl_keccak<F: Field>() -> CrossTableLookup<F> {
Expand Down
3 changes: 1 addition & 2 deletions evm/src/cpu/columns/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ pub struct OpsColumnsView<T: Copy> {
pub dup: T,
pub swap: T,
pub context_op: T,
pub mstore_32bytes: T,
pub mload_32bytes: T,
pub memop_32bytes: T,
pub exit_kernel: T,
pub m_op_general: T,

Expand Down
32 changes: 3 additions & 29 deletions evm/src/cpu/cpu_stark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,38 +125,12 @@ pub fn ctl_arithmetic_shift_rows<F: Field>() -> TableWithColumns<F> {
TableWithColumns::new(Table::Cpu, columns, Some(Column::single(COL_MAP.op.shift)))
}

pub fn ctl_data_byte_packing<F: Field>() -> Vec<Column<F>> {
pub fn ctl_data_byte32<F: Field>() -> Vec<Column<F>> {
ctl_data_keccak_sponge()
}
Comment on lines +132 to 134
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: as we're re-using ctl_data_keccak_sponge both for MLOAD_32BYTES and MSTORE_32BYTES, perhaps a tiny comment explaining the distinction may be useful? (as the doc there is valid for MLOAD but the last memory channel is being read in the case of MSTORE, not pushed to).


pub fn ctl_filter_byte_packing<F: Field>() -> Column<F> {
Column::single(COL_MAP.op.mload_32bytes)
}

pub fn ctl_data_byte_unpacking<F: Field>() -> Vec<Column<F>> {
// When executing MSTORE_32BYTES, the GP memory channels are used as follows:
// GP channel 0: stack[-1] = context
// GP channel 1: stack[-2] = segment
// GP channel 2: stack[-3] = virt
// GP channel 3: stack[-4] = val
// GP channel 4: stack[-5] = len
let context = Column::single(COL_MAP.mem_channels[0].value[0]);
let segment = Column::single(COL_MAP.mem_channels[1].value[0]);
let virt = Column::single(COL_MAP.mem_channels[2].value[0]);
let val = Column::singles(COL_MAP.mem_channels[3].value);
let len = Column::single(COL_MAP.mem_channels[4].value[0]);

let num_channels = F::from_canonical_usize(NUM_CHANNELS);
let timestamp = Column::linear_combination([(COL_MAP.clock, num_channels)]);

let mut res = vec![context, segment, virt, len, timestamp];
res.extend(val);

res
}

pub fn ctl_filter_byte_unpacking<F: Field>() -> Column<F> {
Column::single(COL_MAP.op.mstore_32bytes)
pub fn ctl_filter_byte32<F: Field>() -> Column<F> {
Column::single(COL_MAP.op.memop_32bytes)
}

pub const MEM_CODE_CHANNEL_IDX: usize = 0;
Expand Down
42 changes: 35 additions & 7 deletions evm/src/cpu/decode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use crate::cpu::columns::{CpuColumnsView, COL_MAP};
/// behavior.
/// Note: invalid opcodes are not represented here. _Any_ opcode is permitted to decode to
/// `is_invalid`. The kernel then verifies that the opcode was _actually_ invalid.
const OPCODES: [(u8, usize, bool, usize); 16] = [
const OPCODES: [(u8, usize, bool, usize); 14] = [
// (start index of block, number of top bits to check (log2), kernel-only, flag column)
// ADD, MUL, SUB, DIV, MOD, LT, GT and BYTE flags are handled partly manually here, and partly through the Arithmetic table CTL.
// ADDMOD, MULMOD and SUBMOD flags are handled partly manually here, and partly through the Arithmetic table CTL.
Expand All @@ -39,26 +39,26 @@ const OPCODES: [(u8, usize, bool, usize); 16] = [
(0x58, 0, false, COL_MAP.op.pc),
(0x5b, 0, false, COL_MAP.op.jumpdest),
(0x5f, 0, false, COL_MAP.op.push0),
(0x60, 5, false, COL_MAP.op.push), // 0x60-0x7f
(0x80, 4, false, COL_MAP.op.dup), // 0x80-0x8f
(0x90, 4, false, COL_MAP.op.swap), // 0x90-0x9f
(0xee, 0, true, COL_MAP.op.mstore_32bytes),
(0x60, 5, false, COL_MAP.op.push), // 0x60-0x7f
(0x80, 4, false, COL_MAP.op.dup), // 0x80-0x8f
(0x90, 4, false, COL_MAP.op.swap), // 0x90-0x9f
(0xf6, 1, true, COL_MAP.op.context_op), // 0xf6-0xf7
(0xf8, 0, true, COL_MAP.op.mload_32bytes),
(0xf9, 0, true, COL_MAP.op.exit_kernel),
// MLOAD_GENERAL and MSTORE_GENERAL flags are handled manually here.
// MLOAD_32BYTES and MSTORE_32BYTES flags are handled manually here.
];

/// List of combined opcodes requiring a special handling.
/// Each index in the list corresponds to an arbitrary combination
/// of opcodes defined in evm/src/cpu/columns/ops.rs.
const COMBINED_OPCODES: [usize; 6] = [
const COMBINED_OPCODES: [usize; 7] = [
COL_MAP.op.logic_op,
COL_MAP.op.fp254_op,
COL_MAP.op.binary_op,
COL_MAP.op.ternary_op,
COL_MAP.op.shift,
COL_MAP.op.m_op_general,
COL_MAP.op.memop_32bytes,
];

pub fn generate<F: RichField>(lv: &mut CpuColumnsView<F>) {
Expand Down Expand Up @@ -104,6 +104,10 @@ pub fn generate<F: RichField>(lv: &mut CpuColumnsView<F>) {
if opcode == 0xfb || opcode == 0xfc {
lv.op.m_op_general = F::from_bool(kernel);
}

if opcode == 0xee || opcode == 0xf8 {
lv.op.memop_32bytes = F::from_bool(kernel);
}
}

/// Break up an opcode (which is 8 bits long) into its eight bits.
Expand Down Expand Up @@ -192,6 +196,13 @@ pub fn eval_packed_generic<P: PackedField>(
* (opcode - P::Scalar::from_canonical_usize(0xfc_usize))
* lv.op.m_op_general;
yield_constr.constraint(m_op_constr);

// Manually check lv.memop_32bytes.
yield_constr.constraint((P::ONES - kernel_mode) * lv.op.memop_32bytes);
let memop_32bytes_constr = (opcode - P::Scalar::from_canonical_usize(0xee_usize))
* (opcode - P::Scalar::from_canonical_usize(0xf8_usize))
* lv.op.memop_32bytes;
yield_constr.constraint(memop_32bytes_constr);
}

pub fn eval_ext_circuit<F: RichField + Extendable<D>, const D: usize>(
Expand Down Expand Up @@ -294,4 +305,21 @@ pub fn eval_ext_circuit<F: RichField + Extendable<D>, const D: usize>(
m_op_constr = builder.mul_extension(m_op_constr, lv.op.m_op_general);

yield_constr.constraint(builder, m_op_constr);

// Manually check lv.memop_32bytes.
let mload_32bytes_opcode =
builder.constant_extension(F::Extension::from_canonical_usize(0xf8_usize));
let mstore_32bytes_opcode =
builder.constant_extension(F::Extension::from_canonical_usize(0xee_usize));

let constr = builder.mul_extension(is_not_kernel_mode, lv.op.memop_32bytes);
yield_constr.constraint(builder, constr);

let mload_32bytes_constr = builder.sub_extension(opcode, mload_32bytes_opcode);
let mstore_32bytes_constr = builder.sub_extension(opcode, mstore_32bytes_opcode);
let mut memop_32bytes_constr =
builder.mul_extension(mstore_32bytes_constr, mload_32bytes_constr);
memop_32bytes_constr = builder.mul_extension(memop_32bytes_constr, lv.op.memop_32bytes);

yield_constr.constraint(builder, memop_32bytes_constr);
}
3 changes: 1 addition & 2 deletions evm/src/cpu/gas.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ const SIMPLE_OPCODES: OpsColumnsView<Option<u32>> = OpsColumnsView {
dup: G_VERYLOW,
swap: G_VERYLOW,
context_op: KERNEL_ONLY_INSTR,
mstore_32bytes: KERNEL_ONLY_INSTR,
mload_32bytes: KERNEL_ONLY_INSTR,
memop_32bytes: KERNEL_ONLY_INSTR,
exit_kernel: None,
m_op_general: KERNEL_ONLY_INSTR,
syscall: None,
Expand Down
4 changes: 2 additions & 2 deletions evm/src/cpu/kernel/asm/memory/packing.asm
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ global mload_packing_u64_LE:
// Post stack: offset'
global mstore_unpacking:
// stack: context, segment, offset, value, len, retdest
%stack(context, segment, offset, value, len, retdest) -> (context, segment, offset, value, len, offset, len, retdest)
// stack: context, segment, offset, value, len, offset, len, retdest
%stack(context, segment, offset, value, len, retdest) -> (context, segment, offset, len, value, offset, len, retdest)
// stack: context, segment, offset, len, value, offset, len, retdest
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes the argument order of mstore_unpacking inconsistent with MSTORE_32BYTES. I worry that it could become confusing.

In how many places is mstore_unpacking used? Would it be a lot of work to fix the argument order in those places as well?

MSTORE_32BYTES
// stack: offset, len, retdest
ADD SWAP1
Expand Down
1 change: 1 addition & 0 deletions evm/src/cpu/kernel/asm/memory/syscalls.asm
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ global sys_mstore:
PUSH @SEGMENT_MAIN_MEMORY
GET_CONTEXT
// stack: addr: 3, value, len, kexit_info
%stack (ADDR: 3, value, len, kexit_info) -> (ADDR, len, value, kexit_info)
MSTORE_32BYTES
// stack: kexit_info
EXIT_KERNEL
Expand Down
2 changes: 1 addition & 1 deletion evm/src/cpu/kernel/interpreter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1066,8 +1066,8 @@ impl<'a> Interpreter<'a> {
let context = self.pop().as_usize();
let segment = Segment::all()[self.pop().as_usize()];
let offset = self.pop().as_usize();
let value = self.pop();
let len = self.pop().as_usize();
let value = self.pop();

let mut bytes = vec![0; 32];
value.to_little_endian(&mut bytes);
Expand Down
50 changes: 50 additions & 0 deletions evm/src/cpu/memio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,18 @@ fn eval_packed_load<P: PackedField>(
stack::MLOAD_GENERAL_OP.unwrap(),
yield_constr,
);

// Check the stack for MLOAD_32BYTES.
// The second bit (in little-endian) of MLOAD_32BYTES is 0.
let filter = lv.op.memop_32bytes * (P::ONES - lv.opcode_bits[1]);

stack::eval_packed_one(
lv,
nv,
filter,
stack::MLOAD_32BYTES_OP.unwrap(),
yield_constr,
);
}

fn eval_ext_circuit_load<F: RichField + Extendable<D>, const D: usize>(
Expand Down Expand Up @@ -106,6 +118,20 @@ fn eval_ext_circuit_load<F: RichField + Extendable<D>, const D: usize>(
stack::MLOAD_GENERAL_OP.unwrap(),
yield_constr,
);

// Check the stack for MLOAD_32BYTES.
let one = builder.one_extension();
let mut filter = builder.sub_extension(one, lv.opcode_bits[1]);
filter = builder.mul_extension(lv.op.memop_32bytes, filter);

stack::eval_ext_circuit_one(
builder,
lv,
nv,
filter,
stack::MLOAD_32BYTES_OP.unwrap(),
yield_constr,
);
}

fn eval_packed_store<P: PackedField>(
Expand Down Expand Up @@ -141,6 +167,18 @@ fn eval_packed_store<P: PackedField>(
stack::MSTORE_GENERAL_OP.unwrap(),
yield_constr,
);

// Check the stack for MSTORE_32BYTES.
// The second bit (in little-endian) of MSTORE_32BYTES is 1.
let filter = lv.op.memop_32bytes * lv.opcode_bits[1];

stack::eval_packed_one(
lv,
nv,
filter,
stack::MSTORE_32BYTES_OP.unwrap(),
yield_constr,
);
}

fn eval_ext_circuit_store<F: RichField + Extendable<D>, const D: usize>(
Expand Down Expand Up @@ -199,6 +237,18 @@ fn eval_ext_circuit_store<F: RichField + Extendable<D>, const D: usize>(
stack::MSTORE_GENERAL_OP.unwrap(),
yield_constr,
);

// Check the stack for MSTORE_32BYTES.
let filter = builder.mul_extension(lv.op.memop_32bytes, lv.opcode_bits[1]);

stack::eval_ext_circuit_one(
builder,
lv,
nv,
filter,
stack::MSTORE_32BYTES_OP.unwrap(),
yield_constr,
);
}

pub fn eval_packed<P: PackedField>(
Expand Down
22 changes: 12 additions & 10 deletions evm/src/cpu/stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,17 @@ pub(crate) const MSTORE_GENERAL_OP: Option<StackBehavior> = Some(StackBehavior {
disable_other_channels: false,
});

pub(crate) const MLOAD_32BYTES_OP: Option<StackBehavior> = Some(StackBehavior {
num_pops: 4,
pushes: true,
disable_other_channels: false,
});

pub(crate) const MSTORE_32BYTES_OP: Option<StackBehavior> = Some(StackBehavior {
num_pops: 5,
pushes: false,
disable_other_channels: false,
});
// AUDITORS: If the value below is `None`, then the operation must be manually checked to ensure
// that every general-purpose memory channel is either disabled or has its read flag and address
// propertly constrained. The same applies when `disable_other_channels` is set to `false`,
Expand Down Expand Up @@ -104,16 +115,7 @@ const STACK_BEHAVIORS: OpsColumnsView<Option<StackBehavior>> = OpsColumnsView {
dup: None,
swap: None,
context_op: None, // SET_CONTEXT is special since it involves the old and the new stack.
mstore_32bytes: Some(StackBehavior {
num_pops: 5,
pushes: false,
disable_other_channels: false,
}),
mload_32bytes: Some(StackBehavior {
num_pops: 4,
pushes: true,
disable_other_channels: false,
}),
memop_32bytes: None,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a comment to say where these constraints do live (if only to make clear that they haven't been forgotten).

exit_kernel: Some(StackBehavior {
num_pops: 1,
pushes: false,
Expand Down
2 changes: 1 addition & 1 deletion evm/src/witness/operation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -760,7 +760,7 @@ pub(crate) fn generate_mstore_32bytes<F: Field>(
state: &mut GenerationState<F>,
mut row: CpuColumnsView<F>,
) -> Result<(), ProgramError> {
let [(context, log_in0), (segment, log_in1), (base_virt, log_in2), (val, log_in3), (len, log_in4)] =
let [(context, log_in0), (segment, log_in1), (base_virt, log_in2), (len, log_in3), (val, log_in4)] =
stack_pop_with_log_and_fill::<5, _>(state, &mut row)?;
let len = len.as_usize();

Expand Down
3 changes: 1 addition & 2 deletions evm/src/witness/transition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,7 @@ fn fill_op_flag<F: Field>(op: Operation, row: &mut CpuColumnsView<F>) {
Operation::Pc => &mut flags.pc,
Operation::Jumpdest => &mut flags.jumpdest,
Operation::GetContext | Operation::SetContext => &mut flags.context_op,
Operation::Mload32Bytes => &mut flags.mload_32bytes,
Operation::Mstore32Bytes => &mut flags.mstore_32bytes,
Operation::Mload32Bytes | Operation::Mstore32Bytes => &mut flags.memop_32bytes,
Operation::ExitKernel => &mut flags.exit_kernel,
Operation::MloadGeneral | Operation::MstoreGeneral => &mut flags.m_op_general,
} = F::ONE;
Expand Down
Loading