Eliminate the use of points and scalars from secp256k1_ecmult_strauss_batch. #900
Conversation
roconnor-blockstream
force-pushed
the
20210301-no-points-scalars
branch
from
fcb614a to
5001229
Compare
roconnor-blockstream
force-pushed
the
20210301-no-points-scalars
branch
from
5001229 to
1d6745c
Compare
roconnor-blockstream
force-pushed
the
20210301-no-points-scalars
branch
from
1d6745c to
994e38e
Compare
roconnor-blockstream
force-pushed
the
20210301-no-points-scalars
branch
from
994e38e to
6c171d7
Compare
roconnor-blockstream
force-pushed
the
20210301-no-points-scalars
branch
from
6c171d7 to
3652cec
Compare
There was a problem hiding this comment.
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:
| state.ps = (struct secp256k1_strauss_point_state*)secp256k1_scratch_alloc(error_callback, scratch, n_points * sizeof(struct secp256k1_strauss_point_state)); | ||
|
|
||
| if (points == NULL || scalars == NULL || state.prej == NULL || state.zr == NULL || state.pre_a == NULL || state.pre_a_lam == NULL || state.ps == NULL) { | ||
| if (state.aux == NULL || state.pre_a == NULL || state.ps == NULL || | ||
| !secp256k1_ecmult_strauss_wnaf(&state, r, n_points, &secp256k1_ecmult_adaptor_cb, &adaptor_data, cb_offset, inp_g_sc)) { |
There was a problem hiding this comment.
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.
roconnor-blockstream
force-pushed
the
20210301-no-points-scalars
branch
2 times, most recently
from
b1be8c1 to
c3a2b34
Compare
…_batch. We have secp256k1_ecmult_strauss_wnaf invoke the callback itself.
roconnor-blockstream
force-pushed
the
20210301-no-points-scalars
branch
from
c3a2b34 to
29a1b7e
Compare
|
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. |
We have secp256k1_ecmult_strauss_wnaf invoke the callback itself.
This builds upon PR #899.