Reduce stratch space needed by ecmult_strauss_wnaf. by roconnor-blockstream · Pull Request #899 · bitcoin-core/secp256k1

roconnor-blockstream · 2021-02-26T20:23:37Z

No description provided.

ghost · 2021-03-29T19:26:31Z

Address @sipa 's comments from #900.

Please review this PR first.

ghost · 2021-04-06T20:22:55Z

@real-or-random observes that STRAUSS_SCRATCH_OBJECTS needs to be updated.

ghost · 2021-05-12T15:48:14Z

Rebased and updated STRAUSS_SCRATCH_OBJECTS.

ghost · 2021-08-28T18:07:06Z

rebased.

ghost

Concept ACK, secp256k1_ecmult_strauss_wnaf indeed uses scratch space more efficiently with this change.

I ran ./bench_ecmult strauss_wnaf at both 1cd7ab2 and 9a5a87e just to make sure there's no obvious regression in running time.

Unfortunately it looks like this might have some merge conflicts with #638 depending on which gets merged first.

ghost · 2021-12-01T06:03:33Z

+ *  The omitted z-coordinate are implied by the final a value's z-coordinate and the zr array.
 */
-static void secp256k1_ecmult_odd_multiples_table(int n, secp256k1_gej *prej, secp256k1_fe *zr, const secp256k1_gej *a) {
+static void secp256k1_ecmult_odd_multiples_table(int n, secp256k1_ge *pre_a, secp256k1_fe *zr, secp256k1_gej *a) {


Style nit (feel free to ignore): Instead of making a mutable, would it make sense to have an output parameter for the final z value, since that's the only part of a that gets read later?

I have learned that returning structures is frowned upon in C.

Oh yeah, I agree with you about that. What I was suggesting is to have an extra parameter to the function that's a pointer like secp256k1_fe *global_z (or maybe z_final), instead of updating a in place.

Done. I'm a little tempted to take z as an input and rescale the a value by its value, but it is probably best to leave it as is.

ghost

OK, I took another look and I think I have a better understanding now. This PR looks pretty solid.

I left a few suggestions for comments / style, mostly because I thought some parts were confusing even before your PR. If you prefer I can also push them to some branch as a follow-up commit.

ghost · 2021-12-08T18:36:06Z

Thanks @robot-dreams. I've taken your suggestions and made some minor adjustments to them.

ghost

ACK 5aea2e8 (from inspecting the range diff against ae33232)

ghost · 2021-12-10T02:20:19Z

ACK b6ed302

ghost

Good stuff and despite being an optimization, this PR simplifies a lot of things as well. Also thanks for the new comments. They're really helpful.

ACK mod nits

ghost · 2022-01-19T16:56:30Z

I've rearranged the code a little bit and rewritten the comments a bit.

ghost

ACK b797a50

ghost · 2022-01-25T19:41:30Z

ACK b797a50

roconnor-blockstream force-pushed the no-prej branch from f44de02 to 44e44a6 Compare February 26, 2021 22:17

roconnor-blockstream marked this pull request as ready for review February 26, 2021 22:18

roconnor-blockstream changed the title ~~WIP: Eliminate the prej array from ecmult_strauss_wnaf.~~ Eliminate the prej array from ecmult_strauss_wnaf. Feb 26, 2021

roconnor-blockstream force-pushed the no-prej branch from 44e44a6 to 3bb7260 Compare February 26, 2021 22:27

roconnor-blockstream changed the title ~~Eliminate the prej array from ecmult_strauss_wnaf.~~ Reduce stratch space needed by ecmult_strauss_wnaf. Feb 27, 2021

roconnor-blockstream force-pushed the no-prej branch 3 times, most recently from fcb614a to 4ba9971 Compare March 1, 2021 13:26

roconnor-blockstream mentioned this pull request Mar 1, 2021

Eliminate the use of points and scalars from secp256k1_ecmult_strauss_batch. #900

Closed

roconnor-blockstream force-pushed the no-prej branch from 4ba9971 to 6d5f1de Compare March 29, 2021 19:25

roconnor-blockstream force-pushed the no-prej branch from 6d5f1de to 59745ee Compare March 30, 2021 14:47

roconnor-blockstream force-pushed the no-prej branch from 59745ee to c9bd399 Compare May 12, 2021 15:47

roconnor-blockstream force-pushed the no-prej branch from c9bd399 to 1cd7ab2 Compare August 28, 2021 18:05

robot-dreams reviewed Dec 1, 2021

View reviewed changes

roconnor-blockstream force-pushed the no-prej branch 2 times, most recently from 80fc6f1 to ae33232 Compare December 3, 2021 23:12

robot-dreams reviewed Dec 6, 2021

View reviewed changes

Comment thread src/ecmult_impl.h Outdated

Comment thread src/ecmult_impl.h

Comment thread src/group.h Outdated

Comment thread src/group_impl.h

Comment thread src/field_impl.h Outdated

roconnor-blockstream force-pushed the no-prej branch from ae33232 to 5aea2e8 Compare December 8, 2021 18:33

robot-dreams reviewed Dec 8, 2021

View reviewed changes

Comment thread src/group.h Outdated

Comment thread src/group.h Outdated

roconnor-blockstream force-pushed the no-prej branch from 5aea2e8 to b6ed302 Compare December 9, 2021 15:15

sipa mentioned this pull request Dec 18, 2021

Add another ecmult_multi test #1044

Merged

jonasnick reviewed Dec 23, 2021

View reviewed changes

Comment thread src/group.h Outdated

Comment thread src/ecmult_impl.h Outdated

Comment thread src/ecmult_impl.h Outdated

Comment thread src/ecmult_impl.h Outdated

Comment thread src/ecmult_impl.h

Comment thread src/ecmult_impl.h

robot-dreams mentioned this pull request Dec 23, 2021

ecmult: move _ecmult_odd_multiples_table_globalz_windowa #1053

Merged

roconnor-blockstream force-pushed the no-prej branch from b6ed302 to 9d2c777 Compare January 17, 2022 19:20

Russell O'Connor added 6 commits January 19, 2022 11:51

Eliminate the pre_a_lam array from ecmult_strauss_wnaf.

b3b57ad

Remove the unused pre_a_lam allocations.

7ba3ffc

Eliminate na_1 and na_lam state fields from ecmult_strauss_wnaf.

0397d00

Eliminate input_pos state field from ecmult_strauss_wnaf.

fe34d9f

Replace ECMULT_TABLE_GET_GE_STORAGE macro with a function.

a731200

Create a SECP256K1_ECMULT_TABLE_VERIFY macro.

b797a50

roconnor-blockstream force-pushed the no-prej branch from 4c14457 to b797a50 Compare January 19, 2022 16:55

jonasnick reviewed Jan 19, 2022

View reviewed changes

jonasnick merged commit d8a2463 into bitcoin-core:master Jan 26, 2022

jonasnick mentioned this pull request Mar 30, 2022

Upstream PRs 1064, 1049, 899, 1068, 1072, 1069, 1074, 1026, 1033, 748, 1079, 1088, 1090, 731, 1089, 995, 1094, 1093 BlockstreamResearch/secp256k1-zkp#174

Merged

Uh oh!

Conversation

roconnor-blockstream commented Feb 26, 2021

Uh oh!

ghost commented Mar 29, 2021

Uh oh!

ghost commented Apr 6, 2021

Uh oh!

ghost commented May 12, 2021

Uh oh!

ghost commented Aug 28, 2021

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

ghost Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

ghost Dec 1, 2021 • edited by robot-dreams Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Dec 8, 2021

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ghost commented Dec 10, 2021

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Jan 19, 2022

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ghost Dec 1, 2021 •

edited by robot-dreams

Loading