From 9f4bb0cb7ef4425938ea1d395e69ea3f4dcf622e Mon Sep 17 00:00:00 2001 From: Hamish Ivey-Law Date: Thu, 14 Sep 2023 15:49:48 +0400 Subject: [PATCH 1/2] Cleaner way to handle "simulated" operations SHL and SHR. --- evm/src/arithmetic/arithmetic_stark.rs | 22 +++---- evm/src/arithmetic/mod.rs | 83 +++++++++++++------------- evm/src/witness/gas.rs | 4 +- evm/src/witness/operation.rs | 10 ++-- evm/src/witness/transition.rs | 11 ++-- 5 files changed, 61 insertions(+), 69 deletions(-) diff --git a/evm/src/arithmetic/arithmetic_stark.rs b/evm/src/arithmetic/arithmetic_stark.rs index 23efeb7797..b20f3d9641 100644 --- a/evm/src/arithmetic/arithmetic_stark.rs +++ b/evm/src/arithmetic/arithmetic_stark.rs @@ -303,7 +303,7 @@ mod tests { }; // 123 + 456 == 579 - let add = Operation::binary(BinaryOperator::Add, U256::from(123), U256::from(456), false); + let add = Operation::binary(BinaryOperator::Add, U256::from(123), U256::from(456)); // (123 * 456) % 1007 == 703 let mulmod = Operation::ternary( TernaryOperator::MulMod, @@ -319,27 +319,22 @@ mod tests { U256::from(1007), ); // 123 * 456 == 56088 - let mul = Operation::binary(BinaryOperator::Mul, U256::from(123), U256::from(456), false); + let mul = Operation::binary(BinaryOperator::Mul, U256::from(123), U256::from(456)); // 128 / 13 == 9 - let div = Operation::binary(BinaryOperator::Div, U256::from(128), U256::from(13), false); + let div = Operation::binary(BinaryOperator::Div, U256::from(128), U256::from(13)); // 128 < 13 == 0 - let lt1 = Operation::binary(BinaryOperator::Lt, U256::from(128), U256::from(13), false); + let lt1 = Operation::binary(BinaryOperator::Lt, U256::from(128), U256::from(13)); // 13 < 128 == 1 - let lt2 = Operation::binary(BinaryOperator::Lt, U256::from(13), U256::from(128), false); + let lt2 = Operation::binary(BinaryOperator::Lt, U256::from(13), U256::from(128)); // 128 < 128 == 0 - let lt3 = Operation::binary(BinaryOperator::Lt, U256::from(128), U256::from(128), false); + let lt3 = Operation::binary(BinaryOperator::Lt, U256::from(128), U256::from(128)); // 128 % 13 == 11 - let modop = Operation::binary(BinaryOperator::Mod, U256::from(128), U256::from(13), false); + let modop = Operation::binary(BinaryOperator::Mod, U256::from(128), U256::from(13)); // byte(30, 0xABCD) = 0xAB - let byte = Operation::binary( - BinaryOperator::Byte, - U256::from(30), - U256::from(0xABCD), - false, - ); + let byte = Operation::binary(BinaryOperator::Byte, U256::from(30), U256::from(0xABCD)); let ops: Vec = vec![add, mulmod, addmod, mul, modop, lt1, lt2, lt3, div, byte]; @@ -402,7 +397,6 @@ mod tests { BinaryOperator::Mul, U256::from(rng.gen::<[u8; 32]>()), U256::from(rng.gen::<[u8; 32]>()), - false, ) }) .collect::>(); diff --git a/evm/src/arithmetic/mod.rs b/evm/src/arithmetic/mod.rs index b0cc4280ac..36833f42ba 100644 --- a/evm/src/arithmetic/mod.rs +++ b/evm/src/arithmetic/mod.rs @@ -27,15 +27,17 @@ pub(crate) enum BinaryOperator { MulFp254, SubFp254, Byte, + Shl, // simulated with MUL + Shr, // simulated with DIV } impl BinaryOperator { pub(crate) fn result(&self, input0: U256, input1: U256) -> U256 { match self { BinaryOperator::Add => input0.overflowing_add(input1).0, - BinaryOperator::Mul => input0.overflowing_mul(input1).0, + BinaryOperator::Mul | BinaryOperator::Shl => input0.overflowing_mul(input1).0, BinaryOperator::Sub => input0.overflowing_sub(input1).0, - BinaryOperator::Div => { + BinaryOperator::Div | BinaryOperator::Shr => { if input1.is_zero() { U256::zero() } else { @@ -60,28 +62,25 @@ impl BinaryOperator { } else { input1.byte(31 - input0.as_usize()).into() } - } + } /* + BinaryOperator::Shl => { + if input0 < 256.into() { + input1 << input0 + } else { + U256::zero() + } + } + BinaryOperator::Shr => input1 >> input0, + */ } } - pub(crate) fn row_filter(&self, is_simulated: bool) -> usize { + pub(crate) fn row_filter(&self) -> usize { match self { BinaryOperator::Add => columns::IS_ADD, - BinaryOperator::Mul => { - if is_simulated { - columns::IS_SHL - } else { - columns::IS_MUL - } - } + BinaryOperator::Mul => columns::IS_MUL, BinaryOperator::Sub => columns::IS_SUB, - BinaryOperator::Div => { - if is_simulated { - columns::IS_SHR - } else { - columns::IS_DIV - } - } + BinaryOperator::Div => columns::IS_DIV, BinaryOperator::Mod => columns::IS_MOD, BinaryOperator::Lt => columns::IS_LT, BinaryOperator::Gt => columns::IS_GT, @@ -89,6 +88,8 @@ impl BinaryOperator { BinaryOperator::MulFp254 => columns::IS_MULFP254, BinaryOperator::SubFp254 => columns::IS_SUBFP254, BinaryOperator::Byte => columns::IS_BYTE, + BinaryOperator::Shl => columns::IS_SHL, + BinaryOperator::Shr => columns::IS_SHR, } } } @@ -131,7 +132,6 @@ pub(crate) enum Operation { input0: U256, input1: U256, result: U256, - is_simulated: bool, }, TernaryOperation { operator: TernaryOperator, @@ -143,19 +143,27 @@ pub(crate) enum Operation { } impl Operation { - pub(crate) fn binary( - operator: BinaryOperator, - input0: U256, - input1: U256, - is_simulated: bool, - ) -> Self { + /// Create a binary operator with given inputs. + /// + /// NB: This works as you would expect, EXCEPT for SHL and SHR, + /// whose inputs need a small amount of preprocessing. Specifically, + /// to create `SHL(shift, value)`, call + /// + /// `Operation::binary(BinaryOperator::Shl, value, 1 << shift)` + /// + /// Similarly, to create `SHR(shift, value)`, call + /// + /// `Operation::binary(BinaryOperator::Shr, value, 1 << shift)` + /// + /// See witness/operation.rs::append_shift() for an example (indeed + /// the only call site for such inputs). + pub(crate) fn binary(operator: BinaryOperator, input0: U256, input1: U256) -> Self { let result = operator.result(input0, input1); Self::BinaryOperation { operator, input0, input1, result, - is_simulated, } } @@ -199,8 +207,7 @@ impl Operation { input0, input1, result, - is_simulated, - } => binary_op_to_rows(operator, is_simulated, input0, input1, result), + } => binary_op_to_rows(operator, input0, input1, result), Operation::TernaryOperation { operator, input0, @@ -231,37 +238,29 @@ fn ternary_op_to_rows( fn binary_op_to_rows( op: BinaryOperator, - is_simulated: bool, input0: U256, input1: U256, result: U256, ) -> (Vec, Option>) { let mut row = vec![F::ZERO; columns::NUM_ARITH_COLUMNS]; - row[op.row_filter(is_simulated)] = F::ONE; + row[op.row_filter()] = F::ONE; match op { BinaryOperator::Add | BinaryOperator::Sub | BinaryOperator::Lt | BinaryOperator::Gt => { - addcy::generate(&mut row, op.row_filter(false), input0, input1); + addcy::generate(&mut row, op.row_filter(), input0, input1); (row, None) } - BinaryOperator::Mul => { + BinaryOperator::Mul | BinaryOperator::Shl => { mul::generate(&mut row, input0, input1); (row, None) } - BinaryOperator::Div | BinaryOperator::Mod => { + BinaryOperator::Div | BinaryOperator::Mod | BinaryOperator::Shr => { let mut nv = vec![F::ZERO; columns::NUM_ARITH_COLUMNS]; - divmod::generate( - &mut row, - &mut nv, - op.row_filter(is_simulated), - input0, - input1, - result, - ); + divmod::generate(&mut row, &mut nv, op.row_filter(), input0, input1, result); (row, Some(nv)) } BinaryOperator::AddFp254 | BinaryOperator::MulFp254 | BinaryOperator::SubFp254 => { - ternary_op_to_rows::(op.row_filter(false), input0, input1, BN_BASE, result) + ternary_op_to_rows::(op.row_filter(), input0, input1, BN_BASE, result) } BinaryOperator::Byte => { byte::generate(&mut row, input0, input1); diff --git a/evm/src/witness/gas.rs b/evm/src/witness/gas.rs index 3a46c04439..aa312078a5 100644 --- a/evm/src/witness/gas.rs +++ b/evm/src/witness/gas.rs @@ -25,8 +25,8 @@ pub(crate) fn gas_to_charge(op: Operation) -> u64 { BinaryArithmetic(Lt) => G_VERYLOW, BinaryArithmetic(Gt) => G_VERYLOW, BinaryArithmetic(Byte) => G_VERYLOW, - Shl => G_VERYLOW, - Shr => G_VERYLOW, + BinaryArithmetic(Shl) => G_VERYLOW, + BinaryArithmetic(Shr) => G_VERYLOW, BinaryArithmetic(AddFp254) => KERNEL_ONLY_INSTR, BinaryArithmetic(MulFp254) => KERNEL_ONLY_INSTR, BinaryArithmetic(SubFp254) => KERNEL_ONLY_INSTR, diff --git a/evm/src/witness/operation.rs b/evm/src/witness/operation.rs index 902d109376..23d64be41c 100644 --- a/evm/src/witness/operation.rs +++ b/evm/src/witness/operation.rs @@ -29,8 +29,6 @@ use crate::{arithmetic, logic}; pub(crate) enum Operation { Iszero, Not, - Shl, - Shr, Syscall(u8, usize, bool), // (syscall number, minimum stack length, increases stack length) Eq, BinaryLogic(logic::Op), @@ -79,7 +77,7 @@ pub(crate) fn generate_binary_arithmetic_op( ) -> Result<(), ProgramError> { let [(input0, log_in0), (input1, log_in1)] = stack_pop_with_log_and_fill::<2, _>(state, &mut row)?; - let operation = arithmetic::Operation::binary(operator, input0, input1, false); + let operation = arithmetic::Operation::binary(operator, input0, input1); let log_out = stack_push_log_and_fill(state, &mut row, operation.result())?; if operator == arithmetic::BinaryOperator::AddFp254 @@ -502,11 +500,11 @@ fn append_shift( U256::one() << input0 }; let operator = if is_shl { - BinaryOperator::Mul + BinaryOperator::Shl } else { - BinaryOperator::Div + BinaryOperator::Shr }; - let operation = arithmetic::Operation::binary(operator, input1, input0, true); + let operation = arithmetic::Operation::binary(operator, input1, input0); state.traces.push_arithmetic(operation); state.traces.push_memory(log_in0); diff --git a/evm/src/witness/transition.rs b/evm/src/witness/transition.rs index 8667eb08ee..9532aa339b 100644 --- a/evm/src/witness/transition.rs +++ b/evm/src/witness/transition.rs @@ -70,8 +70,8 @@ fn decode(registers: RegistersState, opcode: u8) -> Result Ok(Operation::BinaryArithmetic( arithmetic::BinaryOperator::Byte, )), - (0x1b, _) => Ok(Operation::Shl), - (0x1c, _) => Ok(Operation::Shr), + (0x1b, _) => Ok(Operation::BinaryArithmetic(arithmetic::BinaryOperator::Shl)), + (0x1c, _) => Ok(Operation::BinaryArithmetic(arithmetic::BinaryOperator::Shr)), (0x1d, _) => Ok(Operation::Syscall(opcode, 2, false)), // SAR (0x20, _) => Ok(Operation::Syscall(opcode, 2, false)), // KECCAK256 (0x21, true) => Ok(Operation::KeccakGeneral), @@ -165,8 +165,9 @@ fn fill_op_flag(op: Operation, row: &mut CpuColumnsView) { Operation::BinaryArithmetic(arithmetic::BinaryOperator::AddFp254) | Operation::BinaryArithmetic(arithmetic::BinaryOperator::MulFp254) | Operation::BinaryArithmetic(arithmetic::BinaryOperator::SubFp254) => &mut flags.fp254_op, + Operation::BinaryArithmetic(arithmetic::BinaryOperator::Shl) + | Operation::BinaryArithmetic(arithmetic::BinaryOperator::Shr) => &mut flags.shift, Operation::BinaryArithmetic(_) => &mut flags.binary_op, - Operation::Shl | Operation::Shr => &mut flags.shift, Operation::TernaryArithmetic(_) => &mut flags.ternary_op, Operation::KeccakGeneral => &mut flags.keccak_general, Operation::ProverInput => &mut flags.prover_input, @@ -194,8 +195,8 @@ fn perform_op( Operation::Swap(n) => generate_swap(n, state, row)?, Operation::Iszero => generate_iszero(state, row)?, Operation::Not => generate_not(state, row)?, - Operation::Shl => generate_shl(state, row)?, - Operation::Shr => generate_shr(state, row)?, + Operation::BinaryArithmetic(arithmetic::BinaryOperator::Shl) => generate_shl(state, row)?, + Operation::BinaryArithmetic(arithmetic::BinaryOperator::Shr) => generate_shr(state, row)?, Operation::Syscall(opcode, stack_values_read, stack_len_increased) => { generate_syscall(opcode, stack_values_read, stack_len_increased, state, row)? } From 5a7356ff4a1d034b8472cd4a95d4a13a9244a5fc Mon Sep 17 00:00:00 2001 From: Hamish Ivey-Law Date: Thu, 14 Sep 2023 16:13:16 +0400 Subject: [PATCH 2/2] Fix comments. --- evm/src/arithmetic/mod.rs | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/evm/src/arithmetic/mod.rs b/evm/src/arithmetic/mod.rs index 36833f42ba..bd6d56e8cb 100644 --- a/evm/src/arithmetic/mod.rs +++ b/evm/src/arithmetic/mod.rs @@ -62,16 +62,7 @@ impl BinaryOperator { } else { input1.byte(31 - input0.as_usize()).into() } - } /* - BinaryOperator::Shl => { - if input0 < 256.into() { - input1 << input0 - } else { - U256::zero() - } - } - BinaryOperator::Shr => input1 >> input0, - */ + } } } @@ -121,10 +112,6 @@ impl TernaryOperator { } /// An enum representing arithmetic operations that can be either binary or ternary. -/// -/// Binary operations include a special `is_simulated` flag to differentiate SHL -/// and SHR operations from MUL and DIV respectively, as the former are simulated -/// with the latter by scaling their inputs. #[derive(Debug)] pub(crate) enum Operation { BinaryOperation { @@ -147,7 +134,8 @@ impl Operation { /// /// NB: This works as you would expect, EXCEPT for SHL and SHR, /// whose inputs need a small amount of preprocessing. Specifically, - /// to create `SHL(shift, value)`, call + /// to create `SHL(shift, value)`, call (note the reversal of + /// argument order): /// /// `Operation::binary(BinaryOperator::Shl, value, 1 << shift)` ///