From e9f2176108d73fefe401d99bc521dc4de792ea8f Mon Sep 17 00:00:00 2001 From: TomerStarkware Date: Tue, 18 Aug 2026 10:39:29 +0300 Subject: [PATCH] fix(values): encode BoundedInt in its compact representation; deref boxed returns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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>` returned garbage. - `parse_result` passed a returned `Box`'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` 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>` (asserting the 1-byte biased element buffer) and a negative-lower range. Co-Authored-By: Claude Fable 5 --- src/arch.rs | 3 + src/executor.rs | 25 +-- src/utils/range_ext.rs | 31 +++- src/values.rs | 155 +++++++++++++----- .../programs/box_return_with_pedersen.cairo | 5 + tests/tests/programs.rs | 34 ++++ 6 files changed, 201 insertions(+), 52 deletions(-) create mode 100644 test_data/programs/box_return_with_pedersen.cairo diff --git a/src/arch.rs b/src/arch.rs index a8c4a8564..b114d141f 100644 --- a/src/arch.rs +++ b/src/arch.rs @@ -79,6 +79,9 @@ impl AbiArgument for ValueWithInfoWrapper<'_> { abi.capacity.to_bytes(buffer)?; } (Value::BoundedInt { .. }, CoreTypeConcrete::BoundedInt(_)) => { + // TODO: implement top-level BoundedInt arguments on top of + // `RangeExt::repr_encode` (dispatch on `repr_bit_width()`: <=64 via the + // `u64` impl, <=128 via `u128`, wider by memory like `Felt`). // See: https://github.com/starkware-libs/cairo_native/issues/1217 native_panic!("todo: implement AbiArgument for Value::BoundedInt case") } diff --git a/src/executor.rs b/src/executor.rs index 48d593be0..d37931e46 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -42,8 +42,7 @@ use cairo_lang_sierra::{ program_registry::ProgramRegistry, }; use libc::c_void; -use num_bigint::BigInt; -use num_traits::One; +use num_bigint::BigUint; use std::{alloc::Layout, arch::global_asm, ptr::NonNull}; mod aot; @@ -454,9 +453,14 @@ fn parse_result( registry, )?), CoreTypeConcrete::Box(info) => unsafe { - let ptr = - return_ptr.unwrap_or_else(|| NonNull::new_unchecked(ret_registers[0] as *mut ())); - let value = Value::from_ptr(ptr, &info.ty, registry)?; + // With a return pointer the returned value is the box's inline + // representation — a slot holding the payload pointer — so it must be + // dereferenced once. Without one, the register holds the payload + // pointer itself. + let ptr = return_ptr.map_or(ret_registers[0] as *mut (), |x| { + *x.cast::<*mut ()>().as_ref() + }); + let value = Value::from_ptr(NonNull::new_unchecked(ptr), &info.ty, registry)?; Ok(value) }, CoreTypeConcrete::EcPoint(_) | CoreTypeConcrete::EcState(_) => Ok(Value::from_ptr( @@ -523,17 +527,14 @@ fn parse_result( match return_ptr { Some(return_ptr) => Ok(Value::from_ptr(return_ptr, type_id, registry)?), None => { - let mut data = if info.range.repr_bit_width() <= 64 { - BigInt::from(ret_registers[0]) + let raw = if info.range.repr_bit_width() <= 64 { + BigUint::from(ret_registers[0]) } else { - BigInt::from(((ret_registers[1] as u128) << 64) | ret_registers[0] as u128) + BigUint::from(((ret_registers[1] as u128) << 64) | ret_registers[0] as u128) }; - data &= (BigInt::one() << info.range.repr_bit_width()) - BigInt::one(); - data += &info.range.lower; - Ok(Value::BoundedInt { - value: data.into(), + value: info.range.repr_decode(raw).into(), range: info.range.clone(), }) } diff --git a/src/utils/range_ext.rs b/src/utils/range_ext.rs index c3f1ccdfb..ead29e7b7 100644 --- a/src/utils/range_ext.rs +++ b/src/utils/range_ext.rs @@ -1,12 +1,21 @@ +use crate::utils::{get_integer_layout, PRIME}; use cairo_lang_sierra::extensions::utils::Range; use num_bigint::{BigInt, BigUint, Sign}; -use num_traits::One; +use num_traits::{Euclid, One}; pub trait RangeExt { /// Width in bits when the offset is zero (aka. the natural representation). fn zero_based_bit_width(&self) -> u32; /// Width in bits when the offset is not necessarily zero (aka. the compact representation). fn repr_bit_width(&self) -> u32; + /// Encode a value into the compact representation: the stored bits are + /// `(value - lower) mod PRIME`, laid out little-endian over the full + /// `get_integer_layout(repr_bit_width())` size. Returns `None` when the + /// value is not within the range. + fn repr_encode(&self, value: &BigInt) -> Option>; + /// Decode raw stored bits back into the value they represent: mask to + /// `repr_bit_width()` bits and add `lower`. + fn repr_decode(&self, raw: BigUint) -> BigInt; } impl RangeExt for Range { @@ -44,4 +53,24 @@ impl RangeExt for Range { // FIXME: Workaround for segfault in canonicalization (including LLVM 19). ((self.size() - BigInt::one()).bits() as u32).max(1) } + + fn repr_encode(&self, value: &BigInt) -> Option> { + // The subtraction is felt arithmetic so that ranges with a negative + // lower bound round-trip: values are field elements in `[0, PRIME)`, + // and `repr_decode`'s addition wraps the same way. + let prime = BigInt::from_biguint(Sign::Plus, PRIME.clone()); + let stored = (value - &self.lower).rem_euclid(&prime); + if stored >= self.size() { + return None; + } + + let mut bytes = stored.magnitude().to_bytes_le(); + bytes.resize(get_integer_layout(self.repr_bit_width()).size(), 0); + Some(bytes) + } + + fn repr_decode(&self, raw: BigUint) -> BigInt { + let mask = (BigUint::one() << self.repr_bit_width()) - BigUint::one(); + BigInt::from_biguint(Sign::Plus, raw & mask) + &self.lower + } } diff --git a/src/values.rs b/src/values.rs index af77f0cfb..55f792f8c 100644 --- a/src/values.rs +++ b/src/values.rs @@ -8,7 +8,7 @@ use crate::{ runtime::FeltDict, starknet::{ArrayAbi, Secp256k1Point, Secp256r1Point}, types::{ec_point, ec_state, TypeBuilder}, - utils::{felt252_bigint, felt_from_slot, get_integer_layout, layout_repeat, RangeExt, PRIME}, + utils::{felt252_bigint, felt_from_slot, get_integer_layout, layout_repeat, RangeExt}, }; use bumpalo::Bump; use cairo_lang_sierra::{ @@ -23,7 +23,7 @@ use cairo_lang_sierra::{ }; use educe::Educe; use num_bigint::{BigInt, BigUint, Sign}; -use num_traits::{Euclid, One}; +use num_traits::One; use starknet_types_core::{curve::ProjectivePoint, felt::Felt}; use std::{ alloc::Layout, @@ -229,22 +229,35 @@ impl Value { .into()); } - let prime = BigInt::from_biguint(Sign::Plus, PRIME.clone()); - let lower = lower.rem_euclid(&prime); - let upper = upper.rem_euclid(&prime); + let range = match Self::resolve_type(ty, registry)? { + CoreTypeConcrete::BoundedInt(info) + | CoreTypeConcrete::BoundedIntGuarantee(info) => &info.range, + _ => { + return Err(Error::UnexpectedValue(format!( + "expected value of type {:?} but got a bounded int", + type_id.debug_name + ))) + } + }; - // Check if value is within the valid range - if !(lower <= value && value < upper) { - return Err(CompilerError::BoundedIntOutOfRange { + // The native representation is the compact one: `value - lower` + // stored in `repr_bit_width()` bits, which is what `from_ptr` + // decodes and what compiled code expects. + let data = range.repr_encode(&value).ok_or_else(|| { + CompilerError::BoundedIntOutOfRange { value: Box::new(value), - range: Box::new((lower, upper)), + range: Box::new((range.lower.clone(), range.upper.clone())), } - .into()); - } + })?; - let ptr = arena.alloc_layout(get_integer_layout(252)).cast(); - let data = felt252_bigint(value).to_bytes_le(); - ptr.cast::<[u8; 32]>().as_mut().copy_from_slice(&data); + let ptr: NonNull<()> = arena + .alloc_layout(get_integer_layout(range.repr_bit_width())) + .cast(); + std::ptr::copy_nonoverlapping( + data.as_ptr(), + ptr.cast::().as_ptr(), + data.len(), + ); ptr } @@ -853,19 +866,13 @@ impl Value { CoreTypeConcrete::Const(_) => native_panic!("implement const from_ptr"), CoreTypeConcrete::BoundedInt(info) | CoreTypeConcrete::BoundedIntGuarantee(info) => { - let mut data = BigInt::from_biguint( - Sign::Plus, - BigUint::from_bytes_le(slice::from_raw_parts( - ptr.cast::().as_ptr(), - (info.range.repr_bit_width().next_multiple_of(8) >> 3) as usize, - )), - ); - - data &= (BigInt::one() << info.range.repr_bit_width()) - BigInt::one(); - data += &info.range.lower; + let raw = BigUint::from_bytes_le(slice::from_raw_parts( + ptr.cast::().as_ptr(), + (info.range.repr_bit_width().next_multiple_of(8) >> 3) as usize, + )); Self::BoundedInt { - value: data.into(), + value: info.range.repr_decode(raw).into(), range: info.range.clone(), } } @@ -1474,23 +1481,93 @@ mod test { // Create the registry for the program let registry = ProgramRegistry::::new(&program).unwrap(); - // Valid case + // Valid case: `BoundedInt<10, 510>` (a `Range` of `[10, 511)`) has a + // 9-bit compact representation (2 bytes), storing `value - lower`. + let value = Value::BoundedInt { + value: Felt::from(16), + range: Range { + lower: BigInt::from(10), + upper: BigInt::from(511), + }, + }; + + let arena = Bump::new(); + let ptr = value + .to_ptr(&arena, ®istry, &program.type_declarations[1].id) + .unwrap(); + + assert_eq!(unsafe { *ptr.cast::<[u8; 2]>().as_ptr() }, [6, 0]); assert_eq!( - unsafe { - *Value::BoundedInt { - value: Felt::from(16), - range: Range { - lower: BigInt::from(10), - upper: BigInt::from(510), - }, - } - .to_ptr(&Bump::new(), ®istry, &program.type_declarations[1].id) - .unwrap() - .cast::<[u32; 8]>() - .as_ptr() + Value::from_ptr(ptr, &program.type_declarations[1].id, ®istry).unwrap(), + value + ); + } + + #[test] + fn test_roundtrip_bounded_int_array() { + // `BoundedInt<3, 10>` has a 3-bit compact representation (1-byte layout + // and stride), storing `value - 3`. + let program = ProgramParser::new() + .parse( + "type B = BoundedInt<3, 10>; + type A = Array;", + ) + .unwrap(); + let registry = ProgramRegistry::::new(&program).unwrap(); + + let bounded = |v: u64| Value::BoundedInt { + value: Felt::from(v), + range: Range { + lower: BigInt::from(3), + upper: BigInt::from(11), }, - [16, 0, 0, 0, 0, 0, 0, 0] + }; + let value = Value::Array(vec![bounded(3), bounded(7), bounded(9)]); + + let arena = Bump::new(); + let ptr = value + .to_ptr(&arena, ®istry, &program.type_declarations[1].id) + .unwrap(); + + // Check the raw element buffer: 1-byte stride of biased values. + let abi = unsafe { ptr.cast::>().as_ref() }; + assert_eq!( + unsafe { std::slice::from_raw_parts(abi.ptr, 3) }, + &[0, 4, 6] ); + + assert_eq!( + Value::from_ptr(ptr, &program.type_declarations[1].id, ®istry).unwrap(), + value + ); + } + + #[test] + fn test_roundtrip_bounded_int_negative_lower() { + let program = ProgramParser::new() + .parse("type B = BoundedInt<-5, 5>;") + .unwrap(); + let registry = ProgramRegistry::::new(&program).unwrap(); + + for v in [-5_i64, -1, 0, 4] { + let value = Value::BoundedInt { + value: Felt::from(v), + range: Range { + lower: BigInt::from(-5), + upper: BigInt::from(6), + }, + }; + + let arena = Bump::new(); + let ptr = value + .to_ptr(&arena, ®istry, &program.type_declarations[0].id) + .unwrap(); + + assert_eq!( + Value::from_ptr(ptr, &program.type_declarations[0].id, ®istry).unwrap(), + value + ); + } } #[test] diff --git a/test_data/programs/box_return_with_pedersen.cairo b/test_data/programs/box_return_with_pedersen.cairo new file mode 100644 index 000000000..58a25d881 --- /dev/null +++ b/test_data/programs/box_return_with_pedersen.cairo @@ -0,0 +1,5 @@ +use core::pedersen::pedersen; + +fn run_test(a: felt252, b: felt252) -> Box { + BoxTrait::new(pedersen(a, b)) +} diff --git a/tests/tests/programs.rs b/tests/tests/programs.rs index 5261eecc8..4cb1a955a 100644 --- a/tests/tests/programs.rs +++ b/tests/tests/programs.rs @@ -314,3 +314,37 @@ fn no_op() { ) .unwrap(); } + +#[test] +fn box_return_forced_through_return_ptr() { + // The pedersen builtin makes the function return more than one value (the + // builtin plus the box), so the box comes back through the return pointer, + // which `parse_result` must dereference once before reading the payload. + let program = &load_program_and_runner("programs/box_return_with_pedersen"); + + let (a, b) = (Felt::from(1234), Felt::from(5678)); + + let result_vm = run_vm_program( + program, + "run_test", + vec![Arg::Value(a), Arg::Value(b)], + Some(DEFAULT_GAS as usize), + ) + .unwrap(); + + let result_native = run_native_program( + program, + "run_test", + &[Value::Felt252(a), Value::Felt252(b)], + Some(DEFAULT_GAS), + Option::::None, + ); + + compare_outputs( + &program.1, + &program.2.find_function("run_test").unwrap().id, + &result_vm, + &result_native, + ) + .expect("boxed return through return pointer must agree between VM and native"); +}