diff --git a/.github/workflows/hyper_threading_benchmarks.yml b/.github/workflows/hyper_threading_benchmarks.yml new file mode 100644 index 0000000000..cebfa0084c --- /dev/null +++ b/.github/workflows/hyper_threading_benchmarks.yml @@ -0,0 +1,110 @@ +name: Benchmark Hyper Threading + +on: + pull_request: + branches: + - main + +jobs: + benchmark: + runs-on: ubuntu-latest + steps: + - name: Checkout PR + uses: actions/checkout@v2 + with: + ref: ${{ github.head_ref }} + + - name: Setup Python + uses: actions/setup-python@v4 + with: + python-version: '3.9' + + - name: Install Dependencies + run: | + pip install -r requirements.txt + sudo apt update + sudo apt-get install -y hyperfine + + - name: Install Rust + uses: dtolnay/rust-toolchain@1.74.1 + with: + components: rustfmt, clippy + + - name: Compile PR Version + run: | + cargo build --release -p hyper_threading + cp target/release/hyper_threading ${{ github.workspace }}/hyper_threading_pr + cp ./examples/hyper_threading/hyper-threading-workflow.sh ${{ github.workspace }}/hyper-threading-workflow.sh + + - name: Upload PR Binary + uses: actions/upload-artifact@v4 + with: + name: hyper_threading_pr_binary + path: ${{ github.workspace }}/hyper_threading_pr + + - name: Upload Workflow Script + uses: actions/upload-artifact@v4 + with: + name: hyper_threading_workflow_script + path: ${{ github.workspace }}/hyper-threading-workflow.sh + + + - name: Checkout Main Branch + uses: actions/checkout@v2 + with: + ref: 'main' + + - name: Compile Main Version + run: | + cargo build --release -p hyper_threading + cp target/release/hyper_threading ${{ github.workspace }}/hyper_threading_main + + - name: Download hyper_threading_pr_binary + uses: actions/download-artifact@v4 + with: + name: hyper_threading_pr_binary + path: ${{ github.workspace }}/ + + - name: Download hyper_threading_workflow_script + uses: actions/download-artifact@v4 + with: + name: hyper_threading_workflow_script + path: ${{ github.workspace }}/ + + - name: Compile programs + run: make cairo_bench_programs + + - name: Run Benchmarks + run: | + cd ${{ github.workspace }} + chmod +x ./hyper_threading_main + chmod +x ./hyper_threading_pr + chmod +x hyper-threading-workflow.sh + ./hyper-threading-workflow.sh + + - name: Compare Results + run: | + cat result.md + + - name: Find comment + uses: peter-evans/find-comment@v2 + id: fc + with: + issue-number: ${{ github.event.pull_request.number }} + comment-author: 'github-actions[bot]' + body-includes: "**Hyper Thereading Benchmark results**" + + - name: Create comment + if: steps.fc.outputs.comment-id == '' + uses: peter-evans/create-or-update-comment@v3 + with: + issue-number: ${{ github.event.pull_request.number }} + body-path: result.md + + - name: Update comment + if: steps.fc.outputs.comment-id != '' + uses: peter-evans/create-or-update-comment@v3 + with: + comment-id: ${{ steps.fc.outputs.comment-id }} + body-path: result.md + edit-mode: replace diff --git a/CHANGELOG.md b/CHANGELOG.md index 64fdf29700..0028c38f85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,13 @@ Notes: Serialization of vm outputs that now contain `BuiltinName` & `Display` implementation of `BuiltinName` have not been affected by this PR +* fix(BREAKING): Remove unsafe impl of `Add for &'a Relocatable`[#1718](https://github.com/lambdaclass/cairo-vm/pull/1718) + +* fix(BREAKING): Handle triple dereference references[#1708](https://github.com/lambdaclass/cairo-vm/pull/1708) + * Replace `ValueAddress` boolean field `dereference` with boolean fields `outer_dereference` & `inner_dereference` + * Replace `HintReference` boolean field `dereference` with boolean fields `outer_dereference` & `inner_dereference` + * Reference parsing now handles the case of dereferences inside the cast. Aka references of type `cast([A + B], type)` such as `cast([[fp + 2] + 2], felt)`. + * Bump `starknet-types-core` version + Use the lib's pedersen hash [#1692](https://github.com/lambdaclass/cairo-vm/pull/1692) * refactor: Remove unused code & use constants whenever possible for builtin instance definitions[#1707](https://github.com/lambdaclass/cairo-vm/pull/1707) diff --git a/docs/references_parsing/README.md b/docs/references_parsing/README.md index 95ffd9975a..59cc044298 100644 --- a/docs/references_parsing/README.md +++ b/docs/references_parsing/README.md @@ -63,6 +63,7 @@ There are some other, more rare cases of reference values found when implementin * ```cast(number, felt)``` * ```[cast(reg + offset1 + offset2, type)]``` +* ```[cast([[reg + offset1] + offset2], type)]``` ## To do For the moment the type of the reference is not being used, this will be included in the future to make the hints code cleaner. diff --git a/examples/hyper_threading/hyper-threading-workflow.sh b/examples/hyper_threading/hyper-threading-workflow.sh new file mode 100644 index 0000000000..5ab1cb1019 --- /dev/null +++ b/examples/hyper_threading/hyper-threading-workflow.sh @@ -0,0 +1,34 @@ +#!/bin/bash + +# Define a list of RAYON_NUM_THREADS +thread_counts=(1 2 4 6 8 16 ) + +# Define binary names +binaries=("hyper_threading_main" "hyper_threading_pr") + +echo -e "**Hyper Thereading Benchmark results**" >> result.md +printf "\n\n" >> result.md + +# Iter over thread_counts +for threads in "${thread_counts[@]}"; do + # Initialize hyperfine command + cmd="hyperfine -r 2" + + # Add each binary to the command with the current threads value + for binary in "${binaries[@]}"; do + cmd+=" -n \"${binary} threads: ${threads}\" 'RAYON_NUM_THREADS=${threads} ./${binary}'" + done + + # Execute + echo "Running benchmark for ${threads} threads" + printf "\n\n" >> result.md + echo -e $cmd >> result.md + eval $cmd >> result.md + printf "\n\n" >> result.md +done + +{ + echo -e '```' + cat result.md + echo -e '```' +} > temp_result.md && mv temp_result.md result.md diff --git a/vm/src/hint_processor/hint_processor_definition.rs b/vm/src/hint_processor/hint_processor_definition.rs index a096d45521..fbf3050b89 100644 --- a/vm/src/hint_processor/hint_processor_definition.rs +++ b/vm/src/hint_processor/hint_processor_definition.rs @@ -103,7 +103,8 @@ fn get_ids_data( pub struct HintReference { pub offset1: OffsetValue, pub offset2: OffsetValue, - pub dereference: bool, + pub inner_dereference: bool, + pub outer_dereference: bool, pub ap_tracking_data: Option, pub cairo_type: Option, } @@ -114,7 +115,8 @@ impl HintReference { offset1: OffsetValue::Reference(Register::FP, offset1, false), offset2: OffsetValue::Value(0), ap_tracking_data: None, - dereference: true, + outer_dereference: true, + inner_dereference: false, cairo_type: None, } } @@ -124,7 +126,8 @@ impl HintReference { offset1: OffsetValue::Reference(Register::FP, offset1, inner_dereference), offset2: OffsetValue::Value(offset2), ap_tracking_data: None, - dereference, + outer_dereference: dereference, + inner_dereference: false, cairo_type: None, } } @@ -135,7 +138,8 @@ impl From for HintReference { HintReference { offset1: reference.value_address.offset1.clone(), offset2: reference.value_address.offset2.clone(), - dereference: reference.value_address.dereference, + outer_dereference: reference.value_address.outer_dereference, + inner_dereference: reference.value_address.inner_dereference, // only store `ap` tracking data if the reference is referred to it ap_tracking_data: match ( &reference.value_address.offset1, diff --git a/vm/src/hint_processor/hint_processor_utils.rs b/vm/src/hint_processor/hint_processor_utils.rs index 31c60a5ce0..f952a9d4e7 100644 --- a/vm/src/hint_processor/hint_processor_utils.rs +++ b/vm/src/hint_processor/hint_processor_utils.rs @@ -59,7 +59,7 @@ pub fn get_ptr_from_reference( ) -> Result { let var_addr = compute_addr_from_reference(hint_reference, vm, ap_tracking) .ok_or(HintError::UnknownIdentifierInternal)?; - if hint_reference.dereference { + if hint_reference.outer_dereference { vm.get_relocatable(var_addr) .map_err(|_| HintError::WrongIdentifierTypeInternal(Box::new(var_addr))) } else { @@ -79,7 +79,7 @@ pub fn get_maybe_relocatable_from_reference( } //Then calculate address let var_addr = compute_addr_from_reference(hint_reference, vm, ap_tracking)?; - if hint_reference.dereference { + if hint_reference.outer_dereference { vm.get_maybe(&var_addr) } else { Some(MaybeRelocatable::from(var_addr)) @@ -94,7 +94,7 @@ pub fn compute_addr_from_reference( //ApTracking of the Hint itself hint_ap_tracking: &ApTracking, ) -> Option { - let offset1 = + let mut offset1 = if let OffsetValue::Reference(_register, _offset, _deref) = &hint_reference.offset1 { get_offset_value_reference( vm, @@ -118,11 +118,15 @@ pub fn compute_addr_from_reference( &hint_reference.offset2, )?; - Some((offset1 + value.get_int_ref()?.to_usize()?).ok()?) + offset1 += value.get_int_ref()?.to_usize()? } - OffsetValue::Value(value) => Some((offset1 + *value).ok()?), - _ => Some(offset1), + OffsetValue::Value(value) => offset1 = (offset1 + *value).ok()?, + _ => {} } + if hint_reference.inner_dereference { + offset1 = vm.get_relocatable(offset1).ok()? + } + Some(offset1) } fn apply_ap_tracking_correction( @@ -217,7 +221,8 @@ mod tests { let hint_ref = HintReference { offset1: OffsetValue::Reference(Register::FP, 0, false), offset2: OffsetValue::Immediate(Felt252::TWO), - dereference: false, + outer_dereference: false, + inner_dereference: false, ap_tracking_data: Default::default(), cairo_type: None, }; @@ -382,4 +387,30 @@ mod tests { None ); } + + #[test] + #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] + fn get_integer_from_reference_with_triple_deref() { + // Reference: [cast([[fp + 2)] + 2], felt*)] + let mut vm = vm!(); + vm.segments = segments![ + ((1, 2), (0, 0)), // [fp + 2] -> [(1, 0) + 2] -> [(1, 2)] -> (0, 0) + ((0, 2), (0, 5)), // [[fp + 2] + 2] -> [(0, 0) + 2] -> [(0, 2)] -> (0, 5) + ((0, 5), 3) // [[[fp + 2] + 2]] -> [(0, 5)] -> 3 + ]; + let hint_ref = HintReference { + offset1: OffsetValue::Reference(Register::FP, 2, true), + offset2: OffsetValue::Value(2), + outer_dereference: true, + inner_dereference: true, + ap_tracking_data: Default::default(), + cairo_type: None, + }; + + assert_eq!( + get_integer_from_reference(&vm, &hint_ref, &ApTracking::new()) + .expect("Unexpected get integer fail"), + Felt252::THREE + ); + } } diff --git a/vm/src/serde/deserialize_program.rs b/vm/src/serde/deserialize_program.rs index 2cc94e1302..6eea8b84c5 100644 --- a/vm/src/serde/deserialize_program.rs +++ b/vm/src/serde/deserialize_program.rs @@ -278,10 +278,11 @@ pub enum OffsetValue { #[cfg_attr(all(feature = "arbitrary", feature = "std"), derive(Arbitrary))] #[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone)] pub struct ValueAddress { - pub offset1: OffsetValue, - pub offset2: OffsetValue, - pub dereference: bool, - pub value_type: String, + pub offset1: OffsetValue, // A in cast(A + B, type) + pub offset2: OffsetValue, // B in cast(A + B, type) + pub outer_dereference: bool, // [] in [cast(A + B, type)] + pub inner_dereference: bool, // [] in cast([A + B], type) + pub value_type: String, // type in cast(A + B, type) } impl ValueAddress { @@ -296,7 +297,8 @@ impl ValueAddress { ValueAddress { offset1: OffsetValue::Value(99), offset2: OffsetValue::Value(99), - dereference: false, + outer_dereference: false, + inner_dereference: false, value_type: String::from("felt"), } } @@ -711,7 +713,8 @@ mod tests { value_address: ValueAddress { offset1: OffsetValue::Reference(Register::FP, -4, false), offset2: OffsetValue::Value(0), - dereference: true, + outer_dereference: true, + inner_dereference: false, value_type: "felt".to_string(), }, }, @@ -724,7 +727,8 @@ mod tests { value_address: ValueAddress { offset1: OffsetValue::Reference(Register::FP, -3, false), offset2: OffsetValue::Value(0), - dereference: true, + outer_dereference: true, + inner_dereference: false, value_type: "felt".to_string(), }, }, @@ -737,7 +741,8 @@ mod tests { value_address: ValueAddress { offset1: OffsetValue::Reference(Register::FP, -3, true), offset2: OffsetValue::Immediate(Felt252::from(2)), - dereference: false, + outer_dereference: false, + inner_dereference: false, value_type: "felt".to_string(), }, }, @@ -750,7 +755,8 @@ mod tests { value_address: ValueAddress { offset1: OffsetValue::Reference(Register::FP, 0, false), offset2: OffsetValue::Value(0), - dereference: true, + outer_dereference: true, + inner_dereference: false, value_type: "felt*".to_string(), }, }, diff --git a/vm/src/serde/deserialize_utils.rs b/vm/src/serde/deserialize_utils.rs index 7462a53cb6..8179605c52 100644 --- a/vm/src/serde/deserialize_utils.rs +++ b/vm/src/serde/deserialize_utils.rs @@ -52,7 +52,14 @@ fn outer_brackets(input: &str) -> IResult<&str, bool> { ))(input) .map(|(rem_input, res_opt)| { if let Some(res) = res_opt { - (res, true) + if !rem_input.is_empty() { + // This means that the parser mistook an offset value's inner dereference for a reference's inner dereference + // For example: [fp + 2] + 2 being parsed as "fp + 2" with "+2" as remaining output + // In this case we discard this parsing step + (input, false) + } else { + (res, true) + } } else { (rem_input, false) } @@ -144,12 +151,14 @@ fn no_inner_dereference(input: &str) -> IResult<&str, OffsetValue> { } pub(crate) fn parse_value(input: &str) -> IResult<&str, ValueAddress> { - let (rem_input, (dereference, second_arg, fst_offset, snd_offset)) = tuple(( - outer_brackets, - take_cast_first_arg, - opt(alt((inner_dereference, no_inner_dereference))), - opt(alt((inner_dereference, no_inner_dereference))), - ))(input)?; + let (rem_input, (outer_dereference, second_arg, inner_dereference, fst_offset, snd_offset)) = + tuple(( + outer_brackets, + take_cast_first_arg, + outer_brackets, + opt(alt((inner_dereference, no_inner_dereference))), + opt(alt((inner_dereference, no_inner_dereference))), + ))(input)?; let (indirection_level, (_, struct_)) = tuple((tag(", "), take_till(|c: char| c == '*')))(second_arg)?; @@ -185,7 +194,8 @@ pub(crate) fn parse_value(input: &str) -> IResult<&str, ValueAddress> { let value_address = ValueAddress { offset1, offset2, - dereference, + outer_dereference, + inner_dereference, value_type: type_, }; @@ -387,7 +397,8 @@ mod tests { ValueAddress { offset2: OffsetValue::Value(2), offset1: OffsetValue::Reference(Register::FP, -1_i32, true), - dereference: true, + outer_dereference: true, + inner_dereference: false, value_type: "felt".to_string(), } )) @@ -407,7 +418,8 @@ mod tests { ValueAddress { offset1: OffsetValue::Reference(Register::AP, 2_i32, false), offset2: OffsetValue::Value(0), - dereference: false, + outer_dereference: false, + inner_dereference: false, value_type: "felt".to_string(), } )) @@ -426,7 +438,8 @@ mod tests { ValueAddress { offset1: OffsetValue::Value(825323), offset2: OffsetValue::Value(0), - dereference: false, + outer_dereference: false, + inner_dereference: false, value_type: "felt".to_string(), } )) @@ -446,7 +459,8 @@ mod tests { ValueAddress { offset1: OffsetValue::Reference(Register::AP, 0_i32, false), offset2: OffsetValue::Value(-1), - dereference: true, + outer_dereference: true, + inner_dereference: false, value_type: "felt".to_string(), } )) @@ -466,7 +480,8 @@ mod tests { ValueAddress { offset1: OffsetValue::Reference(Register::AP, 0_i32, true), offset2: OffsetValue::Value(1), - dereference: true, + outer_dereference: true, + inner_dereference: false, value_type: "__main__.felt".to_string(), } )) @@ -486,7 +501,8 @@ mod tests { ValueAddress { offset1: OffsetValue::Reference(Register::AP, 0_i32, true), offset2: OffsetValue::Immediate(Felt252::ONE), - dereference: true, + outer_dereference: true, + inner_dereference: false, value_type: "felt".to_string(), } )) @@ -506,7 +522,8 @@ mod tests { ValueAddress { offset1: OffsetValue::Reference(Register::AP, 1_i32, true), offset2: OffsetValue::Value(1), - dereference: true, + outer_dereference: true, + inner_dereference: false, value_type: "felt".to_string(), } )) @@ -526,7 +543,8 @@ mod tests { ValueAddress { offset1: OffsetValue::Reference(Register::AP, 0_i32, true), offset2: OffsetValue::Reference(Register::FP, 1_i32, true), - dereference: true, + outer_dereference: true, + inner_dereference: false, value_type: "__main__.felt".to_string(), } )) @@ -546,7 +564,8 @@ mod tests { ValueAddress { offset1: OffsetValue::Reference(Register::AP, 1_i32, true), offset2: OffsetValue::Reference(Register::FP, 1_i32, true), - dereference: true, + outer_dereference: true, + inner_dereference: false, value_type: "__main__.felt".to_string(), } )) @@ -566,7 +585,8 @@ mod tests { ValueAddress { offset1: OffsetValue::Immediate(Felt252::from(825323_i32)), offset2: OffsetValue::Immediate(Felt252::ZERO), - dereference: false, + outer_dereference: false, + inner_dereference: false, value_type: "felt".to_string(), } )) @@ -586,7 +606,8 @@ mod tests { ValueAddress { offset1: OffsetValue::Reference(Register::AP, 0_i32, true), offset2: OffsetValue::Value(1), - dereference: true, + outer_dereference: true, + inner_dereference: false, value_type: "starkware.cairo.common.cairo_secp.ec.EcPoint".to_string(), } )) @@ -606,7 +627,8 @@ mod tests { ValueAddress { offset1: OffsetValue::Reference(Register::AP, 0_i32, true), offset2: OffsetValue::Value(1), - dereference: true, + outer_dereference: true, + inner_dereference: false, value_type: "starkware.cairo.common.cairo_secp.ec.EcPoint*".to_string(), } )) @@ -626,7 +648,29 @@ mod tests { ValueAddress { offset1: OffsetValue::Reference(Register::AP, 0_i32, true), offset2: OffsetValue::Reference(Register::AP, 0_i32, true), - dereference: true, + outer_dereference: true, + inner_dereference: false, + value_type: "felt".to_string(), + } + )) + ); + } + + #[test] + #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] + fn parse_value_to_felt_with_triple_dereference() { + let value = "[cast([[fp + (-3)] + 5], felt*)]"; + let parsed = parse_value(value); + + assert_eq!( + parsed, + Ok(( + "", + ValueAddress { + offset1: OffsetValue::Reference(Register::FP, -3_i32, true), + offset2: OffsetValue::Value(5), + outer_dereference: true, + inner_dereference: true, value_type: "felt".to_string(), } )) @@ -646,7 +690,8 @@ mod tests { ValueAddress { offset1: OffsetValue::Reference(Register::AP, 1_i32, true), offset2: OffsetValue::Reference(Register::AP, 2_i32, true), - dereference: true, + outer_dereference: true, + inner_dereference: false, value_type: "felt".to_string(), } )) diff --git a/vm/src/serde/serialize_program.rs b/vm/src/serde/serialize_program.rs index 03f93bb7b0..f8bd0d076a 100644 --- a/vm/src/serde/serialize_program.rs +++ b/vm/src/serde/serialize_program.rs @@ -198,7 +198,8 @@ impl From<&Program> for ProgramSerializer { value_address: ValueAddress { offset1: r.offset1, offset2: r.offset2, - dereference: r.dereference, + outer_dereference: r.outer_dereference, + inner_dereference: r.inner_dereference, value_type: r.cairo_type.unwrap_or_default(), }, ap_tracking_data: r.ap_tracking_data.unwrap_or_default(), diff --git a/vm/src/types/program.rs b/vm/src/types/program.rs index 5aa7c54596..956847cd5c 100644 --- a/vm/src/types/program.rs +++ b/vm/src/types/program.rs @@ -340,7 +340,8 @@ impl Program { HintReference { offset1: r.value_address.offset1.clone(), offset2: r.value_address.offset2.clone(), - dereference: r.value_address.dereference, + outer_dereference: r.value_address.outer_dereference, + inner_dereference: r.value_address.inner_dereference, // only store `ap` tracking data if the reference is referred to it ap_tracking_data: match (&r.value_address.offset1, &r.value_address.offset2) { (OffsetValue::Reference(Register::AP, _, _), _) diff --git a/vm/src/types/relocatable.rs b/vm/src/types/relocatable.rs index e569ce7707..790dae895b 100644 --- a/vm/src/types/relocatable.rs +++ b/vm/src/types/relocatable.rs @@ -356,17 +356,6 @@ impl MaybeRelocatable { } } -impl<'a> Add for &'a Relocatable { - type Output = Relocatable; - - fn add(self, other: usize) -> Self::Output { - Relocatable { - segment_index: self.segment_index, - offset: self.offset + other, - } - } -} - /// Turns a MaybeRelocatable into a Felt252 value. /// If the value is an Int, it will extract the Felt252 value from it. /// If the value is RelocatableValue, it will relocate it according to the relocation_table diff --git a/vm/src/vm/runners/builtin_runner/signature.rs b/vm/src/vm/runners/builtin_runner/signature.rs index 8f2a21c20e..a95ad508a4 100644 --- a/vm/src/vm/runners/builtin_runner/signature.rs +++ b/vm/src/vm/runners/builtin_runner/signature.rs @@ -183,8 +183,12 @@ impl SignatureBuiltinRunner { pub fn air_private_input(&self, memory: &Memory) -> Vec { let mut private_inputs = vec![]; for (addr, signature) in self.signatures.borrow().iter() { - if let (Ok(pubkey), Ok(msg)) = (memory.get_integer(*addr), memory.get_integer(addr + 1)) - { + if let (Ok(pubkey), Some(msg)) = ( + memory.get_integer(*addr), + (*addr + 1_usize) + .ok() + .and_then(|addr| memory.get_integer(addr).ok()), + ) { private_inputs.push(PrivateInput::Signature(PrivateInputSignature { index: addr .offset diff --git a/vm/src/vm/runners/cairo_runner.rs b/vm/src/vm/runners/cairo_runner.rs index 61d9477ae4..5b6e9d9d45 100644 --- a/vm/src/vm/runners/cairo_runner.rs +++ b/vm/src/vm/runners/cairo_runner.rs @@ -567,10 +567,7 @@ impl CairoRunner { } else { let mut stack_prefix = vec![ Into::::into( - self.execution_base - .as_ref() - .ok_or(RunnerError::NoExecBase)? - + target_offset, + (self.execution_base.ok_or(RunnerError::NoExecBase)? + target_offset)?, ), MaybeRelocatable::from(Felt252::zero()), ]; @@ -588,20 +585,16 @@ impl CairoRunner { )?; } - self.initial_fp = Some( - self.execution_base - .as_ref() - .ok_or(RunnerError::NoExecBase)? - + target_offset, - ); + self.initial_fp = + Some((self.execution_base.ok_or(RunnerError::NoExecBase)? + target_offset)?); self.initial_ap = self.initial_fp; - return Ok(self.program_base.as_ref().ok_or(RunnerError::NoProgBase)? + return Ok((self.program_base.ok_or(RunnerError::NoProgBase)? + self .program .shared_program_data .end - .ok_or(RunnerError::NoProgramEnd)?); + .ok_or(RunnerError::NoProgramEnd)?)?); } let return_fp = vm.segments.add(); diff --git a/vm/src/vm/vm_memory/memory.rs b/vm/src/vm/vm_memory/memory.rs index 2ea342573b..b4b953f5ee 100644 --- a/vm/src/vm/vm_memory/memory.rs +++ b/vm/src/vm/vm_memory/memory.rs @@ -178,24 +178,23 @@ impl Memory { &self.data }; let (i, j) = from_relocatable_to_indexes(relocatable); - Some(self.relocate_value(data.get(i)?.get(j)?.as_ref()?.get_value())) + self.relocate_value(data.get(i)?.get(j)?.as_ref()?.get_value()) + .ok() } // Version of Memory.relocate_value() that doesn't require a self reference fn relocate_address( addr: Relocatable, relocation_rules: &HashMap, - ) -> MaybeRelocatable { - let segment_idx = addr.segment_index; - if segment_idx >= 0 { - return addr.into(); - } - - // Adjust the segment index to begin at zero, as per the struct field's - match relocation_rules.get(&(-(segment_idx + 1) as usize)) { - Some(x) => (x + addr.offset).into(), - None => addr.into(), + ) -> Result { + if addr.segment_index < 0 { + // Adjust the segment index to begin at zero, as per the struct field's + // comment. + if let Some(x) = relocation_rules.get(&(-(addr.segment_index + 1) as usize)) { + return Ok((*x + addr.offset)?.into()); + } } + Ok(addr.into()) } /// Relocates the memory according to the relocation rules and clears `self.relocaction_rules`. @@ -209,7 +208,7 @@ impl Memory { let value = cell.get_value_mut(); match value { MaybeRelocatable::RelocatableValue(addr) if addr.segment_index < 0 => { - *value = Memory::relocate_address(*addr, &self.relocation_rules); + *value = Memory::relocate_address(*addr, &self.relocation_rules)?; } _ => {} } @@ -581,39 +580,42 @@ impl fmt::Display for Memory { /// Applies `relocation_rules` to a value pub(crate) trait RelocateValue<'a, Input: 'a, Output: 'a> { - fn relocate_value(&self, value: Input) -> Output; + fn relocate_value(&self, value: Input) -> Result; } impl RelocateValue<'_, Relocatable, Relocatable> for Memory { - fn relocate_value(&self, addr: Relocatable) -> Relocatable { - let segment_idx = addr.segment_index; - if segment_idx >= 0 { - return addr; - } - - // Adjust the segment index to begin at zero, as per the struct field's - // comment. - match self.relocation_rules.get(&(-(segment_idx + 1) as usize)) { - Some(x) => x + addr.offset, - None => addr, + fn relocate_value(&self, addr: Relocatable) -> Result { + if addr.segment_index < 0 { + // Adjust the segment index to begin at zero, as per the struct field's + // comment. + if let Some(x) = self + .relocation_rules + .get(&(-(addr.segment_index + 1) as usize)) + { + return (*x + addr.offset).map_err(MemoryError::Math); + } } + Ok(addr) } } impl<'a> RelocateValue<'a, &'a Felt252, &'a Felt252> for Memory { - fn relocate_value(&self, value: &'a Felt252) -> &'a Felt252 { - value + fn relocate_value(&self, value: &'a Felt252) -> Result<&'a Felt252, MemoryError> { + Ok(value) } } impl<'a> RelocateValue<'a, &'a MaybeRelocatable, Cow<'a, MaybeRelocatable>> for Memory { - fn relocate_value(&self, value: &'a MaybeRelocatable) -> Cow<'a, MaybeRelocatable> { - match value { + fn relocate_value( + &self, + value: &'a MaybeRelocatable, + ) -> Result, MemoryError> { + Ok(match value { MaybeRelocatable::Int(_) => Cow::Borrowed(value), MaybeRelocatable::RelocatableValue(addr) => { - Cow::Owned(self.relocate_value(*addr).into()) + Cow::Owned(self.relocate_value(*addr)?.into()) } - } + }) } } @@ -1043,7 +1045,9 @@ mod memory_tests { // Test when value is Some(BigInt): assert_eq!( - memory.relocate_value(&MaybeRelocatable::Int(Felt252::from(0))), + memory + .relocate_value(&MaybeRelocatable::Int(Felt252::from(0))) + .unwrap(), Cow::Owned(MaybeRelocatable::Int(Felt252::from(0))), ); } @@ -1061,11 +1065,15 @@ mod memory_tests { // Test when value is Some(MaybeRelocatable) with segment_index >= 0: assert_eq!( - memory.relocate_value(&MaybeRelocatable::RelocatableValue((0, 0).into())), + memory + .relocate_value(&MaybeRelocatable::RelocatableValue((0, 0).into())) + .unwrap(), Cow::Owned(MaybeRelocatable::RelocatableValue((0, 0).into())), ); assert_eq!( - memory.relocate_value(&MaybeRelocatable::RelocatableValue((5, 0).into())), + memory + .relocate_value(&MaybeRelocatable::RelocatableValue((5, 0).into())) + .unwrap(), Cow::Owned(MaybeRelocatable::RelocatableValue((5, 0).into())), ); } @@ -1084,7 +1092,9 @@ mod memory_tests { // Test when value is Some(MaybeRelocatable) with segment_index < 0 and // there are no applicable relocation rules: assert_eq!( - memory.relocate_value(&MaybeRelocatable::RelocatableValue((-5, 0).into())), + memory + .relocate_value(&MaybeRelocatable::RelocatableValue((-5, 0).into())) + .unwrap(), Cow::Owned(MaybeRelocatable::RelocatableValue((-5, 0).into())), ); } @@ -1103,19 +1113,27 @@ mod memory_tests { // Test when value is Some(MaybeRelocatable) with segment_index < 0 and // there are applicable relocation rules: assert_eq!( - memory.relocate_value(&MaybeRelocatable::RelocatableValue((-1, 0).into())), + memory + .relocate_value(&MaybeRelocatable::RelocatableValue((-1, 0).into())) + .unwrap(), Cow::Owned(MaybeRelocatable::RelocatableValue((2, 0).into())), ); assert_eq!( - memory.relocate_value(&MaybeRelocatable::RelocatableValue((-2, 0).into())), + memory + .relocate_value(&MaybeRelocatable::RelocatableValue((-2, 0).into())) + .unwrap(), Cow::Owned(MaybeRelocatable::RelocatableValue((2, 2).into())), ); assert_eq!( - memory.relocate_value(&MaybeRelocatable::RelocatableValue((-1, 5).into())), + memory + .relocate_value(&MaybeRelocatable::RelocatableValue((-1, 5).into())) + .unwrap(), Cow::Owned(MaybeRelocatable::RelocatableValue((2, 5).into())), ); assert_eq!( - memory.relocate_value(&MaybeRelocatable::RelocatableValue((-2, 5).into())), + memory + .relocate_value(&MaybeRelocatable::RelocatableValue((-2, 5).into())) + .unwrap(), Cow::Owned(MaybeRelocatable::RelocatableValue((2, 7).into())), ); } @@ -1498,11 +1516,11 @@ mod memory_tests { .unwrap(); assert_eq!( - Memory::relocate_address((-1, 0).into(), &memory.relocation_rules), + Memory::relocate_address((-1, 0).into(), &memory.relocation_rules).unwrap(), MaybeRelocatable::RelocatableValue((2, 0).into()), ); assert_eq!( - Memory::relocate_address((-2, 1).into(), &memory.relocation_rules), + Memory::relocate_address((-2, 1).into(), &memory.relocation_rules).unwrap(), MaybeRelocatable::RelocatableValue((2, 3).into()), ); } @@ -1512,11 +1530,11 @@ mod memory_tests { fn relocate_address_no_rules() { let memory = Memory::new(); assert_eq!( - Memory::relocate_address((-1, 0).into(), &memory.relocation_rules), + Memory::relocate_address((-1, 0).into(), &memory.relocation_rules).unwrap(), MaybeRelocatable::RelocatableValue((-1, 0).into()), ); assert_eq!( - Memory::relocate_address((-2, 1).into(), &memory.relocation_rules), + Memory::relocate_address((-2, 1).into(), &memory.relocation_rules).unwrap(), MaybeRelocatable::RelocatableValue((-2, 1).into()), ); } @@ -1526,11 +1544,11 @@ mod memory_tests { fn relocate_address_real_addr() { let memory = Memory::new(); assert_eq!( - Memory::relocate_address((1, 0).into(), &memory.relocation_rules), + Memory::relocate_address((1, 0).into(), &memory.relocation_rules).unwrap(), MaybeRelocatable::RelocatableValue((1, 0).into()), ); assert_eq!( - Memory::relocate_address((1, 1).into(), &memory.relocation_rules), + Memory::relocate_address((1, 1).into(), &memory.relocation_rules).unwrap(), MaybeRelocatable::RelocatableValue((1, 1).into()), ); }