Add secure_run flag + integrate verify_secure_runner into `cairo-…

…run` (#771) * Start re-implementing BuiltinRunner::run_security_checks * Add method verify_auto_deductions_for_addr + finish re-implementation * Fix loop variable * Clippy * Style * Fix math + add missing check * Fix logic * Add test + repurpose test * Fix test * Fix potential substraction with overflow * Remove test inconsistencies * Use iterators for offset_max & offset_len * Use assumption for max_offset * Add misc tests * Add more tests * More exhaustive clippy * Save initial progress * Save progress * Return early ok if empty builtin segment * Fix comparison bounds * Remove uneeded mut * Add warning, fix comparison, add tests * Fix test * Fix tests * Quick solution for stop_ptr not being updated * Fix error variant * Save progress * Fix error varaint in test * Fix test * Add temporary addresses check * Add tests for temporary addresses check * Remove previous implementation * Simplify segment info + clippy * Style * Fix * Fix error strings * Add comment * Add changelog entry * Add tests for read_return_values * Add secure_run flag * Remove asserts from ec_op tests & adapt tests * Fix error string * Clippy tests * Add test comment * Add changelog entry * Add clippy::allow_too_many_arguments for cairo_run * Fix bug in ec_op deduce_memory_cell * Add bugfix to changelog * Update CHANGELOG.md * Fix benchmarks * Change return of get_segment_info to vec * Fix typos * Fix typos * Fix comment
lambdaclass · Jan 25, 2023 · 65c20b6 · 65c20b6
1 parent 9d239eb
commit 65c20b6
Show file tree

Hide file tree

Showing 11 changed files with 185 additions and 50 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,22 @@
 ## Cairo-VM Changelog
 
 #### Upcoming Changes
+* Add secure_run flag + integrate verify_secure_runner into cairo-run [#771](https://github.com/lambdaclass/cairo-rs/pull/777)
+    Public Api changes:
+    * Add command_line argument `secure_run`
+    * Add argument `secure_run: Option<bool>` to `cairo_run`
+    * `verify_secure_runner` is now called inside `cairo-run` when `secure_run` is set to true or when it not set and the run is not on `proof_mode`
+    Bugfixes:
+    * `EcOpBuiltinRunner::deduce_memory_cell` now checks that both points are on the curve instead of only the first one
+    * `EcOpBuiltinRunner::deduce_memory_cell` now returns the values of the point coordinates instead of the indices when a `PointNotOnCurve` error is returned
+
+* Refactor `Refactor verify_secure_runner` [#768](https://github.com/lambdaclass/cairo-rs/pull/768)
+    Public Api changes:
+    * Remove builtin name from the return value of `BuiltinRunner::get_memory_segment_addresses`
+    * Simplify the return value of `CairoRunner::get_builtin_segments_info` to `Vec<(usize, usize)>`
+    * CairoRunner::read_return_values now receives a mutable reference to VirtualMachine
+    Bugfixes:
+    * CairoRunner::read_return_values now updates the `stop_ptr` of each builtin after calling `BuiltinRunner::final_stack`
 
 * Refactor `Refactor verify_secure_runner` [#768](https://github.com/lambdaclass/cairo-rs/pull/768)
     Public Api changes:

diff --git a/bench/criterion_benchmark.rs b/bench/criterion_benchmark.rs
@@ -37,6 +37,7 @@ pub fn criterion_benchmarks(c: &mut Criterion) {
                     false,
                     "all",
                     false,
+                    None,
                     &mut hint_executor,
                 )
             })

diff --git a/bench/iai_benchmark.rs b/bench/iai_benchmark.rs
@@ -23,6 +23,7 @@ macro_rules! iai_bench_expand_prog {
                 false,
                 "all",
                 false,
+                None,
                 &mut hint_executor,
             )
         }

diff --git a/cairo_programs/bad_programs/ec_op_not_in_curve.cairo b/cairo_programs/bad_programs/ec_op_not_in_curve.cairo
@@ -24,8 +24,6 @@ func test_ec_op_point_not_on_curve{
     assert ec_op_ptr[0].p = p;
     assert ec_op_ptr[0].q = EcPoint(x=p.x, y=p.y + 1);
     assert ec_op_ptr[0].m = 7;
-    assert ec_op_ptr[0].r.x = ec_op_ptr[0].r.x;
-    assert ec_op_ptr[0].r.y = ec_op_ptr[0].r.y;
     let ec_op_ptr = &ec_op_ptr[1];
     return ();
 }

diff --git a/cairo_programs/bad_programs/ec_op_same_x.cairo b/cairo_programs/bad_programs/ec_op_same_x.cairo
@@ -33,8 +33,6 @@ func test_ec_op_invalid_input{
         0x4fad269cbf860980e38768fe9cb6b0b9ab03ee3fe84cfde2eccce597c874fd8,
         );
     assert ec_op_ptr[0].m = 8;
-    assert ec_op_ptr[0].r.x = ec_op_ptr[0].r.x;
-    assert ec_op_ptr[0].r.y = ec_op_ptr[0].r.y;
     let ec_op_ptr = &ec_op_ptr[1];
     return ();
 }

diff --git a/src/cairo_run.rs b/src/cairo_run.rs
@@ -6,6 +6,7 @@ use crate::{
             cairo_run_errors::CairoRunError, runner_errors::RunnerError, vm_exception::VmException,
         },
         runners::cairo_runner::CairoRunner,
+        security::verify_secure_runner,
         trace::trace_entry::RelocatedTraceEntry,
         vm_core::VirtualMachine,
     },
@@ -17,20 +18,24 @@ use std::{
     path::Path,
 };
 
+#[allow(clippy::too_many_arguments)]
 pub fn cairo_run(
     path: &Path,
     entrypoint: &str,
     trace_enabled: bool,
     print_output: bool,
     layout: &str,
     proof_mode: bool,
+    secure_run: Option<bool>,
     hint_executor: &mut dyn HintProcessor,
 ) -> Result<CairoRunner, CairoRunError> {
     let program = match Program::from_file(path, Some(entrypoint)) {
         Ok(program) => program,
         Err(error) => return Err(CairoRunError::Program(error)),
     };
 
+    let secure_run = secure_run.unwrap_or(!proof_mode);
+
     let mut cairo_runner = CairoRunner::new(&program, layout, proof_mode)?;
     let mut vm = VirtualMachine::new(trace_enabled);
     let end = cairo_runner.initialize(&mut vm)?;
@@ -41,10 +46,13 @@ pub fn cairo_run(
     cairo_runner.end_run(false, false, &mut vm, hint_executor)?;
 
     vm.verify_auto_deductions()?;
+    cairo_runner.read_return_values(&mut vm)?;
     if proof_mode {
-        cairo_runner.read_return_values(&mut vm)?;
         cairo_runner.finalize_segments(&mut vm)?;
     }
+    if secure_run {
+        verify_secure_runner(&cairo_runner, true, &mut vm)?;
+    }
     cairo_runner.relocate(&mut vm)?;
 
     if print_output {
@@ -212,6 +220,7 @@ mod tests {
             false,
             "plain",
             false,
+            None,
             &mut hint_processor
         )
         .is_err());
@@ -230,6 +239,7 @@ mod tests {
             false,
             "plain",
             false,
+            None,
             &mut hint_processor
         )
         .is_err());
@@ -248,6 +258,7 @@ mod tests {
             false,
             "plain",
             false,
+            None,
             &mut hint_processor
         )
         .is_err());

diff --git a/src/main.rs b/src/main.rs
@@ -32,6 +32,8 @@ struct Args {
     layout: String,
     #[structopt(long = "--proof_mode")]
     proof_mode: bool,
+    #[structopt(long = "--secure_run")]
+    secure_run: Option<bool>,
 }
 
 fn validate_layout(value: &str) -> Result<(), String> {
@@ -54,6 +56,7 @@ fn main() -> Result<(), CairoRunError> {
         args.print_output,
         &args.layout,
         args.proof_mode,
+        args.secure_run,
         &mut hint_executor,
     ) {
         Ok(runner) => runner,

diff --git a/src/vm/errors/runner_errors.rs b/src/vm/errors/runner_errors.rs
@@ -52,7 +52,7 @@ pub enum RunnerError {
     #[error("{0}")]
     EcOpSameXCoordinate(String),
     #[error("EcOpBuiltin: point {0:?} is not on the curve")]
-    PointNotOnCurve((usize, usize)),
+    PointNotOnCurve((Felt, Felt)),
     #[error("Builtin(s) {0:?} not present in layout {1}")]
     NoBuiltinForInstance(HashSet<String>, String),
     #[error("Invalid layout {0}")]

diff --git a/src/vm/runners/builtin_runner/ec_op.rs b/src/vm/runners/builtin_runner/ec_op.rs
@@ -181,14 +181,17 @@ impl EcOpBuiltinRunner {
         }*/
 
         // Assert that if the current address is part of a point, the point is on the curve
-        for pair in &EC_POINT_INDICES[0..1] {
+        for pair in &EC_POINT_INDICES[0..2] {
             if !EcOpBuiltinRunner::point_on_curve(
                 input_cells[pair.0].as_ref(),
                 input_cells[pair.1].as_ref(),
                 &alpha,
                 &beta,
             ) {
-                return Err(RunnerError::PointNotOnCurve(*pair));
+                return Err(RunnerError::PointNotOnCurve((
+                    input_cells[pair.0].clone().into_owned(),
+                    input_cells[pair.1].clone().into_owned(),
+                )));
             };
         }
         let prime = BigInt::from_str_radix(&felt::PRIME_STR[2..], 16)
@@ -299,7 +302,7 @@ impl EcOpBuiltinRunner {
         m: num_bigint::BigInt,
         q: (num_bigint::BigInt, num_bigint::BigInt),
     ) -> String {
-        format!("Cannot apply EC operation: computation reched two points with the same x coordinate. \n
+        format!("Cannot apply EC operation: computation reached two points with the same x coordinate. \n
     Attempting to compute P + m * Q where:\n
     P = {p:?} \n
     m = {m:?}\n
@@ -313,6 +316,8 @@ mod tests {
     use crate::hint_processor::builtin_hint_processor::builtin_hint_processor_definition::BuiltinHintProcessor;
     use crate::types::program::Program;
     use crate::utils::test_utils::*;
+    use crate::vm::errors::cairo_run_errors::CairoRunError;
+    use crate::vm::errors::vm_errors::VirtualMachineError;
     use crate::vm::runners::cairo_runner::CairoRunner;
     use crate::vm::{
         errors::{memory_errors::MemoryError, runner_errors::RunnerError},
@@ -1032,38 +1037,50 @@ mod tests {
     }
 
     #[test]
-    fn catch_point_not_in_curve() {
-        let program = Program::from_file(
-            Path::new("cairo_programs/bad_programs/ec_op_not_in_curve.json"),
-            Some("main"),
-        )
-        .expect("Call to `Program::from_file()` failed.");
-
-        let mut hint_processor = BuiltinHintProcessor::new_empty();
-        let mut cairo_runner = cairo_runner!(program, "all", false);
-        let mut vm = vm!();
-
-        let end = cairo_runner.initialize(&mut vm).unwrap();
-        assert!(cairo_runner
-            .run_until_pc(end, &mut vm, &mut hint_processor)
-            .is_err());
+    fn catch_point_same_x() {
+        let program = Path::new("cairo_programs/bad_programs/ec_op_same_x.json");
+        let result = crate::cairo_run::cairo_run(
+            program,
+            "main",
+            false,
+            false,
+            "all",
+            false,
+            None,
+            &mut BuiltinHintProcessor::new_empty(),
+        );
+        assert!(result.is_err());
+        // We need to check this way because CairoRunError doens't implement PartialEq
+        match result {
+            Err(CairoRunError::VirtualMachine(VirtualMachineError::RunnerError(
+                RunnerError::EcOpSameXCoordinate(_),
+            ))) => {}
+            Err(_) => panic!("Wrong error returned, expected RunnerError::EcOpSameXCoordinate"),
+            Ok(_) => panic!("Expected run to fail"),
+        }
     }
 
     #[test]
-    fn catch_point_same_x() {
-        let program = Program::from_file(
-            Path::new("cairo_programs/bad_programs/ec_op_same_x.json"),
-            Some("main"),
-        )
-        .expect("Call to `Program::from_file()` failed.");
-
-        let mut hint_processor = BuiltinHintProcessor::new_empty();
-        let mut cairo_runner = cairo_runner!(program, "all", false);
-        let mut vm = vm!();
-
-        let end = cairo_runner.initialize(&mut vm).unwrap();
-        assert!(cairo_runner
-            .run_until_pc(end, &mut vm, &mut hint_processor)
-            .is_err());
+    fn catch_point_not_in_curve() {
+        let program = Path::new("cairo_programs/bad_programs/ec_op_not_in_curve.json");
+        let result = crate::cairo_run::cairo_run(
+            program,
+            "main",
+            false,
+            false,
+            "all",
+            false,
+            None,
+            &mut BuiltinHintProcessor::new_empty(),
+        );
+        assert!(result.is_err());
+        // We need to check this way because CairoRunError doens't implement PartialEq
+        match result {
+            Err(CairoRunError::VirtualMachine(VirtualMachineError::RunnerError(
+                RunnerError::PointNotOnCurve(_),
+            ))) => {}
+            Err(_) => panic!("Wrong error returned, expected RunnerError::EcOpSameXCoordinate"),
+            Ok(_) => panic!("Expected run to fail"),
+        }
     }
 }
diff --git a/src/vm/runners/cairo_runner.rs b/src/vm/runners/cairo_runner.rs
@@ -1093,17 +1093,19 @@ impl CairoRunner {
         if self.segments_finalized {
             return Err(RunnerError::FailedAddingReturnValues);
         }
-        let exec_base = *self
-            .execution_base
-            .as_ref()
-            .ok_or(RunnerError::NoExecBase)?;
-        let begin = pointer.offset - exec_base.offset;
-        let ap = vm.get_ap();
-        let end = ap.offset - exec_base.offset;
-        self.execution_public_memory
-            .as_mut()
-            .ok_or(RunnerError::NoExecPublicMemory)?
-            .extend(begin..end);
+        if self.proof_mode {
+            let exec_base = *self
+                .execution_base
+                .as_ref()
+                .ok_or(RunnerError::NoExecBase)?;
+            let begin = pointer.offset - exec_base.offset;
+            let ap = vm.get_ap();
+            let end = ap.offset - exec_base.offset;
+            self.execution_public_memory
+                .as_mut()
+                .ok_or(RunnerError::NoExecPublicMemory)?
+                .extend(begin..end);
+        }
         Ok(())
     }
 
@@ -4140,6 +4142,7 @@ mod tests {
             vec![],
         ];
         vm.set_ap(2);
+        // We use 5 as bitwise builtin's segment size as a bitwise instance is 5 cells
         vm.segments.segment_used_sizes = Some(vec![0, 2, 0, 5]);
         //Check values written by first call to segments.finalize()
         assert_eq!(cairo_runner.read_return_values(&mut vm), Ok(()));
-Original file line number
+Diff line change
@@ Expand Up / @@ -37,6 +37,7 @@ pub fn criterion_benchmarks(c: &mut Criterion) { @@
                         false,
                         "all",
                         false,
+                        None,
                         &mut hint_executor,
                     )
                 })
@@ Expand Down @@