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.
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);
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.
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. */
0/** Adds a small integer (up to 0xFFFF) to r. The resulting magnitude increases by one. */
At least that’s what you VERIFY_CHECK.
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.