Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved integer square root. #4403

Merged
merged 26 commits into from
Feb 16, 2024
Merged

Improved integer square root. #4403

Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
171e5c2
Improved integer square root.
chgorman Jun 29, 2023
beec057
Small updates to sqrt function.
chgorman Jul 22, 2023
a48fda8
Merge branch 'master' into master
Amxx Jul 24, 2023
b6c29b1
Fixed variable naming.
chgorman Jul 24, 2023
0d2bbb3
Added some comments to help clarify the algorithm.
chgorman Jul 24, 2023
1c8e8ab
Updated edge case for clarity.
chgorman Jul 25, 2023
8e2ab78
Made small improvements.
chgorman Aug 31, 2023
77db52d
Merge branch 'master' into master
chgorman Jan 22, 2024
8b3f15f
Added additional comments to include associated proof.
chgorman Feb 13, 2024
dc098e8
Merge branch 'master' into master
ernestognw Feb 13, 2024
bd9d969
Reduce sqrt comments
ernestognw Feb 14, 2024
dfe2fbc
Lint
ernestognw Feb 14, 2024
a53d378
Correcting return logic
chgorman Feb 14, 2024
6c74012
remove overflow check (early) in favor of overflow save math at the e…
Amxx Feb 14, 2024
fe8abc3
format
Amxx Feb 14, 2024
382ab6f
Iterate on comments
ernestognw Feb 15, 2024
c60542b
Iterate on comments
ernestognw Feb 15, 2024
ddea292
Lint
ernestognw Feb 15, 2024
360f65d
Codespell
ernestognw Feb 15, 2024
4288c74
Iterate on comments
ernestognw Feb 15, 2024
256189a
rewrite most comments of the sqrt function
Amxx Feb 15, 2024
6d00bbd
improve
Amxx Feb 15, 2024
6c8be26
shift notation e+1 → e
Amxx Feb 15, 2024
67d0328
complete proof
Amxx Feb 15, 2024
85a56d6
improvement
Amxx Feb 15, 2024
e748bfc
remove reference to Heron's method to avoid confusion
Amxx Feb 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions contracts/utils/math/Math.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,12 +225,12 @@ library Math {
/**
* @dev Returns the square root of a number. If the number is not a perfect square, the value is rounded down.
*/
function sqrt(uint256 a) internal pure returns (uint256 result) {
function sqrt(uint256 a) internal pure returns (uint256) {
unchecked {
if (a <= 1) { return a; }
if (a >= ((1 << 128) - 1)**2) { return (1 << 128) - 1; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a risk of overflow ? Can you document which part is succeptible ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this specific method and return logic, it is possible to overflow when computing result**2 during the check result**2 <= a, because it may be that result == 2**128; thus, result**2 == 0 because this is unchecked.

Copy link
Collaborator

@Amxx Amxx Jul 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interresting. I guess this is not necessary if you do min(result, a/result) so the current version is good in that regard.

I'd rewrite that as:

Suggested change
if (a >= ((1 << 128) - 1)**2) { return (1 << 128) - 1; }
if (a >= uint256(type(uint128).max)**2) { return type(uint128).max; }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the cost of that test outweight the cost of the min at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interresting. I guess this is not necessary if you do min(result, a/result) so the current version is good in that regard.

The current version has no proof of correctness. This proposed change does. See Appendix B.4.3. Have you looked at that report?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code was updated with suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, the overflow check (if (a >= uint256(type(uint128).max)**2) { return type(uint128).max; }) costs 23 gas.

uint256 aAux = a;
result = 1;
uint256 result = 1;
if (aAux >= (1 << 128)) { aAux >>= 128; result <<= 64; }
if (aAux >= (1 << 64 )) { aAux >>= 64; result <<= 32; }
if (aAux >= (1 << 32 )) { aAux >>= 32; result <<= 16; }
Expand All @@ -245,7 +245,10 @@ library Math {
result = (result + a / result) >> 1;
result = (result + a / result) >> 1;
result = (result + a / result) >> 1;
return result * result <= a ? result : (result - 1);
if (result * result <= x) {
chgorman marked this conversation as resolved.
Show resolved Hide resolved
return result;
}
return result-1;
}
}

Expand Down