From 9616bdff04f6630fbf2f19d11d5ca76da68b8273 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 17 Jan 2024 15:48:24 +0100 Subject: [PATCH 01/26] add a Math.inv function that inverse a number in Z/nZ --- contracts/utils/math/Math.sol | 27 ++++++++++++++++++++++++++ test/utils/math/Math.t.sol | 25 ++++++++++++++++++++++++ test/utils/math/Math.test.js | 36 +++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+) diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index 3a1d5a4b24d..4e2e6a28b7a 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -218,6 +218,33 @@ library Math { return result; } + /** + * @notice Calculate the inverse of a in Z/nZ. + * + * If p is a prime, then we Z/nZ is a field commonly noted Fp. In that case all elements are inversible, expect 0. + * If p is not a prime, then Z/nZ is not a field, and some elements might not be inversible. + * + * When trying to inverse a value that is not invesible, 0 is returned. + */ + function inv(uint256 a, uint256 p) internal pure returns (uint256) { + uint256 r1 = a % p; + if (r1 == 0) return 0; + uint256 r2 = p; + uint256 t1 = 0; + uint256 t2 = 1; + while (r1 != 0) { + uint256 q = r2 / r1; + (t1, t2, r2, r1) = ( + t2, + addmod(t1, uint256(p - mulmod(t2, q, p)), p), + r1, + addmod(r2, uint256(p - mulmod(r1, q, p)), p) + ); + } + if (r2 != 1) return 0; + return t1; + } + /** * @dev Returns the square root of a number. If the number is not a perfect square, the value is rounded * towards zero. diff --git a/test/utils/math/Math.t.sol b/test/utils/math/Math.t.sol index a757833796f..f55e2810094 100644 --- a/test/utils/math/Math.t.sol +++ b/test/utils/math/Math.t.sol @@ -55,6 +55,31 @@ contract MathTest is Test { return value * value < ref; } + // INV + function testInv(uint256 seed) public { + uint256 p; + uint256 value; + uint256 inverse; + + // 17 is a prime + p = 17; + value = bound(seed, 1, p - 1); + inverse = Math.inv(value, p); + assertEq(mulmod(value, inverse, p), 1); + + // 65537 is a prime + p = 65537; + value = bound(seed, 1, p - 1); + inverse = Math.inv(value, p); + assertEq(mulmod(value, inverse, p), 1); + + // 0xffffffff00000001000000000000000000000000ffffffffffffffffffffffff is a prime + p = 0xffffffff00000001000000000000000000000000ffffffffffffffffffffffff; + value = bound(seed, 1, p - 1); + inverse = Math.inv(value, p); + assertEq(mulmod(value, inverse, p), 1); + } + // LOG2 function testLog2(uint256 input, uint8 r) public { Math.Rounding rounding = _asRounding(r); diff --git a/test/utils/math/Math.test.js b/test/utils/math/Math.test.js index cb25db67cdf..e264eff2d0e 100644 --- a/test/utils/math/Math.test.js +++ b/test/utils/math/Math.test.js @@ -5,6 +5,7 @@ const { PANIC_CODES } = require('@nomicfoundation/hardhat-chai-matchers/panic'); const { Rounding } = require('../../helpers/enums'); const { min, max } = require('../../helpers/math'); +const { randomArray, generators } = require('../../helpers/random'); const RoundingDown = [Rounding.Floor, Rounding.Trunc]; const RoundingUp = [Rounding.Ceil, Rounding.Expand]; @@ -298,6 +299,41 @@ describe('Math', function () { }); }); + describe.only('inv', function () { + for (const factors of [ + [17n], + [65537n], + [0xffffffff00000001000000000000000000000000ffffffffffffffffffffffffn], + [3n, 5n], + [3n, 7n], + [47n, 53n], + ]) { + const p = factors.reduce((acc, f) => acc * f, 1n); + + describe(`using p=${p} which is ${factors.length > 1 ? 'not ' : ''}a prime`, function () { + it('trying to inverse 0 reverts', async function () { + expect(await this.mock.$inv(0, p)).to.equal(0n); + }); + + it('trying to inverse p reverts', async function () { + expect(await this.mock.$inv(p, p)).to.equal(0n); + }); + + for (const value of randomArray(generators.uint256, 16)) { + const isInversible = factors.every(f => value % f); + it(`tring to inverse ${value}`, async function () { + const result = await this.mock.$inv(value, p); + if (isInversible) { + expect((value * result) % p).to.equal(1n); + } else { + expect(result).to.equal(0n); + } + }); + } + }); + } + }); + describe('sqrt', function () { it('rounds down', async function () { for (const rounding of RoundingDown) { From 0408e142b2ddf1448f2f772cd6ec69bd65f53cee Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 17 Jan 2024 15:51:14 +0100 Subject: [PATCH 02/26] add changeset --- .changeset/cool-mangos-compare.md | 5 +++++ contracts/utils/math/Math.sol | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 .changeset/cool-mangos-compare.md diff --git a/.changeset/cool-mangos-compare.md b/.changeset/cool-mangos-compare.md new file mode 100644 index 00000000000..09cbfa5aa22 --- /dev/null +++ b/.changeset/cool-mangos-compare.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`Math`: add an `inv` function that inverse a number in Z/nZ diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index 4e2e6a28b7a..15d1ed3e82e 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -219,7 +219,7 @@ library Math { } /** - * @notice Calculate the inverse of a in Z/nZ. + * @notice Calculate the inverse of a number in Z/nZ. * * If p is a prime, then we Z/nZ is a field commonly noted Fp. In that case all elements are inversible, expect 0. * If p is not a prime, then Z/nZ is not a field, and some elements might not be inversible. From 989a3d6fca9331c3d170be3b7b0d12458c2860a2 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 17 Jan 2024 15:58:47 +0100 Subject: [PATCH 03/26] codespell --- test/utils/math/Math.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/utils/math/Math.test.js b/test/utils/math/Math.test.js index e264eff2d0e..799250ab624 100644 --- a/test/utils/math/Math.test.js +++ b/test/utils/math/Math.test.js @@ -321,7 +321,7 @@ describe('Math', function () { for (const value of randomArray(generators.uint256, 16)) { const isInversible = factors.every(f => value % f); - it(`tring to inverse ${value}`, async function () { + it(`trying to inverse ${value}`, async function () { const result = await this.mock.$inv(value, p); if (isInversible) { expect((value * result) % p).to.equal(1n); From ce04087b9e1158d3f9d05391b996634f714c63e0 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 17 Jan 2024 16:19:24 +0100 Subject: [PATCH 04/26] remove .only --- test/utils/math/Math.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/utils/math/Math.test.js b/test/utils/math/Math.test.js index 799250ab624..f53fcea6a65 100644 --- a/test/utils/math/Math.test.js +++ b/test/utils/math/Math.test.js @@ -299,7 +299,7 @@ describe('Math', function () { }); }); - describe.only('inv', function () { + describe('inv', function () { for (const factors of [ [17n], [65537n], From 4de37d5aba8e96c982b34d1dc7cb73469132912f Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 17 Jan 2024 16:28:48 +0100 Subject: [PATCH 05/26] rewording --- contracts/utils/math/Math.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index 15d1ed3e82e..94c00a1459b 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -224,7 +224,7 @@ library Math { * If p is a prime, then we Z/nZ is a field commonly noted Fp. In that case all elements are inversible, expect 0. * If p is not a prime, then Z/nZ is not a field, and some elements might not be inversible. * - * When trying to inverse a value that is not invesible, 0 is returned. + * If the input values is not inversible, 0 is returned. */ function inv(uint256 a, uint256 p) internal pure returns (uint256) { uint256 r1 = a % p; From 8915daac8471c0b637f18ee37e5f6021093d4fcf Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 17 Jan 2024 17:17:08 +0100 Subject: [PATCH 06/26] Update Math.sol --- contracts/utils/math/Math.sol | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index 94c00a1459b..783f23fb35f 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -227,19 +227,22 @@ library Math { * If the input values is not inversible, 0 is returned. */ function inv(uint256 a, uint256 p) internal pure returns (uint256) { + if (p == 0) return 0; uint256 r1 = a % p; if (r1 == 0) return 0; uint256 r2 = p; uint256 t1 = 0; uint256 t2 = 1; - while (r1 != 0) { - uint256 q = r2 / r1; - (t1, t2, r2, r1) = ( - t2, - addmod(t1, uint256(p - mulmod(t2, q, p)), p), - r1, - addmod(r2, uint256(p - mulmod(r1, q, p)), p) - ); + unchecked { + while (r1 != 0) { + uint256 q = r2 / r1; + (t1, t2, r2, r1) = ( + t2, + addmod(t1, uint256(p - mulmod(t2, q, p)), p), + r1, + addmod(r2, uint256(p - mulmod(r1, q, p)), p) + ); + } } if (r2 != 1) return 0; return t1; From e82cbf486d7dde2fb255abd2c33169b24f36204e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 17 Jan 2024 17:20:12 +0100 Subject: [PATCH 07/26] more fuzzing --- test/utils/math/Math.t.sol | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/utils/math/Math.t.sol b/test/utils/math/Math.t.sol index f55e2810094..4817aea8b0a 100644 --- a/test/utils/math/Math.t.sol +++ b/test/utils/math/Math.t.sol @@ -56,7 +56,7 @@ contract MathTest is Test { } // INV - function testInv(uint256 seed) public { + function testInv1(uint256 seed) public { uint256 p; uint256 value; uint256 inverse; @@ -80,6 +80,13 @@ contract MathTest is Test { assertEq(mulmod(value, inverse, p), 1); } + function testInv2(uint256 value, uint256 p) public { + uint256 inverse = Math.inv(value, p); + if (inverse != 0) { + assertEq(mulmod(value, inverse, p), 1); + } + } + // LOG2 function testLog2(uint256 input, uint8 r) public { Math.Rounding rounding = _asRounding(r); From 658bc65cfcbccd0b9812d4654862bd577cb86128 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 17 Jan 2024 17:22:53 +0100 Subject: [PATCH 08/26] more fuzzing --- test/utils/math/Math.t.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/utils/math/Math.t.sol b/test/utils/math/Math.t.sol index 4817aea8b0a..41707d5651d 100644 --- a/test/utils/math/Math.t.sol +++ b/test/utils/math/Math.t.sol @@ -66,24 +66,28 @@ contract MathTest is Test { value = bound(seed, 1, p - 1); inverse = Math.inv(value, p); assertEq(mulmod(value, inverse, p), 1); + assertLt(inverse, p); // 65537 is a prime p = 65537; value = bound(seed, 1, p - 1); inverse = Math.inv(value, p); assertEq(mulmod(value, inverse, p), 1); + assertLt(inverse, p); // 0xffffffff00000001000000000000000000000000ffffffffffffffffffffffff is a prime p = 0xffffffff00000001000000000000000000000000ffffffffffffffffffffffff; value = bound(seed, 1, p - 1); inverse = Math.inv(value, p); assertEq(mulmod(value, inverse, p), 1); + assertLt(inverse, p); } function testInv2(uint256 value, uint256 p) public { uint256 inverse = Math.inv(value, p); if (inverse != 0) { assertEq(mulmod(value, inverse, p), 1); + assertLt(inverse, p); } } From 6ea4d455b492096b7c82762a3fb4510ea58dcd19 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 17 Jan 2024 17:29:02 +0100 Subject: [PATCH 09/26] refactor fuzzing --- test/utils/math/Math.t.sol | 50 +++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/test/utils/math/Math.t.sol b/test/utils/math/Math.t.sol index 41707d5651d..24876ee95e5 100644 --- a/test/utils/math/Math.t.sol +++ b/test/utils/math/Math.t.sol @@ -56,38 +56,32 @@ contract MathTest is Test { } // INV - function testInv1(uint256 seed) public { - uint256 p; - uint256 value; - uint256 inverse; - - // 17 is a prime - p = 17; - value = bound(seed, 1, p - 1); - inverse = Math.inv(value, p); - assertEq(mulmod(value, inverse, p), 1); - assertLt(inverse, p); - - // 65537 is a prime - p = 65537; - value = bound(seed, 1, p - 1); - inverse = Math.inv(value, p); - assertEq(mulmod(value, inverse, p), 1); - assertLt(inverse, p); - - // 0xffffffff00000001000000000000000000000000ffffffffffffffffffffffff is a prime - p = 0xffffffff00000001000000000000000000000000ffffffffffffffffffffffff; - value = bound(seed, 1, p - 1); - inverse = Math.inv(value, p); - assertEq(mulmod(value, inverse, p), 1); - assertLt(inverse, p); - } - - function testInv2(uint256 value, uint256 p) public { + function testInv(uint256 value, uint256 p) public { + _testInv(value, p, true); + } + + function testInv17(uint256 seed) public { + uint256 p = 17; // prime + _testInv(bound(seed, 1, p - 1), p, false); + } + + function testInv65537(uint256 seed) public { + uint256 p = 65537; // prime + _testInv(bound(seed, 1, p - 1), p, false); + } + + function testInvP256(uint256 seed) public { + uint256 p = 0xffffffff00000001000000000000000000000000ffffffffffffffffffffffff; // prime + _testInv(bound(seed, 1, p - 1), p, false); + } + + function _testInv(uint256 value, uint256 p, bool allowZero) private { uint256 inverse = Math.inv(value, p); if (inverse != 0) { assertEq(mulmod(value, inverse, p), 1); assertLt(inverse, p); + } else { + assertTrue(allowZero); } } From 89fca1b3a3beac20eac3ae5c3cfa77fb20d07802 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 19 Jan 2024 21:59:46 +0100 Subject: [PATCH 10/26] using signed arithmetics more obviously matches Euclid's method and cost significantly less --- contracts/utils/math/Math.sol | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index 783f23fb35f..4a985d5338f 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -227,25 +227,25 @@ library Math { * If the input values is not inversible, 0 is returned. */ function inv(uint256 a, uint256 p) internal pure returns (uint256) { - if (p == 0) return 0; - uint256 r1 = a % p; - if (r1 == 0) return 0; - uint256 r2 = p; - uint256 t1 = 0; - uint256 t2 = 1; unchecked { + if (p == 0) return 0; + uint256 r1 = a % p; + if (r1 == 0) return 0; + uint256 r2 = p; + int256 t1 = 0; + int256 t2 = 1; while (r1 != 0) { uint256 q = r2 / r1; (t1, t2, r2, r1) = ( - t2, - addmod(t1, uint256(p - mulmod(t2, q, p)), p), - r1, - addmod(r2, uint256(p - mulmod(r1, q, p)), p) + t2, + t1 - t2 * int256(q), + r1, + r2 - r1 * q ); } + if (r2 != 1) return 0; + return t1 < 0 ? (p - uint256(-t1)) : uint256(t1); } - if (r2 != 1) return 0; - return t1; } /** From 1f96c34c8ec0bfc2556ecd1180d800d84de27a56 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 19 Jan 2024 22:00:28 +0100 Subject: [PATCH 11/26] fix lint --- contracts/utils/math/Math.sol | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index 4a985d5338f..f90f348d25b 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -236,12 +236,7 @@ library Math { int256 t2 = 1; while (r1 != 0) { uint256 q = r2 / r1; - (t1, t2, r2, r1) = ( - t2, - t1 - t2 * int256(q), - r1, - r2 - r1 * q - ); + (t1, t2, r2, r1) = (t2, t1 - t2 * int256(q), r1, r2 - r1 * q); } if (r2 != 1) return 0; return t1 < 0 ? (p - uint256(-t1)) : uint256(t1); From 32fbe5359d16969578309b561e04c4554b3f7367 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 19 Jan 2024 22:21:36 +0100 Subject: [PATCH 12/26] fix edge case where t1 = type(int256).min --- contracts/utils/math/Math.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index f90f348d25b..0cbd5b82289 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -3,6 +3,8 @@ pragma solidity ^0.8.20; +import "./SignedMath.sol"; + /** * @dev Standard math utilities missing in the Solidity language. */ @@ -239,7 +241,7 @@ library Math { (t1, t2, r2, r1) = (t2, t1 - t2 * int256(q), r1, r2 - r1 * q); } if (r2 != 1) return 0; - return t1 < 0 ? (p - uint256(-t1)) : uint256(t1); + return t1 < 0 ? p - SignedMath.abs(-t1) : uint256(t1); } } From 5d9bcb5bd798071317201d4a24f5062a58d7fada Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 19 Jan 2024 22:22:58 +0100 Subject: [PATCH 13/26] Revert "fix edge case where t1 = type(int256).min" This reverts commit 32fbe5359d16969578309b561e04c4554b3f7367. --- contracts/utils/math/Math.sol | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index 0cbd5b82289..f90f348d25b 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -3,8 +3,6 @@ pragma solidity ^0.8.20; -import "./SignedMath.sol"; - /** * @dev Standard math utilities missing in the Solidity language. */ @@ -241,7 +239,7 @@ library Math { (t1, t2, r2, r1) = (t2, t1 - t2 * int256(q), r1, r2 - r1 * q); } if (r2 != 1) return 0; - return t1 < 0 ? p - SignedMath.abs(-t1) : uint256(t1); + return t1 < 0 ? (p - uint256(-t1)) : uint256(t1); } } From d32f4da93759df77be0be443c0a102bccbd056f6 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 22 Jan 2024 14:05:58 +0100 Subject: [PATCH 14/26] rename Math.inv to Math.invMod --- contracts/utils/math/Math.sol | 16 ++++++++-------- test/utils/math/Math.t.sol | 20 ++++++++++---------- test/utils/math/Math.test.js | 8 ++++---- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index f90f348d25b..36b37e24ee2 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -221,17 +221,17 @@ library Math { /** * @notice Calculate the inverse of a number in Z/nZ. * - * If p is a prime, then we Z/nZ is a field commonly noted Fp. In that case all elements are inversible, expect 0. - * If p is not a prime, then Z/nZ is not a field, and some elements might not be inversible. + * If n is a prime, then Z/nZ is a field. In that case all elements are inversible, expect 0. + * If n is not a prime, then Z/nZ is not a field, and some elements might not be inversible. * - * If the input values is not inversible, 0 is returned. + * If the input value is not inversible, 0 is returned. */ - function inv(uint256 a, uint256 p) internal pure returns (uint256) { + function invMod(uint256 a, uint256 n) internal pure returns (uint256) { unchecked { - if (p == 0) return 0; - uint256 r1 = a % p; + if (n == 0) return 0; + uint256 r1 = a % n; if (r1 == 0) return 0; - uint256 r2 = p; + uint256 r2 = n; int256 t1 = 0; int256 t2 = 1; while (r1 != 0) { @@ -239,7 +239,7 @@ library Math { (t1, t2, r2, r1) = (t2, t1 - t2 * int256(q), r1, r2 - r1 * q); } if (r2 != 1) return 0; - return t1 < 0 ? (p - uint256(-t1)) : uint256(t1); + return t1 < 0 ? (n - uint256(-t1)) : uint256(t1); } } diff --git a/test/utils/math/Math.t.sol b/test/utils/math/Math.t.sol index 24876ee95e5..34e47cd864b 100644 --- a/test/utils/math/Math.t.sol +++ b/test/utils/math/Math.t.sol @@ -56,27 +56,27 @@ contract MathTest is Test { } // INV - function testInv(uint256 value, uint256 p) public { - _testInv(value, p, true); + function testInvMod(uint256 value, uint256 p) public { + _testInvMod(value, p, true); } - function testInv17(uint256 seed) public { + function testInvMod17(uint256 seed) public { uint256 p = 17; // prime - _testInv(bound(seed, 1, p - 1), p, false); + _testInvMod(bound(seed, 1, p - 1), p, false); } - function testInv65537(uint256 seed) public { + function testInvMod65537(uint256 seed) public { uint256 p = 65537; // prime - _testInv(bound(seed, 1, p - 1), p, false); + _testInvMod(bound(seed, 1, p - 1), p, false); } - function testInvP256(uint256 seed) public { + function testInvModP256(uint256 seed) public { uint256 p = 0xffffffff00000001000000000000000000000000ffffffffffffffffffffffff; // prime - _testInv(bound(seed, 1, p - 1), p, false); + _testInvMod(bound(seed, 1, p - 1), p, false); } - function _testInv(uint256 value, uint256 p, bool allowZero) private { - uint256 inverse = Math.inv(value, p); + function _testInvMod(uint256 value, uint256 p, bool allowZero) private { + uint256 inverse = Math.invMod(value, p); if (inverse != 0) { assertEq(mulmod(value, inverse, p), 1); assertLt(inverse, p); diff --git a/test/utils/math/Math.test.js b/test/utils/math/Math.test.js index f53fcea6a65..46712b18e4e 100644 --- a/test/utils/math/Math.test.js +++ b/test/utils/math/Math.test.js @@ -299,7 +299,7 @@ describe('Math', function () { }); }); - describe('inv', function () { + describe('invMod', function () { for (const factors of [ [17n], [65537n], @@ -312,17 +312,17 @@ describe('Math', function () { describe(`using p=${p} which is ${factors.length > 1 ? 'not ' : ''}a prime`, function () { it('trying to inverse 0 reverts', async function () { - expect(await this.mock.$inv(0, p)).to.equal(0n); + expect(await this.mock.$invMod(0, p)).to.equal(0n); }); it('trying to inverse p reverts', async function () { - expect(await this.mock.$inv(p, p)).to.equal(0n); + expect(await this.mock.$invMod(p, p)).to.equal(0n); }); for (const value of randomArray(generators.uint256, 16)) { const isInversible = factors.every(f => value % f); it(`trying to inverse ${value}`, async function () { - const result = await this.mock.$inv(value, p); + const result = await this.mock.$invMod(value, p); if (isInversible) { expect((value * result) % p).to.equal(1n); } else { From 857bea38d280f496a3f199fcc9c5d6647235aa2f Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 22 Jan 2024 15:49:42 +0100 Subject: [PATCH 15/26] doc --- contracts/utils/math/Math.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index 36b37e24ee2..3890d32531e 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -219,7 +219,7 @@ library Math { } /** - * @notice Calculate the inverse of a number in Z/nZ. + * @notice Calculate the modular multiplicative inverse of a number in Z/nZ. * * If n is a prime, then Z/nZ is a field. In that case all elements are inversible, expect 0. * If n is not a prime, then Z/nZ is not a field, and some elements might not be inversible. From a24657009c2430c11912549fb69424843a3632f1 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 22 Jan 2024 15:58:55 +0100 Subject: [PATCH 16/26] remove unecessary check --- contracts/utils/math/Math.sol | 1 - test/utils/math/Math.t.sol | 5 +++++ test/utils/math/Math.test.js | 29 +++++++++++++++++------------ 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index 3890d32531e..42eb5a756a9 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -230,7 +230,6 @@ library Math { unchecked { if (n == 0) return 0; uint256 r1 = a % n; - if (r1 == 0) return 0; uint256 r2 = n; int256 t1 = 0; int256 t2 = 1; diff --git a/test/utils/math/Math.t.sol b/test/utils/math/Math.t.sol index 34e47cd864b..75d28041dc8 100644 --- a/test/utils/math/Math.t.sol +++ b/test/utils/math/Math.t.sol @@ -60,6 +60,11 @@ contract MathTest is Test { _testInvMod(value, p, true); } + function testInvMod2(uint256 seed) public { + uint256 p = 2; // prime + _testInvMod(bound(seed, 1, p - 1), p, false); + } + function testInvMod17(uint256 seed) public { uint256 p = 17; // prime _testInvMod(bound(seed, 1, p - 1), p, false); diff --git a/test/utils/math/Math.test.js b/test/utils/math/Math.test.js index 46712b18e4e..092e0b35ebb 100644 --- a/test/utils/math/Math.test.js +++ b/test/utils/math/Math.test.js @@ -299,8 +299,11 @@ describe('Math', function () { }); }); - describe('invMod', function () { + describe.only('invMod', function () { for (const factors of [ + [0n], + [1n], + [2n], [17n], [65537n], [0xffffffff00000001000000000000000000000000ffffffffffffffffffffffffn], @@ -310,7 +313,7 @@ describe('Math', function () { ]) { const p = factors.reduce((acc, f) => acc * f, 1n); - describe(`using p=${p} which is ${factors.length > 1 ? 'not ' : ''}a prime`, function () { + describe(`using p=${p} which is ${(p > 1 && factors.length > 1) ? 'not ' : ''}a prime`, function () { it('trying to inverse 0 reverts', async function () { expect(await this.mock.$invMod(0, p)).to.equal(0n); }); @@ -319,16 +322,18 @@ describe('Math', function () { expect(await this.mock.$invMod(p, p)).to.equal(0n); }); - for (const value of randomArray(generators.uint256, 16)) { - const isInversible = factors.every(f => value % f); - it(`trying to inverse ${value}`, async function () { - const result = await this.mock.$invMod(value, p); - if (isInversible) { - expect((value * result) % p).to.equal(1n); - } else { - expect(result).to.equal(0n); - } - }); + if (p != 0) { + for (const value of randomArray(generators.uint256, 16)) { + const isInversible = factors.every(f => value % f); + it(`trying to inverse ${value}`, async function () { + const result = await this.mock.$invMod(value, p); + if (isInversible) { + expect((value * result) % p).to.equal(1n); + } else { + expect(result).to.equal(0n); + } + }); + } } }); } From 6f228f7bc305125b500e9bbe22e816030989fc19 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 22 Jan 2024 15:59:16 +0100 Subject: [PATCH 17/26] remove .only --- test/utils/math/Math.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/utils/math/Math.test.js b/test/utils/math/Math.test.js index 092e0b35ebb..e343a2e621d 100644 --- a/test/utils/math/Math.test.js +++ b/test/utils/math/Math.test.js @@ -299,7 +299,7 @@ describe('Math', function () { }); }); - describe.only('invMod', function () { + describe('invMod', function () { for (const factors of [ [0n], [1n], From a1fc06f4726141aa6d6071d72cd6509274974ed1 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 22 Jan 2024 16:00:33 +0100 Subject: [PATCH 18/26] tests update --- test/utils/math/Math.test.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/test/utils/math/Math.test.js b/test/utils/math/Math.test.js index e343a2e621d..d3daeda2010 100644 --- a/test/utils/math/Math.test.js +++ b/test/utils/math/Math.test.js @@ -314,12 +314,9 @@ describe('Math', function () { const p = factors.reduce((acc, f) => acc * f, 1n); describe(`using p=${p} which is ${(p > 1 && factors.length > 1) ? 'not ' : ''}a prime`, function () { - it('trying to inverse 0 reverts', async function () { + it('trying to inverse 0 returns 0', async function () { expect(await this.mock.$invMod(0, p)).to.equal(0n); - }); - - it('trying to inverse p reverts', async function () { - expect(await this.mock.$invMod(p, p)).to.equal(0n); + expect(await this.mock.$invMod(p, p)).to.equal(0n); // p is 0 mod p }); if (p != 0) { From 1ccee5dee46453d44986c2705f4ac2d57bae60d7 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 23 Jan 2024 12:54:39 -0600 Subject: [PATCH 19/26] Lint --- test/utils/math/Math.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/utils/math/Math.test.js b/test/utils/math/Math.test.js index d3daeda2010..abf43f0738d 100644 --- a/test/utils/math/Math.test.js +++ b/test/utils/math/Math.test.js @@ -313,7 +313,7 @@ describe('Math', function () { ]) { const p = factors.reduce((acc, f) => acc * f, 1n); - describe(`using p=${p} which is ${(p > 1 && factors.length > 1) ? 'not ' : ''}a prime`, function () { + describe(`using p=${p} which is ${p > 1 && factors.length > 1 ? 'not ' : ''}a prime`, function () { it('trying to inverse 0 returns 0', async function () { expect(await this.mock.$invMod(0, p)).to.equal(0n); expect(await this.mock.$invMod(p, p)).to.equal(0n); // p is 0 mod p From 9a754726aa8e2e19fcb89c9e326b597b377ba21d Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 23 Jan 2024 12:55:50 -0600 Subject: [PATCH 20/26] Reword changeset --- .changeset/cool-mangos-compare.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/cool-mangos-compare.md b/.changeset/cool-mangos-compare.md index 09cbfa5aa22..f691642e492 100644 --- a/.changeset/cool-mangos-compare.md +++ b/.changeset/cool-mangos-compare.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`Math`: add an `inv` function that inverse a number in Z/nZ +`Math`: add an `invMod` function to get modular multiplicative inverse of a number in Z/nZ From 0dee9d1042ad45ffceb7ba0b67262bbab796c6ce Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 23 Jan 2024 12:56:04 -0600 Subject: [PATCH 21/26] Add missing point --- .changeset/cool-mangos-compare.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/cool-mangos-compare.md b/.changeset/cool-mangos-compare.md index f691642e492..52908535d9b 100644 --- a/.changeset/cool-mangos-compare.md +++ b/.changeset/cool-mangos-compare.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`Math`: add an `invMod` function to get modular multiplicative inverse of a number in Z/nZ +`Math`: add an `invMod` function to get modular multiplicative inverse of a number in Z/nZ. From cadc8e755e49ee00fa80b23d3f65d4c93c4cc74e Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 23 Jan 2024 12:56:33 -0600 Subject: [PATCH 22/26] Typo --- .changeset/cool-mangos-compare.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/cool-mangos-compare.md b/.changeset/cool-mangos-compare.md index 52908535d9b..470ee089456 100644 --- a/.changeset/cool-mangos-compare.md +++ b/.changeset/cool-mangos-compare.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`Math`: add an `invMod` function to get modular multiplicative inverse of a number in Z/nZ. +`Math`: add an `invMod` function to get the modular multiplicative inverse of a number in Z/nZ. From 284c2bba5be8cf514b543b112d4cba56aefeb9e0 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 23 Jan 2024 15:57:38 -0600 Subject: [PATCH 23/26] Improve readability --- contracts/utils/math/Math.sol | 46 ++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index 42eb5a756a9..3cf38007c2e 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -229,16 +229,44 @@ library Math { function invMod(uint256 a, uint256 n) internal pure returns (uint256) { unchecked { if (n == 0) return 0; - uint256 r1 = a % n; - uint256 r2 = n; - int256 t1 = 0; - int256 t2 = 1; - while (r1 != 0) { - uint256 q = r2 / r1; - (t1, t2, r2, r1) = (t2, t1 - t2 * int256(q), r1, r2 - r1 * q); + + // The inverse modulo is calculated using the Extended Euclidean Algorithm (iterative version) + // Used to compute integers x and y such that: ax + ny = gcd(a, n). + // When the gcd is 1, then the inverse of a modulo n exists and it's x. + // ax + ny = 1 + // ax = 1 + (-y)n + // ax ≡ 1 (mod n) # x is the inverse of a modulo n + + // If the remainder is 0 the gcd is n right away. + uint256 remainder = a % n; + uint256 gcd = n; + + // Therefore the initial coefficients are: + // ax + ny = gcd(a, n) = n + // 0a + 1n = n + int256 x = 0; + int256 y = 1; + + while (remainder != 0) { + uint256 quotient = gcd / remainder; + + (gcd, remainder) = ( + // The old remainder is the next gcd to try. + remainder, + // Compute the next remainder. + gcd - remainder * quotient + ); + + (x, y) = ( + // Increment the coefficient of a. + y, + // Decrement the coefficient of n. + x - y * int256(quotient) + ); } - if (r2 != 1) return 0; - return t1 < 0 ? (n - uint256(-t1)) : uint256(t1); + + if (gcd != 1) return 0; // No inverse exists. + return x < 0 ? (n - uint256(-x)) : uint256(x); // Wrap the result if it's negative. } } From eb6b2eddc8a8cbef31c35e7afdf400f1c75d5ad8 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 23 Jan 2024 16:15:48 -0600 Subject: [PATCH 24/26] Explain overflows --- contracts/utils/math/Math.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index 3cf38007c2e..e143cc7927b 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -254,6 +254,8 @@ library Math { // The old remainder is the next gcd to try. remainder, // Compute the next remainder. + // Can't overflow given that (a % gcd) * (gcd // (a % gcd)) <= gcd + // where gcd is at most n (capped to type(uint256).max) gcd - remainder * quotient ); @@ -261,6 +263,8 @@ library Math { // Increment the coefficient of a. y, // Decrement the coefficient of n. + // Can overflow, but the result is casted to uint256 so that the + // next value of y is "wrapped around" to a value between 0 and n - 1. x - y * int256(quotient) ); } From 16a10a1bf0155b746332db7f43762e803f834e56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 23 Jan 2024 16:16:25 -0600 Subject: [PATCH 25/26] Update contracts/utils/math/Math.sol --- contracts/utils/math/Math.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index e143cc7927b..00325825860 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -219,7 +219,7 @@ library Math { } /** - * @notice Calculate the modular multiplicative inverse of a number in Z/nZ. + * @notice Calculate the modular multiplicative inverse of a number in Z/nZ. * * If n is a prime, then Z/nZ is a field. In that case all elements are inversible, expect 0. * If n is not a prime, then Z/nZ is not a field, and some elements might not be inversible. From 69000cc52be0e185361131c47244dcfce2d74677 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 24 Jan 2024 09:38:00 +0100 Subject: [PATCH 26/26] =?UTF-8?q?@notice=20=E2=86=92=20@dev?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- contracts/utils/math/Math.sol | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/contracts/utils/math/Math.sol b/contracts/utils/math/Math.sol index 42eb5a756a9..9dc770c0e93 100644 --- a/contracts/utils/math/Math.sol +++ b/contracts/utils/math/Math.sol @@ -121,9 +121,10 @@ library Math { } /** - * @notice Calculates floor(x * y / denominator) with full precision. Throws if result overflows a uint256 or + * @dev Calculates floor(x * y / denominator) with full precision. Throws if result overflows a uint256 or * denominator == 0. - * @dev Original credit to Remco Bloemen under MIT license (https://xn--2-umb.com/21/muldiv) with further edits by + * + * Original credit to Remco Bloemen under MIT license (https://xn--2-umb.com/21/muldiv) with further edits by * Uniswap Labs also under MIT license. */ function mulDiv(uint256 x, uint256 y, uint256 denominator) internal pure returns (uint256 result) { @@ -208,7 +209,7 @@ library Math { } /** - * @notice Calculates x * y / denominator with full precision, following the selected rounding direction. + * @dev Calculates x * y / denominator with full precision, following the selected rounding direction. */ function mulDiv(uint256 x, uint256 y, uint256 denominator, Rounding rounding) internal pure returns (uint256) { uint256 result = mulDiv(x, y, denominator); @@ -219,7 +220,7 @@ library Math { } /** - * @notice Calculate the modular multiplicative inverse of a number in Z/nZ. + * @dev Calculate the modular multiplicative inverse of a number in Z/nZ. * * If n is a prime, then Z/nZ is a field. In that case all elements are inversible, expect 0. * If n is not a prime, then Z/nZ is not a field, and some elements might not be inversible. @@ -282,7 +283,7 @@ library Math { } /** - * @notice Calculates sqrt(a), following the selected rounding direction. + * @dev Calculates sqrt(a), following the selected rounding direction. */ function sqrt(uint256 a, Rounding rounding) internal pure returns (uint256) { unchecked {