Remove libbitcoinconsensus

fanquake commented at 2:34 pm on March 13, 2024: member

This was deprecated in v27.0, for removal in v28.0. See discussion in PR #29189.

DrahtBot commented at 2:34 pm on March 13, 2024: 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	TheCharlatan, hebasto
Concept ACK	theuni, m3dwards

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:

#29494 (build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes by maflcko)
#29221 (Implement 64 bit arithmetic op codes in the Script interpreter by Christewart)
#29050 (Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes by stevenroose)
#29015 (kernel: Streamline util library by ryanofsky)

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.

fanquake force-pushed on Mar 13, 2024

DrahtBot commented at 3:21 pm on March 13, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/22614510960

DrahtBot added the label CI failed on Mar 13, 2024

DrahtBot removed the label CI failed on Mar 13, 2024

glozow added this to the milestone 28.0 on Mar 14, 2024

hebasto commented at 12:40 pm on March 18, 2024: member

Concept ACK.

To fix MSVC build, suggesting:

0--- a/build_msvc/libbitcoin_consensus/libbitcoin_consensus.vcxproj
1+++ b/build_msvc/libbitcoin_consensus/libbitcoin_consensus.vcxproj
2@@ -15,7 +15,6 @@
3     <ClCompile Include="..\..\src\primitives\block.cpp" />
4     <ClCompile Include="..\..\src\primitives\transaction.cpp" />
5     <ClCompile Include="..\..\src\pubkey.cpp" />
6-    <ClCompile Include="..\..\src\script\bitcoinconsensus.cpp" />
7     <ClCompile Include="..\..\src\script\interpreter.cpp" />
8     <ClCompile Include="..\..\src\script\script.cpp" />
9     <ClCompile Include="..\..\src\script\script_error.cpp" />

fanquake force-pushed on Mar 18, 2024

fanquake marked this as ready for review on Mar 18, 2024

fanquake commented at 2:58 pm on March 18, 2024: member

To fix MSVC build

Taken.

in doc/design/libraries.md:12 in a4b3a7cc41 outdated

11+| *libbitcoin_consensus*   | Stable, backwards-compatible consensus functionality used by *libbitcoin_node* and *libbitcoin_wallet*. |
12+| *libbitcoin_kernel*      | Consensus engine and support library used for validation by *libbitcoin_node*. |
13+| *libbitcoinqt*           | GUI functionality used by *bitcoin-qt* and *bitcoin-gui* executables. |
14 | *libbitcoin_ipc*         | IPC functionality used by *bitcoin-node*, *bitcoin-wallet*, *bitcoin-gui* executables to communicate when [`--enable-multiprocess`](multiprocess.md) is used. |
15 | *libbitcoin_node*        | P2P and RPC server functionality used by *bitcoind* and *bitcoin-qt* executables. |
16 | *libbitcoin_util*        | Home for common functionality shared by different executables and libraries. Similar to *libbitcoin_common*, but lower-level (see [Dependencies](#dependencies)). |

TheCharlatan commented at 4:31 pm on March 18, 2024:

The first point in the Conventions section below should be corrected too, since it refers to the now removed shared-libraries.md. Also need to remove the link to shared-libraries in doc/README.md.

fanquake commented at 4:45 pm on March 18, 2024:

Thanks. Adjusted the docs a bit.

Also need to remove the link to shared-libraries in doc/README.md.

Dropped.

TheCharlatan changes_requested

TheCharlatan commented at 4:39 pm on March 18, 2024: contributor

Just a small comment; seems good to go.

fanquake force-pushed on Mar 18, 2024

in doc/design/libraries.md:19 in 78e1d72ada outdated

15@@ -17,7 +16,7 @@
16 
17 ## Conventions
18 
19-- Most libraries are internal libraries and have APIs which are completely unstable! There are few or no restrictions on backwards compatibility or rules about external dependencies. Exceptions are *libbitcoin_consensus* and *libbitcoin_kernel* which have external interfaces documented at [../shared-libraries.md](../shared-libraries.md).
20+- Most libraries are internal libraries and have APIs which are completely unstable! There are few or no restrictions on backwards compatibility or rules about external dependencies. Exceptions are *libbitcoin_consensus* and *libbitcoin_kernel*, which, at some future point, will have documented external interfaces.

hebasto commented at 4:48 pm on March 18, 2024:

In the future, libbitcoin_kernel will have “documented external interface”, but not libbitcoin_consensus, right?

fanquake force-pushed on Mar 18, 2024

remove libbitcoinconsensus

This was deprecated in v27.0, for removal in v28.0.
See discussion in PR #29189.

80f8b92f4f

in configure.ac:1766 in 56ba9c76d0 outdated

1762@@ -1780,8 +1763,8 @@ else
1763   AC_MSG_RESULT([no])
1764 fi
1765 
1766-if test "$build_bitcoin_wallet$build_bitcoin_cli$build_bitcoin_tx$build_bitcoin_util$build_bitcoin_libs$build_bitcoind$bitcoin_enable_qt$enable_fuzz_binary$use_bench$use_tests" = "nononononononononono"; then
1767-  AC_MSG_ERROR([No targets! Please specify at least one of: --with-utils --with-libs --with-daemon --with-gui --enable-fuzz(-binary) --enable-bench or --enable-tests])
1768+if test "$build_bitcoin_wallet$build_bitcoin_cli$build_bitcoin_tx$build_bitcoin_util$build_bitcoind$bitcoin_enable_qt$enable_fuzz_binary$use_bench$use_tests" = "nononononononononono"; then

hebasto commented at 4:56 pm on March 18, 2024:

56ba9c76d0f8314f0fc1f3bce9774eb7cb6d32c5: There are 9 concatenated variables being tested, but 10 concatenated “no”.

fanquake force-pushed on Mar 18, 2024

TheCharlatan approved

TheCharlatan commented at 5:44 pm on March 19, 2024: contributor

ACK 80f8b92f4f2311b9e9a25361c9dd973244e6f95c

DrahtBot requested review from hebasto on Mar 19, 2024

fanquake requested review from theuni on Mar 20, 2024

theuni commented at 6:23 pm on March 20, 2024: member

Concept ACK and light review ACK 80f8b92f4f2311b9e9a25361c9dd973244e6f95c. My only hesitation here is that (afaics?) there’s now nothing keeping undesired features like threading or globals from working their way into the interpreter in future commits.

Is there anything I’m missing in that regard? Otherwise, any ideas for checks to keep them out via c-i?

m3dwards commented at 7:39 pm on March 20, 2024: contributor

Concept ACK https://github.com/bitcoin/bitcoin/pull/29648/commits/80f8b92f4f2311b9e9a25361c9dd973244e6f95c

I’ve tested this branch on MSVC, Mac and Debian and reviewed the changes.

I still feel a bit too green to this area to give stronger than concept ACK.

hebasto approved

hebasto commented at 11:31 am on March 21, 2024: member

ACK 80f8b92f4f2311b9e9a25361c9dd973244e6f95c, I have reviewed the code and it looks OK.

theuni commented at 7:52 pm on March 28, 2024: member

I didn’t mean for my comment above to be a blocker btw. I think we can just be vigilant in review about dependencies in the interpreter.

fanquake merged this on Apr 1, 2024

fanquake closed this on Apr 1, 2024

fanquake deleted the branch on Apr 1, 2024

fanquake referenced this in commit d83b2ef5f2 on Apr 2, 2024

fanquake commented at 12:28 pm on April 2, 2024: member

Followup in #29787.

fanquake referenced this in commit fd8527a20e on Apr 2, 2024

fanquake referenced this in commit 3b12fc7bcd on Apr 2, 2024

davidgumberg referenced this in commit 46ba1978f6 on Apr 2, 2024

hebasto referenced this in commit a4f007e98d on Apr 3, 2024

hebasto referenced this in commit 3cb80febb8 on Apr 3, 2024

Remove libbitcoinconsensus #29648

Code Coverage

Reviews

Conflicts