Skip to content

ecmult: Clean up integer types in _ecmult_wnaf#1795

Open
real-or-random wants to merge 2 commits into
bitcoin-core:masterfrom
real-or-random:202601-ecmult-wnaf-types
Open

ecmult: Clean up integer types in _ecmult_wnaf#1795
real-or-random wants to merge 2 commits into
bitcoin-core:masterfrom
real-or-random:202601-ecmult-wnaf-types

Conversation

@real-or-random

Copy link
Copy Markdown
Contributor

Fixes #1769.

On top of #1794.

@hebasto

hebasto commented Jan 7, 2026

Copy link
Copy Markdown
Member

Concept ACK.

@theStack theStack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Concept ACK

Comment thread src/ecmult_impl.h
int bit = 0;
int sign = 1;
int carry = 0;
size_t last_set_bit = -1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: maybe

Suggested change
size_t last_set_bit = -1;
size_t last_set_bit = SIZE_MAX;

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

@mnfadel

mnfadel commented Jul 18, 2026

Copy link
Copy Markdown

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

_ecmult_wnaf relies on int being at least 32 bits

4 participants