Update overflow check #1218

pull roconnor-blockstream wants to merge 1 commits into bitcoin-core:master from roconnor-blockstream:20230306_update_de changing 2 files +8 −8

roconnor-blockstream commented at 10:14 PM on March 6, 2023: contributor

One does not simply check for integer overlow.
sipa commented at 10:17 PM on March 6, 2023: contributor

The new tests are not exactly equivalent, because if both upper bounds are reached, the sum of their absolute values may still overflow. E.g. if $u = v = 2^{30}$, then $|u| + |v| = 2^{31}$ which isn't representable in a int32_t.
roconnor-blockstream force-pushed on Mar 6, 2023
roconnor-blockstream commented at 10:49 PM on March 6, 2023: contributor

Trying again.
sipa commented at 11:12 PM on March 6, 2023: contributor

ACK a2930137d214500b8cd02ea6bfd9c3971cd29546

Update overflow check

One does not simply check for integer overlow.

2ef1c9b387

roconnor-blockstream force-pushed on Mar 6, 2023
roconnor-blockstream commented at 11:15 PM on March 6, 2023: contributor

Third time. (Sorry for nulling your ack; I think this variant is marginally better because it is a little bit stricter while also logical).
sipa commented at 11:27 PM on March 6, 2023: contributor

ACK 2ef1c9b38700b7cca2ee1aace2f020ee834729c0
real-or-random approved
real-or-random commented at 8:11 AM on March 7, 2023: contributor

ACK 2ef1c9b38700b7cca2ee1aace2f020ee834729c0
real-or-random merged this on Mar 7, 2023
real-or-random closed this on Mar 7, 2023

in src/modinv64_impl.h:422 in 2ef1c9b387

 418 | @@ -419,10 +419,10 @@ static void secp256k1_modinv64_update_de_62(secp256k1_modinv64_signed62 *d, secp
 419 |      VERIFY_CHECK(secp256k1_modinv64_mul_cmp_62(d, 5, &modinfo->modulus, 1) < 0);  /* d <    modulus */
 420 |      VERIFY_CHECK(secp256k1_modinv64_mul_cmp_62(e, 5, &modinfo->modulus, -2) > 0); /* e > -2*modulus */
 421 |      VERIFY_CHECK(secp256k1_modinv64_mul_cmp_62(e, 5, &modinfo->modulus, 1) < 0);  /* e <    modulus */
 422 | -    VERIFY_CHECK((secp256k1_modinv64_abs(u) + secp256k1_modinv64_abs(v)) >= 0); /* |u|+|v| doesn't overflow */
 423 | -    VERIFY_CHECK((secp256k1_modinv64_abs(q) + secp256k1_modinv64_abs(r)) >= 0); /* |q|+|r| doesn't overflow */
 424 | -    VERIFY_CHECK((secp256k1_modinv64_abs(u) + secp256k1_modinv64_abs(v)) <= (int64_t)1 << 62); /* |u|+|v| <= 2^62 */
 425 | -    VERIFY_CHECK((secp256k1_modinv64_abs(q) + secp256k1_modinv64_abs(r)) <= (int64_t)1 << 62); /* |q|+|r| <= 2^62 */
 426 | +    VERIFY_CHECK(secp256k1_modinv64_abs(v) <= (int64_t)1 << 62);                                 /*     |v| <= 2^62 */

roconnor-blockstream commented at 12:56 PM on March 7, 2023:

Do you think we should just remove this bounds check? The line below is signed arithmetic an thus will not underflow. And the success of the line below implies this is line is true anyways.

real-or-random commented at 1:33 PM on March 7, 2023:

I guess that's a very minor improvement, but I wouldn't care. If this bothers you and you want to open a PR, I'm happy to ACK it.

roconnor-blockstream deleted the branch on Mar 7, 2023
dhruv referenced this in commit a5df79db12 on Mar 7, 2023
dhruv referenced this in commit 77b510d84c on Mar 7, 2023
real-or-random referenced this in commit 6048e6c03e on Mar 8, 2023
sipa referenced this in commit 763079a3f1 on Mar 8, 2023
div72 referenced this in commit 945b094575 on Mar 14, 2023
vmta referenced this in commit e1120c94a1 on Jun 4, 2023
vmta referenced this in commit 8f03457eed on Jul 1, 2023
delta1 referenced this in commit 3f32c20932 on Aug 8, 2023
delta1 referenced this in commit 31ac0c1081 on Aug 31, 2023
janus referenced this in commit 4816e2a921 on Sep 3, 2023
backpacker69 referenced this in commit 26f6d832fb on Mar 5, 2024
str4d referenced this in commit b350ac56ac on Jun 4, 2025
Fabcien referenced this in commit 3810ccbcc3 on Apr 11, 2026
Fabcien referenced this in commit 601096b987 on Apr 11, 2026

Contributors

roconnor-blockstream

sipa

real-or-random

Linked (view graph)

#1222 Remove redundant checks.