Naor/reapply/cairo function runner after revert

[naor-starkware]

TITLE

Description

Description of the pull request changes and motivation.

Checklist

• Linked to Github Issue
• Unit tests added
• Integration tests added.
• This change requires new documentation.
  • Documentation has been added/updated.
  • CHANGELOG has been updated.

---

This change is 

[github-actions]

**Hyper Thereading Benchmark results**




hyperfine -r 2 -n "hyper_threading_main threads: 1" 'RAYON_NUM_THREADS=1 ./hyper_threading_main' -n "hyper_threading_pr threads: 1" 'RAYON_NUM_THREADS=1 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 1
  Time (mean ± σ):     20.630 s ±  0.054 s    [User: 19.437 s, System: 1.189 s]
  Range (min … max):   20.592 s … 20.668 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 1
  Time (mean ± σ):     20.666 s ±  0.013 s    [User: 19.497 s, System: 1.165 s]
  Range (min … max):   20.657 s … 20.676 s    2 runs
 
Summary
  hyper_threading_main threads: 1 ran
    1.00 ± 0.00 times faster than hyper_threading_pr threads: 1




hyperfine -r 2 -n "hyper_threading_main threads: 2" 'RAYON_NUM_THREADS=2 ./hyper_threading_main' -n "hyper_threading_pr threads: 2" 'RAYON_NUM_THREADS=2 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 2
  Time (mean ± σ):     11.120 s ±  0.015 s    [User: 19.480 s, System: 1.213 s]
  Range (min … max):   11.109 s … 11.131 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 2
  Time (mean ± σ):     11.181 s ±  0.037 s    [User: 19.575 s, System: 1.202 s]
  Range (min … max):   11.154 s … 11.207 s    2 runs
 
Summary
  hyper_threading_main threads: 2 ran
    1.01 ± 0.00 times faster than hyper_threading_pr threads: 2




hyperfine -r 2 -n "hyper_threading_main threads: 4" 'RAYON_NUM_THREADS=4 ./hyper_threading_main' -n "hyper_threading_pr threads: 4" 'RAYON_NUM_THREADS=4 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 4
  Time (mean ± σ):     10.420 s ±  0.126 s    [User: 38.671 s, System: 1.326 s]
  Range (min … max):   10.331 s … 10.510 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 4
  Time (mean ± σ):     10.464 s ±  0.076 s    [User: 39.041 s, System: 1.293 s]
  Range (min … max):   10.410 s … 10.517 s    2 runs
 
Summary
  hyper_threading_main threads: 4 ran
    1.00 ± 0.01 times faster than hyper_threading_pr threads: 4




hyperfine -r 2 -n "hyper_threading_main threads: 6" 'RAYON_NUM_THREADS=6 ./hyper_threading_main' -n "hyper_threading_pr threads: 6" 'RAYON_NUM_THREADS=6 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 6
  Time (mean ± σ):     10.573 s ±  0.251 s    [User: 38.244 s, System: 1.252 s]
  Range (min … max):   10.395 s … 10.751 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 6
  Time (mean ± σ):     10.697 s ±  0.349 s    [User: 39.273 s, System: 1.250 s]
  Range (min … max):   10.450 s … 10.944 s    2 runs
 
Summary
  hyper_threading_main threads: 6 ran
    1.01 ± 0.04 times faster than hyper_threading_pr threads: 6




hyperfine -r 2 -n "hyper_threading_main threads: 8" 'RAYON_NUM_THREADS=8 ./hyper_threading_main' -n "hyper_threading_pr threads: 8" 'RAYON_NUM_THREADS=8 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 8
  Time (mean ± σ):     10.469 s ±  0.100 s    [User: 39.063 s, System: 1.189 s]
  Range (min … max):   10.398 s … 10.539 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 8
  Time (mean ± σ):     10.658 s ±  0.203 s    [User: 39.501 s, System: 1.158 s]
  Range (min … max):   10.514 s … 10.801 s    2 runs
 
Summary
  hyper_threading_main threads: 8 ran
    1.02 ± 0.02 times faster than hyper_threading_pr threads: 8




hyperfine -r 2 -n "hyper_threading_main threads: 16" 'RAYON_NUM_THREADS=16 ./hyper_threading_main' -n "hyper_threading_pr threads: 16" 'RAYON_NUM_THREADS=16 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 16
  Time (mean ± σ):     10.744 s ±  0.076 s    [User: 39.323 s, System: 1.252 s]
  Range (min … max):   10.690 s … 10.797 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 16
  Time (mean ± σ):     10.682 s ±  0.086 s    [User: 39.940 s, System: 1.272 s]
  Range (min … max):   10.621 s … 10.743 s    2 runs
 
Summary
  hyper_threading_pr threads: 16 ran
    1.01 ± 0.01 times faster than hyper_threading_main threads: 16

[github-actions]

Benchmark Results for unmodified programs 🚀

Command	Mean [s]	Min [s]	Max [s]	Relative
base big_factorial	2.142 ± 0.021	2.119	2.181	1.01 ± 0.02
head big_factorial	2.121 ± 0.034	2.099	2.212	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base big_fibonacci	2.081 ± 0.019	2.055	2.122	1.02 ± 0.01
head big_fibonacci	2.045 ± 0.014	2.031	2.079	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base blake2s_integration_benchmark	7.409 ± 0.022	7.363	7.438	1.00
head blake2s_integration_benchmark	7.497 ± 0.146	7.382	7.830	1.01 ± 0.02

Command	Mean [s]	Min [s]	Max [s]	Relative
base compare_arrays_200000	2.205 ± 0.020	2.178	2.242	1.01 ± 0.01
head compare_arrays_200000	2.174 ± 0.015	2.161	2.213	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base dict_integration_benchmark	1.449 ± 0.016	1.432	1.480	1.03 ± 0.01
head dict_integration_benchmark	1.410 ± 0.007	1.403	1.427	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base field_arithmetic_get_square_benchmark	1.232 ± 0.013	1.220	1.264	1.01 ± 0.01
head field_arithmetic_get_square_benchmark	1.220 ± 0.005	1.216	1.233	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base integration_builtins	7.520 ± 0.022	7.490	7.558	1.00 ± 0.00
head integration_builtins	7.518 ± 0.028	7.480	7.559	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base keccak_integration_benchmark	7.707 ± 0.182	7.580	8.129	1.01 ± 0.03
head keccak_integration_benchmark	7.629 ± 0.059	7.571	7.756	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base linear_search	2.188 ± 0.014	2.167	2.206	1.00 ± 0.01
head linear_search	2.177 ± 0.010	2.162	2.197	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base math_cmp_and_pow_integration_benchmark	1.519 ± 0.008	1.511	1.538	1.01 ± 0.01
head math_cmp_and_pow_integration_benchmark	1.498 ± 0.009	1.488	1.521	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base math_integration_benchmark	1.464 ± 0.004	1.457	1.469	1.00 ± 0.01
head math_integration_benchmark	1.457 ± 0.006	1.449	1.471	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base memory_integration_benchmark	1.240 ± 0.017	1.225	1.282	1.01 ± 0.02
head memory_integration_benchmark	1.230 ± 0.010	1.216	1.248	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base operations_with_data_structures_benchmarks	1.573 ± 0.019	1.552	1.603	1.00 ± 0.02
head operations_with_data_structures_benchmarks	1.573 ± 0.016	1.540	1.593	1.00

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base pedersen	536.3 ± 1.3	534.3	539.2	1.00 ± 0.01
head pedersen	533.8 ± 2.4	531.5	539.9	1.00

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base poseidon_integration_benchmark	619.4 ± 3.9	614.6	628.9	1.00
head poseidon_integration_benchmark	619.5 ± 4.4	613.4	629.3	1.00 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
base secp_integration_benchmark	1.839 ± 0.014	1.825	1.869	1.01 ± 0.01
head secp_integration_benchmark	1.818 ± 0.009	1.808	1.839	1.00

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base set_integration_benchmark	637.5 ± 3.0	633.7	643.2	1.00
head set_integration_benchmark	665.8 ± 3.4	662.8	672.6	1.04 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
base uint256_integration_benchmark	4.273 ± 0.018	4.246	4.310	1.00 ± 0.01
head uint256_integration_benchmark	4.266 ± 0.020	4.244	4.312	1.00

[OmriEshhar1]

@OmriEshhar1 reviewed 10 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on efrat-starkware, igaray, juanbono, pefontana, and YairVaknin-starkware).

[efrat-starkware]

@efrat-starkware reviewed 4 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on igaray, juanbono, pefontana, and YairVaknin-starkware).

[YairVaknin-starkware]

@YairVaknin-starkware reviewed 1 file and all commit messages, and made 8 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on igaray, juanbono, naor-starkware, and pefontana).

---

CHANGELOG.md line 23 at r1 (raw file):

* chore: Add `CairoFunctionRunner` for running Cairo entrypoints by name or PC, and broaden `CairoArg`/`MaybeRelocatable` conversions to support primitive signed/unsigned integers and big integers [#2352](https://github.com/lambdaclass/cairo-vm/pull/2352)

* chore: Add unit tests for `CairoFunctionRunner`, `CairoArg` conversions/macros, and `MaybeRelocatable` conversion macro coverage [#2354](https://github.com/lambdaclass/cairo-vm/pull/2354)

Why are there two added PRs here? Also, pls change the PR to the new one (this one) after the revert.

Code quote:

* chore: Add `CairoFunctionRunner` for running Cairo entrypoints by name or PC, and broaden `CairoArg`/`MaybeRelocatable` conversions to support primitive signed/unsigned integers and big integers [#2352](https://github.com/lambdaclass/cairo-vm/pull/2352)

* chore: Add unit tests for `CairoFunctionRunner`, `CairoArg` conversions/macros, and `MaybeRelocatable` conversion macro coverage [#2354](https://github.com/lambdaclass/cairo-vm/pull/2354)

---

vm/src/math_utils/mod.rs line 400 at r1 (raw file):

// Simplified as a & p are nonnegative
// Asumes p is a prime number
pub(crate) fn is_quad_residue(a: &BigUint, p: &BigUint) -> Result<bool, MathError> {

why?

Code quote:

pub(crate) fn is_quad_residue

---

vm/src/types/relocatable.rs line 109 at r1 (raw file):

    u8, u16, u32, u64, u128, usize, i8, i16, i32, i64, i128, isize, BigUint, BigInt, &BigUint,
    &BigInt
);

Do we really need all of these?

Code quote:

impl_from_for_maybe_relocatable!(
    u8, u16, u32, u64, u128, usize, i8, i16, i32, i64, i128, isize, BigUint, BigInt, &BigUint,
    &BigInt
);

---

vm/src/vm/runners/cairo_function_runner.rs line 53 at r1 (raw file):

        $runner.vm.builtin_runners.push($builtin.into());
    };
}

Isn't this overkill? Why not make this a method of the runner's? I prefer to avoid macros unless it's a must/ much cleaner.

Code quote:

/// Pushes a builtin runner into `runner.vm.builtin_runners` after converting it with `.into()`.
///
/// Example expansion:
/// `push_builtin!(runner, HashBuiltinRunner::new(Some(32), true));`
/// becomes:
/// `runner.vm.builtin_runners.push(HashBuiltinRunner::new(Some(32), true).into());`
macro_rules! push_builtin {
    ($runner:expr, $builtin:expr) => {
        $runner.vm.builtin_runners.push($builtin.into());
    };
}

---

vm/src/vm/runners/cairo_function_runner.rs line 67 at r1 (raw file):

        &mut self.runner
    }
}

This isn't a very good design choice in rust in my opinion. Makes the CairoFunctionRunner wrapper redundant. I would prefer you access self.runner, or consider just having the added functionality in the runner itself. I'm not convinced we need a CairoFunctionRunner struct...

Code quote:

impl std::ops::Deref for CairoFunctionRunner {
    type Target = CairoRunner;

    fn deref(&self) -> &Self::Target {
        &self.runner
    }
}

impl std::ops::DerefMut for CairoFunctionRunner {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.runner
    }
}

---

vm/src/vm/runners/cairo_function_runner.rs line 128 at r1 (raw file):

    /// Initializes a fixed set of 11 builtins used by this function runner.
    fn initialize_all_builtins(runner: &mut CairoRunner) -> Result<(), RunnerError> {
        runner.vm.builtin_runners.clear();

Maybe you should just fail if it's not empty? It doesn't seem we ever expect this to be invoked on a non new runner.

Code quote:

runner.vm.builtin_runners.clear();

---

vm/src/vm/runners/cairo_function_runner.rs line 171 at r1 (raw file):

    ///   verification fails.
    #[allow(clippy::result_large_err)]
    pub fn run(

Do we really need both this and run_from_entrypoint? Just add the logic from run_from_entrypoint here.

Code quote:

    pub fn run(

---

vm/src/vm/runners/mod.rs line 4 at r1 (raw file):

pub mod cairo_function_runner;
#[cfg(test)]
mod cairo_function_runner_test;

Maybe add the tests to mod cairo_function_runner, the same way it is across the repo?

Code quote:

pub mod cairo_function_runner;
#[cfg(test)]
mod cairo_function_runner_test;

[naor-starkware]

vm/src/vm/runners/cairo_function_runner.rs line 67 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

This isn't a very good design choice in rust in my opinion. Makes the CairoFunctionRunner wrapper redundant. I would prefer you access self.runner, or consider just having the added functionality in the runner itself. I'm not convinced we need a CairoFunctionRunner struct...

I discussed this design with Omri, and we decided to go with this approach to maintain a clear separation between production code and test code.
In addition, the use of Deref to CairoRunner was introduced following Efrat’s suggestion, in order to improve code cleanliness and avoid repetitive patterns

[naor-starkware]

vm/src/vm/runners/cairo_function_runner.rs line 171 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

Do we really need both this and run_from_entrypoint? Just add the logic from run_from_entrypoint here.

I think it makes sense to keep these functions separate for readability and clarity.

The run function is responsible for preparing and initializing all the arguments required by run_from_entrypoint, while run_from_entrypoint contains the actual execution logic.

This also allows us to introduce additional run variants in the future for different use cases that may require different initialization flows, which we’ll likely need as the code evolves

[YairVaknin-starkware]

@YairVaknin-starkware made 2 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on igaray, juanbono, naor-starkware, and pefontana).

---

vm/src/vm/runners/cairo_function_runner.rs line 67 at r1 (raw file):

Previously, naor-starkware wrote…

I discussed this design with Omri, and we decided to go with this approach to maintain a clear separation between production code and test code.
In addition, the use of Deref to CairoRunner was introduced following Efrat’s suggestion, in order to improve code cleanliness and avoid repetitive patterns

You can create a trait and have the runner impl it instead of this and/or introduce the added functionality under a feature flag. It's such a shallow wrapper and I don't see a justification for it.

---

vm/src/vm/runners/cairo_function_runner.rs line 171 at r1 (raw file):

Previously, naor-starkware wrote…

I think it makes sense to keep these functions separate for readability and clarity.

The run function is responsible for preparing and initializing all the arguments required by run_from_entrypoint, while run_from_entrypoint contains the actual execution logic.

This also allows us to introduce additional run variants in the future for different use cases that may require different initialization flows, which we’ll likely need as the code evolves

It's not good practice to leave unecessary code in case it's needed in the future. The run function doesn't do anything much aside from calling run_from_entrypoint. I don't see the point in both and it's less clear/readable imo.

[naor-starkware]

vm/src/math_utils/mod.rs line 400 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

why?

This is a bit of messiness on my side across PRs, but I’ll need this for my next PR for the math.cairo tests.

[YairVaknin-starkware]

@YairVaknin-starkware made 1 comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on igaray, juanbono, naor-starkware, and pefontana).

---

vm/src/math_utils/mod.rs line 400 at r1 (raw file):

Previously, naor-starkware wrote…

This is a bit of messiness on my side across PRs, but I’ll need this for my next PR for the math.cairo tests.

Pls revert for this PR, then

[naor-starkware]

vm/src/types/relocatable.rs line 109 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

Do we really need all of these?

This effectively closes the transitivity: each of these types can be converted into Felt, and Felt can be converted into MaybeRelocatable.

This makes the usage much more convenient, as it avoids having to write conversions like
mr_value = MaybeRelocatable::from(Felt252::from(value)) every time.

[naor-starkware]

vm/src/vm/runners/cairo_function_runner.rs line 53 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

Isn't this overkill? Why not make this a method of the runner's? I prefer to avoid macros unless it's a must/ much cleaner.

The previous implementation initialized all the builtins manually. Following Efrat’s suggestion, I refactored this into a macro to make the code cleaner and more maintainable.

[YairVaknin-starkware]

@YairVaknin-starkware made 2 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on igaray, juanbono, naor-starkware, and pefontana).

---

vm/src/types/relocatable.rs line 109 at r1 (raw file):

Previously, naor-starkware wrote…

This effectively closes the transitivity: each of these types can be converted into Felt, and Felt can be converted into MaybeRelocatable.

This makes the usage much more convenient, as it avoids having to write conversions like
mr_value = MaybeRelocatable::from(Felt252::from(value)) every time.

Do we have usages for all theses types?

---

vm/src/vm/runners/cairo_function_runner.rs line 53 at r1 (raw file):

Previously, naor-starkware wrote…

The previous implementation initialized all the builtins manually. Following Efrat’s suggestion, I refactored this into a macro to make the code cleaner and more maintainable.

Why not make it a method? Nothing here requires a macro is what I mean.

[YairVaknin-starkware]

@YairVaknin-starkware made 1 comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on igaray, juanbono, naor-starkware, and pefontana).

---

vm/src/vm/runners/cairo_function_runner.rs line 53 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

Why not make it a method? Nothing here requires a macro is what I mean.

Something like fn push_builtin(&mut self, builtin: impl Into<BuiltinRunner>) { ....

[naor-starkware]

vm/src/vm/runners/cairo_function_runner.rs line 171 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

It's not good practice to leave unecessary code in case it's needed in the future. The run function doesn't do anything much aside from calling run_from_entrypoint. I don't see the point in both and it's less clear/readable imo.

I’ll need to wrap run_from_entrypoint, since some Cairo files use a different HintProcessor, such as BootloaderHintProcessor or BuiltinHintProcessor. This also mirrors the execution flow of the Python VM

[naor-starkware]

CHANGELOG.md line 23 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

Why are there two added PRs here? Also, pls change the PR to the new one (this one) after the revert.

I initially split this into two PRs :
one for the logic and one for the tests .
but Codecov failed, so I combined them to get it passing.

[naor-starkware]

vm/src/types/relocatable.rs line 109 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

Do we have usages for all theses types?

Most types require this

[YairVaknin-starkware]

@YairVaknin-starkware made 2 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on igaray, juanbono, naor-starkware, and pefontana).

---

CHANGELOG.md line 23 at r1 (raw file):

Previously, naor-starkware wrote…

I initially split this into two PRs :
one for the logic and one for the tests .
but Codecov failed, so I combined them to get it passing.

Okay, so create a single cahngelog entry with the correct PR number.

---

vm/src/vm/runners/cairo_function_runner.rs line 171 at r1 (raw file):

Previously, naor-starkware wrote…

I’ll need to wrap run_from_entrypoint, since some Cairo files use a different HintProcessor, such as BootloaderHintProcessor or BuiltinHintProcessor. This also mirrors the execution flow of the Python VM

wdym? You can pass a different hint processor to run or am I missing your point?

[YairVaknin-starkware]

@YairVaknin-starkware made 1 comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on igaray, juanbono, naor-starkware, and pefontana).

---

vm/src/types/relocatable.rs line 109 at r1 (raw file):

Previously, naor-starkware wrote…

Most types require this

cursor scan didn't find any usages for u16, u32, u64, u128, i8, i16, i128, isize. Let's add them when/if needed.

[naor-starkware]

vm/src/vm/runners/cairo_function_runner.rs line 171 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

wdym? You can pass a different hint processor to run or am I missing your point?

Would it make more sense for each run_from_entrypoint wrapper to include its own run setup logic?

For example, for the cairo0 libs tests, I could introduce a wrapper like run_default_cairo0 with:

• verify_secure = false
• program_segment_size = None
• hint_processor = BuiltinHintProcessor

This way, each wrapper would define the initialization flow according to its specific use case.

[naor-starkware]

vm/src/vm/runners/cairo_function_runner.rs line 128 at r1 (raw file):

Previously, naor-starkware wrote…

Done.

I think we shouldn’t fail it, since it’s for tests

[naor-starkware]

vm/src/tests/mod.rs line 264 at r2 (raw file):

Previously, YairVaknin-starkware wrote…

revert?

The 0.into() was ambiguous because MaybeRelocatable implements From for multiple integer types.
Adding _i64 tells the compiler which conversion to use.

[naor-starkware]

vm/src/vm/runners/function_runner.rs line 119 at r2 (raw file):

Previously, YairVaknin-starkware wrote…

Can you add a separate unit test for this as well?

I wrote these two tests:
get_function_pc_assert_nn_resolves_alias_to_pc_0
get_function_pc_assert_nn_manual_implementation_returns_pc_4

Or should I add more?

[naor-starkware]

@naor-starkware made 6 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on igaray, juanbono, pefontana, and YairVaknin-starkware).

---

CHANGELOG.md line 23 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

Okay, so create a single cahngelog entry with the correct PR number.

Done.

---

vm/src/types/relocatable.rs line 109 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

Some of the types you listed are still used in the state of the code of this PR, so could leave those.

Done.

---

vm/src/vm/runners/mod.rs line 4 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

Maybe add the tests to mod cairo_function_runner, the same way it is across the repo?

Done.

---

vm/src/math_utils/mod.rs line 400 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

Pls revert for this PR, then

Done.

---

vm/src/vm/runners/cairo_function_runner.rs line 53 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

something like:
fn push_builtin(&mut self, builtin: impl Into<BuiltinRunner>) { self.vm.builtin_runners.push(builtin.into()); }
Then use it like your macro.

Done.

---

vm/src/vm/runners/cairo_function_runner.rs line 67 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

Yeah, you can use a "factory" function to create the runner with the init logic that's needed. Just call this instead of calling CairoFunctionRunner::new. This is the same overhead in this regard. It's just that it's a shallow wrapper that doesnt have any additional state justifying it.

Done.

[YairVaknin-starkware]

@YairVaknin-starkware made 3 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on igaray, juanbono, naor-starkware, and pefontana).

---

vm/src/tests/mod.rs line 264 at r2 (raw file):

Previously, naor-starkware wrote…

The 0.into() was ambiguous because MaybeRelocatable implements From for multiple integer types.
Adding _i64 tells the compiler which conversion to use.

I think it's fine. The compiler knows how to handle it. It defaults to i32.
Please try to remove these from this PR as much as possible (I see you used this in a lot of places). This make the code much messier and isn't necessary.

---

vm/src/vm/runners/function_runner.rs line 119 at r2 (raw file):

Previously, naor-starkware wrote…

I wrote these two tests:
get_function_pc_assert_nn_resolves_alias_to_pc_0
get_function_pc_assert_nn_manual_implementation_returns_pc_4

Or should I add more?

I meant a one calling get_pc_from_identifier directly.

---

vm/src/tests/mod.rs line 149 at r2 (raw file):

        0_i64.into(),
        0_i64.into(),
        0_i64.into(),

also here

Code quote:

        0_i64.into(),
        0_i64.into(),
        0_i64.into(),
        0_i64.into(),

[YairVaknin-starkware]

@YairVaknin-starkware made 1 comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on igaray, juanbono, naor-starkware, and pefontana).

---

vm/src/vm/runners/cairo_function_runner.rs line 128 at r1 (raw file):

Previously, naor-starkware wrote…

I think we shouldn’t fail it, since it’s for tests

I still see you clearing at the start of pub fn initialize_all_builtins

[naor-starkware]

@naor-starkware made 3 comments.
Reviewable status: 9 of 11 files reviewed, 4 unresolved discussions (waiting on igaray, juanbono, pefontana, and YairVaknin-starkware).

---

vm/src/tests/mod.rs line 264 at r2 (raw file):

Previously, YairVaknin-starkware wrote…

I think it's fine. The compiler knows how to handle it. It defaults to i32.
Please try to remove these from this PR as much as possible (I see you used this in a lot of places). This make the code much messier and isn't necessary.

Done.

---

vm/src/vm/runners/function_runner.rs line 119 at r2 (raw file):

Previously, YairVaknin-starkware wrote…

I meant a one calling get_pc_from_identifier directly.

Done.

---

vm/src/vm/runners/cairo_function_runner.rs line 128 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

I still see you clearing at the start of pub fn initialize_all_builtins

Done.

[YairVaknin-starkware]

@YairVaknin-starkware made 1 comment and resolved 3 discussions.
Reviewable status: 9 of 11 files reviewed, 1 unresolved discussion (waiting on igaray, juanbono, naor-starkware, and pefontana).

---

vm/src/vm/runners/function_runner.rs line 119 at r2 (raw file):

Previously, naor-starkware wrote…

Done.

Can you also add one also for the happy flow? I see you only added error-case tests.

[YairVaknin-starkware]

@YairVaknin-starkware reviewed 2 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on igaray, juanbono, naor-starkware, and pefontana).

[naor-starkware]

@naor-starkware made 1 comment.
Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on igaray, juanbono, pefontana, and YairVaknin-starkware).

---

vm/src/vm/runners/function_runner.rs line 119 at r2 (raw file):

Previously, YairVaknin-starkware wrote…

Can you also add one also for the happy flow? I see you only added error-case tests.

Done.

[codecov]

Codecov Report

❌ Patch coverage is 98.50374% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.07%. Comparing base (cf6a26b) to head (7621d3c).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
vm/src/vm/runners/function_runner.rs	98.28%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2359      +/-   ##
==========================================
+ Coverage   96.02%   96.07%   +0.04%     
==========================================
  Files         104      105       +1     
  Lines       37470    37737     +267     
==========================================
+ Hits        35979    36254     +275     
+ Misses       1491     1483       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[YairVaknin-starkware]

@YairVaknin-starkware reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on igaray, juanbono, and pefontana).