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
  1. roconnor-blockstream commented at 10:14 PM on March 6, 2023: contributor

    One does not simply check for integer overlow.

  2. 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.

  3. real-or-random cross-referenced this on Mar 6, 2023 from issue modinv: Avoid that signed overflow may occur when tests fail by real-or-random
  4. roconnor-blockstream force-pushed on Mar 6, 2023
  5. roconnor-blockstream commented at 10:49 PM on March 6, 2023: contributor

    Trying again.

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

    ACK a2930137d214500b8cd02ea6bfd9c3971cd29546

  7. Update overflow check
    One does not simply check for integer overlow.
    2ef1c9b387
  8. roconnor-blockstream force-pushed on Mar 6, 2023
  9. 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).

  10. sipa commented at 11:27 PM on March 6, 2023: contributor

    ACK 2ef1c9b38700b7cca2ee1aace2f020ee834729c0

  11. real-or-random approved
  12. real-or-random commented at 8:11 AM on March 7, 2023: contributor

    ACK 2ef1c9b38700b7cca2ee1aace2f020ee834729c0

  13. real-or-random merged this on Mar 7, 2023
  14. real-or-random closed this on Mar 7, 2023
  15. 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.

  16. roconnor-blockstream deleted the branch on Mar 7, 2023
  17. roconnor-blockstream cross-referenced this on Mar 7, 2023 from issue Remove redundant checks. by roconnor-blockstream
  18. dhruv referenced this in commit a5df79db12 on Mar 7, 2023
  19. dhruv referenced this in commit 77b510d84c on Mar 7, 2023
  20. real-or-random referenced this in commit 6048e6c03e on Mar 8, 2023
  21. sipa referenced this in commit 763079a3f1 on Mar 8, 2023
  22. div72 referenced this in commit 945b094575 on Mar 14, 2023
  23. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
  24. vmta referenced this in commit 8f03457eed on Jul 1, 2023
  25. jonasnick cross-referenced this on Jul 20, 2023 from issue Upstream PRs 1160, 1193, 1169, 1190, 1192, 1194, 1196, 1195, 1170, 1172, 1200, 1199, 1203, 1201, 1206, 1078, 1209, 979, 1212, 1218, 1217, 1221, 1222 by jonasnick
Contributors
roconnor-blockstreamsipareal-or-random
Linked (view graph)
#1219 modinv: Avoid that signed overflow may occur when tests fail#1222 Remove redundant checks.
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-14 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me