build: Remove HAVE_CONSENSUS_LIB #29123
Conversation
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`.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK. I think another way of looking at this is that Honestly I don't really care because it's kinda funky either way. Curious to see if anyone else has a preference. |
It seems worth noting that we do not use the |
|
Concept ACK on moving the
That is very similar to what I suggested in #24994.
For example, to fix the symbol visibility issue. |
|
This library has existed for nearly 10 years with very little known uptake or impact. It has become a maintenance burden. In several cases it dictates our code/library structure (for example necessatating LIBBITCOIN_CRYPTO_BASE), as well as build-system procedures (building multiple copies of object files especially for the lib). Several discussions have arisen wrt migrating it to CMake and it has become difficult to justify adding more complexity for a library that is virtually unused anyway. See for example the discussions: hebasto#41 bitcoin#29123 Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not migrating it to CMake and letting it end with v26. Any remaining use-cases could be handled in the future by libbitcoinkernel.
This library has existed for nearly 10 years with very little known uptake or impact. It has become a maintenance burden. In several cases it dictates our code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as well as build-system procedures (building multiple copies of object files especially for the lib). Several discussions have arisen wrt migrating it to CMake and it has become difficult to justify adding more complexity for a library that is virtually unused anyway. See for example the discussions: hebasto#41 bitcoin#29123 Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not migrating it to CMake and letting it end with v26. Any remaining use-cases could be handled in the future by libbitcoinkernel.
This library has existed for nearly 10 years with very little known uptake or impact. It has become a maintenance burden. In several cases it dictates our code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as well as build-system procedures (building multiple copies of object files especially for the lib). Several discussions have arisen wrt migrating it to CMake and it has become difficult to justify adding more complexity for a library that is virtually unused anyway. See for example the discussions: hebasto#41 bitcoin#29123 Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not migrating it to CMake and letting it end with v27. Any remaining use-cases could be handled in the future by libbitcoinkernel.
|
Maybe draft for now, while we hash out #29189? |
This library has existed for nearly 10 years with very little known uptake or impact. It has become a maintenance burden. In several cases it dictates our code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as well as build-system procedures (building multiple copies of object files especially for the lib). Several discussions have arisen wrt migrating it to CMake and it has become difficult to justify adding more complexity for a library that is virtually unused anyway. See for example the discussions: hebasto#41 bitcoin#29123 Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not migrating it to CMake and letting it end with v27. Any remaining use-cases could be handled in the future by libbitcoinkernel.
This library has existed for nearly 10 years with very little known uptake or impact. It has become a maintenance burden. In several cases it dictates our code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as well as build-system procedures (building multiple copies of object files especially for the lib). Several discussions have arisen wrt migrating it to CMake and it has become difficult to justify adding more complexity for a library that is virtually unused anyway. See for example the discussions: hebasto#41 bitcoin#29123 Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not migrating it to CMake and letting it end with v27. Any remaining use-cases could be handled in the future by libbitcoinkernel.
25dc87e libconsensus: deprecate (Cory Fields) Pull request description: This library has existed for nearly 10 years with very little known uptake or impact. It has become a maintenance burden. In several cases it dictates our code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as well as build-system procedures (building multiple copies of object files especially for the lib). Several discussions have arisen wrt migrating it to CMake and it has become difficult to justify adding more complexity for a library that is virtually unused anyway. See for example the discussions: hebasto#41 #29123 And here: #29180 Where it is pointed out that the libbitcoinconsensus functions are slower than those the internal bitcoind equivalents due to the missing sha2 implementations. Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not migrating it to CMake and letting it end with v27. Any remaining use-cases could be handled in the future by libbitcoinkernel. If there are any users currently using libbitcoinconsensus, please chime in with your use-case! Edit: Corrected final release to be v27. ACKs for top commit: TheCharlatan: ACK 25dc87e fanquake: ACK 25dc87e - this library has very little, if any impactful real world usage. It has been entirely broken (on various platforms) for long periods of its existence, where nobody even noticed. Pruning this out to save porting, and starting anew with the kernel, is the sane thing to do. Tree-SHA512: baff2b3c4f76f520c96021035f751fdcb51bedf00e767660249e92a7bc7c5c176786bcf2c4cfe2d2351c200f932b39eb886bcfb22fbec824a41617590d6a1638
|
Could turn this PR into a full removal for 28.x? Otherwise I guess close, and we can followup with that in future? |
|
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. |
This library has existed for nearly 10 years with very little known uptake or impact. It has become a maintenance burden. In several cases it dictates our code/library structure (for example necessitating LIBBITCOIN_CRYPTO_BASE), as well as build-system procedures (building multiple copies of object files especially for the lib). Several discussions have arisen wrt migrating it to CMake and it has become difficult to justify adding more complexity for a library that is virtually unused anyway. See for example the discussions: hebasto/bitcoin#41 bitcoin/bitcoin#29123 Instead, we (fanquake, hebasto, TheCharlatan, and I) propose simply not migrating it to CMake and letting it end with v27. Any remaining use-cases could be handled in the future by libbitcoinkernel.
The
script/bitcoinconsensusmodule defines the public interface for thebitcoinconsensuslibrary. Even though this module is only required by the tests and thebitcoinconsensuslibrary, it is currently compiled into the static internallibbitcoin_consensuslibrary, 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_LIBdefine used by the tests is essentiallymeaningless, since the module is always available. This can be verified on master by removing the
HAVE_CONSENSUS_LIBchecks in the tests, configuring with--with-libs=no, and runningmake check.Improve all of this by including the
bitcoinconsensusmodule only where it is required and removing theHAVE_CONSENSUS_LIBdefine.An alternative to this patch was discussed in hebasto#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.