Follow-ups to making all tables fully static

[sipa]

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]

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

[sipa]

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]

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]

@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]

ACK e05da9e

[hebasto]

It looks like this change breaks building Bitcoin Core with MSVC:

...
     Creating library C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\x64\Release\bitcoind.lib and object C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\x64\Release\bitcoind.exp
libsecp256k1.lib(secp256k1.obj) : error LNK2019: unresolved external symbol secp256k1_pre_g referenced in function secp256k1_ecmult_strauss_wnaf [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\bitcoind\bitcoind.vcxproj]
libsecp256k1.lib(secp256k1.obj) : error LNK2019: unresolved external symbol secp256k1_pre_g_128 referenced in function secp256k1_ecmult_strauss_wnaf [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\bitcoind\bitcoind.vcxproj]
libsecp256k1.lib(secp256k1.obj) : error LNK2019: unresolved external symbol secp256k1_ecmult_gen_prec_table referenced in function secp256k1_ecmult_gen [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\bitcoind\bitcoind.vcxproj]
C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\x64\Release\bitcoind.exe : fatal error LNK1120: 3 unresolved externals [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\bitcoind\bitcoind.vcxproj]

[real-or-random]

@hebasto This is because we now have a separate compilation unit for the precomputed tables. So Core needs to tell MSVC to build libsecp256k1_precomputed separately and link it to the library.

But I have no idea about the required changes to https://github.com/bitcoin/bitcoin/blob/master/build_msvc/libsecp256k1/libsecp256k1.vcxproj that would be equivalent to this automake snippet.

If this is anyway maintained within the Bitcoin Core org, maybe we should move the vcxproj file to this repo in the future. This will help us add MSVC builds to the CI here.

[hebasto]

@sipsorcery

Could you look into this?

[sipsorcery]

@hebasto inlcuding the new precompute .c files in the libsecp25k1.vcxproj got the build working for me.

  <ItemGroup>
    <ClCompile Include="..\..\src\secp256k1\src\precomputed_ecmult.c" />
    <ClCompile Include="..\..\src\secp256k1\src\precomputed_ecmult_gen.c" />
    <ClCompile Include="..\..\src\secp256k1\src\precompute_ecmult.c" />
    <ClCompile Include="..\..\src\secp256k1\src\precompute_ecmult_gen.c" />
    <ClCompile Include="..\..\src\secp256k1\src\secp256k1.c" />
  </ItemGroup>

[hebasto]

@sipsorcery

Thanks! It works flawlessly.

[real-or-random]

@sipsorcery That works but it's too much: You don't need the precompute_ files, the precomputed files should be enough.

Also, this will then recompile the precomputed_ files every time, which makes recompilation somewhat slower than what we have in our Makefile. (This was the reason why we introduced the libsecp256k1_precomputed as a separate object.) But yeah, this only affects compilation time, so it's probably not worth to optimize the vcxproj for this.

edit: Oh, now I see that @sipa already replied in bitcoin/bitcoin#23432 (comment).

[hebasto]

@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]

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