fix(Arrays): optimize _quickSort to prevent stack overflow#6504
fix(Arrays): optimize _quickSort to prevent stack overflow#6504BhariGowda wants to merge 3 commits intoOpenZeppelin:masterfrom
Conversation
Replace recursive _quickSort with iterative stack-based implementation. The recursive version was limited to ~169 elements before hitting EVM stack overflow (depth 1024, 5 slots per call). The iterative version uses an explicit memory stack, eliminating recursion depth limits entirely. Fixes OpenZeppelin#6289
🦋 Changeset detectedLatest commit: ecd93e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis pull request converts the 🚥 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/generate/templates/Arrays.js (1)
58-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale doc: "preserved in subcalls" no longer applies.
The function is no longer recursive, so the wording about the invariant being preserved "in subcalls" is misleading. Reword to reflect the iterative structure (e.g., "preserved across iterations / when popping ranges from the stack").
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/generate/templates/Arrays.js` at line 58, Update the stale JSDoc in Arrays.js that currently says "preserved in subcalls" for the invariant `begin <= end` (the comment associated with the {sort} implementation); reword it to reflect the iterative implementation instead (for example: "Invariant: `begin <= end`. This holds when initially called by {sort} and is preserved across iterations (when popping ranges from the stack)."), making sure the wording and punctuation match surrounding comments and references to the {sort} function.
🧹 Nitpick comments (2)
scripts/generate/templates/Arrays.js (2)
71-74: 💤 Low valueRedundant
end - begin < 0x40guard inside the loop.Both push sites (Lines 84 and 88) already enforce subrange size ≥ 2 before pushing, and the entry point only enters the loop when
size >= 2. After the initial pop on the first iteration,end - beginis always ≥0x40, so this guard never fires. Safe to drop, or — if kept for defense in depth — worth a comment to document why.♻️ Drop the redundant guard
while (top > 0) { end = stack[--top]; begin = stack[--top]; - if (end - begin < 0x40) continue; uint256 pivot = _mload(begin);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/generate/templates/Arrays.js` around lines 71 - 74, The loop in Arrays.js that pops subranges using stack and top contains a redundant defensive check "if (end - begin < 0x40) continue" because pushes (at the sites that push onto stack) and the initial entry condition guarantee subranges are >= 2 and, after the first pop, always >= 0x40; remove that guard to simplify the loop (or if you prefer to keep it, add a comment next to the check explaining why it will never trigger due to the invariants enforced by the push sites and the initial size check) — locate the loop that references stack, top, end, and begin and either delete the if-statement or add the explanatory comment.
65-94: 🏗️ Heavy liftConsider the hybrid (loop‑on‑larger / recurse‑on‑smaller) variant — it solves
#6289without the O(n) memory cost.The current change correctly removes the recursion-depth limit, but it does so at a real gas cost:
new uint256[](2 * size)is allocated on every sort, regardless of input shape. For already-sorted / reverse-sorted / random inputs alike, the actual peak stack usage is far below2 * size, yet the contract pays for the full allocation up front and (for largen) for quadratic memory expansion on top of the array itself. That's a regression for the common case where the prior recursive impl worked fine.A standard fix that addresses
#6289without any extra memory is to keep the recursive structure but always tail‑iterate on the larger partition and only recurse on the smaller one. This caps Solidity recursion depth at≈ log2(n)(≤ ~256 even for the largest conceivable ranges) — comfortably under the 1024 EVM stack limit — and removes the explicit memory stack entirely. Worth considering before merging the iterative approach.♻️ Sketch of the smaller‑side‑recurse / larger‑side‑loop variant
function _quickSort(uint256 begin, uint256 end, function(uint256, uint256) pure returns (bool) comp) private pure { unchecked { - uint256 size = (end - begin) / 0x20; - if (size < 2) return; - uint256[] memory stack = new uint256[](2 * size); - uint256 top = 0; - stack[top++] = begin; - stack[top++] = end; - while (top > 0) { - end = stack[--top]; - begin = stack[--top]; - if (end - begin < 0x40) continue; + while (end - begin >= 0x40) { uint256 pivot = _mload(begin); uint256 pos = begin; for (uint256 it = begin + 0x20; it < end; it += 0x20) { if (comp(_mload(it), pivot)) { pos += 0x20; _swap(pos, it); } } _swap(begin, pos); - if (pos > begin + 0x20) { - stack[top++] = begin; - stack[top++] = pos; - } - if (pos + 0x40 < end) { - stack[top++] = pos + 0x20; - stack[top++] = end; + // Recurse on the smaller side, iterate on the larger one. + if (pos - begin < end - pos - 0x20) { + _quickSort(begin, pos, comp); + begin = pos + 0x20; + } else { + _quickSort(pos + 0x20, end, comp); + end = pos; } } } }Same fix applies to
contracts/utils/Arrays.sol.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/generate/templates/Arrays.js` around lines 65 - 94, The current iterative quicksort allocates a full stack array (stack/top) causing O(n) memory and gas; replace it with the hybrid "recurse on smaller partition, loop on larger" approach: remove the uint256[] stack and top variables, keep the partitioning logic using pivot = _mload(begin), comp, _mload, _swap and pos, then after partitioning compute leftSize = (pos - begin) / 0x20 and rightSize = (end - (pos + 0x20)) / 0x20 and if leftSize < rightSize recurse (call the same sorting function) on (begin, pos) then set begin = pos + 0x20 to continue looping on the larger side, otherwise recurse on (pos + 0x20, end) and set end = pos to loop on the larger side; only recurse when the chosen smaller side size >= 2 and loop will terminate when size < 2 — this removes new uint256[](2 * size) and avoids quadratic memory growth while keeping recursion depth <= O(log n).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/generate/templates/Arrays.js`:
- Around line 65-67: Add a regression test that verifies the iterative quicksort
fix avoids EVM stack overflow for n > 169: create a reverse‑sorted uint256[] of
size ~200–1000 (e.g., 200), call the same public sort entry used by Arrays.js
(the code path that computes uint256 size = (end - begin) / 0x20 and uses
uint256[] memory stack), and assert the returned array is sorted ascending;
include the test in your Hardhat/Foundry suite and also add a short changeset
entry noting the regression fix to prevent future regressions.
---
Outside diff comments:
In `@scripts/generate/templates/Arrays.js`:
- Line 58: Update the stale JSDoc in Arrays.js that currently says "preserved in
subcalls" for the invariant `begin <= end` (the comment associated with the
{sort} implementation); reword it to reflect the iterative implementation
instead (for example: "Invariant: `begin <= end`. This holds when initially
called by {sort} and is preserved across iterations (when popping ranges from
the stack)."), making sure the wording and punctuation match surrounding
comments and references to the {sort} function.
---
Nitpick comments:
In `@scripts/generate/templates/Arrays.js`:
- Around line 71-74: The loop in Arrays.js that pops subranges using stack and
top contains a redundant defensive check "if (end - begin < 0x40) continue"
because pushes (at the sites that push onto stack) and the initial entry
condition guarantee subranges are >= 2 and, after the first pop, always >= 0x40;
remove that guard to simplify the loop (or if you prefer to keep it, add a
comment next to the check explaining why it will never trigger due to the
invariants enforced by the push sites and the initial size check) — locate the
loop that references stack, top, end, and begin and either delete the
if-statement or add the explanatory comment.
- Around line 65-94: The current iterative quicksort allocates a full stack
array (stack/top) causing O(n) memory and gas; replace it with the hybrid
"recurse on smaller partition, loop on larger" approach: remove the uint256[]
stack and top variables, keep the partitioning logic using pivot =
_mload(begin), comp, _mload, _swap and pos, then after partitioning compute
leftSize = (pos - begin) / 0x20 and rightSize = (end - (pos + 0x20)) / 0x20 and
if leftSize < rightSize recurse (call the same sorting function) on (begin, pos)
then set begin = pos + 0x20 to continue looping on the larger side, otherwise
recurse on (pos + 0x20, end) and set end = pos to loop on the larger side; only
recurse when the chosen smaller side size >= 2 and loop will terminate when size
< 2 — this removes new uint256[](2 * size) and avoids quadratic memory growth
while keeping recursion depth <= O(log n).
🪄 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: 92e210b7-547a-471a-b5d6-99cd0cc04ee9
📒 Files selected for processing (2)
contracts/utils/Arrays.solscripts/generate/templates/Arrays.js
| uint256 size = (end - begin) / 0x20; | ||
| if (size < 2) return; | ||
| uint256[] memory stack = new uint256[](2 * size); |
There was a problem hiding this comment.
Please add a regression test exercising n > 169 before merging.
The PR description and #6289 both reference the ~169 element threshold where the recursive version overflowed the EVM stack. The PR checklist still has tests unchecked. A focused test that sorts an array of, say, 200–1000 elements (e.g., reverse‑sorted to stress the partition pattern) would lock in the fix and prevent future regressions. Same for a changeset entry given OZ’s release process.
Want me to draft a Hardhat/Foundry test that sorts a large reverse‑sorted uint256[] and asserts ascending order, plus a changeset stub?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/generate/templates/Arrays.js` around lines 65 - 67, Add a regression
test that verifies the iterative quicksort fix avoids EVM stack overflow for n >
169: create a reverse‑sorted uint256[] of size ~200–1000 (e.g., 200), call the
same public sort entry used by Arrays.js (the code path that computes uint256
size = (end - begin) / 0x20 and uses uint256[] memory stack), and assert the
returned array is sorted ascending; include the test in your Hardhat/Foundry
suite and also add a short changeset entry noting the regression fix to prevent
future regressions.
Exercises the stack overflow fix from OpenZeppelin#6289. The old recursive _quickSort would fail at n>169 with EVM stack overflow.
|
Added regression test with n=200 (above the ~169 threshold where the recursive version would stack overflow). All 288 tests pass. |
1 participant
Replace recursive _quickSort with iterative stack-based implementation. The recursive version was limited to ~169 elements before hitting EVM stack overflow (depth 1024, 5 slots per call).
The iterative version uses an explicit memory stack, eliminating recursion depth limits entirely.
Fixes #6289
Fixes #????
PR Checklist
npx changeset add)