ecmult: Clean up integer types in _ecmult_wnaf #1795

pull real-or-random wants to merge 2 commits into bitcoin-core:master from real-or-random:202601-ecmult-wnaf-types changing 1 files +31 −29

real-or-random commented at 2:20 PM on January 7, 2026: contributor

Fixes #1769.

On top of #1794.
real-or-random added the label tweak/refactor on Jan 7, 2026
ecmult: Clean up integer types in strauss 0f75e12a90

ecmult: Fix VERIFY_CHECK to exclude UB

Left-shifting an int32_t by 31 may be signed overflow

242df7f18a

real-or-random force-pushed on Jan 7, 2026
hebasto commented at 2:49 PM on January 7, 2026: member

Concept ACK.

in src/ecmult_impl.h:164 in 0f75e12a90

 164 |      secp256k1_scalar s;
 165 | -    int last_set_bit = -1;
 166 | -    int bit = 0;
 167 | -    int sign = 1;
 168 | -    int carry = 0;
 169 | +    size_t last_set_bit = -1;

theStack commented at 2:08 PM on June 30, 2026:

nit: maybe

    size_t last_set_bit = SIZE_MAX;

(that's what we use in secp256k1_ge_set_all_gej_var, fwiw)

theStack commented at 2:09 PM on June 30, 2026: contributor

Concept ACK
mnfadel commented at 10:44 PM on July 18, 2026: none
Concept ACK on moving these to explicit widths — the size_t / int32_t split matches the array-index convention established in #1794, and dropping 0 <= len is right now that len is unsigned.

Two observations, one of which I think deserves a mention in the commit message.

The w bound change looks like a correctness fix, not a type cleanup
```
-    VERIFY_CHECK(2 <= w && w <= 31);
+    VERIFY_CHECK(2 <= w && w <= 30);
```
Unless I'm misreading this, it isn't cosmetic. In the loop body:
```
carry = (word >> (w-1)) & 1;
word -= carry << w;
```
carry is int32_t, so at w == 31 the expression carry << w shifts a 1 into the sign bit of a 32-bit signed type, which is UB. If that's right, the previous w <= 31 bound already admitted UB for a caller passing the maximum permitted window, and this PR quietly closes it.

In practice w is only ever WINDOW_A / WINDOW_G, so nothing is affected today — but VERIFY_CHECK documents the contract, and it seems worth stating that 30 is required rather than merely tidier. A reader scanning a commit titled "clean up integer types" wouldn't expect the accepted parameter range to narrow, so calling it out in the message (or splitting it into its own commit) would make the history easier to follow.

size_t last_set_bit = -1;

This is well-defined, and the wrap is load-bearing: with no set bits it stays SIZE_MAX, and last_set_bit + 1 wraps back to 0, which is the intended return. But the correctness of the return value now rests on a wraparound that nothing in the function mentions.

Would size_t last_set_bit = SIZE_MAX; be preferable? It denotes the same value explicitly and avoids an implicit -1 → unsigned conversion — which is the class of diagnostic that motivated the ECMULT_TABLE_SIZE change in #1794. Failing that, a one-line comment noting the wrap is deliberate would cover it.

Happy to build and run the test suite on this branch and follow up with a tested ACK.

Contributors

Labels

Linked (view graph)

#1769 `_ecmult_wnaf` relies on `int` being at least 32 bits #1794 ecmult: Use size_t for array indices

ecmult: Clean up integer types in _ecmult_wnaf #1795

The `w` bound change looks like a correctness fix, not a type cleanup

`size_t last_set_bit = -1;`

ecmult: Clean up integer types in _ecmult_wnaf #1795

The w bound change looks like a correctness fix, not a type cleanup

size_t last_set_bit = -1;

The `w` bound change looks like a correctness fix, not a type cleanup

`size_t last_set_bit = -1;`