Clean up configuration in gen

real-or-random commented at 1:58 pm on April 12, 2021: contributor

gen_context: Don’t include basic-config.h

Before this commit, gen_context.c both included libsecp256k1-config.h and basic-config.h: The former only to obtain ECMULT_GEN_PREC_BITS and the latter to obtain a basic working configuration to be able to use the library.

This was inelegant and confusing: It meant that basic-config.h needs to #undef all the macros defined in libsecp256k1-config.h. Moreover, it meant that basic-config.h cannot define ECMULT_GEN_PREC_BITS, essentially making this file specific for use in gen_context.c.

After this commit, gen_context.c include only libsecp256k1-config.h. basic-config.h is not necessary anymore for the modules used in gen_context.c because 79f1f7a made the preprocessor detect all the relevant config options.

On the way, we remove an unused #define in basic-config.h.

This now cherrypicks https://github.com/bitcoin-core/secp256k1/pull/918/commits/71d76d1b3cb0480620a2071c5cdee033218eb37d from #918.

After this PR, basic-config.h is unused. We could simply remove it. But with the goal of improving non-autotools (#622) in mind, I don’t think that’s the best approach. Here are better options:

We keep the file and it can help users to facilitate non-autotools builds. (This would mean we’ll merge #916 on top of this PR, which now should work properly now).
We take a step back and think about a better solution for non-autotools builds. I’m willing to work on a PR.
- The library currently builds cleanly with a simply compiler invocation except that ECMULT_WINDOW_SIZE and ECMULT_GEN_PREC_BITS are not defined. So I suggest we define default values for those in the corresponding files. Then the library would be cleanly without any necessary config.
- We provide the user with a template config file that documents the available config options. Creating such a template is not too hard using autotools, which supports this for the specific purpose of allowing non-autotools builds.
- We add a (then hopefully simple) section to the README that explains building without autotools.
- We add a CI job that tests that the build works with the command from the README.

real-or-random commented at 5:09 pm on April 12, 2021: contributor

  * We provide the user with a template config file that documents the available config options.

I just noticed that if we’re going to do this, then gen_context.c should at least include libsecp256k1-config.h. I guess one config files is fine, just two is too much. We can probably just#undef ECMULT_USE_STATIC_PRECOMPUTATION in gen_context.c. I’ll update the PR.

edit: I’d still be happy to hear comments on the above plans.

real-or-random marked this as a draft on Apr 12, 2021

sipa commented at 10:52 pm on April 12, 2021: contributor

The problem here really stems from the fact that we have two classes of configuration options:

Things that are specific to the build environment (e.g. presence of ARM assembly, and to a lesser extent the now-autodetected endianness/128bitness)
Things that are configuration tweaks like table precomputation size

The latter has to be identical for the real build and gen_context, but the former cannot be (because gen_context needs to be built natively, while the main build may be cross-compiled).

Random thought: what about replacing gen_context with a Python script? We have some experience with Python code for EC arithmetic already, it’d be more than performant enough for this purpose, constant-timeness is not a concern, and the result would still be subject to all tests of the resulting build (though we way want to add a test that is designed to access all table entries). That would avoid all complications the current build system has that result from cross-compilation.

Other than that… it feels a bit strange to treat this ECMULT_GEN_PREC_BITS configuration for gen_context different from everything else, but I don’t see a better solution.

Another vague thought: I think we should aim for having the library not requiring any #defines at all for building (except for weird platforms where e.g. endianness/64bitness detection fail, or for deviating from defaults like ecmult table size, or for things that require explicit build system support like the arm assembly). With the changes to make endianness and 64bitness automatically detected at compile time, I think we’re most of the way there.

sipa cross-referenced this on Apr 13, 2021 from issue Replace gen_context.c with a Python implementation by sipa

real-or-random commented at 11:49 am on April 13, 2021: contributor

The problem here really stems from the fact that we have two classes of configuration options:
* Things that are specific to the build environment (e.g. presence of ARM assembly, and to a lesser extent the now-autodetected endianness/128bitness)

* Things that are configuration tweaks like table precomputation size
The latter has to be identical for the real build and gen_context, but the former cannot be (because gen_context needs to be built natively, while the main build may be cross-compiled).

Interesting observation. It may be a good idea to split these options into two sections or two files.

Random thought: what about replacing gen_context with a Python script? We have some experience with Python code for EC arithmetic already, it’d be more than performant enough for this purpose, constant-timeness is not a concern, and the result would still be subject to all tests of the resulting build (though we way want to add a test that is designed to access all table entries). That would avoid all complications the current build system has that result from cross-compilation.

Our current code comes with a lot of autotools complexity and can be annoying for the user when it comes to cross-compilation but the approach is pretty solid nowadays. I’m not convinced that a build dependency on Python will be less annoying on average.

But isn’t the real solution here to simply add the generated files to the repo, ideally for all supported values of ECMULT_GEN_PREC_BITS? This will save the user all the trouble and we’re free to do whatever we like in the backend (keep the current approach or switch to Python). Here’s a table of file sizes:

All of this is not an issue. If we care, we can restrict the constant to be 2 or 4. Nobody will need 8 and we anyway want to make the test matrix smaller.

Other than that… it feels a bit strange to treat this ECMULT_GEN_PREC_BITS configuration for gen_context different from everything else, but I don’t see a better solution.

Yeah it’s not elegant but it would work.

Another vague thought: I think we should aim for having the library not requiring any #defines at all for building (except for weird platforms where e.g. endianness/64bitness detection fail, or for deviating from defaults like ecmult table size, or for things that require explicit build system support like the arm assembly). With the changes to make endianness and 64bitness automatically detected at compile time, I think we’re most of the way there.

Right, as I said above, we’re almost there for a basic config. The only missing thing is defaults for the two tweak values of ECMULT_GEN_PREC_BITS and ECMULT_WINDOW_SIZE. After we have those, the library should always compile. You may not get ASM or precomputation but at least it will work out of the box.

sipa commented at 3:40 pm on April 13, 2021: contributor

@real-or-random With #693 the number of possible configurations for the ecmult_gen table will go up a lot though, and in some cases even pretty big ones may be reasonable to use there.

Also if we’d want to provide a mode where the ecmult table is precomputed, perhaps it is not desirable that all its code is checked in?

A possibility perhaps is shipping the precomputed tables for the default configuration in the repository, and use a runtime created one for others (either through native-compiled code, or Python generated code).

real-or-random commented at 4:06 pm on April 13, 2021: contributor

@real-or-random With #693 the number of possible configurations for the ecmult_gen table will go up a lot though, and in some cases even pretty big ones may be reasonable to use there.

Oh I always forget about this PR.

Also if we’d want to provide a mode where the ecmult table is precomputed, perhaps it is not desirable that all its code is checked in?

A possibility perhaps is shipping the precomputed tables for the default configuration in the repository, and use a runtime created one for others (either through native-compiled code, or Python generated code).

That sounds very reasonable, and we can still see if we want to ship just one file for a single configuration or more. The precomputed files are probably still smaller than any installation of Python or a native compiler.

Let me still update this PR as planned. What do other people think about the current approach vs Python?

real-or-random force-pushed on Apr 15, 2021

gen_context: Don't include basic-config.h

Before this commit, gen_context.c both included libsecp256k1-config.h
and basic-config.h: The former only to obtain ECMULT_GEN_PREC_BITS
and the latter to obtain a basic working configuration to be able to
use the library.

This was inelegant and confusing: It meant that basic-config.h needs
to #undef all the macros defined in libsecp256k1-config.h. Moreover,
it meant that basic-config.h cannot define ECMULT_GEN_PREC_BITS,
essentially making this file specific for use in gen_context.c.

After this commit, gen_context.c include only libsecp256k1-config.h.
basic-config.h is not necessary anymore for the modules used in
gen_context.c because 79f1f7a made the preprocessor detect all the
relevant config options.

On the way, we remove an unused #define in basic-config.h.

a3aa2628c7

add ECMULT_GEN_PREC_BITS to basic_config.h

set ECMULT_GEN_PREC_BITS to the "auto" value of 4 in basic_config.h, so libsecp can be used without autoconf

07067967ee

real-or-random force-pushed on Apr 15, 2021

real-or-random marked this as ready for review on Apr 15, 2021

real-or-random commented at 3:23 pm on April 15, 2021: contributor

I fixed this and edited the initial comment.

sipa commented at 8:37 pm on April 15, 2021: contributor

utACK 07067967ee9dcc4af10fd3a565ffb846a2593e92

The switch to Python if desired can always be done later.

jonasnick commented at 9:03 pm on April 18, 2021: contributor

Nice clean up.

We keep the file and it can help users to facilitate non-autotools builds.

How about removing the USE_BASIC_CONFIG ifdef which seems to have no use? Generally, the “we take a step back plan” sounds like a good solution to #622 to me.

ACK mod nit

real-or-random commented at 12:17 pm on April 19, 2021: contributor

Nice clean up.

We keep the file and it can help users to facilitate non-autotools builds.

How about removing the USE_BASIC_CONFIG ifdef which seems to have no use? Generally, the “we take a step back plan” sounds like a good solution to #622 to me.

I think if we keep the file for now, then we should keep this ifdef for now to avoid touching the “API”. Some people may define the macro conditionally.

We’ll probably remove the file later but then at least we interrupt the compile process of users only once then.

real-or-random commented at 12:18 pm on April 19, 2021: contributor

@sipa said somehere else:

I was thinking something like this:

In util.h (or another new file) do all autodetection as much as it can for everything, but supports #ifdefs for overriding things (e.g. endianness, int128, …) and other optional features that need build support (arm asm)

util.h (or this new file) also has an early #ifdef for HAVE_CONFIG_H that includes config.h, which can be written by the build system. Everything else includes just util, not the config directly (because the config only overrides things for autodetection)

building without -D… anything gives you exactly the defaults/autodetected values, nothing else

with extensions util.h could also work without HAVE_CONFIG_H

This sounds very reasonable to me.

jonasnick commented at 12:52 pm on April 19, 2021: contributor

the “API”

Haha, okay. ACK 07067967ee9dcc4af10fd3a565ffb846a2593e92

jonasnick merged this on Apr 19, 2021

jonasnick closed this on Apr 19, 2021

jonasnick cross-referenced this on Apr 19, 2021 from issue add ECMULT_GEN_PREC_BITS to basic_config.h by voisine

real-or-random cross-referenced this on May 2, 2021 from issue Tracking issue for manual (non-autotools) builds by real-or-random

real-or-random cross-referenced this on May 17, 2021 from issue Better lowmemory feature with precomputed ECMULT by devrandom

jonasnick cross-referenced this on Jun 14, 2021 from issue Upstream PRs 831, 907, 903, 889, 918, 906, 928, 922, 933, Merge bitcoin-core/secp256k1#936: Fix gen_context/ASM build on ARM, 925, 937, 926, Merge bitcoin-core/secp256k1#940: contrib: Explain explicit header guards, 850, 930, 941, 846, 947, 662, 950 by jonasnick

real-or-random cross-referenced this on Jun 20, 2021 from issue Fully static precomputation tables by real-or-random

gmaxwell commented at 6:36 pm on June 20, 2021: contributor

With #693 the number of possible configurations for the ecmult_gen table will go up a lot though,

I know I said this on IRC– but I see it’s not here. The configurations there shouldn’t go up: It’s not a good use of resources to test a large number of configurations and 99.999999% of users will have no more idea how to set these things than we do. Instead, there should be a small number of exposed configurations (like small, medium, large) just for protecting the sanity of the user and the testability of the software – and as a side effect it keeps the shipped configurations manageable.

sipa commented at 6:44 pm on June 20, 2021: contributor

@gmaxwell I mean: the number of supportable configurations goes up, if we keep things through the same kind of set-the-individual-tunables-via-./configure interface. I fully agree we should independently (with or without multicomb), change that to a small/medium/large interface.

Clean up configuration in gen_context #918