From 6ecdf3df521c0deb6bf107bc99b5ad4eead7e5de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juli=C3=A1n=20Gonz=C3=A1lez=20Calder=C3=B3n?= Date: Thu, 19 Sep 2024 13:44:37 -0300 Subject: [PATCH 1/7] Enforce prover constraints in add, sub, and mul --- vm/src/vm/runners/builtin_runner/modulo.rs | 146 ++++++++++++++------- 1 file changed, 97 insertions(+), 49 deletions(-) diff --git a/vm/src/vm/runners/builtin_runner/modulo.rs b/vm/src/vm/runners/builtin_runner/modulo.rs index 572ccf1897..2324d42148 100644 --- a/vm/src/vm/runners/builtin_runner/modulo.rs +++ b/vm/src/vm/runners/builtin_runner/modulo.rs @@ -47,6 +47,7 @@ pub struct ModBuiltinRunner { // Precomputed powers used for reading and writing values that are represented as n_words words of word_bit_len bits each. shift: BigUint, shift_powers: [BigUint; N_WORDS], + k_bound: BigUint, } #[derive(Debug, Clone)] @@ -60,7 +61,7 @@ pub enum Operation { Mul, Add, Sub, - DivMod(BigUint), + DivMod, } impl Display for Operation { @@ -69,7 +70,7 @@ impl Display for Operation { Operation::Mul => "*".fmt(f), Operation::Add => "+".fmt(f), Operation::Sub => "-".fmt(f), - Operation::DivMod(_) => "/".fmt(f), + Operation::DivMod => "/".fmt(f), } } } @@ -85,17 +86,29 @@ struct Inputs { impl ModBuiltinRunner { pub(crate) fn new_add_mod(instance_def: &ModInstanceDef, included: bool) -> Self { - Self::new(instance_def.clone(), included, ModBuiltinType::Add) + Self::new( + instance_def.clone(), + included, + ModBuiltinType::Add, + Some(2u32.into()), + ) } pub(crate) fn new_mul_mod(instance_def: &ModInstanceDef, included: bool) -> Self { - Self::new(instance_def.clone(), included, ModBuiltinType::Mul) + Self::new(instance_def.clone(), included, ModBuiltinType::Mul, None) } - fn new(instance_def: ModInstanceDef, included: bool, builtin_type: ModBuiltinType) -> Self { + fn new( + instance_def: ModInstanceDef, + included: bool, + builtin_type: ModBuiltinType, + k_bound: Option, + ) -> Self { let shift = BigUint::one().shl(instance_def.word_bit_len); let shift_powers = core::array::from_fn(|i| shift.pow(i as u32)); let zero_segment_size = core::cmp::max(N_WORDS, instance_def.batch_size * 3); + let int_lim = BigUint::from(2_u32).pow(N_WORDS as u32 * instance_def.word_bit_len); + dbg!(&int_lim); Self { builtin_type, base: 0, @@ -106,6 +119,7 @@ impl ModBuiltinRunner { zero_segment_size, shift, shift_powers, + k_bound: k_bound.unwrap_or(int_lim), } } @@ -458,19 +472,19 @@ impl ModBuiltinRunner { match (a, b, c) { // Deduce c from a and b and write it to memory. (Some(a), Some(b), None) => { - let value = apply_op(a, b, op)?.mod_floor(&inputs.p); + let value = apply_op(op, a, b, &inputs.p, &self.k_bound)?; self.write_n_words_value(memory, addresses[2], value)?; Ok(true) } // Deduce b from a and c and write it to memory. (Some(a), None, Some(c)) => { - let value = apply_op(c, a, inv_op)?.mod_floor(&inputs.p); + let value = apply_op(inv_op, c, a, &inputs.p, &self.k_bound)?; self.write_n_words_value(memory, addresses[1], value)?; Ok(true) } // Deduce a from b and c and write it to memory. (None, Some(b), Some(c)) => { - let value = apply_op(c, b, inv_op)?.mod_floor(&inputs.p); + let value = apply_op(inv_op, c, b, &inputs.p, &self.k_bound)?; self.write_n_words_value(memory, addresses[0], value)?; Ok(true) } @@ -539,44 +553,45 @@ impl ModBuiltinRunner { Default::default() }; - // Get one of the builtin runners - the rest of this function doesn't depend on batch_size. - let mod_runner = if let Some((_, add_mod, _)) = add_mod { - add_mod - } else { - mul_mod.unwrap().1 - }; // Fill the values table. let mut add_mod_index = 0; let mut mul_mod_index = 0; - // Create operation here to avoid cloning p in the loop - let div_operation = Operation::DivMod(mul_mod_inputs.p.clone()); + while add_mod_index < add_mod_n || mul_mod_index < mul_mod_n { - if add_mod_index < add_mod_n - && mod_runner.fill_value( - memory, - &add_mod_inputs, - add_mod_index, - &Operation::Add, - &Operation::Sub, - )? - { - add_mod_index += 1; - } else if mul_mod_index < mul_mod_n - && mod_runner.fill_value( - memory, - &mul_mod_inputs, - mul_mod_index, - &Operation::Mul, - &div_operation, - )? - { - mul_mod_index += 1; - } else { - return Err(RunnerError::FillMemoryCoudNotFillTable( - add_mod_index, - mul_mod_index, - )); + if add_mod_index < add_mod_n { + if let Some((_, add_mod_runner, _)) = add_mod { + if add_mod_runner.fill_value( + memory, + &add_mod_inputs, + add_mod_index, + &Operation::Add, + &Operation::Sub, + )? { + add_mod_index += 1; + continue; + } + } } + + if mul_mod_index < mul_mod_n { + if let Some((_, mul_mod_runner, _)) = mul_mod { + if mul_mod_runner.fill_value( + memory, + &mul_mod_inputs, + mul_mod_index, + &Operation::Mul, + &Operation::DivMod, + )? { + mul_mod_index += 1; + } + continue; + } + } + + return Err(RunnerError::FillMemoryCoudNotFillTable( + add_mod_index, + mul_mod_index, + )); } Ok(()) } @@ -629,7 +644,7 @@ impl ModBuiltinRunner { ModBuiltinType::Add => Operation::Add, ModBuiltinType::Mul => Operation::Mul, }; - let a_op_b = apply_op(&a, &b, &op)?.mod_floor(&inputs.p); + let a_op_b = apply_op(&op, &a, &b, &inputs.p, &self.k_bound)?; if a_op_b != c.mod_floor(&inputs.p) { // Build error string let p = inputs.p; @@ -669,13 +684,46 @@ impl ModBuiltinRunner { } } -fn apply_op(lhs: &BigUint, rhs: &BigUint, op: &Operation) -> Result { - Ok(match op { - Operation::Mul => lhs * rhs, - Operation::Add => lhs + rhs, - Operation::Sub => lhs - rhs, - Operation::DivMod(ref p) => div_mod_unsigned(lhs, rhs, p)?, - }) +fn apply_op( + op: &Operation, + lhs: &BigUint, + rhs: &BigUint, + p: &BigUint, + k_bound: &BigUint, +) -> Result { + println!("== GATE =="); + println!("op = {:?}", op); + println!("lhs = {:?}", lhs); + println!("rhs = {:?}", rhs); + println!("k_bound = {:?}", k_bound); + let value = match op { + Operation::Mul => { + let value = lhs * rhs; + if value < k_bound * p { + value.mod_floor(p) + } else { + value - (k_bound - 1u32) * p + } + } + Operation::Add => { + let value = lhs + rhs; + if value < k_bound * p { + value.mod_floor(p) + } else { + value - (k_bound - 1u32) * p + } + } + Operation::Sub => { + if rhs <= lhs { + lhs - rhs + } else { + lhs + p - rhs + } + } + Operation::DivMod => div_mod_unsigned(lhs, rhs, p)?, + }; + println!("value = {:?}", value); + Ok(value) } #[cfg(test)] From 26308458d66f113ce8e015e17ee084207cbc447f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juli=C3=A1n=20Gonz=C3=A1lez=20Calder=C3=B3n?= Date: Thu, 19 Sep 2024 14:03:01 -0300 Subject: [PATCH 2/7] Remove debug prints --- vm/src/vm/runners/builtin_runner/modulo.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/vm/src/vm/runners/builtin_runner/modulo.rs b/vm/src/vm/runners/builtin_runner/modulo.rs index 2324d42148..8361acee1f 100644 --- a/vm/src/vm/runners/builtin_runner/modulo.rs +++ b/vm/src/vm/runners/builtin_runner/modulo.rs @@ -108,7 +108,6 @@ impl ModBuiltinRunner { let shift_powers = core::array::from_fn(|i| shift.pow(i as u32)); let zero_segment_size = core::cmp::max(N_WORDS, instance_def.batch_size * 3); let int_lim = BigUint::from(2_u32).pow(N_WORDS as u32 * instance_def.word_bit_len); - dbg!(&int_lim); Self { builtin_type, base: 0, @@ -691,11 +690,6 @@ fn apply_op( p: &BigUint, k_bound: &BigUint, ) -> Result { - println!("== GATE =="); - println!("op = {:?}", op); - println!("lhs = {:?}", lhs); - println!("rhs = {:?}", rhs); - println!("k_bound = {:?}", k_bound); let value = match op { Operation::Mul => { let value = lhs * rhs; @@ -722,7 +716,6 @@ fn apply_op( } Operation::DivMod => div_mod_unsigned(lhs, rhs, p)?, }; - println!("value = {:?}", value); Ok(value) } From fa384fb36dbf3566be770510f071106c5ae78e0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juli=C3=A1n=20Gonz=C3=A1lez=20Calder=C3=B3n?= Date: Thu, 19 Sep 2024 14:32:01 -0300 Subject: [PATCH 3/7] Add tests --- vm/src/tests/compare_outputs_dynamic_layouts.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/vm/src/tests/compare_outputs_dynamic_layouts.sh b/vm/src/tests/compare_outputs_dynamic_layouts.sh index d7d5a8bddb..cb87faad5f 100755 --- a/vm/src/tests/compare_outputs_dynamic_layouts.sh +++ b/vm/src/tests/compare_outputs_dynamic_layouts.sh @@ -74,6 +74,9 @@ CASES=( "cairo_programs/proof_programs/fibonacci.json;all_cairo" "cairo_programs/proof_programs/factorial.json;double_all_cairo" "cairo_programs/proof_programs/fibonacci.json;double_all_cairo" + "cairo_programs/mod_builtin_feature/proof/mod_builtin.json;all_cairo" + "cairo_programs/mod_builtin_feature/proof/mod_builtin_failure.json;all_cairo" + "cairo_programs/mod_builtin_feature/proof/apply_poly.json;all_cairo" ) passed_tests=0 From 6991904025b56ab67dcb3a13951580a391efc99d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juli=C3=A1n=20Gonz=C3=A1lez=20Calder=C3=B3n?= Date: Thu, 19 Sep 2024 15:23:29 -0300 Subject: [PATCH 4/7] Update CHANGELOG.md --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3317000d50..1f4001b5a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ #### Upcoming Changes +* fix: [#1841](https://github.com/lambdaclass/cairo-vm/pull/1841): + * Fix modulo builtin to comply with prover constraints + * feat(BREAKING): [#1824](https://github.com/lambdaclass/cairo-vm/pull/1824): * Add support for dynamic layout * CLI change(BREAKING): The flag `cairo_layout_params_file` must be specified when using dynamic layout. From deab241f6e3f0280bcf3d1f5f22499b3b6bf658b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juli=C3=A1n=20Gonz=C3=A1lez=20Calder=C3=B3n?= Date: Thu, 19 Sep 2024 15:48:45 -0300 Subject: [PATCH 5/7] Fix serialization --- vm/src/air_private_input.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/vm/src/air_private_input.rs b/vm/src/air_private_input.rs index b76575eaff..f257fe96cb 100644 --- a/vm/src/air_private_input.rs +++ b/vm/src/air_private_input.rs @@ -126,6 +126,8 @@ pub struct ModInputInstance { pub values_ptr: usize, pub offsets_ptr: usize, pub n: usize, + #[serde(deserialize_with = "mod_input_instance_batch_serde::deserialize")] + #[serde(serialize_with = "mod_input_instance_batch_serde::serialize")] pub batch: BTreeMap, } @@ -205,6 +207,29 @@ impl AirPrivateInputSerializable { } } +mod mod_input_instance_batch_serde { + use super::*; + + use serde::{Deserializer, Serializer}; + + pub(crate) fn serialize( + value: &BTreeMap, + s: S, + ) -> Result { + let value = value.iter().map(|v| v.1).collect::>(); + + value.serialize(s) + } + + pub(crate) fn deserialize<'de, D: Deserializer<'de>>( + d: D, + ) -> Result, D::Error> { + let value = Vec::::deserialize(d)?; + + Ok(value.into_iter().enumerate().collect()) + } +} + #[cfg(test)] mod tests { use crate::types::layout_name::LayoutName; From 9db796af7805ee23690056f96f0d55b26249f67e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juli=C3=A1n=20Gonz=C3=A1lez=20Calder=C3=B3n?= Date: Thu, 19 Sep 2024 15:49:14 -0300 Subject: [PATCH 6/7] Comment failing test --- vm/src/tests/compare_outputs_dynamic_layouts.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vm/src/tests/compare_outputs_dynamic_layouts.sh b/vm/src/tests/compare_outputs_dynamic_layouts.sh index cb87faad5f..d628701c1e 100755 --- a/vm/src/tests/compare_outputs_dynamic_layouts.sh +++ b/vm/src/tests/compare_outputs_dynamic_layouts.sh @@ -76,7 +76,7 @@ CASES=( "cairo_programs/proof_programs/fibonacci.json;double_all_cairo" "cairo_programs/mod_builtin_feature/proof/mod_builtin.json;all_cairo" "cairo_programs/mod_builtin_feature/proof/mod_builtin_failure.json;all_cairo" - "cairo_programs/mod_builtin_feature/proof/apply_poly.json;all_cairo" + # "cairo_programs/mod_builtin_feature/proof/apply_poly.json;all_cairo" ) passed_tests=0 From dd6d6ffd8908e3d237c65e72173150de75b218da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juli=C3=A1n=20Gonz=C3=A1lez=20Calder=C3=B3n?= Date: Thu, 19 Sep 2024 16:26:07 -0300 Subject: [PATCH 7/7] Uncomment test --- vm/src/tests/compare_outputs_dynamic_layouts.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vm/src/tests/compare_outputs_dynamic_layouts.sh b/vm/src/tests/compare_outputs_dynamic_layouts.sh index d628701c1e..cb87faad5f 100755 --- a/vm/src/tests/compare_outputs_dynamic_layouts.sh +++ b/vm/src/tests/compare_outputs_dynamic_layouts.sh @@ -76,7 +76,7 @@ CASES=( "cairo_programs/proof_programs/fibonacci.json;double_all_cairo" "cairo_programs/mod_builtin_feature/proof/mod_builtin.json;all_cairo" "cairo_programs/mod_builtin_feature/proof/mod_builtin_failure.json;all_cairo" - # "cairo_programs/mod_builtin_feature/proof/apply_poly.json;all_cairo" + "cairo_programs/mod_builtin_feature/proof/apply_poly.json;all_cairo" ) passed_tests=0