Sort_ecdsa_and_mod_builtins_private_inputs_by_idx

[YairVaknin-starkware]

TITLE

Sort_ecdsa_and_mod_builtins_private_inputs_by_idx

Description

Making sure to sort the resulted list of those builtins in the air_private_input_file by index, so it is aligned with the python vm's outputs.

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.

[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 ± σ):     33.592 s ±  0.641 s    [User: 32.856 s, System: 0.735 s]
  Range (min … max):   33.139 s … 34.045 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 1
  Time (mean ± σ):     31.939 s ±  0.056 s    [User: 31.132 s, System: 0.806 s]
  Range (min … max):   31.900 s … 31.979 s    2 runs
 
Summary
  'hyper_threading_pr threads: 1' ran
    1.05 ± 0.02 times faster than 'hyper_threading_main 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 ± σ):     18.290 s ±  0.034 s    [User: 32.016 s, System: 0.832 s]
  Range (min … max):   18.266 s … 18.315 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 2
  Time (mean ± σ):     17.676 s ±  0.026 s    [User: 30.856 s, System: 0.830 s]
  Range (min … max):   17.658 s … 17.695 s    2 runs
 
Summary
  'hyper_threading_pr threads: 2' ran
    1.03 ± 0.00 times faster than 'hyper_threading_main 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 ± σ):     12.245 s ±  0.381 s    [User: 43.299 s, System: 0.997 s]
  Range (min … max):   11.975 s … 12.514 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 4
  Time (mean ± σ):     11.905 s ±  0.259 s    [User: 43.324 s, System: 0.947 s]
  Range (min … max):   11.722 s … 12.089 s    2 runs
 
Summary
  'hyper_threading_pr threads: 4' ran
    1.03 ± 0.04 times faster than 'hyper_threading_main 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 ± σ):     12.335 s ±  0.373 s    [User: 44.126 s, System: 1.054 s]
  Range (min … max):   12.072 s … 12.599 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 6
  Time (mean ± σ):     12.012 s ±  0.132 s    [User: 43.611 s, System: 0.990 s]
  Range (min … max):   11.918 s … 12.105 s    2 runs
 
Summary
  'hyper_threading_pr threads: 6' ran
    1.03 ± 0.03 times faster than 'hyper_threading_main 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 ± σ):     11.830 s ±  0.018 s    [User: 44.016 s, System: 1.055 s]
  Range (min … max):   11.817 s … 11.843 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 8
  Time (mean ± σ):     11.885 s ±  0.160 s    [User: 44.062 s, System: 1.042 s]
  Range (min … max):   11.771 s … 11.998 s    2 runs
 
Summary
  'hyper_threading_main threads: 8' ran
    1.00 ± 0.01 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 ± σ):     12.023 s ±  0.171 s    [User: 44.798 s, System: 1.099 s]
  Range (min … max):   11.903 s … 12.144 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 16
  Time (mean ± σ):     11.624 s ±  0.075 s    [User: 44.454 s, System: 1.028 s]
  Range (min … max):   11.571 s … 11.677 s    2 runs
 
Summary
  'hyper_threading_pr threads: 16' ran
    1.03 ± 0.02 times faster than 'hyper_threading_main threads: 16'

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.33%. Comparing base (6025257) to head (adfe27b).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1851   +/-   ##
=======================================
  Coverage   96.32%   96.33%           
=======================================
  Files         102      102           
  Lines       40355    40427   +72     
=======================================
+ Hits        38872    38944   +72     
  Misses       1483     1483           

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

[github-actions]

Benchmark Results for unmodified programs 🚀

Command	Mean [s]	Min [s]	Max [s]	Relative
base big_factorial	2.520 ± 0.076	2.463	2.694	1.01 ± 0.03
head big_factorial	2.491 ± 0.015	2.469	2.513	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base big_fibonacci	2.455 ± 0.048	2.401	2.560	1.01 ± 0.03
head big_fibonacci	2.442 ± 0.046	2.409	2.569	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base blake2s_integration_benchmark	9.115 ± 0.074	9.022	9.235	1.00
head blake2s_integration_benchmark	9.278 ± 0.344	9.029	10.221	1.02 ± 0.04

Command	Mean [s]	Min [s]	Max [s]	Relative
base compare_arrays_200000	2.564 ± 0.027	2.539	2.627	1.00
head compare_arrays_200000	2.570 ± 0.033	2.540	2.646	1.00 ± 0.02

Command	Mean [s]	Min [s]	Max [s]	Relative
base dict_integration_benchmark	1.696 ± 0.035	1.666	1.788	1.01 ± 0.02
head dict_integration_benchmark	1.687 ± 0.014	1.661	1.715	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base field_arithmetic_get_square_benchmark	1.425 ± 0.008	1.417	1.440	1.00
head field_arithmetic_get_square_benchmark	1.428 ± 0.009	1.414	1.446	1.00 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
base integration_builtins	9.148 ± 0.062	9.086	9.261	1.00
head integration_builtins	9.275 ± 0.396	9.074	10.392	1.01 ± 0.04

Command	Mean [s]	Min [s]	Max [s]	Relative
base keccak_integration_benchmark	9.496 ± 0.224	9.336	10.078	1.00
head keccak_integration_benchmark	9.543 ± 0.176	9.392	9.984	1.00 ± 0.03

Command	Mean [s]	Min [s]	Max [s]	Relative
base linear_search	2.583 ± 0.040	2.537	2.675	1.00
head linear_search	2.607 ± 0.058	2.556	2.754	1.01 ± 0.03

Command	Mean [s]	Min [s]	Max [s]	Relative
base math_cmp_and_pow_integration_benchmark	1.750 ± 0.011	1.726	1.761	1.01 ± 0.02
head math_cmp_and_pow_integration_benchmark	1.738 ± 0.030	1.711	1.817	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base math_integration_benchmark	1.686 ± 0.017	1.673	1.714	1.00
head math_integration_benchmark	1.697 ± 0.035	1.675	1.792	1.01 ± 0.02

Command	Mean [s]	Min [s]	Max [s]	Relative
base memory_integration_benchmark	1.438 ± 0.023	1.414	1.500	1.00 ± 0.02
head memory_integration_benchmark	1.432 ± 0.008	1.424	1.444	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base operations_with_data_structures_benchmarks	1.818 ± 0.037	1.790	1.914	1.00 ± 0.02
head operations_with_data_structures_benchmarks	1.815 ± 0.008	1.800	1.827	1.00

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base pedersen	597.9 ± 3.2	593.1	604.4	1.01 ± 0.01
head pedersen	593.2 ± 5.3	587.0	605.1	1.00

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base poseidon_integration_benchmark	703.7 ± 6.4	697.1	719.3	1.00
head poseidon_integration_benchmark	705.8 ± 8.1	697.3	725.3	1.00 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
base secp_integration_benchmark	2.075 ± 0.017	2.053	2.107	1.00 ± 0.01
head secp_integration_benchmark	2.072 ± 0.024	2.050	2.131	1.00

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base set_integration_benchmark	663.2 ± 4.6	657.5	674.0	1.00
head set_integration_benchmark	703.2 ± 3.9	698.3	712.3	1.06 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
base uint256_integration_benchmark	5.090 ± 0.111	5.007	5.397	1.01 ± 0.02
head uint256_integration_benchmark	5.027 ± 0.020	4.983	5.051	1.00

[pefontana]

Great @YairVaknin-starkware !
Is there a possibility to add a test? I don't know what these changes are for
If we could add an integration test, if not a unit test, it would be good

[YairVaknin-starkware]

Great @YairVaknin-starkware ! Is there a possibility to add a test? I don't know what these changes are for If we could add an integration test, if not a unit test, it would be good

@pefontana It's just sorting the array in the resulting air_private_input json output file according to indices, so comparing air_private_inputs using https://github.com/lambdaclass/cairo-vm/blob/main/vm/src/tests/air_private_input_comparator.py for example doesn't fail unnecessarily if either of those two builtins are included (when comparing to a python cairo run).
Does it still sound to you like it's necessary to add test?

[pefontana]

How much work is to add an error handling here?
If it is not possible at least add an error message to the user and a line documenting why it is not possible to get into the unreachable

[YairVaknin-starkware]

Added a msg here and also added a test for the signature's air_public_input fn.

[YairVaknin-starkware]

How much work is to add an error handling here?

If it is not possible at least add an error message to the user and a line documenting why it is not possible to get into the unreachable

I can add error handling, but I'm not sure how elegant it will be. Lmk if it's important, or if it's okay the way it is now.

[YairVaknin-starkware]

Also, currently air_private_input doesn't return a Result type for any builtin variant. Do we want to panic or refactor the code that uses it? Because this case can never happen, I think if we do add an error panic is probably okay(?).

[pefontana]

Great @YairVaknin-starkware ! Is there a possibility to add a test? I don't know what these changes are for If we could add an integration test, if not a unit test, it would be good

@pefontana It's just sorting the array in the resulting air_private_input json output file according to indices, so comparing air_private_inputs using https://github.com/lambdaclass/cairo-vm/blob/main/vm/src/tests/air_private_input_comparator.py for example doesn't fail unnecessarily if either of those two builtins are included (when comparing to a python cairo run). Does it still sound to you like it's necessary to add test?

Okey! no problem, it is okey to don't add test in this case
It would be great if we can remove that unreachable and add some error handling

[JulianGCalderon]

Hi @YairVaknin-starkware! It would be great if you could remove the unreachable. I think there is a way to do it without changing the signature.

The air_private_input function iterates self.signatures and builds Vec<PrivateInput> with only the variant PrivateInput::Signature. Then, it is sorted by asserting that the variant is indeed Signature.

Instead of doing this, you could:

1. Iterate self.signatures and build a Vec<PrivateInputSignature> (the inner concrete type).
2. Sort the vector without needing an unreachable, as we don't have an enum anymore.
3. Build the Vec<PrivateInput> while preserving the order.

Let me know your thoughts on this approach.

[YairVaknin-starkware]

Hi @YairVaknin-starkware! It would be great if you could remove the unreachable. I think there is a way to do it without changing the signature.

The air_private_input function iterates self.signatures and builds Vec<PrivateInput> with only the variant PrivateInput::Signature. Then, it is sorted by asserting that the variant is indeed Signature.

Instead of doing this, you could:

1. Iterate self.signatures and build a Vec<PrivateInputSignature> (the inner concrete type).
2. Sort the vector without needing an unreachable, as we don't have an enum anymore.
3. Build the Vec<PrivateInput> while preserving the order.

Let me know your thoughts on this approach.

This might be the

JulianGCalderon

Hi @JulianGCalderon. I opted to sort the signatures before iterating. Please lmk what you think.