fix(utils): convert _quickSort tail recursion to iterative loop (fixes #6289)#6476
Conversation
Fixes OpenZeppelin#6289 Converts one of the two recursive calls in _quickSort to a while loop, targeting the larger partition. This raises the maximum sortable array size from ~170 elements (limited by EVM stack depth of 1024) to >1000 elements. The fix follows the approach described in the issue: - The larger partition is processed iteratively (loop) - The smaller partition is processed recursively (call) - This maintains O(n log n) time complexity while eliminating stack overflow Added testSortLargeArrayDescending (N=500) and testSortVeryLargeArrayRandom (N=1000) to verify the fix.
|
WalkthroughThe 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/utils/Arrays.t.sol (1)
410-412: Consider usingassertTrue()or reusing the_assertSorthelper.The assertions compare booleans to
true, which is less idiomatic than usingassertTrue(). Additionally,testSortVeryLargeArrayRandomcould reuse the existing_assertSorthelper.♻️ Suggested improvements
For
testSortLargeArrayDescending(strict ascending check):for (uint256 i = 1; i < N; i++) { - assertEq(a[i - 1] < a[i], true); + assertTrue(a[i - 1] < a[i]); }For
testSortVeryLargeArrayRandom(non-decreasing check), reuse the existing helper:Arrays.sort(a); - // Verify sorted ascending - for (uint256 i = 1; i < N; i++) { - assertEq(a[i - 1] <= a[i], true); - } + _assertSort(a);Also applies to: 427-429
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/Arrays.t.sol` around lines 410 - 412, Replace boolean comparisons like assertEq(a[i - 1] < a[i], true) with the idiomatic assertTrue(...) and reuse the existing _assertSort helper where appropriate: update testSortVeryLargeArrayRandom to call _assertSort(a) (which performs the non-decreasing check) instead of duplicating the loop, and ensure testSortLargeArrayDescending uses assertTrue for the strict ascending check (e.g., assertTrue(a[i - 1] < a[i])) to match intent; update occurrences around testSortVeryLargeArrayRandom and testSortLargeArrayDescending accordingly.contracts/utils/Arrays.sol (1)
115-118: Consider clarifying the comment to reflect current behavior.The comment describes the old limitation before explaining the fix, which could be slightly confusing. Consider rephrasing to emphasize the current capability.
📝 Suggested documentation improvement
- // `@custom`:oz-reentrancy-enginespec note - // The function reverts if called with arrays larger than ~170 elements due to EVM stack depth limits - // when using two recursive calls. This limit is raised to >1000 elements by using an iterative - // approach for the larger partition (tail recursion optimization via loop). + // `@custom`:oz-reentrancy-enginespec note + // Uses tail-recursion optimization (iterative loop for larger partition, recursion for smaller) + // to support arrays >1000 elements. Without this optimization, EVM stack depth limits would + // cause stack overflow for arrays ≥170 elements due to dual recursive calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/utils/Arrays.sol` around lines 115 - 118, Update the comment block tagged with `@custom`:oz-reentrancy-enginespec to lead with the current behavior rather than the historical limitation: explicitly state that the function now supports partitions larger than ~1000 elements by converting one recursive branch to an iterative loop (tail-recursion elimination), and then note that previously a purely recursive approach would revert around ~170 elements due to EVM stack depth. Keep the same technical details (approximate limits and that the larger partition is handled iteratively) and replace the existing wording so readers immediately see the current capability before the historical note.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/utils/Arrays.t.sol`:
- Around line 419-424: The assignment to variable seed uses keccak256 which
returns bytes32, causing a type mismatch; change the assignment to cast the
keccak256 result to uint256 (e.g., set seed = uint256(keccak256(abi.encode(seed,
i)))) so seed remains uint256 and subsequent use in a[i] = uint256(seed) % (N *
10) is valid; update the line that calls keccak256(abi.encode(seed, i))
accordingly and keep references to seed, keccak256, abi.encode, and array a.
---
Nitpick comments:
In `@contracts/utils/Arrays.sol`:
- Around line 115-118: Update the comment block tagged with
`@custom`:oz-reentrancy-enginespec to lead with the current behavior rather than
the historical limitation: explicitly state that the function now supports
partitions larger than ~1000 elements by converting one recursive branch to an
iterative loop (tail-recursion elimination), and then note that previously a
purely recursive approach would revert around ~170 elements due to EVM stack
depth. Keep the same technical details (approximate limits and that the larger
partition is handled iteratively) and replace the existing wording so readers
immediately see the current capability before the historical note.
In `@test/utils/Arrays.t.sol`:
- Around line 410-412: Replace boolean comparisons like assertEq(a[i - 1] <
a[i], true) with the idiomatic assertTrue(...) and reuse the existing
_assertSort helper where appropriate: update testSortVeryLargeArrayRandom to
call _assertSort(a) (which performs the non-decreasing check) instead of
duplicating the loop, and ensure testSortLargeArrayDescending uses assertTrue
for the strict ascending check (e.g., assertTrue(a[i - 1] < a[i])) to match
intent; update occurrences around testSortVeryLargeArrayRandom and
testSortLargeArrayDescending accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 241b02cd-b4c7-4acb-98d5-707b222ceec3
📒 Files selected for processing (2)
contracts/utils/Arrays.soltest/utils/Arrays.t.sol
| uint256 seed = 42; | ||
| for (uint256 i = 0; i < N; i++) { | ||
| // Deterministic pseudo-random for reproducibility | ||
| seed = keccak256(abi.encode(seed, i)); | ||
| a[i] = uint256(seed) % (N * 10); | ||
| } |
There was a problem hiding this comment.
Type mismatch: keccak256 returns bytes32, not uint256.
Line 422 assigns a bytes32 value to a uint256 variable without explicit conversion. In Solidity 0.8.x, this implicit conversion is not allowed and will cause a compilation error.
🐛 Proposed fix
uint256 seed = 42;
for (uint256 i = 0; i < N; i++) {
// Deterministic pseudo-random for reproducibility
- seed = keccak256(abi.encode(seed, i));
- a[i] = uint256(seed) % (N * 10);
+ seed = uint256(keccak256(abi.encode(seed, i)));
+ a[i] = seed % (N * 10);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint256 seed = 42; | |
| for (uint256 i = 0; i < N; i++) { | |
| // Deterministic pseudo-random for reproducibility | |
| seed = keccak256(abi.encode(seed, i)); | |
| a[i] = uint256(seed) % (N * 10); | |
| } | |
| uint256 seed = 42; | |
| for (uint256 i = 0; i < N; i++) { | |
| // Deterministic pseudo-random for reproducibility | |
| seed = uint256(keccak256(abi.encode(seed, i))); | |
| a[i] = seed % (N * 10); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/utils/Arrays.t.sol` around lines 419 - 424, The assignment to variable
seed uses keccak256 which returns bytes32, causing a type mismatch; change the
assignment to cast the keccak256 result to uint256 (e.g., set seed =
uint256(keccak256(abi.encode(seed, i)))) so seed remains uint256 and subsequent
use in a[i] = uint256(seed) % (N * 10) is valid; update the line that calls
keccak256(abi.encode(seed, i)) accordingly and keep references to seed,
keccak256, abi.encode, and array a.
1 participant
Summary
Fixes #6289 — converts
_quickSort()from a two-recursive-call approach to an iterative loop for the larger partition, raising the maximum sortable array size from ~170 elements to >1000 elements.Motivation
The original
_quickSort()makes two recursive calls per invocation. Since Solidity does not optimize tail recursion, each recursive call consumes EVM stack depth (~1024 limit, ~169 recursion depth for this function). Arrays of 170+ elements causedEvmError: StackOverflow.The proposed fix converts one recursive call into a
while (true)loop:Changes
contracts/utils/Arrays.solTesting
Added two new tests:
testSortLargeArrayDescending— N=500, descending input, verifies ascending outputtestSortVeryLargeArrayRandom— N=1000, deterministic pseudo-random inputNote:
forgeis not available in this environment. Tests should be run via GitHub Actions CI on the PR.References
_quickSort()can be optimized #6289