Suggested here: #1118 (review)
This adds a new field function secp256k1_fe_add_int which is equivalent to secp256k1_fe_set_int + secp256k1_fe_add, which is a pattern that occurs a few times, and uses a needlessly convoluted computation for an absolutely trivial effect (incrementing one limb variable). Existing PRs like #1118 and #1129 add more ways this function would be useful.
375 | @@ -376,7 +376,7 @@ static void secp256k1_gej_add_var(secp256k1_gej *r, const secp256k1_gej *a, cons 376 | secp256k1_gej_double_var(r, a, rzr); 377 | } else { 378 | if (rzr != NULL) { 379 | - secp256k1_fe_set_int(rzr, 0); 380 | + secp256k1_fe_clear(rzr);
This reminds me of https://github.com/bitcoin-core/secp256k1/pull/636/commits/b33a8e49e89f5a1ea02a9b9b9b208cb4f3d59e44 Okay, the PR is almost 4 years old now... :/ But I should continue the work at some point.
Currently, clear is mostly used to kill secrets (though that doesn't work due to dead-store elimination), and this should be separated from setting to 0 in the future for the purposes of the aforementioned PR. Now, the advantage of clear over set_int(0) is that the former sets the magnitude to 0 instead of 1. In the mentioned PR, I change set_int to do the same for input 0 but that's a bad idea because we want statically implied magnitude in the long term. The proper solution to all of this is a set_zero function that guarantees to set the value and magnitude to 0 (and won't just clear out memory).
What does this mean for this PR? To avoid getting side tracked again, I'd say don't introduce that new function here, but maybe keep that line and the instances below just how it is. That avoids introducing more code locations where clear is not used for clearing memory. The current behavior of setting magnitude of 1 is apparently good enough with the current code.
Done, I've refrained from touching any set_int(..., 0) -> clear(...).
424 | @@ -425,6 +425,20 @@ SECP256K1_INLINE static void secp256k1_fe_mul_int(secp256k1_fe *r, int a) { 425 | #endif 426 | } 427 | 428 | +SECP256K1_INLINE static void secp256k1_fe_add_int(secp256k1_fe *r, int a) { 429 | +#ifdef VERIFY 430 | + secp256k1_fe_verify(r); 431 | + VERIFY_CHECK(a >= 0); 432 | + VERIFY_CHECK(a <= 0xFFFF);
Is there a reason to keep it in these bounds? Sorry if it's obvious, but I don't see it.
Apparently (for unclear reasons...) set_int is restricted to range 0...0x7FFF. The 0xFFFF is a typo here; I wanted to copy the bound, so that the tests can verify correspondence between add_int and set_int + add.
0x7FFF is the minimum portable MAX_INT, see #943 (review)
84 | @@ -85,6 +85,9 @@ static void secp256k1_fe_get_b32(unsigned char *r, const secp256k1_fe *a); 85 | * as an argument. The magnitude of the output is one higher. */ 86 | static void secp256k1_fe_negate(secp256k1_fe *r, const secp256k1_fe *a, int m); 87 | 88 | +/** Adds a small integer (up to 0x7FFF) to r. The resulting magnitude increases by one. */
/** Adds a small integer (up to 0xFFFF) to r. The resulting magnitude increases by one. */
At least that's what you VERIFY_CHECK.
Changed to 0x7FFF everywhere instead.
Nice! Concept ACK
utACK b081f7e4cbfd27edc36e823dcd93537a46f7d2a6
ACK b081f7e4cbfd27edc36e823dcd93537a46f7d2a6
226 | @@ -227,7 +227,7 @@ static int secp256k1_ge_set_xo_var(secp256k1_ge *r, const secp256k1_fe *x, int o 227 | secp256k1_fe_sqr(&x2, x); 228 | secp256k1_fe_mul(&x3, x, &x2); 229 | r->infinity = 0; 230 | - secp256k1_fe_add(&x3, &secp256k1_fe_const_b);
secp256k1_fe_const_b is now unused.
@roconnor-blockstream Good catch. Do you want to open a PR to remove it?
I suppose I can do that.