fix(values): encode BoundedInt in its compact representation; deref boxed returns - #1657
Merged
Merged
Conversation
|
✅ Code is now correctly formatted. |
TomerStarkware
force-pushed
the
tomer/values_nested_box_nullable
branch
from
August 18, 2026 09:43
9bed290 to
c09a4f1
Compare
TomerStarkware
force-pushed
the
tomer/values_bounded_int_box_return
branch
from
August 18, 2026 09:43
bff9388 to
44e5111
Compare
TomerStarkware
force-pushed
the
tomer/values_nested_box_nullable
branch
from
August 18, 2026 15:39
c09a4f1 to
b7dd582
Compare
TomerStarkware
force-pushed
the
tomer/values_bounded_int_box_return
branch
2 times, most recently
from
August 19, 2026 16:07
86ba869 to
cc5f00f
Compare
TomerStarkware
force-pushed
the
tomer/values_nested_box_nullable
branch
from
August 19, 2026 16:07
b7dd582 to
3d5c68c
Compare
orizi
approved these changes
Aug 20, 2026
orizi
left a comment
Collaborator
There was a problem hiding this comment.
@orizi reviewed 6 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on TomerStarkware).
TomerStarkware
force-pushed
the
tomer/values_nested_box_nullable
branch
from
August 23, 2026 10:33
3d5c68c to
e42ae5e
Compare
…oxed returns Two remaining marshaling asymmetries: - `Value::to_ptr` wrote a BoundedInt as the un-biased value in a 32-byte felt slot, while the native representation (and `from_ptr`) is the compact one: `value - lower` stored in `repr_bit_width()` bits. A BoundedInt nested in an aggregate (top-level arguments are still gated by the arch.rs panic, #1217) was both silently mis-decoded and overran its slot, corrupting neighboring elements — `Array<BoundedInt<3, 10>>` returned garbage. - `parse_result` passed a returned `Box<T>`'s return-pointer slot straight to `from_ptr` without dereferencing it. Any function that uses a non-ZST builtin returns through a return pointer, so e.g. `fn(...) -> Box<felt252>` using pedersen decoded two raw heap addresses as the payload. Mirror the Nullable arm's deref. Add `RangeExt::repr_encode`/`repr_decode` as the single implementation of the compact encoding (felt-wrapping subtraction so negative lower bounds round-trip) and use the decode side from both existing duplicates (`Value::from_ptr` and the register path in `parse_result`). The `to_ptr` arm now also validates against the type's range rather than only the value's embedded one. Fixes `test_to_ptr_bounded_int_valid`, which asserted the buggy 32-byte encoding ([16, ...] instead of `value - lower` = [6, 0] in the 2-byte 9-bit representation), and its embedded range, which didn't match the type's (`BoundedInt<10, 510>` parses to the range `[10, 511)`). Adds a VM-vs-native test for a boxed return forced through the return pointer, and round-trip unit tests for `Array<BoundedInt<3, 10>>` (asserting the 1-byte biased element buffer) and a negative-lower range. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TomerStarkware
force-pushed
the
tomer/values_bounded_int_box_return
branch
from
August 23, 2026 12:21
cc5f00f to
e9f2176
Compare
Benchmarking resultsBenchmark for program
|
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
5.767 ± 0.053 | 5.695 | 5.888 | 5.56 ± 0.06 |
cairo-native (embedded AOT) |
1.037 ± 0.006 | 1.030 | 1.047 | 1.00 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
1.044 ± 0.005 | 1.039 | 1.052 | 1.01 ± 0.01 |
Benchmark for program dict_snapshot
Open benchmarks
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
306.2 ± 1.8 | 302.5 | 308.4 | 1.00 |
cairo-native (embedded AOT) |
929.8 ± 8.4 | 918.9 | 942.5 | 3.04 ± 0.03 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
946.7 ± 3.3 | 942.8 | 951.8 | 3.09 ± 0.02 |
Benchmark for program factorial_2M
Open benchmarks
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
2.499 ± 0.038 | 2.465 | 2.584 | 2.60 ± 0.04 |
cairo-native (embedded AOT) |
0.963 ± 0.004 | 0.957 | 0.970 | 1.00 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
0.973 ± 0.003 | 0.968 | 0.976 | 1.01 ± 0.01 |
Benchmark for program fib_2M
Open benchmarks
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
2.419 ± 0.029 | 2.386 | 2.484 | 2.59 ± 0.04 |
cairo-native (embedded AOT) |
0.933 ± 0.009 | 0.924 | 0.957 | 1.00 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
0.945 ± 0.003 | 0.940 | 0.950 | 1.01 ± 0.01 |
Benchmark for program linear_search
Open benchmarks
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
332.4 ± 3.9 | 326.4 | 339.0 | 1.00 |
cairo-native (embedded AOT) |
945.5 ± 2.9 | 942.4 | 952.5 | 2.84 ± 0.03 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
960.7 ± 4.4 | 954.8 | 966.7 | 2.89 ± 0.04 |
Benchmark for program logistic_map
Open benchmarks
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
Cairo-vm (Rust, Cairo 1) |
282.3 ± 1.5 | 279.8 | 284.2 | 1.00 |
cairo-native (embedded AOT) |
932.9 ± 4.9 | 924.4 | 939.8 | 3.30 ± 0.02 |
cairo-native (embedded JIT using LLVM's ORC Engine) |
951.4 ± 3.4 | 948.0 | 957.2 | 3.37 ± 0.02 |
Benchmark results Main vs HEAD.Base
Head
Base
Head
Base
Head
Base
Head
Base
Head
Base
Head
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #1655 and the nested-Box/Nullable PR.
Problem
Two remaining marshaling asymmetries:
BoundedInt encode:
to_ptrwrote the un-biased value into a 32-byte felt slot, while the native representation (and whatfrom_ptrdecodes) is the compact one —value - lowerstored inget_integer_layout(repr_bit_width()). Nested in an aggregate (top-level args are still gated by the arch.rs panic, implementAbiArgument::to_ptrforValue::BoundedInt#1217), it was both silently mis-decoded and overran its slot:Array<BoundedInt<3, 10>>has a 1-byte stride and got 32 bytes written per element. The existing unit testtest_to_ptr_bounded_int_validasserted the buggy encoding.Boxed returns:
parse_resultpassed a returnedBox<T>'s return-pointer slot straight tofrom_ptrwithout dereferencing it. Any function using a non-ZST builtin returns through a return pointer, so e.g.fn(...) -> Box<felt252>using pedersen decoded two raw heap addresses as the payload.Fix
RangeExt::repr_encode/repr_decodeas the single implementation of the compact encoding (felt-wrapping subtraction so negative lower bounds round-trip).to_ptruses the encode side — allocating the actualrepr_bit_widthlayout and validating against the type's range — and both existing decode duplicates (Value::from_ptrand the register path inparse_result) now share the decode side.parse_result's Box arm dereferences the return-pointer slot once, mirroring the Nullable arm.AbiArgument::to_ptrforValue::BoundedInt#1217) stays, with a TODO noting it can now be implemented on top ofrepr_encode.Tests
Array<BoundedInt<3, 10>>asserting the 1-byte biased element buffer ([0, 4, 6]for values[3, 7, 9]), and a negative-lower range (BoundedInt<-5, 5>).test_to_ptr_bounded_int_validcorrected: value 16 inBoundedInt<10, 510>encodes as[6, 0]in 2 bytes (and its embedded range fixed to the parsed[10, 511)).🤖 Generated with Claude Code
This change is