From 7698c7618309c59b367152f9533511ed597ab01b Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Thu, 21 Dec 2023 15:19:32 +0100 Subject: [PATCH 01/22] Fix: make the project compile Some issues occurred when compiling the project with the Madara prover API. --- vm/src/hint_processor/builtin_hint_processor/bootloader/mod.rs | 2 +- vm/src/hint_processor/builtin_hint_processor/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/mod.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/mod.rs index aedfca2518..21e02f8186 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/mod.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/mod.rs @@ -7,5 +7,5 @@ mod program_hash; mod program_loader; pub(crate) mod select_builtins; pub(crate) mod simple_bootloader_hints; -pub(crate) mod types; +pub mod types; pub(crate) mod vars; diff --git a/vm/src/hint_processor/builtin_hint_processor/mod.rs b/vm/src/hint_processor/builtin_hint_processor/mod.rs index 271ad444bc..76d5c78854 100644 --- a/vm/src/hint_processor/builtin_hint_processor/mod.rs +++ b/vm/src/hint_processor/builtin_hint_processor/mod.rs @@ -1,7 +1,7 @@ pub mod bigint; pub mod blake2s_hash; pub mod blake2s_utils; -mod bootloader; +pub mod bootloader; pub mod builtin_hint_processor_definition; pub mod cairo_keccak; pub mod dict_hint_utils; From 7c3c33840bb90cdf43631c6c94abc49991ec6221 Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Thu, 21 Dec 2023 15:43:10 +0100 Subject: [PATCH 02/22] Fix: use correct type when setting `simple_bootloader_input` Store the `simple_bootloader_input` field of the bootloader input instead of storing the whole bootloader_input. --- .../bootloader/bootloader_hints.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/bootloader_hints.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/bootloader_hints.rs index ff26da9814..d8a9a5b136 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/bootloader_hints.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/bootloader_hints.rs @@ -87,7 +87,10 @@ pub fn prepare_simple_bootloader_output_segment( /// Implements %{ simple_bootloader_input = bootloader_input %} pub fn prepare_simple_bootloader_input(exec_scopes: &mut ExecutionScopes) -> Result<(), HintError> { let bootloader_input: BootloaderInput = exec_scopes.get(vars::BOOTLOADER_INPUT)?; - exec_scopes.insert_value(vars::SIMPLE_BOOTLOADER_INPUT, bootloader_input); + exec_scopes.insert_value( + vars::SIMPLE_BOOTLOADER_INPUT, + bootloader_input.simple_bootloader_input, + ); Ok(()) } @@ -564,10 +567,13 @@ mod tests { prepare_simple_bootloader_input(&mut exec_scopes).expect("Hint failed unexpectedly"); - let simple_bootloader_input: BootloaderInput = exec_scopes + let simple_bootloader_input: SimpleBootloaderInput = exec_scopes .get(vars::SIMPLE_BOOTLOADER_INPUT) .expect("Simple bootloader input not in scope"); - assert_eq!(simple_bootloader_input, bootloader_input); + assert_eq!( + simple_bootloader_input, + bootloader_input.simple_bootloader_input + ); } #[test] From 7b5b3dfcc06d28075da6f8a4ea230dbb3f67b91b Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Thu, 21 Dec 2023 15:47:16 +0100 Subject: [PATCH 03/22] Fix: use correct hint code for %{ 0 %} The hint is compiled to a different code. --- .../bootloader/simple_bootloader_hints.rs | 2 +- vm/src/hint_processor/builtin_hint_processor/hint_code.rs | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/simple_bootloader_hints.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/simple_bootloader_hints.rs index 65bdcbe499..0cad6261ff 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/simple_bootloader_hints.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/simple_bootloader_hints.rs @@ -91,7 +91,7 @@ pub fn divide_num_by_2( Ok(()) } -/// Implements %{ 0 %}. +/// Implements %{ 0 %} (compiled to %{ memory[ap] = to_felt_or_relocatable(0) %}). /// /// Stores 0 in the AP and returns. /// Used as `tempvar use_poseidon = nondet %{ 0 %}`. diff --git a/vm/src/hint_processor/builtin_hint_processor/hint_code.rs b/vm/src/hint_processor/builtin_hint_processor/hint_code.rs index bfbb95740e..895414f7d7 100644 --- a/vm/src/hint_processor/builtin_hint_processor/hint_code.rs +++ b/vm/src/hint_processor/builtin_hint_processor/hint_code.rs @@ -1534,7 +1534,8 @@ pub const SIMPLE_BOOTLOADER_SET_CURRENT_TASK: &str = task_id = len(simple_bootloader_input.tasks) - ids.n_tasks task = simple_bootloader_input.tasks[task_id].load_task()"; -pub const SIMPLE_BOOTLOADER_ZERO: &str = "0"; +// Appears as nondet %{ 0 %} in the code. +pub const SIMPLE_BOOTLOADER_ZERO: &str = "memory[ap] = to_felt_or_relocatable(0)"; pub const EXECUTE_TASK_ALLOCATE_PROGRAM_DATA_SEGMENT: &str = "ids.program_data_ptr = program_data_base = segments.add()"; From 8d6ccf6f4944f3f86d0e9041470032b2f32dc1ad Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Thu, 21 Dec 2023 15:57:59 +0100 Subject: [PATCH 04/22] Fix: use correct hint code for validate_hash A backslash was encoded the wrong way in the hint code. --- vm/src/hint_processor/builtin_hint_processor/hint_code.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/hint_code.rs b/vm/src/hint_processor/builtin_hint_processor/hint_code.rs index 895414f7d7..bafa62f842 100644 --- a/vm/src/hint_processor/builtin_hint_processor/hint_code.rs +++ b/vm/src/hint_processor/builtin_hint_processor/hint_code.rs @@ -1552,7 +1552,7 @@ segments.finalize(program_data_base.segment_index, program_data_size)"; pub const EXECUTE_TASK_VALIDATE_HASH: &str = "# Validate hash. from starkware.cairo.bootloaders.hash_program import compute_program_hash_chain -assert memory[ids.output_ptr + 1] == compute_program_hash_chain(task.get_program()), \ +assert memory[ids.output_ptr + 1] == compute_program_hash_chain(task.get_program()), \\ 'Computed hash does not match input.'"; pub const EXECUTE_TASK_ASSERT_PROGRAM_ADDRESS: &str = "# Sanity check. From baac9efb714f8bfb968eb614364e83bd8c56e868 Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Thu, 21 Dec 2023 11:00:48 +0100 Subject: [PATCH 05/22] Inner select builtins hint: select builtin Implement the hint that determines whether a specific builtin is selected for the given program. --- .../bootloader/execute_task_hints.rs | 2 +- .../builtin_hint_processor_definition.rs | 3 +++ .../hint_processor/builtin_hint_processor/hint_code.rs | 9 +++++++++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs index d36cdcd9f6..139d477aaa 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs @@ -503,7 +503,7 @@ mod tests { use crate::types::relocatable::Relocatable; use crate::utils::test_utils::*; use crate::vm::runners::builtin_runner::{BuiltinRunner, OutputBuiltinRunner}; - use crate::vm::runners::cairo_pie::{BuiltinAdditionalData, PublicMemoryPage}; + use crate::vm::runners::cairo_pie::{BuiltinAdditionalData, CairoPie, PublicMemoryPage}; use super::*; diff --git a/vm/src/hint_processor/builtin_hint_processor/builtin_hint_processor_definition.rs b/vm/src/hint_processor/builtin_hint_processor/builtin_hint_processor_definition.rs index 85a1bbff8d..3e588edf95 100644 --- a/vm/src/hint_processor/builtin_hint_processor/builtin_hint_processor_definition.rs +++ b/vm/src/hint_processor/builtin_hint_processor/builtin_hint_processor_definition.rs @@ -931,6 +931,9 @@ impl HintProcessorLogic for BuiltinHintProcessor { &hint_data.ids_data, &hint_data.ap_tracking, ), + hint_code::INNER_SELECT_BUILTINS_SELECT_BUILTIN => { + select_builtin(vm, exec_scopes, &hint_data.ids_data, &hint_data.ap_tracking) + } #[cfg(feature = "skip_next_instruction_hint")] hint_code::SKIP_NEXT_INSTRUCTION => skip_next_instruction(vm), #[cfg(feature = "print")] diff --git a/vm/src/hint_processor/builtin_hint_processor/hint_code.rs b/vm/src/hint_processor/builtin_hint_processor/hint_code.rs index bafa62f842..489ca127b2 100644 --- a/vm/src/hint_processor/builtin_hint_processor/hint_code.rs +++ b/vm/src/hint_processor/builtin_hint_processor/hint_code.rs @@ -1631,3 +1631,12 @@ if ids.select_builtin: pub const SELECT_BUILTINS_ENTER_SCOPE: &str = "vm_enter_scope({'n_selected_builtins': ids.n_selected_builtins})"; + +pub const INNER_SELECT_BUILTINS_SELECT_BUILTIN: &str = + "# A builtin should be selected iff its encoding appears in the selected encodings list +# and the list wasn't exhausted. +# Note that testing inclusion by a single comparison is possible since the lists are sorted. +ids.select_builtin = int( + n_selected_builtins > 0 and memory[ids.selected_encodings] == memory[ids.all_encodings]) +if ids.select_builtin: + n_selected_builtins = n_selected_builtins - 1"; From 9cdcdc54c0ce6ed090cfe0d73d21abb80f07523e Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Thu, 21 Dec 2023 18:12:27 +0100 Subject: [PATCH 06/22] Fix: consider output_ptr as a relocatable instead of a value `pre_execution_builtin_ptrs` is an array of pointers, not of values as we previously understood. --- .../bootloader/bootloader_hints.rs | 4 ++-- .../bootloader/execute_task_hints.rs | 12 ++++-------- vm/src/vm/runners/builtin_runner/output.rs | 8 ++++++++ 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/bootloader_hints.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/bootloader_hints.rs index d8a9a5b136..b613a5476b 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/bootloader_hints.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/bootloader_hints.rs @@ -580,8 +580,8 @@ mod tests { fn test_restore_bootloader_output() { let mut vm: VirtualMachine = vm!(); // The VM must have an existing output segment - vm.builtin_runners = - vec![OutputBuiltinRunner::from_segment(&vm.add_memory_segment(), true).into()]; + let output_segment = vm.add_memory_segment(); + vm.builtin_runners = vec![OutputBuiltinRunner::from_segment(&output_segment, true).into()]; let mut exec_scopes = ExecutionScopes::new(); let new_segment = vm.add_memory_segment(); diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs index 139d477aaa..4ba7978c40 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs @@ -432,12 +432,8 @@ pub fn call_task( // output_ptr=ids.pre_execution_builtin_ptrs.output) let pre_execution_builtin_ptrs_addr = get_relocatable_from_var_name(vars::PRE_EXECUTION_BUILTIN_PTRS, vm, ids_data, ap_tracking)?; - let output = vm - .get_integer(pre_execution_builtin_ptrs_addr)? - .into_owned(); - let output_ptr = output - .to_usize() - .ok_or(MathError::Felt252ToUsizeConversion(Box::new(output)))?; + // The output field is the first one in the BuiltinData struct + let output_ptr = vm.get_relocatable(&pre_execution_builtin_ptrs_addr + 0)?; let output_runner_data = util::prepare_output_runner(&task, vm.get_output_builtin()?, output_ptr)?; @@ -465,7 +461,7 @@ mod util { pub(crate) fn prepare_output_runner( task: &Task, output_builtin: &mut OutputBuiltinRunner, - output_ptr: usize, + output_ptr: Relocatable, ) -> Result, HintError> { return match task { Task::Program(_) => { @@ -477,7 +473,7 @@ mod util { .into_boxed_str(), )), }?; - output_builtin.base = output_ptr; + output_builtin.new_state(output_ptr.segment_index as usize, true); Ok(Some(output_state)) } Task::Pie(_) => Ok(None), diff --git a/vm/src/vm/runners/builtin_runner/output.rs b/vm/src/vm/runners/builtin_runner/output.rs index 19aaf36e85..a0b6559014 100644 --- a/vm/src/vm/runners/builtin_runner/output.rs +++ b/vm/src/vm/runners/builtin_runner/output.rs @@ -41,6 +41,14 @@ impl OutputBuiltinRunner { } } + pub fn new_state(&mut self, base: usize, included: bool) { + self.base = base; + self.pages = HashMap::default(); + self.attributes = HashMap::default(); + self.stop_ptr = None; + self.included = included; + } + pub fn initialize_segments(&mut self, segments: &mut MemorySegmentManager) { self.base = segments.add().segment_index as usize // segments.add() always returns a positive index } From 0e7c7817d005c4c62e0d608c487c5e73cc4b96f9 Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Tue, 26 Dec 2023 22:45:22 +0100 Subject: [PATCH 07/22] Fix: do not force integer type when copying builtins Problem: `write_return_builtins` copies data from the pre-execution builtins for each builtin not used by the program. We specify that the value of the pre-execution builtins is integer, which is not the case. Solution: just copy the memory without enforcing a specific type. --- .../bootloader/execute_task_hints.rs | 14 ++++++++++---- .../builtin_hint_processor_definition.rs | 3 --- .../builtin_hint_processor/hint_code.rs | 9 --------- 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs index 4ba7978c40..e269de217b 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs @@ -24,6 +24,7 @@ use crate::types::errors::math_errors::MathError; use crate::types::exec_scope::ExecutionScopes; use crate::types::relocatable::Relocatable; use crate::vm::errors::hint_errors::HintError; +use crate::vm::errors::memory_errors::MemoryError; use crate::vm::runners::cairo_pie::{CairoPie, OutputBuiltinAdditionalData, StrippedProgram}; use crate::vm::vm_core::VirtualMachine; use crate::vm::vm_memory::memory::Memory; @@ -273,8 +274,12 @@ fn write_return_builtins( } // The builtin is unused, hence its value is the same as before calling the program. else { + let pre_execution_builtin_addr = pre_execution_builtins_addr + index; let pre_execution_value = memory - .get_integer(pre_execution_builtins_addr + index)? + .get(&pre_execution_builtin_addr) + .ok_or_else(|| { + MemoryError::UnknownMemoryCell(Box::new(pre_execution_builtin_addr)) + })? .into_owned(); memory.insert_value(return_builtins_addr + index, pre_execution_value)?; } @@ -387,7 +392,8 @@ pub fn call_task( let task: Task = exec_scopes.get(vars::TASK)?; // n_builtins = len(task.get_program().builtins) - let num_builtins = get_program_from_task(&task)?.builtins.len(); + let n_builtins = get_program_from_task(&task)?.builtins.len(); + exec_scopes.insert_value(vars::N_BUILTINS, n_builtins); let mut new_task_locals = HashMap::new(); @@ -418,7 +424,7 @@ pub fn call_task( cairo_pie, vm, program_address, - (vm.get_ap() - num_builtins)?, + (vm.get_ap() - n_builtins)?, vm.get_fp(), vm.get_pc(), ) @@ -721,7 +727,7 @@ mod tests { pages: HashMap::new(), attributes: HashMap::new(), }; - exec_scopes.insert_value(vars::OUTPUT_RUNNER_DATA, output_runner_data.clone()); + exec_scopes.insert_value(vars::OUTPUT_RUNNER_DATA, Some(output_runner_data.clone())); exec_scopes.insert_value(vars::TASK, task); exec_scopes.insert_value(vars::FACT_TOPOLOGIES, Vec::::new()); diff --git a/vm/src/hint_processor/builtin_hint_processor/builtin_hint_processor_definition.rs b/vm/src/hint_processor/builtin_hint_processor/builtin_hint_processor_definition.rs index 3e588edf95..0a114a8a94 100644 --- a/vm/src/hint_processor/builtin_hint_processor/builtin_hint_processor_definition.rs +++ b/vm/src/hint_processor/builtin_hint_processor/builtin_hint_processor_definition.rs @@ -922,9 +922,6 @@ impl HintProcessorLogic for BuiltinHintProcessor { hint_code::EXECUTE_TASK_APPEND_FACT_TOPOLOGIES => { append_fact_topologies(vm, exec_scopes, &hint_data.ids_data, &hint_data.ap_tracking) } - hint_code::INNER_SELECT_BUILTINS_SELECT_BUILTIN => { - select_builtin(vm, exec_scopes, &hint_data.ids_data, &hint_data.ap_tracking) - } hint_code::SELECT_BUILTINS_ENTER_SCOPE => select_builtins_enter_scope( vm, exec_scopes, diff --git a/vm/src/hint_processor/builtin_hint_processor/hint_code.rs b/vm/src/hint_processor/builtin_hint_processor/hint_code.rs index 489ca127b2..1693e849df 100644 --- a/vm/src/hint_processor/builtin_hint_processor/hint_code.rs +++ b/vm/src/hint_processor/builtin_hint_processor/hint_code.rs @@ -1620,15 +1620,6 @@ fact_topologies.append(get_task_fact_topology( output_runner_data=output_runner_data, ))"; -pub const INNER_SELECT_BUILTINS_SELECT_BUILTIN: &str = - "# A builtin should be selected iff its encoding appears in the selected encodings list -# and the list wasn't exhausted. -# Note that testing inclusion by a single comparison is possible since the lists are sorted. -ids.select_builtin = int( - n_selected_builtins > 0 and memory[ids.selected_encodings] == memory[ids.all_encodings]) -if ids.select_builtin: - n_selected_builtins = n_selected_builtins - 1"; - pub const SELECT_BUILTINS_ENTER_SCOPE: &str = "vm_enter_scope({'n_selected_builtins': ids.n_selected_builtins})"; From 4a4d1fb650abb714f68ac5e8b3ae14a65f0f9cbd Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Tue, 26 Dec 2023 23:08:49 +0100 Subject: [PATCH 08/22] Fix: output_runner_data is an optional Implemented `get_task_fact_topologies` for Cairo PIE tasks at the same time. --- .../bootloader/execute_task_hints.rs | 8 ++-- .../bootloader/fact_topologies.rs | 42 ++++++++++++++++++- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs index e269de217b..d49b09b80e 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs @@ -8,7 +8,7 @@ use crate::Felt252; use crate::any_box; use crate::hint_processor::builtin_hint_processor::bootloader::fact_topologies::{ - get_program_task_fact_topology, FactTopology, + get_task_fact_topology, FactTopology, }; use crate::hint_processor::builtin_hint_processor::bootloader::load_cairo_pie::load_cairo_pie; use crate::hint_processor::builtin_hint_processor::bootloader::program_hash::compute_program_hash_chain; @@ -120,7 +120,7 @@ pub fn append_fact_topologies( ap_tracking: &ApTracking, ) -> Result<(), HintError> { let task: Task = exec_scopes.get(vars::TASK)?; - let output_runner_data: OutputBuiltinAdditionalData = + let output_runner_data: Option = exec_scopes.get(vars::OUTPUT_RUNNER_DATA)?; let fact_topologies: &mut Vec = exec_scopes.get_mut_ref(vars::FACT_TOPOLOGIES)?; @@ -145,7 +145,7 @@ pub fn append_fact_topologies( let output_builtin = vm.get_output_builtin()?; let fact_topology = - get_program_task_fact_topology(output_size, &task, output_builtin, output_runner_data) + get_task_fact_topology(output_size, &task, output_builtin, output_runner_data) .map_err(Into::::into)?; fact_topologies.push(fact_topology); @@ -443,7 +443,7 @@ pub fn call_task( let output_runner_data = util::prepare_output_runner(&task, vm.get_output_builtin()?, output_ptr)?; - exec_scopes.insert_box(vars::OUTPUT_RUNNER_DATA, any_box!(output_runner_data)); + exec_scopes.insert_value(vars::OUTPUT_RUNNER_DATA, output_runner_data); exec_scopes.enter_scope(new_task_locals); diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/fact_topologies.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/fact_topologies.rs index ca3dc8dd0f..e5d65b4aa3 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/fact_topologies.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/fact_topologies.rs @@ -75,6 +75,9 @@ pub enum FactTopologyError { #[error("Could not add page to output: {0}")] FailedToAddOutputPage(#[from] RunnerError), + #[error("Could not load output builtin additional data from Cairo PIE")] + CairoPieHasNoOutputBuiltinData, + #[error("Unexpected error: {0}")] Internal(Box), } @@ -333,9 +336,8 @@ fn get_fact_topology_from_additional_data( } // TODO: implement for CairoPieTask -pub fn get_program_task_fact_topology( +fn get_program_task_fact_topology( output_size: usize, - _task: &Task, output_builtin: &mut OutputBuiltinRunner, output_runner_data: OutputBuiltinAdditionalData, ) -> Result { @@ -357,6 +359,42 @@ pub fn get_program_task_fact_topology( Ok(fact_topology) } +pub fn get_task_fact_topology( + output_size: usize, + task: &Task, + output_builtin: &mut OutputBuiltinRunner, + output_runner_data: Option, +) -> Result { + match task { + Task::Program(_program) => { + let output_runner_data = output_runner_data.ok_or(FactTopologyError::Internal( + "Output runner data not set for program task" + .to_string() + .into_boxed_str(), + ))?; + get_program_task_fact_topology(output_size, output_builtin, output_runner_data) + } + Task::Pie(cairo_pie) => { + if let Some(_) = output_runner_data { + return Err(FactTopologyError::Internal( + "Output runner data set for Cairo PIE task" + .to_string() + .into_boxed_str(), + )); + } + let additional_data = cairo_pie + .additional_data + .get("output_builtin") + .ok_or(FactTopologyError::CairoPieHasNoOutputBuiltinData)?; + let additional_data = match additional_data { + BuiltinAdditionalData::Output(output_builtin_data) => output_builtin_data, + _ => return Err(FactTopologyError::CairoPieHasNoOutputBuiltinData), + }; + get_fact_topology_from_additional_data(output_size, additional_data) + } + } +} + /// Writes fact topologies to a file, as JSON. /// /// * `path`: File path. From b1055e4a077ebe642c4335f2205382ebedbd7d42 Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Tue, 26 Dec 2023 23:13:00 +0100 Subject: [PATCH 09/22] Fix tests: use relocatables for builtin pointers --- .../builtin_hint_processor/bootloader/execute_task_hints.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs index d49b09b80e..ece8e53e2f 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs @@ -617,7 +617,7 @@ mod tests { // Allocate space for pre-execution (8 felts), which mimics the `BuiltinData` struct in the // Bootloader's Cairo code. Our code only uses the first felt (`output` field in the struct) - vm.segments = segments![((1, 0), 0)]; + vm.segments = segments![((1, 0), (2, 0))]; vm.run_context.fp = 8; add_segments!(vm, 1); @@ -653,7 +653,7 @@ mod tests { // the Bootloader Cairo code. Our code only uses the first felt (`output` field in the // struct). Finally, we put the mocked output of `select_input_builtins` in the next // memory address and increase the AP register accordingly. - vm.segments = segments![((1, 0), (2, 0)), ((1, 1), 42), ((1, 9), (4, 0))]; + vm.segments = segments![((1, 0), (2, 0)), ((1, 1), (4, 0)), ((1, 9), (4, 42))]; vm.run_context.ap = 10; vm.run_context.fp = 9; add_segments!(vm, 3); From 615866bb3a31a851c7e400bd656d3722d87949a9 Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Tue, 26 Dec 2023 23:36:08 +0100 Subject: [PATCH 10/22] Fix: treat output pointers as relocatables in append_fact_topologies --- .../bootloader/execute_task_hints.rs | 22 +++++-------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs index ece8e53e2f..8b6a736d94 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs @@ -1,7 +1,6 @@ use std::any::Any; use std::collections::HashMap; -use num_traits::ToPrimitive; use starknet_crypto::FieldElement; use crate::Felt252; @@ -20,7 +19,6 @@ use crate::hint_processor::builtin_hint_processor::hint_utils::{ }; use crate::hint_processor::hint_processor_definition::HintReference; use crate::serde::deserialize_program::{ApTracking, BuiltinName}; -use crate::types::errors::math_errors::MathError; use crate::types::exec_scope::ExecutionScopes; use crate::types::relocatable::Relocatable; use crate::vm::errors::hint_errors::HintError; @@ -130,18 +128,9 @@ pub fn append_fact_topologies( get_relocatable_from_var_name("return_builtin_ptrs", vm, ids_data, ap_tracking)?; // The output field is the first one in the BuiltinData struct - let output_start = vm - .get_integer(pre_execution_builtin_ptrs_addr)? - .into_owned(); - let output_end = vm.get_integer(return_builtin_ptrs_addr)?.into_owned(); - let output_size = { - let output_size_felt = output_end - output_start; - output_size_felt - .to_usize() - .ok_or(MathError::Felt252ToUsizeConversion(Box::new( - output_size_felt, - ))) - }?; + let output_start = vm.get_relocatable(pre_execution_builtin_ptrs_addr)?; + let output_end = vm.get_relocatable(return_builtin_ptrs_addr)?; + let output_size = (output_end - output_start)?; let output_builtin = vm.get_output_builtin()?; let fact_topology = @@ -693,8 +682,9 @@ mod tests { // Allocate space for the pre-execution and return builtin structs (2 x 8 felts). // The pre-execution struct starts at (1, 0) and the return struct at (1, 8). - // We only set the output values to 0 and 10, respectively, to get an output size of 10. - vm.segments = segments![((1, 0), 0), ((1, 8), 10),]; + // We only set the output values to (2, 0) and (2, 10), respectively, to get an output size + // of 10. + vm.segments = segments![((1, 0), (2, 0)), ((1, 8), (2, 10)),]; vm.run_context.fp = 16; add_segments!(vm, 1); From 0c5c9dec4808d316055db40ac6712f104cc96673 Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Tue, 26 Dec 2023 23:41:16 +0100 Subject: [PATCH 11/22] Fix: use correct hint code for nondet %{ ids.num // 2 %} The hint is compiled to a different code. --- .../bootloader/simple_bootloader_hints.rs | 4 ++-- vm/src/hint_processor/builtin_hint_processor/hint_code.rs | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/simple_bootloader_hints.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/simple_bootloader_hints.rs index 0cad6261ff..005ae91e4c 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/simple_bootloader_hints.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/simple_bootloader_hints.rs @@ -74,8 +74,8 @@ pub fn set_tasks_variable(exec_scopes: &mut ExecutionScopes) -> Result<(), HintE Ok(()) } -/// Implements -/// %{ ids.num // 2 %} +/// Implements %{ ids.num // 2 %} +/// (compiled to %{ memory[ap] = to_felt_or_relocatable(ids.num // 2) %}). pub fn divide_num_by_2( vm: &mut VirtualMachine, ids_data: &HashMap, diff --git a/vm/src/hint_processor/builtin_hint_processor/hint_code.rs b/vm/src/hint_processor/builtin_hint_processor/hint_code.rs index 1693e849df..c01b881335 100644 --- a/vm/src/hint_processor/builtin_hint_processor/hint_code.rs +++ b/vm/src/hint_processor/builtin_hint_processor/hint_code.rs @@ -1525,7 +1525,9 @@ fact_topologies = []"; pub const SIMPLE_BOOTLOADER_SET_TASKS_VARIABLE: &str = "tasks = simple_bootloader_input.tasks"; -pub const SIMPLE_BOOTLOADER_DIVIDE_NUM_BY_2: &str = "ids.num // 2"; +// Appears as nondet %{ ids.num // 2 %} in the code. +pub const SIMPLE_BOOTLOADER_DIVIDE_NUM_BY_2: &str = + "memory[ap] = to_felt_or_relocatable(ids.num // 2)"; pub const SIMPLE_BOOTLOADER_SET_CURRENT_TASK: &str = "from starkware.cairo.bootloaders.simple_bootloader.objects import Task From 67d3b460588e3b2c7942aaf94e63c3c7d95ae1dd Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Tue, 26 Dec 2023 23:45:45 +0100 Subject: [PATCH 12/22] Fix: use correct hint code for isinstance(packed_output, PlainPackedOutput) The hint is compiled to a different code. --- .../builtin_hint_processor/bootloader/bootloader_hints.rs | 1 + vm/src/hint_processor/builtin_hint_processor/hint_code.rs | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/bootloader_hints.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/bootloader_hints.rs index b613a5476b..f57e49175a 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/bootloader_hints.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/bootloader_hints.rs @@ -229,6 +229,7 @@ pub fn import_packed_output_schemas() -> Result<(), HintError> { } /// Implements %{ isinstance(packed_output, PlainPackedOutput) %} +/// (compiled to %{ memory[ap] = to_felt_or_relocatable(isinstance(packed_output, PlainPackedOutput)) %}). /// /// Stores the result in the `ap` register to be accessed by the program. pub fn is_plain_packed_output( diff --git a/vm/src/hint_processor/builtin_hint_processor/hint_code.rs b/vm/src/hint_processor/builtin_hint_processor/hint_code.rs index c01b881335..553b9a2698 100644 --- a/vm/src/hint_processor/builtin_hint_processor/hint_code.rs +++ b/vm/src/hint_processor/builtin_hint_processor/hint_code.rs @@ -1466,7 +1466,9 @@ pub const BOOTLOADER_IMPORT_PACKED_OUTPUT_SCHEMAS: &str = PlainPackedOutput, )"; -pub const BOOTLOADER_IS_PLAIN_PACKED_OUTPUT: &str = "isinstance(packed_output, PlainPackedOutput)"; +// Appears as nondet %{ isinstance(packed_output, PlainPackedOutput) %} in the code. +pub const BOOTLOADER_IS_PLAIN_PACKED_OUTPUT: &str = + "memory[ap] = to_felt_or_relocatable(isinstance(packed_output, PlainPackedOutput))"; pub const BOOTLOADER_SAVE_OUTPUT_POINTER: &str = "output_start = ids.output_ptr"; From 9fd7790d4d329967479f53f0e4ace868778f60af Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Wed, 3 Jan 2024 15:19:41 +0100 Subject: [PATCH 13/22] Fix: remove useless clone() and commented code --- .../bootloader/program_loader.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/program_loader.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/program_loader.rs index c41d51bdb8..61fed717da 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/program_loader.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/program_loader.rs @@ -35,13 +35,7 @@ fn builtin_to_felt(builtin: &BuiltinName) -> Result .strip_suffix("_builtin") .unwrap_or(builtin.name()); - // let buf = { - // let mut padding: Vec = vec![0; 32 - builtin_name.len()]; - // padding.extend_from_slice(builtin_name.as_bytes()); - // padding - // }; - let buf = builtin_name.as_bytes(); - Ok(Felt252::from_bytes_be_slice(&buf)) + Ok(Felt252::from_bytes_be_slice(builtin_name.as_bytes())) } pub struct LoadedProgram { @@ -200,7 +194,7 @@ mod tests { "Could not decode builtin from memory (expected {})", builtin ) - .as_ref(), + .as_ref(), ); // Compare the last N characters, builtin_from_felt is padded left with zeroes assert_eq!(&builtin_from_felt[32 - builtin.len()..32], builtin); From 640bee94fa6c9f105010042b9e0f79e935b25405 Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Wed, 3 Jan 2024 15:45:45 +0100 Subject: [PATCH 14/22] Fix: hardcode return PC for Cairo PIE --- .../bootloader/execute_task_hints.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs index 8b6a736d94..19a68d2661 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs @@ -405,6 +405,15 @@ pub fn call_task( let program_address: Relocatable = exec_scopes.get("program_address")?; // ret_pc = ids.ret_pc_label.instruction_offset_ - ids.call_task.instruction_offset_ + pc + // TODO: we hardcode the return PC for now, we need a way to access the program + // identifiers to compute this correctly + let ret_pc = Relocatable::from((0, 285)); + // let ret_pc_label = get_ptr_from_var_name("ret_pc_label", vm, ids_data, ap_tracking)?; + // let call_task = get_ptr_from_var_name("call_task", vm, ids_data, ap_tracking)?; + // + // let ret_pc = (ret_pc_label - call_task)?; + // let ret_pc = (vm.run_context.pc + ret_pc)?; + // load_cairo_pie( // task=task.cairo_pie, memory=memory, segments=segments, // program_address=program_address, execution_segment_address= ap - n_builtins, @@ -415,7 +424,7 @@ pub fn call_task( program_address, (vm.get_ap() - n_builtins)?, vm.get_fp(), - vm.get_pc(), + ret_pc, ) .map_err(Into::::into)?; } From 97ebd55efd823eda31faaeb561b6388b3236a11b Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Wed, 3 Jan 2024 15:46:07 +0100 Subject: [PATCH 15/22] Fix: use pointer instead of integer in write_return_builtins, again --- .../bootloader/execute_task_hints.rs | 56 ++++++++++--------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs index 19a68d2661..b70cebe436 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs @@ -204,19 +204,16 @@ fn check_cairo_pie_builtin_usage( return_builtins_addr: &Relocatable, pre_execution_builtins_addr: &Relocatable, ) -> Result<(), HintError> { - let return_builtin_value = memory - .get_integer(return_builtins_addr + builtin_index)? - .into_owned(); - let pre_execution_builtin_value = memory - .get_integer(pre_execution_builtins_addr + builtin_index)? - .into_owned(); - let expected_builtin_size = return_builtin_value - pre_execution_builtin_value; + let return_builtin_value = memory.get_relocatable(return_builtins_addr + builtin_index)?; + let pre_execution_builtin_value = + memory.get_relocatable(pre_execution_builtins_addr + builtin_index)?; + let expected_builtin_size = (return_builtin_value - pre_execution_builtin_value)?; let builtin_name = builtin .name() .strip_suffix("_builtin") .unwrap_or(builtin.name()); - let builtin_size = Felt252::from(cairo_pie.metadata.builtin_segments[builtin_name].size); + let builtin_size = cairo_pie.metadata.builtin_segments[builtin_name].size; if builtin_size != expected_builtin_size { return Err(HintError::AssertionFailed( @@ -244,9 +241,7 @@ fn write_return_builtins( let mut used_builtin_offset: usize = 0; for (index, builtin) in ALL_BUILTINS.iter().enumerate() { if used_builtins.contains(builtin) { - let builtin_value = memory - .get_integer(used_builtins_addr + used_builtin_offset)? - .into_owned(); + let builtin_value = memory.get_relocatable(used_builtins_addr + used_builtin_offset)?; memory.insert_value(return_builtins_addr + index, builtin_value)?; used_builtin_offset += 1; @@ -492,15 +487,13 @@ mod tests { use assert_matches::assert_matches; use rstest::{fixture, rstest}; - use crate::Felt252; - use crate::any_box; use crate::hint_processor::builtin_hint_processor::builtin_hint_processor_definition::BuiltinHintProcessor; use crate::hint_processor::builtin_hint_processor::builtin_hint_processor_definition::HintProcessorData; use crate::hint_processor::builtin_hint_processor::hint_code; use crate::hint_processor::builtin_hint_processor::hint_utils::get_ptr_from_var_name; use crate::hint_processor::hint_processor_definition::HintProcessorLogic; - use crate::types::relocatable::Relocatable; + use crate::types::relocatable::{MaybeRelocatable, Relocatable}; use crate::utils::test_utils::*; use crate::vm::runners::builtin_runner::{BuiltinRunner, OutputBuiltinRunner}; use crate::vm::runners::cairo_pie::{BuiltinAdditionalData, CairoPie, PublicMemoryPage}; @@ -762,16 +755,16 @@ mod tests { // are used by the field arithmetic program. Note that the used builtins list // does not contain empty elements (i.e. offsets are 8 and 9 instead of 10 and 12). vm.segments = segments![ - ((1, 0), 1), - ((1, 1), 2), - ((1, 2), 3), - ((1, 3), 4), - ((1, 4), 5), - ((1, 5), 6), - ((1, 6), 7), - ((1, 7), 8), - ((1, 8), 30), - ((1, 9), 50), + ((1, 0), (2, 1)), + ((1, 1), (2, 2)), + ((1, 2), (2, 3)), + ((1, 3), (2, 4)), + ((1, 4), (2, 5)), + ((1, 5), (2, 6)), + ((1, 6), (2, 7)), + ((1, 7), (2, 8)), + ((1, 8), (2, 30)), + ((1, 9), (2, 50)), ((1, 24), (1, 8)), ]; vm.run_context.fp = 25; @@ -797,12 +790,21 @@ mod tests { let return_builtins = vm .segments .memory - .get_integer_range(Relocatable::from((1, 16)), 8) + .get_continuous_range(Relocatable::from((1, 16)), 8) .expect("Return builtin was not properly written to memory."); - let expected_builtins = vec![1, 2, 30, 4, 50, 6, 7, 8]; + let expected_builtins = vec![ + Relocatable::from((2, 1)), + Relocatable::from((2, 2)), + Relocatable::from((2, 30)), + Relocatable::from((2, 4)), + Relocatable::from((2, 50)), + Relocatable::from((2, 6)), + Relocatable::from((2, 7)), + Relocatable::from((2, 8)), + ]; for (expected, actual) in std::iter::zip(expected_builtins, return_builtins) { - assert_eq!(Felt252::from(expected), actual.into_owned()); + assert_eq!(MaybeRelocatable::RelocatableValue(expected), actual); } // Check that the exec scope changed From 392c4b676fa5fddd5a7058e4cd5b04d0d532e427 Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Thu, 4 Jan 2024 11:45:20 +0100 Subject: [PATCH 16/22] Fix: use program identifiers to determine ret_pc for Cairo PIEs Remove the hardcoded value for `ret_pc` and compute it using the program identifiers instead. We now expect the bootloader program to be accessible as the `bootloader_program` variable in the root execution scope. --- .../bootloader/execute_task_hints.rs | 49 +++++++++++++++---- .../builtin_hint_processor/bootloader/vars.rs | 3 ++ 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs index b70cebe436..2b6980cbcc 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs @@ -18,8 +18,9 @@ use crate::hint_processor::builtin_hint_processor::hint_utils::{ get_ptr_from_var_name, get_relocatable_from_var_name, insert_value_from_var_name, }; use crate::hint_processor::hint_processor_definition::HintReference; -use crate::serde::deserialize_program::{ApTracking, BuiltinName}; +use crate::serde::deserialize_program::{ApTracking, BuiltinName, Identifier}; use crate::types::exec_scope::ExecutionScopes; +use crate::types::program::Program; use crate::types::relocatable::Relocatable; use crate::vm::errors::hint_errors::HintError; use crate::vm::errors::memory_errors::MemoryError; @@ -329,6 +330,33 @@ pub fn write_return_builtins_hint( Ok(()) } +fn get_bootloader_program(exec_scopes: &ExecutionScopes) -> Result<&Program, HintError> { + if let Some(boxed_program) = exec_scopes.data[0].get(vars::BOOTLOADER_PROGRAM) { + if let Some(program) = boxed_program.downcast_ref::() { + return Ok(program); + } + } + + Err(HintError::VariableNotInScopeError( + vars::BOOTLOADER_PROGRAM.to_string().into_boxed_str(), + )) +} + +fn get_identifier( + identifiers: &HashMap, + name: &str, +) -> Result { + if let Some(identifier) = identifiers.get(name) { + if let Some(pc) = identifier.pc { + return Ok(pc); + } + } + + Err(HintError::VariableNotInScopeError( + name.to_string().into_boxed_str(), + )) +} + /* Implements hint: %{ @@ -381,7 +409,6 @@ pub fn call_task( let mut new_task_locals = HashMap::new(); - // TODO: remove clone here when RunProgramTask has proper variant data (not String) match &task { // if isinstance(task, RunProgramTask): Task::Program(_program) => { @@ -400,14 +427,16 @@ pub fn call_task( let program_address: Relocatable = exec_scopes.get("program_address")?; // ret_pc = ids.ret_pc_label.instruction_offset_ - ids.call_task.instruction_offset_ + pc - // TODO: we hardcode the return PC for now, we need a way to access the program - // identifiers to compute this correctly - let ret_pc = Relocatable::from((0, 285)); - // let ret_pc_label = get_ptr_from_var_name("ret_pc_label", vm, ids_data, ap_tracking)?; - // let call_task = get_ptr_from_var_name("call_task", vm, ids_data, ap_tracking)?; - // - // let ret_pc = (ret_pc_label - call_task)?; - // let ret_pc = (vm.run_context.pc + ret_pc)?; + let program = get_bootloader_program(exec_scopes)?; + let identifiers = &program.shared_program_data.identifiers; + let ret_pc_label = get_identifier(identifiers, "starkware.cairo.bootloaders.simple_bootloader.execute_task.execute_task.ret_pc_label")?; + let call_task = get_identifier( + identifiers, + "starkware.cairo.bootloaders.simple_bootloader.execute_task.execute_task.call_task", + )?; + + let ret_pc_offset = ret_pc_label - call_task; + let ret_pc = (vm.run_context.pc + ret_pc_offset)?; // load_cairo_pie( // task=task.cairo_pie, memory=memory, segments=segments, diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/vars.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/vars.rs index c34e8fb143..324fffb84e 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/vars.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/vars.rs @@ -1,6 +1,9 @@ /// Deserialized bootloader input. pub(crate) const BOOTLOADER_INPUT: &str = "bootloader_input"; +/// The bootloader program, as a Program object. +pub(crate) const BOOTLOADER_PROGRAM: &str = "bootloader_program"; + /// Saved state of the output builtin. pub(crate) const OUTPUT_BUILTIN_STATE: &str = "output_builtin_state"; From 25d35b1bc0cab3efbbcb7d88ca83f3d0e23fd36d Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Thu, 11 Jan 2024 10:37:37 +0100 Subject: [PATCH 17/22] Fix: deserialize PIE additional data as struct instead of hashmap Problem: Deserializing the PIE additional data as a hashmap of `BuiltinAdditionalData` enums because of an issue with deserializing untagged unions in `serde` (see https://github.com/serde-rs/json/issues/1103). Solution: add a new `AdditionalData` struct with explicit fields for each builtin, circumventing the untagged union issue. This solution has the advantage of always associating the correct data type for each builtin (it's not possible anymore to associate a builtin with a different data type), but requires modifications if a new builtin is added. --- .../pie_additional_data_test.json | 50 +++++++++ .../bootloader/fact_topologies.rs | 9 +- .../bootloader/load_cairo_pie.rs | 12 +-- vm/src/tests/cairo_pie_test.rs | 41 +++---- vm/src/vm/runners/cairo_pie.rs | 100 ++++++++++++++++-- vm/src/vm/runners/cairo_runner.rs | 5 +- 6 files changed, 167 insertions(+), 50 deletions(-) create mode 100644 cairo_programs/manually_compiled/pie_additional_data_test.json diff --git a/cairo_programs/manually_compiled/pie_additional_data_test.json b/cairo_programs/manually_compiled/pie_additional_data_test.json new file mode 100644 index 0000000000..a0c245b9c1 --- /dev/null +++ b/cairo_programs/manually_compiled/pie_additional_data_test.json @@ -0,0 +1,50 @@ +{ + "output_builtin": { + "pages": { + "1": [ + 18, + 46 + ] + }, + "attributes": { + "gps_fact_topology": [ + 2, + 1, + 0, + 2 + ] + } + }, + "pedersen_builtin": [ + [ + 3, + 2 + ], + [ + 3, + 5 + ], + [ + 3, + 8 + ], + [ + 3, + 11 + ], + [ + 3, + 14 + ], + [ + 3, + 17 + ] + ], + "range_check_builtin": null, + "ecdsa_builtin": [], + "bitwise_builtin": null, + "ec_op_builtin": null, + "keccak_builtin": null, + "poseidon_builtin": null +} \ No newline at end of file diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/fact_topologies.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/fact_topologies.rs index e5d65b4aa3..0430453ce2 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/fact_topologies.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/fact_topologies.rs @@ -384,13 +384,10 @@ pub fn get_task_fact_topology( } let additional_data = cairo_pie .additional_data - .get("output_builtin") + .output_builtin + .as_ref() .ok_or(FactTopologyError::CairoPieHasNoOutputBuiltinData)?; - let additional_data = match additional_data { - BuiltinAdditionalData::Output(output_builtin_data) => output_builtin_data, - _ => return Err(FactTopologyError::CairoPieHasNoOutputBuiltinData), - }; - get_fact_topology_from_additional_data(output_size, additional_data) + get_fact_topology_from_additional_data(output_size, &additional_data) } } } diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/load_cairo_pie.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/load_cairo_pie.rs index 6cc0d063b8..4aa12ea181 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/load_cairo_pie.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/load_cairo_pie.rs @@ -2,7 +2,7 @@ use crate::types::relocatable::{MaybeRelocatable, Relocatable}; use crate::vm::errors::hint_errors::HintError; use crate::vm::errors::memory_errors::MemoryError; use crate::vm::runners::builtin_runner::SignatureBuiltinRunner; -use crate::vm::runners::cairo_pie::{BuiltinAdditionalData, CairoPie, CairoPieMemory}; +use crate::vm::runners::cairo_pie::{CairoPie, CairoPieMemory}; use crate::vm::vm_core::VirtualMachine; use crate::Felt252; use std::collections::HashMap; @@ -28,9 +28,6 @@ pub enum SignatureRelocationError { #[error("The PIE requires ECDSA but the VM is not configured to use it")] EcdsaBuiltinNotFound, - #[error("The data of the Cairo PIE ECDSA builtin does not match the expected type")] - UnexpectedBuiltinDataType, - #[error("Relocated signature data ({0} not on signature builtin segment {1}")] RelocatedDataNotOnBuiltinSegment(Relocatable, isize), } @@ -226,12 +223,9 @@ fn relocate_builtin_additional_data( vm: &mut VirtualMachine, relocation_table: &RelocationTable, ) -> Result<(), SignatureRelocationError> { - let ecdsa_additional_data = match cairo_pie.additional_data.get("ecdsa_builtin") { - Some(additional_data) => match additional_data { - BuiltinAdditionalData::Signature(data) => data, - _ => return Err(SignatureRelocationError::UnexpectedBuiltinDataType), - }, + let ecdsa_additional_data = match &cairo_pie.additional_data.ecdsa_builtin { None => return Ok(()), + Some(data) => data, }; let ecdsa_builtin = vm diff --git a/vm/src/tests/cairo_pie_test.rs b/vm/src/tests/cairo_pie_test.rs index 639b28b93d..769b2f35ea 100644 --- a/vm/src/tests/cairo_pie_test.rs +++ b/vm/src/tests/cairo_pie_test.rs @@ -19,13 +19,12 @@ use crate::{ HASH_BUILTIN_NAME, OUTPUT_BUILTIN_NAME, RANGE_CHECK_BUILTIN_NAME, SIGNATURE_BUILTIN_NAME, }, - cairo_pie::{ - BuiltinAdditionalData, CairoPieMemory, OutputBuiltinAdditionalData, SegmentInfo, - }, + cairo_pie::{CairoPieMemory, OutputBuiltinAdditionalData, SegmentInfo}, cairo_runner::ExecutionResources, }, }; +use crate::vm::runners::cairo_pie::AdditionalData; #[cfg(all(not(feature = "std"), feature = "alloc"))] use alloc::{ string::{String, ToString}, @@ -92,24 +91,15 @@ fn pedersen_test() { }; assert_eq!(cairo_pie.execution_resources, expected_execution_resources); // additional_data - let expected_additional_data = HashMap::from([ - ( - OUTPUT_BUILTIN_NAME.to_string(), - BuiltinAdditionalData::Output(OutputBuiltinAdditionalData { - base: 2, - pages: HashMap::new(), - attributes: HashMap::new(), - }), - ), - ( - HASH_BUILTIN_NAME.to_string(), - BuiltinAdditionalData::Hash(vec![Relocatable::from((3, 2))]), - ), - ( - RANGE_CHECK_BUILTIN_NAME.to_string(), - BuiltinAdditionalData::None, - ), - ]); + let expected_additional_data = AdditionalData { + output_builtin: Some(OutputBuiltinAdditionalData { + base: 2, + pages: HashMap::new(), + attributes: HashMap::new(), + }), + pedersen_builtin: Some(vec![Relocatable::from((3, 2))]), + ..Default::default() + }; assert_eq!(cairo_pie.additional_data, expected_additional_data); // memory assert_eq!( @@ -171,9 +161,8 @@ fn common_signature() { }; assert_eq!(cairo_pie.execution_resources, expected_execution_resources); // additional_data - let expected_additional_data = HashMap::from([( - SIGNATURE_BUILTIN_NAME.to_string(), - BuiltinAdditionalData::Signature(HashMap::from([( + let expected_additional_data = AdditionalData { + ecdsa_builtin: Some(HashMap::from([( Relocatable::from((2, 0)), ( felt_str!( @@ -184,7 +173,9 @@ fn common_signature() { ), ), )])), - )]); + ..Default::default() + }; + assert_eq!(cairo_pie.additional_data, expected_additional_data); // memory assert_eq!( diff --git a/vm/src/vm/runners/cairo_pie.rs b/vm/src/vm/runners/cairo_pie.rs index 385cbefb48..05db1a1da2 100644 --- a/vm/src/vm/runners/cairo_pie.rs +++ b/vm/src/vm/runners/cairo_pie.rs @@ -9,6 +9,9 @@ use super::cairo_runner::ExecutionResources; use crate::serde::deserialize_utils::deserialize_biguint_from_number; use crate::stdlib::prelude::{String, Vec}; use crate::types::errors::cairo_pie_error::{CairoPieError, DeserializeMemoryError}; +use crate::vm::runners::builtin_runner::{ + HASH_BUILTIN_NAME, OUTPUT_BUILTIN_NAME, SIGNATURE_BUILTIN_NAME, +}; use crate::{ serde::deserialize_program::BuiltinName, stdlib::{collections::HashMap, prelude::*}, @@ -79,12 +82,54 @@ pub enum BuiltinAdditionalData { None, } +#[derive(Serialize, Deserialize, Clone, Debug, Default, PartialEq, Eq)] +pub struct AdditionalData { + pub output_builtin: Option, + pub pedersen_builtin: Option>, + #[serde(flatten)] + pub ecdsa_builtin: Option>, + pub range_check_builtin: Option<()>, +} + +impl AdditionalData { + pub fn is_empty(&self) -> bool { + self.output_builtin.is_none() + && self.pedersen_builtin.is_none() + && self.ecdsa_builtin.is_none() + && self.range_check_builtin.is_none() + } +} + +impl From> for AdditionalData { + fn from(mut value: HashMap) -> Self { + let output_builtin_data = match value.remove(OUTPUT_BUILTIN_NAME) { + Some(BuiltinAdditionalData::Output(output_data)) => Some(output_data), + _ => None, + }; + let ecdsa_builtin_data = match value.remove(SIGNATURE_BUILTIN_NAME) { + Some(BuiltinAdditionalData::Signature(signature_data)) => Some(signature_data), + _ => None, + }; + let pedersen_builtin_data = match value.remove(HASH_BUILTIN_NAME) { + Some(BuiltinAdditionalData::Hash(pedersen_data)) => Some(pedersen_data), + _ => None, + }; + + Self { + output_builtin: output_builtin_data, + ecdsa_builtin: ecdsa_builtin_data, + pedersen_builtin: pedersen_builtin_data, + range_check_builtin: None, + } + } +} + #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)] pub struct CairoPie { pub metadata: CairoPieMetadata, pub memory: CairoPieMemory, pub execution_resources: ExecutionResources, - pub additional_data: HashMap, + pub additional_data: AdditionalData, pub version: CairoPieVersion, } @@ -173,8 +218,7 @@ impl CairoPie { let metadata: CairoPieMetadata = parse_zip_file(zip.by_name("metadata.json")?)?; let execution_resources: ExecutionResources = parse_zip_file(zip.by_name("execution_resources.json")?)?; - let additional_data: HashMap = - parse_zip_file(zip.by_name("additional_data.json")?)?; + let additional_data: AdditionalData = parse_zip_file(zip.by_name("additional_data.json")?)?; let version: CairoPieVersion = parse_zip_file(zip.by_name("version.json")?)?; let addr_size: usize = 8; @@ -497,6 +541,7 @@ mod serde_impl { mod test { use super::*; use crate::utils::CAIRO_PRIME; + use assert_matches::assert_matches; use rstest::rstest; use std::fs::File; @@ -658,16 +703,55 @@ mod test { assert_eq!( cairo_pie.additional_data, - HashMap::from([( - "output_builtin".to_string(), - BuiltinAdditionalData::Output(OutputBuiltinAdditionalData { + AdditionalData { + output_builtin: Some(OutputBuiltinAdditionalData { base: 0, pages: Default::default(), attributes: Default::default(), - }) - )]) + }), + pedersen_builtin: None, + ecdsa_builtin: None, + range_check_builtin: None, + } ); assert_eq!(cairo_pie.version.cairo_pie, CAIRO_PIE_VERSION); } + + #[test] + fn test_deserialize_additional_data() { + let data = include_bytes!( + "../../../../cairo_programs/manually_compiled/pie_additional_data_test.json" + ); + let additional_data: AdditionalData = serde_json::from_slice(data).unwrap(); + let output_data = additional_data.output_builtin.unwrap(); + assert_eq!( + output_data.pages, + HashMap::from([( + 1, + PublicMemoryPage { + start: 18, + size: 46 + } + )]) + ); + assert_eq!( + output_data.attributes, + HashMap::from([("gps_fact_topology".to_string(), vec![2, 1, 0, 2])]) + ); + let pedersen_data = additional_data.pedersen_builtin.unwrap(); + assert_eq!( + pedersen_data, + vec![ + Relocatable::from((3, 2)), + Relocatable::from((3, 5)), + Relocatable::from((3, 8)), + Relocatable::from((3, 11)), + Relocatable::from((3, 14)), + Relocatable::from((3, 17)), + ] + ); + // TODO: add a test case with signature data + assert_matches!(additional_data.ecdsa_builtin, None); + } } diff --git a/vm/src/vm/runners/cairo_runner.rs b/vm/src/vm/runners/cairo_runner.rs index 15cd1c2088..fb7b44b07c 100644 --- a/vm/src/vm/runners/cairo_runner.rs +++ b/vm/src/vm/runners/cairo_runner.rs @@ -14,7 +14,7 @@ use crate::{ }, }; -use crate::vm::runners::cairo_pie::CAIRO_PIE_VERSION; +use crate::vm::runners::cairo_pie::{BuiltinAdditionalData, CAIRO_PIE_VERSION}; use crate::Felt252; use crate::{ hint_processor::hint_processor_definition::{HintProcessor, HintReference}, @@ -1403,7 +1403,8 @@ impl CairoRunner { .builtin_runners .iter() .map(|b| (b.name().to_string(), b.get_additional_data())) - .collect(), + .collect::>() + .into(), version: CairoPieVersion { cairo_pie: CAIRO_PIE_VERSION.to_string(), }, From c95396d33434c4e73757315ec1a73c116a5ab277 Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Sun, 14 Jan 2024 23:54:51 +0100 Subject: [PATCH 18/22] Fix: call_task test --- .../bootloader/execute_task_hints.rs | 48 ++++++++++++++++++- 1 file changed, 46 insertions(+), 2 deletions(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs index 2b6980cbcc..b40a4157d8 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/execute_task_hints.rs @@ -427,8 +427,8 @@ pub fn call_task( let program_address: Relocatable = exec_scopes.get("program_address")?; // ret_pc = ids.ret_pc_label.instruction_offset_ - ids.call_task.instruction_offset_ + pc - let program = get_bootloader_program(exec_scopes)?; - let identifiers = &program.shared_program_data.identifiers; + let bootloader_program = get_bootloader_program(exec_scopes)?; + let identifiers = &bootloader_program.shared_program_data.identifiers; let ret_pc_label = get_identifier(identifiers, "starkware.cairo.bootloaders.simple_bootloader.execute_task.execute_task.ret_pc_label")?; let call_task = get_identifier( identifiers, @@ -664,6 +664,42 @@ mod tests { ); } + /// Creates a fake Program struct to act as a placeholder for the `BOOTLOADER_PROGRAM` variable. + /// These other options have been considered: + /// * a `HasIdentifiers` trait cannot be used as exec_scopes requires to cast to `Box`, + /// making casting back to the trait impossible. + /// * using an enum requires defining test-only variants. + fn mock_program_with_identifiers(symbols: HashMap) -> Program { + let identifiers = symbols + .into_iter() + .map(|(name, pc)| { + ( + name, + Identifier { + pc: Some(pc), + type_: None, + value: None, + full_name: None, + members: None, + cairo_type: None, + }, + ) + }) + .collect(); + + let shared_program_data = SharedProgramData { + identifiers, + ..Default::default() + }; + let program = Program { + shared_program_data: Arc::new(shared_program_data), + constants: Default::default(), + builtins: vec![], + }; + + program + } + #[rstest] fn test_call_cairo_pie_task(fibonacci_pie: CairoPie) { let mut vm = vm!(); @@ -694,7 +730,15 @@ mod tests { let task = Task::Pie(fibonacci_pie); exec_scopes.insert_value(vars::TASK, task); + let bootloader_identifiers = HashMap::from( + [ + ("starkware.cairo.bootloaders.simple_bootloader.execute_task.execute_task.ret_pc_label".to_string(), 10usize), + ("starkware.cairo.bootloaders.simple_bootloader.execute_task.execute_task.call_task".to_string(), 8usize) + ] + ); + let bootloader_program = mock_program_with_identifiers(bootloader_identifiers); exec_scopes.insert_value(vars::PROGRAM_DATA_BASE, program_header_ptr.clone()); + exec_scopes.insert_value(vars::BOOTLOADER_PROGRAM, bootloader_program); // Load the program in memory load_program_hint(&mut vm, &mut exec_scopes, &ids_data, &ap_tracking) From b7685a1e6463fb9fcf5bb90e96fe40bc5dbd83c3 Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Sun, 14 Jan 2024 23:55:32 +0100 Subject: [PATCH 19/22] Fix: deserialize ECDSA builtin data from Python VM format Problem: the ECDSA/signature builtin additional data is stored internally as a hashmap, but the Python VM stores it as a vector of tuples. Solution: Add a `SignatureBuiltinAdditionalData` struct and implement a custom deserializer for it that can take either a hashmap or a vector. --- .../bootloader/load_cairo_pie.rs | 2 +- vm/src/tests/cairo_pie_test.rs | 6 +- vm/src/vm/runners/builtin_runner/signature.rs | 7 +- vm/src/vm/runners/cairo_pie.rs | 110 +++++++++++++----- 4 files changed, 88 insertions(+), 37 deletions(-) diff --git a/vm/src/hint_processor/builtin_hint_processor/bootloader/load_cairo_pie.rs b/vm/src/hint_processor/builtin_hint_processor/bootloader/load_cairo_pie.rs index 4aa12ea181..383fe4e2e7 100644 --- a/vm/src/hint_processor/builtin_hint_processor/bootloader/load_cairo_pie.rs +++ b/vm/src/hint_processor/builtin_hint_processor/bootloader/load_cairo_pie.rs @@ -232,7 +232,7 @@ fn relocate_builtin_additional_data( .get_signature_builtin() .map_err(|_| SignatureRelocationError::EcdsaBuiltinNotFound)?; - extend_additional_data(ecdsa_builtin, ecdsa_additional_data, relocation_table)?; + extend_additional_data(ecdsa_builtin, &ecdsa_additional_data.0, relocation_table)?; Ok(()) } diff --git a/vm/src/tests/cairo_pie_test.rs b/vm/src/tests/cairo_pie_test.rs index 769b2f35ea..e6935d5efd 100644 --- a/vm/src/tests/cairo_pie_test.rs +++ b/vm/src/tests/cairo_pie_test.rs @@ -24,7 +24,7 @@ use crate::{ }, }; -use crate::vm::runners::cairo_pie::AdditionalData; +use crate::vm::runners::cairo_pie::{AdditionalData, SignatureBuiltinAdditionalData}; #[cfg(all(not(feature = "std"), feature = "alloc"))] use alloc::{ string::{String, ToString}, @@ -162,7 +162,7 @@ fn common_signature() { assert_eq!(cairo_pie.execution_resources, expected_execution_resources); // additional_data let expected_additional_data = AdditionalData { - ecdsa_builtin: Some(HashMap::from([( + ecdsa_builtin: Some(SignatureBuiltinAdditionalData(HashMap::from([( Relocatable::from((2, 0)), ( felt_str!( @@ -172,7 +172,7 @@ fn common_signature() { "598673427589502599949712887611119751108407514580626464031881322743364689811" ), ), - )])), + )]))), ..Default::default() }; diff --git a/vm/src/vm/runners/builtin_runner/signature.rs b/vm/src/vm/runners/builtin_runner/signature.rs index d854afa05a..b2ccfad7e2 100644 --- a/vm/src/vm/runners/builtin_runner/signature.rs +++ b/vm/src/vm/runners/builtin_runner/signature.rs @@ -4,7 +4,7 @@ use crate::stdlib::{cell::RefCell, collections::HashMap, prelude::*, rc::Rc}; use crate::types::errors::math_errors::MathError; use crate::types::instance_definitions::ecdsa_instance_def::CELLS_PER_SIGNATURE; -use crate::vm::runners::cairo_pie::BuiltinAdditionalData; +use crate::vm::runners::cairo_pie::{BuiltinAdditionalData, SignatureBuiltinAdditionalData}; use crate::Felt252; use crate::{ types::{ @@ -239,7 +239,7 @@ impl SignatureBuiltinRunner { ) }) .collect(); - BuiltinAdditionalData::Signature(signatures) + BuiltinAdditionalData::Signature(SignatureBuiltinAdditionalData(signatures)) } pub fn air_private_input(&self, memory: &Memory) -> Vec { @@ -289,6 +289,7 @@ mod tests { }; use crate::felt_str; + use crate::vm::runners::cairo_pie::SignatureBuiltinAdditionalData; #[cfg(target_arch = "wasm32")] use wasm_bindgen_test::*; @@ -640,7 +641,7 @@ mod tests { )]); assert_eq!( builtin.get_additional_data(), - BuiltinAdditionalData::Signature(signatures) + BuiltinAdditionalData::Signature(SignatureBuiltinAdditionalData(signatures)) ) } } diff --git a/vm/src/vm/runners/cairo_pie.rs b/vm/src/vm/runners/cairo_pie.rs index 05db1a1da2..34ac0ee92a 100644 --- a/vm/src/vm/runners/cairo_pie.rs +++ b/vm/src/vm/runners/cairo_pie.rs @@ -69,6 +69,15 @@ pub struct OutputBuiltinAdditionalData { pub attributes: Attributes, } +#[derive(Serialize, Clone, Debug, PartialEq, Eq)] +pub struct SignatureBuiltinAdditionalData(pub HashMap); + +impl Default for SignatureBuiltinAdditionalData { + fn default() -> Self { + Self(HashMap::default()) + } +} + #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)] #[serde(untagged)] pub enum BuiltinAdditionalData { @@ -77,17 +86,19 @@ pub enum BuiltinAdditionalData { Hash(Vec), Output(OutputBuiltinAdditionalData), // Signatures are composed of (r, s) tuples - #[serde(serialize_with = "serde_impl::serialize_signature_additional_data")] - Signature(HashMap), + Signature(SignatureBuiltinAdditionalData), None, } #[derive(Serialize, Deserialize, Clone, Debug, Default, PartialEq, Eq)] pub struct AdditionalData { + #[serde(skip_serializing_if = "Option::is_none")] pub output_builtin: Option, + #[serde(skip_serializing_if = "Option::is_none")] pub pedersen_builtin: Option>, - #[serde(flatten)] - pub ecdsa_builtin: Option>, + #[serde(skip_serializing_if = "Option::is_none")] + pub ecdsa_builtin: Option, + #[serde(skip_serializing_if = "Option::is_none")] pub range_check_builtin: Option<()>, } @@ -310,14 +321,16 @@ mod serde_impl { Felt252, }; use num_bigint::BigUint; - use serde::de::SeqAccess; - use serde::{de, ser::SerializeSeq, Deserializer, Serialize, Serializer}; + use serde::de::{MapAccess, SeqAccess}; + use serde::{de, ser::SerializeSeq, Deserialize, Deserializer, Serialize, Serializer}; use serde_json::Number; use std::fmt; + use std::fmt::Formatter; use crate::serde::deserialize_utils::felt_from_number; use crate::utils::CAIRO_PRIME; + use crate::vm::runners::cairo_pie::SignatureBuiltinAdditionalData; pub const ADDR_BYTE_LEN: usize = 8; pub const FIELD_BYTE_LEN: usize = 32; @@ -499,27 +512,6 @@ mod serde_impl { d.deserialize_seq(MaybeRelocatableNumberVisitor) } - pub fn serialize_signature_additional_data( - values: &HashMap, - serializer: S, - ) -> Result - where - S: Serializer, - { - let mut seq_serializer = serializer.serialize_seq(Some(values.len()))?; - - for (key, (x, y)) in values { - seq_serializer.serialize_element(&[ - [ - Felt252Wrapper(&Felt252::from(key.segment_index)), - Felt252Wrapper(&Felt252::from(key.offset)), - ], - [Felt252Wrapper(x), Felt252Wrapper(y)], - ])?; - } - seq_serializer.end() - } - pub fn serialize_hash_additional_data( values: &[Relocatable], serializer: S, @@ -535,13 +527,67 @@ mod serde_impl { seq_serializer.end() } + + struct SignatureBuiltinAdditionalDataVisitor; + + impl<'de> de::Visitor<'de> for SignatureBuiltinAdditionalDataVisitor { + type Value = SignatureBuiltinAdditionalData; + + fn expecting(&self, formatter: &mut Formatter) -> fmt::Result { + write!( + formatter, + "a Vec<(Relocatable, (Felt252, Felt252))> or a HashMap" + ) + } + + fn visit_seq(self, mut seq: A) -> Result + where + A: SeqAccess<'de>, + { + let mut map = HashMap::with_capacity(seq.size_hint().unwrap_or(0)); + + // While there are entries remaining in the input, add them + // into our map. + while let Some((key, value)) = seq.next_element()? { + map.insert(key, value); + } + + Ok(SignatureBuiltinAdditionalData(map)) + } + + fn visit_map(self, mut access: A) -> Result + where + A: MapAccess<'de>, + { + let mut map = HashMap::with_capacity(access.size_hint().unwrap_or(0)); + + // While there are entries remaining in the input, add them + // into our map. + while let Some((key, value)) = access.next_entry()? { + map.insert(key, value); + } + + Ok(SignatureBuiltinAdditionalData(map)) + } + } + + // This is the trait that informs Serde how to deserialize MyMap. + impl<'de> Deserialize<'de> for SignatureBuiltinAdditionalData { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + // Instantiate our Visitor and ask the Deserializer to drive + // it over the input data, resulting in an instance of MyMap. + deserializer.deserialize_any(SignatureBuiltinAdditionalDataVisitor {}) + } + } } #[cfg(test)] mod test { use super::*; use crate::utils::CAIRO_PRIME; - use assert_matches::assert_matches; use rstest::rstest; use std::fs::File; @@ -731,7 +777,7 @@ mod test { 1, PublicMemoryPage { start: 18, - size: 46 + size: 46, } )]) ); @@ -752,6 +798,10 @@ mod test { ] ); // TODO: add a test case with signature data - assert_matches!(additional_data.ecdsa_builtin, None); + let expected_signature_additional_data = Some(SignatureBuiltinAdditionalData::default()); + assert_eq!( + additional_data.ecdsa_builtin, + expected_signature_additional_data + ); } } From 76622d88b6b7204db94e893838668a0d2ba30d01 Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Mon, 15 Jan 2024 00:25:00 +0100 Subject: [PATCH 20/22] Fix: support loading PIE memory files > 32KB Fixed a bug in the memory file loader implementation. --- vm/src/vm/runners/cairo_pie.rs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/vm/src/vm/runners/cairo_pie.rs b/vm/src/vm/runners/cairo_pie.rs index 34ac0ee92a..7444d76ecf 100644 --- a/vm/src/vm/runners/cairo_pie.rs +++ b/vm/src/vm/runners/cairo_pie.rs @@ -183,23 +183,23 @@ fn read_memory_file( addr_size: usize, felt_size: usize, ) -> Result { - let memory_cell_size = addr_size + felt_size; + let pair_size = addr_size + felt_size; let mut memory = CairoPieMemory::new(); let mut pos: usize = 0; loop { - let mut element = vec![0; memory_cell_size]; - match reader.read(&mut element) { - Ok(n) => { - if n == 0 { - break; - } - if n != memory_cell_size { - return Err(DeserializeMemoryError::UnexpectedEof); - } - } - Err(e) => return Err(e.into()), + let mut element = Vec::with_capacity(pair_size); + let n = reader + .by_ref() + .take(pair_size as u64) + .read_to_end(&mut element)?; + if n == 0 { + break; } + if n != pair_size { + return Err(DeserializeMemoryError::UnexpectedEof); + } + let (address_bytes, value_bytes) = element.split_at(addr_size); let address = maybe_relocatable_from_le_bytes(address_bytes); let value = maybe_relocatable_from_le_bytes(value_bytes); @@ -215,7 +215,7 @@ fn read_memory_file( return Err(DeserializeMemoryError::AddressIsNotRelocatable(pos)); } } - pos += memory_cell_size; + pos += pair_size; } Ok(memory) From d3a33c23d0e37d8c00030a382b3b452634e226c4 Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Wed, 17 Jan 2024 15:47:13 +0100 Subject: [PATCH 21/22] Feature: support output pages in public input memory Problem: output page IDs do not appear when exporting the builtin memory, but default to page 0 instead. Solution: add a `get_public_memory` to the output builtin to export page IDs correctly. --- vm/src/vm/runners/builtin_runner/output.rs | 33 ++++++++++++++++++++++ vm/src/vm/runners/cairo_runner.rs | 6 ++-- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/vm/src/vm/runners/builtin_runner/output.rs b/vm/src/vm/runners/builtin_runner/output.rs index a0b6559014..7b6c54f920 100644 --- a/vm/src/vm/runners/builtin_runner/output.rs +++ b/vm/src/vm/runners/builtin_runner/output.rs @@ -170,6 +170,21 @@ impl OutputBuiltinRunner { Ok(()) } + + pub fn get_public_memory(&self) -> Result, RunnerError> { + let size = self + .stop_ptr + .ok_or(RunnerError::NoStopPointer(Box::new(OUTPUT_BUILTIN_NAME)))?; + + let mut public_memory: Vec<(usize, usize)> = (0..size).map(|i| (i, 0)).collect(); + for (page_id, page) in self.pages.iter() { + for index in 0..page.size { + public_memory[page.start + index].1 = page_id.clone(); + } + } + + Ok(public_memory) + } } impl Default for OutputBuiltinRunner { @@ -551,4 +566,22 @@ mod tests { matches!(result, Err(RunnerError::PageNotOnSegment(relocatable, base)) if relocatable == page_start && base == builtin.base()) ) } + + #[test] + fn get_public_memory() { + let mut builtin = OutputBuiltinRunner::new(true); + let page_start = Relocatable { + segment_index: builtin.base() as isize, + offset: 2, + }; + builtin + .add_page(1, page_start.clone(), 2) + .expect("Failed to add page"); + + // Mock the effects of `final_stack()` + builtin.stop_ptr = Some(4); + + let public_memory = builtin.get_public_memory().unwrap(); + assert_eq!(public_memory, vec![(0, 0), (1, 0), (2, 1), (3, 1)]); + } } diff --git a/vm/src/vm/runners/cairo_runner.rs b/vm/src/vm/runners/cairo_runner.rs index fb7b44b07c..ee3e050478 100644 --- a/vm/src/vm/runners/cairo_runner.rs +++ b/vm/src/vm/runners/cairo_runner.rs @@ -56,7 +56,7 @@ use num_traits::{ToPrimitive, Zero}; use serde::{Deserialize, Serialize}; use super::{ - builtin_runner::{KeccakBuiltinRunner, PoseidonBuiltinRunner, OUTPUT_BUILTIN_NAME}, + builtin_runner::{KeccakBuiltinRunner, PoseidonBuiltinRunner}, cairo_pie::{self, CairoPie, CairoPieMetadata, CairoPieVersion}, }; @@ -1091,8 +1091,8 @@ impl CairoRunner { let (_, size) = builtin_runner .get_used_cells_and_allocated_size(vm) .map_err(RunnerError::FinalizeSegements)?; - if builtin_runner.name() == OUTPUT_BUILTIN_NAME { - let public_memory = (0..size).map(|i| (i, 0)).collect(); + if let BuiltinRunner::Output(output_builtin) = builtin_runner { + let public_memory = output_builtin.get_public_memory()?; vm.segments .finalize(Some(size), builtin_runner.base(), Some(&public_memory)) } else { From e0a4653aa5634664a3f792b38715a572e9f89b44 Mon Sep 17 00:00:00 2001 From: Olivier Desenfans Date: Tue, 23 Jan 2024 13:04:26 +0100 Subject: [PATCH 22/22] Feature: deserialize Cairo PIE from bytes Added a `from_bytes` class method to build a `CairoPie` method in addition to the existing `from_file` method. --- vm/src/vm/runners/cairo_pie.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/vm/src/vm/runners/cairo_pie.rs b/vm/src/vm/runners/cairo_pie.rs index 7444d76ecf..b822805eef 100644 --- a/vm/src/vm/runners/cairo_pie.rs +++ b/vm/src/vm/runners/cairo_pie.rs @@ -225,7 +225,7 @@ impl CairoPie { #[cfg(feature = "std")] pub fn from_zip_archive( mut zip: zip::ZipArchive, - ) -> Result { + ) -> Result { let metadata: CairoPieMetadata = parse_zip_file(zip.by_name("metadata.json")?)?; let execution_resources: ExecutionResources = parse_zip_file(zip.by_name("execution_resources.json")?)?; @@ -242,7 +242,7 @@ impl CairoPie { }; let memory = read_memory_file(zip.by_name("memory.bin")?, addr_size, felt_bytes)?; - Ok(CairoPie { + Ok(Self { metadata, memory, execution_resources, @@ -252,11 +252,19 @@ impl CairoPie { } #[cfg(feature = "std")] - pub fn from_file(path: &Path) -> Result { + pub fn from_bytes(bytes: &[u8]) -> Result { + let reader = std::io::Cursor::new(bytes); + let zip_archive = zip::ZipArchive::new(reader)?; + + Self::from_zip_archive(zip_archive) + } + + #[cfg(feature = "std")] + pub fn from_file(path: &Path) -> Result { let file = std::fs::File::open(path)?; let zip = zip::ZipArchive::new(file)?; - CairoPie::from_zip_archive(zip) + Self::from_zip_archive(zip) } }