diff --git a/contracts/utils/Arrays.sol b/contracts/utils/Arrays.sol index f837004ff92..2e060ed1e1d 100644 --- a/contracts/utils/Arrays.sol +++ b/contracts/utils/Arrays.sol @@ -112,27 +112,41 @@ library Arrays { * IMPORTANT: Memory locations between `begin` and `end` are not validated/zeroed. This function should * be used only if the limits are within a memory array. */ + // @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). function _quickSort(uint256 begin, uint256 end, function(uint256, uint256) pure returns (bool) comp) private pure { unchecked { - if (end - begin < 0x40) return; - - // Use first element as pivot - uint256 pivot = _mload(begin); - // Position where the pivot should be at the end of the loop - uint256 pos = begin; - - for (uint256 it = begin + 0x20; it < end; it += 0x20) { - if (comp(_mload(it), pivot)) { - // If the value stored at the iterator's position comes before the pivot, we increment the - // position of the pivot and move the value there. - pos += 0x20; - _swap(pos, it); + while (true) { + if (end - begin < 0x40) return; + + // Use first element as pivot + uint256 pivot = _mload(begin); + // Position where the pivot should be at the end of the loop + uint256 pos = begin; + + for (uint256 it = begin + 0x20; it < end; it += 0x20) { + if (comp(_mload(it), pivot)) { + // If the value stored at the iterator's position comes before the pivot, we increment the + // position of the pivot and move the value there. + pos += 0x20; + _swap(pos, it); + } } - } - _swap(begin, pos); // Swap pivot into place - _quickSort(begin, pos, comp); // Sort the left side of the pivot - _quickSort(pos + 0x20, end, comp); // Sort the right side of the pivot + _swap(begin, pos); // Swap pivot into place + // Recurse into the smaller partition and loop for the larger one. + // This converts one recursive call into an iterative tail call, dramatically + // reducing stack depth and raising the maximum sortable array size from ~170 to >1000. + if (pos - begin < end - (pos + 0x20)) { + _quickSort(begin, pos, comp); // Sort the left side of the pivot + begin = pos + 0x20; // do another loop without recursion + } else { + _quickSort(pos + 0x20, end, comp); // Sort the right side of the pivot + end = pos; // do another loop without recursion + } + } } } diff --git a/test/utils/Arrays.t.sol b/test/utils/Arrays.t.sol index 3fc63fabbcd..6cdf411b679 100644 --- a/test/utils/Arrays.t.sol +++ b/test/utils/Arrays.t.sol @@ -394,6 +394,41 @@ contract ArraysTest is Test, SymTest { } } + /// Large array sort tests (stack depth fix) + // See https://github.com/OpenZeppelin/openzeppelin-contracts/issues/6289 + + function testSortLargeArrayDescending() public pure { + // Previously caused stack overflow with N >= 170 (EVM stack depth limit) + // With the iterative fix, this should now succeed for arrays >> 170 elements + uint256 N = 500; + uint256[] memory a = new uint256[](N); + for (uint256 i = 0; i < N; i++) { + a[i] = N - i; // descending: N, N-1, ..., 1 + } + Arrays.sort(a); + // After sort, should be ascending: 1, 2, ..., N + for (uint256 i = 1; i < N; i++) { + assertEq(a[i - 1] < a[i], true); + } + } + + function testSortVeryLargeArrayRandom() public pure { + // Even larger array to confirm iterative approach handles high element counts + uint256 N = 1000; + uint256[] memory a = new uint256[](N); + 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); + } + Arrays.sort(a); + // Verify sorted ascending + for (uint256 i = 1; i < N; i++) { + assertEq(a[i - 1] <= a[i], true); + } + } + /// Helpers function _copyArray(uint256[] memory values) internal pure returns (uint256[] memory) {