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

Add sqrt for math #3242

Merged
merged 21 commits into from Jun 7, 2022
Merged

Add sqrt for math #3242

merged 21 commits into from Jun 7, 2022

Conversation

jjz
Copy link
Contributor

@jjz jjz commented Mar 6, 2022

Fixes #3149

When we use SWAP, we often need to use the sqrt operation, which is not in Math, so I added it.

https://github.com/Uniswap/v2-core/blob/4dd59067c76dea4a0e8e4bfdda41877a6b16dedc/contracts/UniswapV2Pair.sol#L95

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@frangio
Copy link
Contributor

frangio commented Mar 30, 2022

Thanks @jjz.

Some things we want to do before merging:

  • Measure worst-case gas cost.
  • Compare worst-case gas cost with Balancer's LogExpMath.

@jjz
Copy link
Contributor Author

jjz commented Apr 10, 2022

I don't know what kind of evaluation is needed, because this method is pure

@Amxx
Copy link
Collaborator

Amxx commented May 30, 2022

The version in #3282 is simpler (no special case, but has the drawback of overflowing for sqrt(type(uint256).max)

   function sqrt(uint256 x) public pure returns (uint256 y) {
        uint256 z = (x + 1) / 2; // This can overflow
        y = x;
        while (z < y) {
            y = z;
            z = (x / z + z) / 2;
        }
        return y;
    }

This can be fixed by doing

   function sqrt(uint256 x) public pure returns (uint256 y) {
        uint256 z = x / 2 + x % 2; // This cannot overflow
        y = x;
        while (z < y) {
            y = z;
            z = (x / z + z) / 2;
        }
        return y;
    }

I suggest we use that version.

@Amxx
Copy link
Collaborator

Amxx commented May 30, 2022

Gas costs are worryingly high. Finding the sqrt of type(uint256).max cost about 35k gas

@Amxx
Copy link
Collaborator

Amxx commented Jun 1, 2022

PRB has a version with constant cost

IMO this would be much better. It's a bit more expensive for small values, but it is orders of magnitude cheaper for big values.

@Amxx Amxx requested a review from frangio June 2, 2022 12:54
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -149,4 +149,62 @@ library Math {
}
return result;
}

/**
* @dev Returns the square root of a number.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only the case for perfect squares, we should document the behavior for the other numbers. I suppose it rounds down.

Does it make sense (is it possible?) to provide a version with a Rounding argument?

Do you think we can document some aspect of the efficiency of this function? It's O(1) but can we say anything stronger in terms of the constants involved?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Added a version with Rounding

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we could provide an upper bound on the gas cost, I'm not sure what else we can say

// of the bits. the partial result produced is a power of two that verifies `result <= sqrt(a) < 2 * result`
uint256 result = 1;
uint256 x = a;
if (x > 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would write this like this, otherwise we basically need to count Fs to know if the code is right.

        if (x > (1 << 128) - 1) {

The compiler seems to optimize this expression to a constant, even when optimizations are disabled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

then what about doing x >= 1 << 128. Would that also get optimized? to the same bytecode. Because there is no gte opcode I'm worried it would be compiled to not(lt(..., ...))

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I found an even better rewording.

Comment on lines 163 to 164
// For our first guess, we use the log2 of the square root. We do that by shifting `a` and only counting half
// of the bits. the partial result produced is a power of two that verifies `result <= sqrt(a) < 2 * result`
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is clear or accurate.

For our first guess, we use the log2 of the square root.

What does it mean that we "use the log2"? I read this like saying that our guess is log2(sqrt(a)), but I don't think this is true.

I also find the part about shifting a confusing.

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Amxx Amxx merged commit 3ac4add into OpenZeppelin:master Jun 7, 2022
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.

Add Math.sqrt
3 participants