Remove libbitcoinconsensus #29648

fanquake · 2024-03-13T14:34:12Z

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

DrahtBot · 2024-03-13T14:34:16Z

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.

DrahtBot · 2024-03-13T15:21:15Z

🚧 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}

hebasto

Concept ACK.

To fix MSVC build, suggesting:

--- a/build_msvc/libbitcoin_consensus/libbitcoin_consensus.vcxproj
+++ b/build_msvc/libbitcoin_consensus/libbitcoin_consensus.vcxproj
@@ -15,7 +15,6 @@
     <ClCompile Include="..\..\src\primitives\block.cpp" />
     <ClCompile Include="..\..\src\primitives\transaction.cpp" />
     <ClCompile Include="..\..\src\pubkey.cpp" />
-    <ClCompile Include="..\..\src\script\bitcoinconsensus.cpp" />
     <ClCompile Include="..\..\src\script\interpreter.cpp" />
     <ClCompile Include="..\..\src\script\script.cpp" />
     <ClCompile Include="..\..\src\script\script_error.cpp" />

fanquake · 2024-03-18T14:58:51Z

To fix MSVC build

Taken.

TheCharlatan

Just a small comment; seems good to go.

doc/design/libraries.md

configure.ac

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

TheCharlatan

ACK 80f8b92

theuni · 2024-03-20T18:23:01Z

Concept ACK and light review ACK 80f8b92. 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 · 2024-03-20T19:39:02Z

Concept ACK 80f8b92

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

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

theuni · 2024-03-28T19:52:36Z

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.

3cb80fe guix: Remove another leftover from #29648 (Hennadii Stepanov) Pull request description: It was overlooked in #29787. ACKs for top commit: TheCharlatan: ACK 3cb80fe Tree-SHA512: c4eae65ffa0a79f4d57ba07730effee6aeff9d9625bc00a4534ffe46d3a16ae56bc8753e3fec93d7ff81ea7be39662282c631861a21ea8a9dc5d31b79acb231d

We no longer build a lib, so a non-existent dir is causing builds to fail.

bbcba09 build: remove check for __declspec(dllexport) (fanquake) 37c9abd build: remove check for __attribute__((visibility.. (fanquake) Pull request description: These are unused (since libbitcoinconsensus / #29648), and the current CMake port doesn't quite match behaviour, such that there's no real point in doing the check. So rather than port anything, just remove it. If these are needed again in future (i.e for kernel or similar), they can be revisted, and it might be the case that build-system level checks will not be wanted. ACKs for top commit: hebasto: ACK bbcba09. I've verified that neither `HAVE_DEFAULT_VISIBILITY_ATTRIBUTE` nor `HAVE_DLLEXPORT_ATTRIBUTE` are used or evaluated in the current codebase. TheCharlatan: ACK bbcba09 willcl-ark: ACK bbcba09 Tree-SHA512: 332f018c50a159d2cbfd2f9ce018538fa11cf06a94e27ed42146945b86645af5881095df39cadd2f775a8ae348ebfc949d54f7eb4b62264bf48119c9f9952c20

This has been unused since bitcoin#29648. Noticed while running a newer version of clang-tidy (19.1.1): ```bash [127/391][6.2s] /opt/homebrew/opt/llvm/bin/clang-tidy -p=build -quiet --config-file=/bitcoin/src/.clang-tidy /bitcoin/src/test/script_tests.cpp bitcoin/src/test/script_tests.cpp:126:25: error: local copy 'tx2' of the variable 'tx' is never modified and never used; consider removing the statement [performance-unnecessary-copy-initialization,-warnings-as-errors] 126 | CMutableTransaction tx2 = tx; | ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~ 127 | BOOST_CHECK_MESSAGE(VerifyScript(scriptSig, scriptPubKey, &scriptWitness, flags, MutableTransactionSignatureChecker(&tx, 0, txCredit.vout[0].nValue, MissingDataBehavior::ASSERT_FAIL), &err) == expect, message); 512 warnings generated. ``` I would have expected this to show up under some other `-Wunused*` type warning during compilation.

This has been unused since bitcoin#29648. Noticed while running a newer version of clang-tidy (19.1.1): ```bash [127/391][6.2s] /opt/homebrew/opt/llvm/bin/clang-tidy -p=build -quiet --config-file=/bitcoin/src/.clang-tidy /bitcoin/src/test/script_tests.cpp bitcoin/src/test/script_tests.cpp:126:25: error: local copy 'tx2' of the variable 'tx' is never modified and never used; consider removing the statement [performance-unnecessary-copy-initialization,-warnings-as-errors] 126 | CMutableTransaction tx2 = tx; | ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~ 127 | BOOST_CHECK_MESSAGE(VerifyScript(scriptSig, scriptPubKey, &scriptWitness, flags, MutableTransactionSignatureChecker(&tx, 0, txCredit.vout[0].nValue, MissingDataBehavior::ASSERT_FAIL), &err) == expect, message); 512 warnings generated. ```

e0287bc test: remove unused code from script_tests (fanquake) Pull request description: This has been unused since #29648. Noticed while running a newer version of clang-tidy (19.1.1): ```bash [127/391][6.2s] /opt/homebrew/opt/llvm/bin/clang-tidy -p=build -quiet --config-file=/bitcoin/src/.clang-tidy /bitcoin/src/test/script_tests.cpp bitcoin/src/test/script_tests.cpp:126:25: error: local copy 'tx2' of the variable 'tx' is never modified and never used; consider removing the statement [performance-unnecessary-copy-initialization,-warnings-as-errors] 126 | CMutableTransaction tx2 = tx; | ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~ 127 | BOOST_CHECK_MESSAGE(VerifyScript(scriptSig, scriptPubKey, &scriptWitness, flags, MutableTransactionSignatureChecker(&tx, 0, txCredit.vout[0].nValue, MissingDataBehavior::ASSERT_FAIL), &err) == expect, message); 512 warnings generated. ``` ACKs for top commit: maflcko: review ACK e0287bc BrandonOdiwuor: ACK e0287bc Tree-SHA512: f69513d0b898e0e9afad047bcec200707b057e3718a3d35bd479a788a1973e49ee7e5f48feadb8731ab5fdbd12a2b53b0bcf65296701e2296c3fdb67cdcabfb5

This has been unused since bitcoin#29648. Noticed while running a newer version of clang-tidy (19.1.1): ```bash [127/391][6.2s] /opt/homebrew/opt/llvm/bin/clang-tidy -p=build -quiet --config-file=/bitcoin/src/.clang-tidy /bitcoin/src/test/script_tests.cpp bitcoin/src/test/script_tests.cpp:126:25: error: local copy 'tx2' of the variable 'tx' is never modified and never used; consider removing the statement [performance-unnecessary-copy-initialization,-warnings-as-errors] 126 | CMutableTransaction tx2 = tx; | ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~ 127 | BOOST_CHECK_MESSAGE(VerifyScript(scriptSig, scriptPubKey, &scriptWitness, flags, MutableTransactionSignatureChecker(&tx, 0, txCredit.vout[0].nValue, MissingDataBehavior::ASSERT_FAIL), &err) == expect, message); 512 warnings generated. ```

3cb80fe guix: Remove another leftover from bitcoin#29648 (Hennadii Stepanov) Pull request description: It was overlooked in bitcoin#29787. ACKs for top commit: TheCharlatan: ACK 3cb80fe Tree-SHA512: c4eae65ffa0a79f4d57ba07730effee6aeff9d9625bc00a4534ffe46d3a16ae56bc8753e3fec93d7ff81ea7be39662282c631861a21ea8a9dc5d31b79acb231d

fd8527a guix: remove errant leftover from bitcoin#29648 (fanquake) Pull request description: We no longer build a lib, so a non-existent dir is causing builds to fail. ACKs for top commit: josibake: ACK bitcoin@fd8527a hebasto: ACK fd8527a. TheCharlatan: ACK fd8527a Tree-SHA512: 9175a0de3f95f56939b3eaa3e89dca2cfae4996bcd84ef6b8e2872672bef39cb0550c9f4a79475d887eb8fac92c15dfa8c352648ff167d54a0b736978412226c

This reverts commit 3cb80fe (bitcoin#29797)

This reverts commit fd8527a (bitcoin#29787)

This reverts commit 80f8b92 (bitcoin#29648)

This reverts commit 3cb80fe (bitcoin#29797)

This reverts commit fd8527a (bitcoin#29787)

This reverts commit 80f8b92 (bitcoin#29648)

This reverts commit 3cb80fe (bitcoin#29797)

This reverts commit fd8527a (bitcoin#29787)

This reverts commit 80f8b92 (bitcoin#29648)

This reverts commit 3cb80fe (bitcoin#29797)

This reverts commit fd8527a (bitcoin#29787)

This reverts commit 80f8b92 (bitcoin#29648)

This reverts commit 3cb80fe (bitcoin#29797)

This reverts commit fd8527a (bitcoin#29787)

This reverts commit 80f8b92 (bitcoin#29648)

fanquake force-pushed the remove_libbitcoin_consensus branch from 891a373 to 87b9494 Compare March 13, 2024 15:21

DrahtBot added the CI failed label Mar 13, 2024

This was referenced Mar 13, 2024

build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes #29494

Merged

Implement 64 bit arithmetic op codes in the Script interpreter #29221

Closed

Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes #29050

Closed

DrahtBot removed the CI failed label Mar 13, 2024

glozow added this to the 28.0 milestone Mar 14, 2024

hebasto reviewed Mar 18, 2024

View reviewed changes

fanquake force-pushed the remove_libbitcoin_consensus branch from 87b9494 to a4b3a7c Compare March 18, 2024 13:48

fanquake marked this pull request as ready for review March 18, 2024 14:58

TheCharlatan suggested changes Mar 18, 2024

View reviewed changes

doc/design/libraries.md Show resolved Hide resolved

fanquake force-pushed the remove_libbitcoin_consensus branch from a4b3a7c to 78e1d72 Compare March 18, 2024 16:44

hebasto reviewed Mar 18, 2024

View reviewed changes

doc/design/libraries.md Outdated Show resolved Hide resolved

fanquake force-pushed the remove_libbitcoin_consensus branch from 78e1d72 to 56ba9c7 Compare March 18, 2024 16:50

hebasto reviewed Mar 18, 2024

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

remove libbitcoinconsensus

80f8b92

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

fanquake force-pushed the remove_libbitcoin_consensus branch from 56ba9c7 to 80f8b92 Compare March 18, 2024 17:01

TheCharlatan approved these changes Mar 19, 2024

View reviewed changes

DrahtBot requested a review from hebasto March 19, 2024 17:44

fanquake requested a review from theuni March 20, 2024 14:47

hebasto approved these changes Mar 21, 2024

View reviewed changes

DrahtBot mentioned this pull request Mar 28, 2024

kernel: Streamline util library #29015

Merged

fanquake merged commit 948ecf1 into bitcoin:master Apr 1, 2024

fanquake deleted the remove_libbitcoin_consensus branch April 1, 2024 15:54

ajtowns mentioned this pull request May 6, 2024

Backports for 27.x bitcoin-inquisition/bitcoin#52

Merged

ajtowns pushed a commit to ajtowns/bitcoin that referenced this pull request May 17, 2024

guix: remove errant leftover from bitcoin#29648

7f36abd

We no longer build a lib, so a non-existent dir is causing builds to fail.

fanquake mentioned this pull request Aug 5, 2024

build: Remove unused visibility checks #30590

Merged

stickies-v mentioned this pull request Oct 1, 2024

[28.x] backports and finalize #30959

Merged

fanquake mentioned this pull request Oct 7, 2024

test: remove unused code from script_tests #31051

Merged

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 22, 2025

Revert "guix: Remove another leftover from bitcoin#29648"

49a8015

This reverts commit 3cb80fe (bitcoin#29797)

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 22, 2025

Revert "guix: remove errant leftover from bitcoin#29648"

d8a79a2

This reverts commit fd8527a (bitcoin#29787)

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 22, 2025

Revert "remove libbitcoinconsensus"

b63443c

This reverts commit 80f8b92 (bitcoin#29648)

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 24, 2025

Revert "guix: Remove another leftover from bitcoin#29648"

56abdb4

This reverts commit 3cb80fe (bitcoin#29797)

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 24, 2025

Revert "guix: remove errant leftover from bitcoin#29648"

596bbf0

This reverts commit fd8527a (bitcoin#29787)

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 24, 2025

Revert "remove libbitcoinconsensus"

2224a5b

This reverts commit 80f8b92 (bitcoin#29648)

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 26, 2025

Revert "guix: Remove another leftover from bitcoin#29648"

a3f9e06

This reverts commit 3cb80fe (bitcoin#29797)

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 26, 2025

Revert "guix: remove errant leftover from bitcoin#29648"

fdcbdb9

This reverts commit fd8527a (bitcoin#29787)

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 26, 2025

Revert "remove libbitcoinconsensus"

7ad32d3

This reverts commit 80f8b92 (bitcoin#29648)

xyephy mentioned this pull request Feb 26, 2025

Upgrade to bitcoin v28 whiveio/whive#12

Open

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 28, 2025

Revert "guix: Remove another leftover from bitcoin#29648"

a475bdc

This reverts commit 3cb80fe (bitcoin#29797)

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 28, 2025

Revert "guix: remove errant leftover from bitcoin#29648"

0ddd133

This reverts commit fd8527a (bitcoin#29787)

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 28, 2025

Revert "remove libbitcoinconsensus"

81fdcb1

This reverts commit 80f8b92 (bitcoin#29648)

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2025

Revert "guix: Remove another leftover from bitcoin#29648"

6ff758d

This reverts commit 3cb80fe (bitcoin#29797)

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2025

Revert "guix: remove errant leftover from bitcoin#29648"

3e63c46

This reverts commit fd8527a (bitcoin#29787)

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2025

Revert "remove libbitcoinconsensus"

c317b89

This reverts commit 80f8b92 (bitcoin#29648)

bitcoin locked and limited conversation to collaborators Apr 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove libbitcoinconsensus #29648

Remove libbitcoinconsensus #29648

Uh oh!

fanquake commented Mar 13, 2024

Uh oh!

DrahtBot commented Mar 13, 2024 •

edited

Loading

Uh oh!

DrahtBot commented Mar 13, 2024

Uh oh!

hebasto left a comment

Uh oh!

fanquake commented Mar 18, 2024

Uh oh!

TheCharlatan left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheCharlatan left a comment

Uh oh!

theuni commented Mar 20, 2024 •

edited

Loading

Uh oh!

m3dwards commented Mar 20, 2024

Uh oh!

hebasto left a comment

Uh oh!

theuni commented Mar 28, 2024

Uh oh!

Uh oh!

Remove libbitcoinconsensus #29648

Remove libbitcoinconsensus #29648

Uh oh!

Conversation

fanquake commented Mar 13, 2024

Uh oh!

DrahtBot commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

DrahtBot commented Mar 13, 2024

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Mar 18, 2024

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

theuni commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m3dwards commented Mar 20, 2024

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

theuni commented Mar 28, 2024

Uh oh!

Uh oh!

DrahtBot commented Mar 13, 2024 •

edited

Loading

theuni commented Mar 20, 2024 •

edited

Loading