Remove libbitcoinconsensus #29648
Merged
Remove libbitcoinconsensus #29648
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
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. |
fanquake
force-pushed
the
remove_libbitcoin_consensus
branch
from
891a373 to
87b9494
Compare
Contributor
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
This was referenced Mar 13, 2024
hebasto
reviewed
Mar 18, 2024
Member
hebasto
left a comment
hebasto
left a comment
There was a problem hiding this comment.
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
force-pushed
the
remove_libbitcoin_consensus
branch
from
87b9494 to
a4b3a7c
Compare
Member
Author
Taken. |
sedited
requested changes
Mar 18, 2024
Contributor
sedited
left a comment
sedited
left a comment
There was a problem hiding this comment.
Just a small comment; seems good to go.
fanquake
force-pushed
the
remove_libbitcoin_consensus
branch
from
a4b3a7c to
78e1d72
Compare
hebasto
reviewed
Mar 18, 2024
fanquake
force-pushed
the
remove_libbitcoin_consensus
branch
from
78e1d72 to
56ba9c7
Compare
hebasto
reviewed
Mar 18, 2024
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
sedited
approved these changes
Mar 19, 2024
Member
|
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? |
Contributor
|
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. |
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
added a commit
that referenced
this pull request
Apr 4, 2024
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
ajtowns
pushed a commit
to ajtowns/bitcoin
that referenced
this pull request
May 17, 2024
We no longer build a lib, so a non-existent dir is causing builds to fail.
fanquake
added a commit
that referenced
this pull request
Aug 6, 2024
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
fanquake
added a commit
to fanquake/bitcoin
that referenced
this pull request
Oct 7, 2024
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. ```
fanquake
added a commit
that referenced
this pull request
Oct 8, 2024
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
luke-jr
added a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Feb 28, 2025
This reverts commit 3cb80fe (bitcoin#29797)
luke-jr
added a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Feb 28, 2025
This reverts commit fd8527a (bitcoin#29787)
luke-jr
added a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Feb 28, 2025
This reverts commit 80f8b92 (bitcoin#29648)
luke-jr
added a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Mar 5, 2025
This reverts commit 3cb80fe (bitcoin#29797)
luke-jr
added a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Mar 5, 2025
This reverts commit fd8527a (bitcoin#29787)
luke-jr
added a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Mar 5, 2025
This reverts commit 80f8b92 (bitcoin#29648)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was deprecated in
v27.0, for removal inv28.0. See discussion in PR #29189.