fix: Add boundary tests for SecpSplitOutOfRange in secp_utils #2062

VolodymyrBg · 2025-04-07T19:47:56Z

Description

Implemented the TODO in secp_utils.rs by adding proper boundary tests for the
bigint3_split function. Tests now verify that values up to BASE³-1 work correctly,
while BASE³ and above properly trigger SecpSplitOutOfRange errors. This provides
complete test coverage for the function's range validation.

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.

JulianGCalderon · 2025-04-14T13:54:51Z

Hi @VolodymyrBg, thanks for the contribution! Could you fix the CI lints?

VolodymyrBg · 2025-04-14T18:04:27Z

Hi @VolodymyrBg, thanks for the contribution! Could you fix the CI lints?

Done , but i'm still not sure with these tests
Benchmark Hyper Threading / benchmark (pull_request) Failing after 8m
Details

Hyperfine Benchmark / Run benchmarks for unmodified programs (pull_request) Failing after 16m
i ran locally and they passed

JulianGCalderon · 2025-04-14T18:40:06Z

Hi @VolodymyrBg, thanks for the contribution! Could you fix the CI lints?

Done , but i'm still not sure with these tests Benchmark Hyper Threading / benchmark (pull_request) Failing after 8m Details

Hyperfine Benchmark / Run benchmarks for unmodified programs (pull_request) Failing after 16m i ran locally and they passed

Don't worry about those jobs, they always fail for external contributors.

VolodymyrBg · 2025-04-28T17:50:59Z

@JulianGCalderon Is this PR actual, or should I close it?

JulianGCalderon · 2025-04-28T20:53:01Z

Hey @VolodymyrBg. We would like to keep only the changes in the test. Could you remove the optimization?

We don't have any benchmark to support that optimization, and given that we don't know how much it will improve, we prefer not to add complexity to the function.

VolodymyrBg · 2025-04-29T11:17:16Z

Hey @VolodymyrBg. We would like to keep only the changes in the test. Could you remove the optimization?

We don't have any benchmark to support that optimization, and given that we don't know how much it will improve, we prefer not to add complexity to the function.

Done

JulianGCalderon · 2025-04-29T13:06:09Z

Thanks @VolodymyrBg, could you also restore the changelog?

JulianGCalderon · 2025-04-29T14:55:01Z

Hey @VolodymyrBg, sorry for the confusion. What I meant is for you to remove the changelog entry entirely. We don't modify the changelog on test-only changes.

VolodymyrBg · 2025-04-29T15:19:46Z

Hey @VolodymyrBg, sorry for the confusion. What I meant is for you to remove the changelog entry entirely. We don't modify the changelog on test-only changes.

Sorry for misunderstanding

JulianGCalderon

LGTM

…class#2062) * fix: Add boundary tests for SecpSplitOutOfRange in secp_utils * Update secp_utils.rs * Update CHANGELOG.md * Update secp_utils.rs * remove optimizations * Update CHANGELOG.md * Update CHANGELOG.md * Update CHANGELOG.md * empty commit to fix CI --------- Co-authored-by: Julian Gonzalez Calderon <[email protected]>

* fix: Add boundary tests for SecpSplitOutOfRange in secp_utils * Update secp_utils.rs * Update CHANGELOG.md * Update secp_utils.rs * remove optimizations * Update CHANGELOG.md * Update CHANGELOG.md * Update CHANGELOG.md * empty commit to fix CI --------- Co-authored-by: Julian Gonzalez Calderon <[email protected]>

* Remove duplicated `get_val` (#2065) * avoid installing gmp if it is already installed * revert part of the last commit * changelog * remove unwanted file * CI: Make cache keys depend on Cairo version (#2056) * Add Makefile and requirements.txt to cache key * Don't fetch cairo stwo exclusive programs * Remove empty file * Restrict caching Cairo programs Revert how the Rust workflow uses the Cairo program paths to calculate the cache keys. This change would imply the branch only adds Makefile and requirements.txt to the cache key computation. --------- Co-authored-by: gabrielbosio <[email protected]> Co-authored-by: Gabriel Bosio <[email protected]> * dev: make Memory::get `pub` (#2039) * dev: make Memory::get `pub` * edit changelog * dev: expose get_maybe_relocatable instead * add comment on borrowed variant * add tests * feat(breaking): add support for hint accessible scopes (#2042) * feat: add support for hint accessible scopes update changelog * Update vm/src/hint_processor/hint_processor_definition.rs Co-authored-by: Julian Gonzalez Calderon <[email protected]> --------- Co-authored-by: Julian Gonzalez Calderon <[email protected]> * feat: add support for alias identifiers destination in program serde (#2071) * feat: add support for aliases destination in program serde * add changelog --------- Co-authored-by: Julián González Calderón <[email protected]> * fix: Add boundary tests for SecpSplitOutOfRange in secp_utils (#2062) * fix: Add boundary tests for SecpSplitOutOfRange in secp_utils * Update secp_utils.rs * Update CHANGELOG.md * Update secp_utils.rs * remove optimizations * Update CHANGELOG.md * Update CHANGELOG.md * Update CHANGELOG.md * empty commit to fix CI --------- Co-authored-by: Julian Gonzalez Calderon <[email protected]> * docs: add --run_from_cairo_pie documentation for Cairo 0 and Cairo 1 (#2077) * docs: add --run_from_cairo_pie flag usage documentation for Cairo 0 * docs: add --run_from_cairo_pie flag usage documentation for Cairo 1 * Update README.md Co-authored-by: Julian Gonzalez Calderon <[email protected]> * Update README.md * Update README.md * Update README.md Co-authored-by: Julian Gonzalez Calderon <[email protected]> * Update README.md Co-authored-by: Gabriel Bosio <[email protected]> * Update README.md Co-authored-by: Julian Gonzalez Calderon <[email protected]> --------- Co-authored-by: Julian Gonzalez Calderon <[email protected]> Co-authored-by: Gabriel Bosio <[email protected]> * Fix WRITE_DIVMOD_SEGMENT hint (#2078) * Minor fixes - use CAIRO_PRIME * Add error handling * Update CHANGELOG.md * Bump cairo-lang to 0.13.5 (#1959) * Update cairo-lang versin * Update changelog * Add new hints * Specify prime in new hints * Only apply mod floor if prime is not CAIRO_PRIME * Properly implement pack for a different prime * Fix pack in compute_doubling_slope * Add negative_points.cairo test * Add try_get_point_from_x for negative points * Add double_x test for negative points * Allow to many arguments * Add codecov to cairo-0-secp-hints feature * Prepare for Release v3.0.0-rc.1 (#2092) * Update version * Update lock * Update toolchain * Update changelog * Fix clippy * Update toolchain in CI * fix: correct cache keys for no-std test coverage in CI (#2093) * Update rust.yml * Update rust.yml * Update rust toolchain to 1.87.0 (#2100) * update rust toolchain to 1.87.0 * remove uneeded file * update rust in workflows * update changelog * clippy * clippy * remove unneeded clippy allow * fix Changelog link (#2104) * Improve memory_segments coverage (#2110) * Add tests for .gen_arg() and .write_arg() * Add test for .is_valid_memory_value() * Assert contents of memory segments --------- Co-authored-by: Franco Giachetta <[email protected]> Co-authored-by: Julian Gonzalez Calderon <[email protected]> Co-authored-by: Mathieu <[email protected]> Co-authored-by: VolodymyrBg <[email protected]> Co-authored-by: GarmashAlex <[email protected]> Co-authored-by: crStiv <[email protected]> Co-authored-by: DiegoC <[email protected]>

VolodymyrBg and others added 2 commits April 7, 2025 22:47

fix: Add boundary tests for SecpSplitOutOfRange in secp_utils

bbb670c

Merge branch 'main' into bgg

98062a2

JulianGCalderon added the tests Implementation of tests label Apr 14, 2025

VolodymyrBg added 3 commits April 14, 2025 20:52

Update secp_utils.rs

ec3ce4e

Update CHANGELOG.md

731d42e

Update secp_utils.rs

5da56c0

Merge branch 'main' into bgg

6d7de3d

remove optimizations

918fc32

Update CHANGELOG.md

25487b2

VolodymyrBg added 2 commits April 29, 2025 18:19

Update CHANGELOG.md

5300149

Update CHANGELOG.md

5c53c00

empty commit to fix CI

36337ec

JulianGCalderon approved these changes Apr 29, 2025

View reviewed changes

gabrielbosio mentioned this pull request Apr 29, 2025

[DNM] fix: Add boundary tests for SecpSplitOutOfRange in secp_utils #2082

Closed

gabrielbosio approved these changes Apr 29, 2025

View reviewed changes

gabrielbosio added this pull request to the merge queue Apr 29, 2025

Merged via the queue into lambdaclass:main with commit 3b36cd9 Apr 29, 2025
181 of 185 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Add boundary tests for SecpSplitOutOfRange in secp_utils #2062

fix: Add boundary tests for SecpSplitOutOfRange in secp_utils #2062

Uh oh!

VolodymyrBg commented Apr 7, 2025

Uh oh!

JulianGCalderon commented Apr 14, 2025 •

edited

Loading

Uh oh!

VolodymyrBg commented Apr 14, 2025

Uh oh!

JulianGCalderon commented Apr 14, 2025

Uh oh!

VolodymyrBg commented Apr 28, 2025

Uh oh!

JulianGCalderon commented Apr 28, 2025

Uh oh!

VolodymyrBg commented Apr 29, 2025

Uh oh!

JulianGCalderon commented Apr 29, 2025

Uh oh!

JulianGCalderon commented Apr 29, 2025

Uh oh!

VolodymyrBg commented Apr 29, 2025

Uh oh!

JulianGCalderon left a comment

Uh oh!

Uh oh!

Uh oh!

fix: Add boundary tests for SecpSplitOutOfRange in secp_utils #2062

fix: Add boundary tests for SecpSplitOutOfRange in secp_utils #2062

Uh oh!

Conversation

VolodymyrBg commented Apr 7, 2025

Description

Checklist

Uh oh!

JulianGCalderon commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VolodymyrBg commented Apr 14, 2025

Uh oh!

JulianGCalderon commented Apr 14, 2025

Uh oh!

VolodymyrBg commented Apr 28, 2025

Uh oh!

JulianGCalderon commented Apr 28, 2025

Uh oh!

VolodymyrBg commented Apr 29, 2025

Uh oh!

JulianGCalderon commented Apr 29, 2025

Uh oh!

JulianGCalderon commented Apr 29, 2025

Uh oh!

VolodymyrBg commented Apr 29, 2025

Uh oh!

JulianGCalderon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JulianGCalderon commented Apr 14, 2025 •

edited

Loading