We have secp256k1_ecmult_strauss_wnaf invoke the callback itself.
This builds upon PR #899.
We have secp256k1_ecmult_strauss_wnaf invoke the callback itself.
This builds upon PR #899.
81 | @@ -82,7 +82,7 @@
82 | * contain prej[0].z / a.z. The other zr[i] values = prej[i].z / prej[i-1].z.
In commit c531ff8fb96cdaa871e637631a346bb1352cca3d:
The documentation above secp256k1_ecmult_odd_multiples_table is now outdated (it is only fixed in the next commit, I believe).
It's probably because I don't know how to GitHub, but I intended for these comments to end up on #899 instead.
Done.
76 | * the same global z "denominator". zr must contain the known z-ratios such 77 | - * that mul(a[i].z, zr[i+1]) == a[i+1].z. zr[0] is ignored. The x and y 78 | - * coordinates of the result are stored in r, the common z coordinate is 79 | - * stored in globalz. */ 80 | -static void secp256k1_ge_globalz_set_table_gej(size_t len, secp256k1_ge *r, secp256k1_fe *globalz, const secp256k1_gej *a, const secp256k1_fe *zr); 81 | + * that mul(a[i].z, zr[i+1]) == a[i+1].z. zr[0] is ignored.
In c531ff8fb96cdaa871e637631a346bb1352cca3d:
perhaps document which global z everything is set to?
Done.
438 | - state.pre_a_lam = (secp256k1_ge*)secp256k1_scratch_alloc(error_callback, scratch, n_points * ECMULT_TABLE_SIZE(WINDOW_A) * sizeof(secp256k1_ge)); 439 | state.ps = (struct secp256k1_strauss_point_state*)secp256k1_scratch_alloc(error_callback, scratch, n_points * sizeof(struct secp256k1_strauss_point_state)); 440 | 441 | - if (points == NULL || scalars == NULL || state.prej == NULL || state.zr == NULL || state.pre_a == NULL || state.pre_a_lam == NULL || state.ps == NULL) { 442 | + if (state.aux == NULL || state.pre_a == NULL || state.ps == NULL || 443 | + !secp256k1_ecmult_strauss_wnaf(&state, r, n_points, &secp256k1_ecmult_adaptor_cb, &adaptor_data, cb_offset, inp_g_sc)) {
If you're checking the return value of secp256k1_ecmult_strauss_wnaf when you call it here, would you also need to check it when you call it at the end of secp256k1_ecmult?
My concern is that if the callback (and thus the call to secp256k1_ecmult_strauss_wnaf) "fails", this could either (1) introduce a way for secp256k1_ecmult to fail unexpectedly, or (2) force secp256k1_ecmult to return int instead of void, and either way this would break the "public" API.
I only reviewed the last 2 commits, since the rest are from #899 .
I think the callback/adaptor is a nice idea for optimizing space usage, but my main concern is about possibly breaking the "public" API (see inline comment).
A quick check of ./bench_ecmult strauss_wnaf at three different versions shows that there are no immediately obvious performance regressions:

We have secp256k1_ecmult_strauss_wnaf invoke the callback itself.
I'm going to close this PR. I don't feel like pursing it; I'm not really convinced it is worthwhile. If, for whatever reason, someone disagree and does think it is worthwhile, feel free to take it over.