configure: Allow users to explicitly enable libsecp256k1's GMP bignum support

luke-jr commented at 2:42 pm on October 11, 2020: member

By moving the --with-bignum=no option to the start of the configure args, users can explicitly override it when configuring Bitcoin Core.

When this option was originally added in 2015 (7fd5b801ff16d748b5ca13ded09ed5da8eacf7e7 #6210), there was mention of a runtime crash reported by gmaxwell. Do details of that incident exist anywhere? Was it ever resolved? Did it crash at startup? If not, could it be triggered remotely?

DrahtBot added the label Build system on Oct 11, 2020

DrahtBot commented at 8:25 pm on October 11, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

laanwj commented at 8:55 am on October 14, 2020: member

The reason for disabling gmp by default is that the releases should not have a dependency on gmp. There are some good reasons to have the default configure options equivalent (or as close as possible) to the release options. Especially for something that affects consensus so directly if there was a bug in this less-tested path.

Concept ACK on being able to override it on demand though.

fanquake commented at 1:59 am on October 15, 2020: member

Can you clarify why this needs to be moved? Using master (661fe5d65cc6516439f9d6e0f1a5e2db0e129059) I can do:

0./autogen.sh
1./configure --with-bignum=gmp
2....
3  asm                     = x86_64
4  bignum                  = gmp
5  ecmult window size      = 15

and secp256k1 is configured with bignum support using gmp. I guess there’s somewhere this doesn’t currently work? @sipa also mentioned on IRC:

2020-10-11T16:16:27 luke-jr: fwiw, after secp256k1 PR 767 we’ll probably remove bignum support entirely

gmaxwell commented at 9:21 am on October 15, 2020: contributor

This is going to be removed, so I don’t think it makes sense to worry about it.

I don’t know if it would be correct to say GMP to be maintained in a way suitable for consensus consistency: Although GMP testing is extremely good, they would– for example, quickly fix a bug that slipped through in a minor release where consensus code might want to be handled in a more coordinated manner. (Although the secp256k1 GMP actually double-checks the GMP computation and will runtime assert for an incorrect result with 100% confidence, though if a GMP bug causes memory corruption all bets are off).

Use of GMP also potentially has licensing implications for users/redistributors as GMP is LGPLv3/GPLv2.

As far as the crash– I don’t recall and don’t have access to old IRC logs, it’s extremely likely that it would have been discussed in public. It would have been debian on PPC64 (big endian).

DrahtBot added the label Needs rebase on Oct 15, 2020

luke-jr commented at 6:38 pm on October 24, 2020: member

@fanquake No idea how you’re getting that…

0$ ./configure --with-bignum=gmp
1...
2  bignum                  = no

configure: Allow users to explicitly enable libsecp256k1's GMP bignum support a26496f6a7

luke-jr commented at 6:40 pm on October 24, 2020: member

Rebased

luke-jr force-pushed on Oct 24, 2020

DrahtBot removed the label Needs rebase on Oct 24, 2020

laanwj commented at 10:15 am on November 19, 2020: member

This is going to be removed, so I don’t think it makes sense to worry about it.

I think that alone is enough reason to not do this, let alone the other reasons you mention. NACK

luke-jr closed this on Nov 19, 2020

DrahtBot locked this on Feb 15, 2022

configure: Allow users to explicitly enable libsecp256k1’s GMP bignum support #20121

Conflicts