build: Remove HAVE_CONSENSUS

TheCharlatan commented at 4:50 pm on December 20, 2023: contributor

The script/bitcoinconsensus module defines the public interface for the bitcoinconsensus library. Even though this module is only required by the tests and the bitcoinconsensus library, it is currently compiled into the static internal libbitcoin_consensus library, and therefore used by a bunch of build targets that do not require it.

Since it is always part of the internal library, the HAVE_CONSENSUS_LIB define used by the tests is essentially meaningless, since the module is always available. This can be verified on master by removing the HAVE_CONSENSUS_LIB checks in the tests, configuring with --with-libs=no, and running make check.

Improve all of this by including the bitcoinconsensus module only where it is required and removing the HAVE_CONSENSUS_LIB define.

An alternative to this patch was discussed in https://github.com/hebasto/bitcoin/pull/41: Actually linking the test binaries with the external consensus library if it is available. This has the disadvantage though that if the dynamic consensus library is built, it has to be available for the test binaries to load whenever and wherever they are being run.

build: Remove HAVE_CONSENSUS_LIB

The `script/bitcoinconsensus` module defines the public interface for
the `bitcoinconsensus` library. Even though this module is only required
by the tests and the `bitcoinconsensus` library, it is currently
compiled into the static internal `libbitcoin_consensus` library, and
therefore used by a bunch of build targets that do not require it.

Since it is always part of the internal library, the
`HAVE_CONSENSUS_LIB` define used by the tests is essentially
meaningless, since the module is always available. This can be verified
on master by removing the `HAVE_CONSENSUS_LIB` checks in the tests,
configuring with `--with-libs=no`, and running `make check`.

b37a171dbb

DrahtBot commented at 4:50 pm on December 20, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, vasild
Concept ACK	theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29265 (Add SigVersion parameter to IsOpSuccess() by Christewart)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Build system on Dec 20, 2023

TheCharlatan commented at 4:50 pm on December 20, 2023: contributor

cc @hebasto @theuni

theuni commented at 5:22 pm on December 20, 2023: member

Concept ACK.

I think another way of looking at this is that script/bitcoinconsensus.cpp is moving out into its own convenience lib, it just happens to be a single file. We could imagine if the api were written in multiple translation units, we’d actually want to build that convenience lib. But the question is: absent that, is it worth the trouble?

Honestly I don’t really care because it’s kinda funky either way. Curious to see if anyone else has a preference.

hebasto commented at 11:56 am on December 21, 2023: member

Since it is always part of the internal library, the HAVE_CONSENSUS_LIB define used by the tests is essentially meaningless, since the module is always available.

It seems worth noting that we do not use the HAVE_CONSENSUS_LIB macro in /src/test/fuzz/script_bitcoin_consensus.cpp in the master branch.

hebasto commented at 12:22 pm on December 21, 2023: member

Concept ACK on moving the script/bitcoinconsensus.cpp out from the internal libbitcoin_consensus.a library. @theuni

I think another way of looking at this is that script/bitcoinconsensus.cpp is moving out into its own convenience lib, it just happens to be a single file. We could imagine if the api were written in multiple translation units, we’d actually want to build that convenience lib.

That is very similar to what I suggested in #24994.

But the question is: absent that, is it worth the trouble?

For example, to fix the symbol visibility issue.

theuni commented at 5:35 pm on December 21, 2023: member

@hebasto #24994 counted on linker side-effects to fix things IIRC. As do your other suggestions as far as I can see. If there’s an issue with a compiler/stdlib symbol visibility, let’s track it down and get the issues reported upstream as opposed to working around them with brittle workarounds.

hebasto commented at 5:41 pm on December 21, 2023: member

@theuni

If there’s an issue with a compiler/stdlib symbol visibility, let’s track it down and get the issues reported upstream as opposed to working around them with brittle workarounds.

#26698 (comment):

The underlying problem is described in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36022, and it was not considered as a bug.

hebasto approved

hebasto commented at 5:42 pm on December 21, 2023: member

ACK b37a171dbbcd9a7a8934099be1b508a0ea4f05a8.

DrahtBot requested review from theuni on Dec 21, 2023

vasild approved

vasild commented at 11:55 am on December 22, 2023: contributor

ACK b37a171dbbcd9a7a8934099be1b508a0ea4f05a8

maflcko added the label DrahtBot Guix build requested on Dec 22, 2023

DrahtBot commented at 5:53 am on December 23, 2023: contributor

Guix builds (on x86_64)

File	commit 4b1196a9855dcd188a24f393aa2fa21e2d61f061(master)	commit 162a169e845215e52eae692202c3222af21b74a1(master and this pull)
SHA256SUMS.part	`7aefd803a584a152...`	`c9c88507e2b1c29c...`
*-aarch64-linux-gnu-debug.tar.gz	`735f38fb37b239f8...`	`e8751fcba6a3857d...`
*-aarch64-linux-gnu.tar.gz	`83a257c74e184b96...`	`524f9c0663e78d16...`
*-arm-linux-gnueabihf-debug.tar.gz	`513acf435b18fab2...`	`ddab696a8c6d3861...`
*-arm-linux-gnueabihf.tar.gz	`581f911fcca5ae02...`	`777f8c934210246c...`
*-arm64-apple-darwin-unsigned.tar.gz	`7a3f2b4e01f27896...`	`d2a1614bc2186e5a...`
*-arm64-apple-darwin-unsigned.zip	`dff45b014236e990...`	`58709693ef349088...`
*-arm64-apple-darwin.tar.gz	`0d9696b7bd76b36f...`	`d8d72316859b7f5d...`
*-powerpc64-linux-gnu-debug.tar.gz	`dfe6cfdcd6ad3c6f...`	`a6cbbf09fc81e90f...`
*-powerpc64-linux-gnu.tar.gz	`54c013201daad1ff...`	`ab8d50fd98bb7b35...`
*-powerpc64le-linux-gnu-debug.tar.gz	`8e944b002273b678...`	`20f4572fe366c3a7...`
*-powerpc64le-linux-gnu.tar.gz	`1ba8199984d7c43a...`	`df39769bd8b934e2...`
*-riscv64-linux-gnu-debug.tar.gz	`e9abe48aeb7acc68...`	`16da8f0afde2a02e...`
*-riscv64-linux-gnu.tar.gz	`f4d1484dec65b967...`	`8f6d556eda7ee5fc...`
*-x86_64-apple-darwin-unsigned.tar.gz	`915650aa35e6e322...`	`9d5b2c3aafeefc35...`
*-x86_64-apple-darwin-unsigned.zip	`6a6139cb415de743...`	`0e8e2ec7ab7ecceb...`
*-x86_64-apple-darwin.tar.gz	`c479d099d1df9a85...`	`f20ed8546f6a72cf...`
*-x86_64-linux-gnu-debug.tar.gz	`9e27161179f18a4a...`	`5585b7fdf4aef067...`
*-x86_64-linux-gnu.tar.gz	`f69632ca3dc98896...`	`3e5c37e6b2def850...`
*.tar.gz	`c04997c9f9e51d80...`	`1dde913fa303d6e4...`
guix_build.log	`e5ef1439ef10a87a...`	`052d056473d09f7b...`
guix_build.log.diff		`2de91436d107e8a2...`

DrahtBot removed the label DrahtBot Guix build requested on Dec 23, 2023

theuni referenced this in commit 35b8bf8ec4 on Jan 5, 2024

theuni referenced this in commit 5545ee21e6 on Jan 5, 2024

theuni referenced this in commit a6745f8be3 on Jan 5, 2024

fanquake commented at 6:14 pm on January 11, 2024: member

Maybe draft for now, while we hash out #29189?

TheCharlatan marked this as a draft on Jan 12, 2024

DrahtBot added the label CI failed on Jan 15, 2024

theuni referenced this in commit 157f42b35e on Jan 30, 2024

theuni referenced this in commit 25dc87e6f8 on Jan 30, 2024

fanquake referenced this in commit 5b8c5970bd on Feb 1, 2024

fanquake commented at 4:18 pm on February 1, 2024: member

Could turn this PR into a full removal for 28.x? Otherwise I guess close, and we can followup with that in future?

TheCharlatan commented at 4:23 pm on February 1, 2024: contributor

Seems cleaner to close and do the removal in a separate one. Might be good to open a PR for the full removal soon, so it can get enough review before branch-off.

TheCharlatan closed this on Feb 1, 2024

build: Remove HAVE_CONSENSUS_LIB #29123

Code Coverage

Reviews

Conflicts

Guix builds (on x86_64)