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.

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

fix: inconsistency in the sqrt function #91

Merged
merged 1 commit into from
Jul 3, 2022
Merged

Conversation

Amxx
Copy link
Contributor

@Amxx Amxx commented Jun 1, 2022

Fixes #97.

The sqrt function using newton methods with a first guess that is the most significant bit of the sqrt or x. This is done using an algorithm that is similar to mostSignificantBit, but that increase the result by the half of what mostSignificantBit does.

While mostSignificantBit is correct, sqrt uses 0x8 instead of 0x4 for 2**2


Note: a cleaner, but slightly more expensive, version of the code could do

function sqrt(uint256 x) internal pure returns (uint256 result) {
        if (x == 0) {
            return 0;
        }

        // Set the initial guess to the least power of two that is greater than or equal to sqrt(x).
        result = 1 << (mostSignificantBit(x) >> 1); 
        
        // The operations can never overflow because the result is max 2^127 when it enters this block.
        unchecked {
            result = (result + x / result) >> 1;
            result = (result + x / result) >> 1;
            result = (result + x / result) >> 1;
            result = (result + x / result) >> 1;
            result = (result + x / result) >> 1;
            result = (result + x / result) >> 1;
            result = (result + x / result) >> 1; // Seven iterations should be enough
            uint256 roundedDownResult = x / result;
            return result >= roundedDownResult ? roundedDownResult : result;
        }
    }

@PaulRBerg
Copy link
Owner

PaulRBerg commented Jul 3, 2022

Hello @Amxx, I'm sorry for the delay.

This is great, thanks so much for spotting this error and opening a PR to fix.

@PaulRBerg PaulRBerg merged commit 5d72349 into PaulRBerg:main Jul 3, 2022
@PaulRBerg PaulRBerg added the bug label Nov 26, 2022
@Amxx Amxx deleted the fix/sqrt branch February 23, 2023 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect hardcoded value of 0x8 in sqrt function (should be 0x4)
2 participants