From 86f85b9fa4bfe5788ec6d1f452b062d4b930da30 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 24 Jul 2026 16:21:47 -0300 Subject: [PATCH 1/2] fix: place values at sorted key positions in create SparseArray::create and MutSparseArrayBase::create populated value slots with _values[sort_indices[i]], but sort_indices maps input position to sorted position, so this applies the permutation in the wrong direction. For any key ordering whose sort permutation is not its own inverse (e.g. a 3-cycle like [3, 1, 2]), get(key) returned a different key's value. Write each input value to its key's sorted slot instead, and add regression tests using a non-involution key ordering. Fixes https://github.com/noir-lang/noir-library-claude/issues/6 Co-Authored-By: Claude Fable 5 --- src/lib.nr | 18 +++++++++++++++++- src/mut_sparse_array.nr | 19 ++++++++++++++++++- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/lib.nr b/src/lib.nr index 7ee3806..cb642c2 100644 --- a/src/lib.nr +++ b/src/lib.nr @@ -80,8 +80,10 @@ where // note: self.keys[i] maps to self.values[i+1] // self.values[0] does not map to any key. we use it to store the default empty value, // which is returned when `get(idx)` is called and `idx` does not exist in `self.keys` + // sort_indices[i] is the sorted position of input key i, so write each + // input value to its key's sorted slot for i in 0..N { - r.values[i + 2] = _values[sorted_keys.sort_indices[i]]; + r.values[sorted_keys.sort_indices[i] + 2] = _values[i]; } // insert values that map to our key start and endpoints // if _keys[0] = 0 then values[0] must equal _values[0], so some conditional logic is required @@ -216,6 +218,20 @@ mod test { SparseArray::create([0, 5, 7, 0xffffffff], [123, 456, 789, 101112], 0xffffffff); assert(example.maximum == 0xffffffff); } + + // Regression test for https://github.com/noir-lang/noir-library-claude/issues/6 + // The keys [3, 1, 2] sort via a 3-cycle permutation, so `sort_indices` differs + // from its inverse. `create` must use the inverse map when populating values, + // otherwise `get(key)` returns a different key's value. + #[test] + fn test_sparse_lookup_non_involution_key_order() { + let example = SparseArray::create([3, 1, 2], [30, 10, 20], 100); + + assert(example.get(1) == 10); + assert(example.get(2) == 20); + assert(example.get(3) == 30); + } + #[test] fn test_sparse_lookup_u32() { let example = SparseArray::create( diff --git a/src/mut_sparse_array.nr b/src/mut_sparse_array.nr index 3f3f41b..c885844 100644 --- a/src/mut_sparse_array.nr +++ b/src/mut_sparse_array.nr @@ -95,8 +95,10 @@ where // note: self.keys[i] maps to self.values[i+1] // self.values[0] does not map to any key. we use it to store the default empty value, // which is returned when `get(idx)` is called and `idx` does not exist in `self.keys` + // sort_indices[i] is the sorted position of input key i, so write each + // input value to its key's sorted slot for i in 0..M { - r.values[i + 2] = _values[sorted_keys.sort_indices[i]]; + r.values[sorted_keys.sort_indices[i] + 2] = _values[i]; } // insert values that map to our key start and endpoints // if _keys[0] = 0 then values[0] must equal _values[0], so some conditional logic is required @@ -316,6 +318,21 @@ mod test { MutSparseArray::create([0, 5, 7, 0xffffffff], [123, 456, 789, 101112], 0xffffffff); assert(example.length() == 0xffffffff); } + + // Regression test for https://github.com/noir-lang/noir-library-claude/issues/6 + // The keys [3, 1, 2] sort via a 3-cycle permutation, so `sort_indices` differs + // from its inverse. `create` must use the inverse map when populating values, + // otherwise `get(key)` returns a different key's value. + #[test] + fn test_sparse_lookup_non_involution_key_order() { + let example: MutSparseArray<4, Field> = + MutSparseArray::create([3, 1, 2], [30, 10, 20], 100); + + assert(example.get(1) == 10); + assert(example.get(2) == 20); + assert(example.get(3) == 30); + } + #[test] fn test_sparse_lookup_u32() { let example: MutSparseArray<8, _> = MutSparseArray::create( From 1bb2be5accd0b2e14d20cf186bd5af9388e3115e Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 24 Jul 2026 16:24:11 -0300 Subject: [PATCH 2/2] refactor: share create logic between SparseArray and MutSparseArray Extract the duplicated sort-and-populate block (key sorting, start/end point insertion, value placement, boundary values, maximum check) into a single insert_sorted_keys_and_values helper, and drop the duplicated __sort/assert_sorted functions from mut_sparse_array.nr. The wrong-index bug fixed in the previous commit existed identically in both copies; this ensures the logic lives in one place. No circuit cost change: ACIR opcode count is identical before and after (verified with nargo info on a scratch binary exercising create/get/set for both types), since Noir fully inlines constrained functions. Co-Authored-By: Claude Fable 5 --- src/lib.nr | 97 +++++++++++++++++++++++------------------ src/mut_sparse_array.nr | 52 +--------------------- 2 files changed, 57 insertions(+), 92 deletions(-) diff --git a/src/lib.nr b/src/lib.nr index cb642c2..b32276e 100644 --- a/src/lib.nr +++ b/src/lib.nr @@ -10,6 +10,60 @@ fn assert_sorted(lhs: u32, rhs: u32) { assert(lhs < rhs); } +/** + * @brief shared constructor logic for SparseArray and MutSparseArrayBase: + * sort `input_keys`, write them into `keys[1..M+1]` with 0 and `maximum` + * as start/endpoints, and place each input value at its key's sorted slot + * (keys[i] maps to values[i+1]; values[0] stays the default empty value) + **/ +fn insert_sorted_keys_and_values( + input_keys: [u32; M], + input_values: [T; M], + maximum: u32, + keys: &mut [u32; K], + values: &mut [T; V], +) +where + T: std::default::Default, +{ + // for any valid index, we want to ensure the following is satified: + // keys[X] <= index <= keys[X+1] + // this requires us to sort the keys, and insert a startpoint and endpoint + let sorted_keys = sort_advanced(input_keys, __sort, assert_sorted); + + // insert start and endpoints + keys[0] = 0; + for i in 0..M { + keys[i + 1] = sorted_keys.sorted[i]; + } + keys[M + 1] = maximum; + + // sort_indices[i] is the sorted position of input key i, so write each + // input value to its key's sorted slot + for i in 0..M { + values[sorted_keys.sort_indices[i] + 2] = input_values[i]; + } + + // insert values that map to our key start and endpoints + // if input_keys[0] = 0 then values[1] must equal input_values[0], so some + // conditional logic is required (same for input_keys[M-1]) + let mut initial_value = T::default(); + if (input_keys[0] == 0) { + initial_value = input_values[0]; + } + let mut final_value = T::default(); + if (input_keys[M - 1] == maximum) { + final_value = input_values[M - 1]; + } + values[1] = initial_value; + values[M + 2] = final_value; + + // perform boundary checks! + // because the keys are sorted, checking the largest key does not exceed + // `maximum` validates every key + assert(maximum >= sorted_keys.sorted[M - 1]); +} + /** * @brief MutSparseArray, a sparse array of configurable size with `N` nonzero entries. * Can be read from and written into @@ -64,48 +118,7 @@ where let mut r: Self = SparseArray { keys: [0; N + 2], values: [T::default(); N + 3], maximum: _maximum }; - // for any valid index, we want to ensure the following is satified: - // self.keys[X] <= index <= self.keys[X+1] - // this requires us to sort hte keys, and insert a startpoint and endpoint - let sorted_keys = sort_advanced(_keys, __sort, assert_sorted); - - // insert start and endpoints - r.keys[0] = 0; - for i in 0..N { - r.keys[i + 1] = sorted_keys.sorted[i]; - } - r.keys[N + 1] = _maximum; - - // populate values based on the sorted keys - // note: self.keys[i] maps to self.values[i+1] - // self.values[0] does not map to any key. we use it to store the default empty value, - // which is returned when `get(idx)` is called and `idx` does not exist in `self.keys` - // sort_indices[i] is the sorted position of input key i, so write each - // input value to its key's sorted slot - for i in 0..N { - r.values[sorted_keys.sort_indices[i] + 2] = _values[i]; - } - // insert values that map to our key start and endpoints - // if _keys[0] = 0 then values[0] must equal _values[0], so some conditional logic is required - // (same for _keys[N-1]) - let mut initial_value = T::default(); - if (_keys[0] == 0) { - initial_value = _values[0]; - } - let mut final_value = T::default(); - if (_keys[N - 1] == _maximum) { - final_value = _values[N - 1]; - } - r.values[1] = initial_value; - r.values[N + 2] = final_value; - - // perform boundary checks! - // the maximum size of the sparse array is 2^32 - // we need to check that every element in `self.keys` is less than 2^32 - // because `self.keys` is sorted, we can simply validate that - // sorted_keys.sorted[0] < 2^32 - // sorted_keys.sorted[N-1] < maximum - assert(_maximum >= sorted_keys.sorted[N - 1]); + insert_sorted_keys_and_values(_keys, _values, _maximum, &mut r.keys, &mut r.values); r } diff --git a/src/mut_sparse_array.nr b/src/mut_sparse_array.nr index c885844..a83027a 100644 --- a/src/mut_sparse_array.nr +++ b/src/mut_sparse_array.nr @@ -1,12 +1,4 @@ -use crate::{MutSparseArray, MutSparseArrayBase, U32RangeTraits}; -use ::sort::sort_advanced; -unconstrained fn __sort(lhs: u32, rhs: u32) -> bool { - lhs < rhs -} - -fn assert_sorted(lhs: u32, rhs: u32) { - assert(lhs < rhs); -} +use crate::{insert_sorted_keys_and_values, MutSparseArray, MutSparseArrayBase, U32RangeTraits}; trait RangeTraits { fn less_than(lhs: Field, rhs: Field) -> bool; @@ -73,17 +65,7 @@ where tail_ptr: 0, }; - // for any valid index, we want to ensure the following is satified: - // self.keys[X] <= index <= self.keys[X+1] - // this requires us to sort hte keys, and insert a startpoint and endpoint - let sorted_keys = sort_advanced(_keys, __sort, assert_sorted); - - // insert start and endpoints - r.keys[0] = 0; - for i in 0..M { - r.keys[i + 1] = sorted_keys.sorted[i]; - } - r.keys[M + 1] = _maximum; + insert_sorted_keys_and_values(_keys, _values, _maximum, &mut r.keys, &mut r.values); for i in 0..M + 2 { r.linked_keys[i] = i + 1; @@ -91,36 +73,6 @@ where // set the last linked key to 2^32 - 1 r.linked_keys[M + 1] = 0xFFFFFFFF; - // populate values based on the sorted keys - // note: self.keys[i] maps to self.values[i+1] - // self.values[0] does not map to any key. we use it to store the default empty value, - // which is returned when `get(idx)` is called and `idx` does not exist in `self.keys` - // sort_indices[i] is the sorted position of input key i, so write each - // input value to its key's sorted slot - for i in 0..M { - r.values[sorted_keys.sort_indices[i] + 2] = _values[i]; - } - // insert values that map to our key start and endpoints - // if _keys[0] = 0 then values[0] must equal _values[0], so some conditional logic is required - // (same for _keys[N-1]) - let mut initial_value = T::default(); - if (_keys[0] == 0) { - initial_value = _values[0]; - } - let mut final_value = T::default(); - if (_keys[M - 1] == _maximum) { - final_value = _values[M - 1]; - } - r.values[1] = initial_value; - r.values[M + 2] = final_value; - - // perform boundary checks! - // the maximum size of the sparse array is 2^32 - // we need to check that every element in `self.keys` is less than 2^32 - // because `self.keys` is sorted, we can simply validate that - // sorted_keys.sorted[0] < 2^32 - // sorted_keys.sorted[N-1] < maximum - assert(_maximum >= sorted_keys.sorted[M - 1]); r.tail_ptr = M + 2; r }