Follow-ups to making all tables fully static by sipa · Pull Request #1042 · bitcoin-core/secp256k1

sipa · 2021-12-17T19:42:23Z

This PR implements a number of changes to follow up after merging #988:

Naming consistency:
- All precomputed table files now have name precomputed_*.*
- All source files related to the creation of the precomputed table files have name precompute_*.*.
- All source files related to the computation of tables (whether they go in precomputed files or not) have name *_compute_table.*.
Make the tables for exhaustive tests be computed at runtime rather than compile time (this was already the case for ecmult_gen, but not ecmult). This is a preparation for the next point, as the alternative would be to have separate precomputed libraries for the exhaustive tests and other binaries.
Moves the actual tables to separate precomputed_*.c files, which are compiled only once as part of a new libsecp256k1_precomputed.la, included where relevant. The corresponding precomputed_*.h file are normal source files.

Retry of #1041.

real-or-random

ACK mod nits

real-or-random · 2021-12-18T09:58:33Z

+    /* Recreate the ecmult{,_gen} tables using the right generator (as selected via EXHAUSTIVE_TEST_ORDER) */
    secp256k1_ecmult_gen_create_prec_table(&secp256k1_ecmult_gen_prec_table[0][0], &secp256k1_ge_const_g, ECMULT_GEN_PREC_BITS);
+    secp256k1_ecmult_compute_two_tables(secp256k1_pre_g, secp256k1_pre_g_128, WINDOW_G, &secp256k1_ge_const_g);
+


Just for curiosity: I haven't read the tests in detail now but is there a simple reason why the exhaustive tests worked with the wrong ecmult tables but not with the wrong ecmult_gen tables? The existence of known-output tests?

nit: no need to add a second newline here

The existing ecmult tables (from #956) contain separate tables for the exhaustive test case. So they're not using wrong tables.

The problem is that this approach isn't really compatible with the separately-compiled .c file with tables, unless there is a separately-compiled module for (each) exhaustive test generator. So instead I'm modifying the code to use the same approach for ecmult as ecmult_gen does since #988.

real-or-random · 2021-12-18T10:13:17Z

I think so far it was in theory possible to use libsecp by just including the secp256k1.c file, i.e., by including the source instead of linking binaries. I assume this is not a use case we care about (and I don't expect anyone to do this) but I would like to draw attention to this.

sipa · 2021-12-18T21:18:35Z

@real-or-random Good to point that out, but I don't think it's really a concern. I think we should preserve (and improve) the ability to build without a complex build/config system around it, but whether that involves building one or two modules isn't the end of the world. If there is actual desire for that, it wouldn't be hard to make src/precomputed_ecmult_{,gen_}.h include the corresponding .c file.

jonasnick

otherwise looks good

jonasnick

ACK e05da9e

real-or-random · 2021-12-20T10:21:58Z

ACK e05da9e

hebasto · 2023-03-13T11:58:05Z

@sipa

During reviewing of the current master branch, I've noticed that extern "C" {...} guarded by #ifdef __cplusplus were added to non-public headers in the commit e05da9e "Fix c++ build".

But I cannot see any scenario where those headers, i.e., src/precomputed_ecmult.h and src/precomputed_ecmult_gen.h, are processed by a C++ compiler.

FWIW, no issues are observed when compiling Bitcoin Core after removal of extern "C" {...} from the mentioned headers.

Did I miss something?

real-or-random · 2023-03-26T04:49:13Z

But I cannot see any scenario where those headers, i.e., src/precomputed_ecmult.h and src/precomputed_ecmult_gen.h, are processed by a C++ compiler.

I haven't checked, but maybe those will would be relevant when compiling the precomputed compilation unit with a C compiler but the rest with a C++ compiler? But why should anyone do this? ping @sipa

sipa added 5 commits December 17, 2021 11:15

Rename gen_ecmult_gen_static_prec_table -> precompute_ecmult_gen

bae7768

Rename gen_ecmult_static_pre_g -> precompute_ecmult

f95b810

Rename ecmult_gen_static_prec_table -> precomputed_ecmult_gen

7cf47f7

Rename ecmult_static_pre_g -> precomputed_ecmult

075252c

Rename ecmult_gen_prec -> ecmult_gen_compute_table

725370c

sipa force-pushed the 202112_precdotc2 branch 5 times, most recently from fd2a2a6 to 7e2eb52 Compare December 17, 2021 21:25

real-or-random reviewed Dec 18, 2021

View reviewed changes

sipa added 9 commits December 18, 2021 16:11

Rename function secp256k1_ecmult_gen_{create_prec -> compute}_table

31feab0

Split ecmult table computation and printing

fc1bf9f

Move ecmult table computation code to separate file

e458ec2

Compute ecmult tables at runtime for tests_exhaustive

38cd84a

Simplify precompute_ecmult_print_*

bb36331

Split off .c file from precomputed_ecmult_gen.h

1a6691a

Split off .c file from precomputed_ecmult.h

19d96e1

Cleanup preprocessor indentation in precompute{,d}_ecmult{,_gen}

c45386d

Fix c++ build

e05da9e

sipa force-pushed the 202112_precdotc2 branch from 7e2eb52 to e05da9e Compare December 18, 2021 21:13

jonasnick reviewed Dec 18, 2021

View reviewed changes

Comment thread src/ecmult_compute_table.h

jonasnick reviewed Dec 19, 2021

View reviewed changes

real-or-random merged commit be6944a into bitcoin-core:master Dec 20, 2021

This was referenced Dec 20, 2021

Tracking issue for manual (non-autotools) builds #929

Open

Make all source and header files self-contained #1039

Open

jonasnick mentioned this pull request Jan 2, 2022

Sync Upstream BlockstreamResearch/secp256k1-zkp#159

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Follow-ups to making all tables fully static#1042

Follow-ups to making all tables fully static#1042
real-or-random merged 14 commits into
bitcoin-core:masterfrom
sipa:202112_precdotc2

sipa commented Dec 17, 2021 •

edited

Loading

Uh oh!

real-or-random left a comment

Uh oh!

Uh oh!

real-or-random Dec 18, 2021

Uh oh!

sipa Dec 18, 2021

Uh oh!

real-or-random commented Dec 18, 2021

Uh oh!

sipa commented Dec 18, 2021

Uh oh!

jonasnick left a comment

Uh oh!

Uh oh!

jonasnick left a comment

Uh oh!

real-or-random commented Dec 20, 2021

Uh oh!

hebasto commented Mar 13, 2023

Uh oh!

real-or-random commented Mar 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

sipa commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

real-or-random left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

real-or-random Dec 18, 2021

Choose a reason for hiding this comment

Uh oh!

sipa Dec 18, 2021

Choose a reason for hiding this comment

Uh oh!

real-or-random commented Dec 18, 2021

Uh oh!

sipa commented Dec 18, 2021

Uh oh!

jonasnick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonasnick left a comment

Choose a reason for hiding this comment

Uh oh!

real-or-random commented Dec 20, 2021

Uh oh!

hebasto commented Mar 13, 2023

Uh oh!

real-or-random commented Mar 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sipa commented Dec 17, 2021 •

edited

Loading