Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove libbitcoinconsensus #29648

Merged
merged 1 commit into from Apr 1, 2024

Conversation

fanquake
Copy link
Member

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 13, 2024

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 fanquake force-pushed the remove_libbitcoin_consensus branch from 891a373 to 87b9494 Compare March 13, 2024 15:21
@DrahtBot
Copy link
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

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 fanquake force-pushed the remove_libbitcoin_consensus branch from 87b9494 to a4b3a7c Compare March 18, 2024 13:48
@fanquake fanquake marked this pull request as ready for review March 18, 2024 14:58
@fanquake
Copy link
Member Author

To fix MSVC build

Taken.

Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small comment; seems good to go.

doc/design/libraries.md Show resolved Hide resolved
@fanquake fanquake force-pushed the remove_libbitcoin_consensus branch from a4b3a7c to 78e1d72 Compare March 18, 2024 16:44
doc/design/libraries.md Outdated Show resolved Hide resolved
@fanquake fanquake force-pushed the remove_libbitcoin_consensus branch from 78e1d72 to 56ba9c7 Compare March 18, 2024 16:50
configure.ac Outdated Show resolved Hide resolved
This was deprecated in v27.0, for removal in v28.0.
See discussion in PR bitcoin#29189.
@fanquake fanquake force-pushed the remove_libbitcoin_consensus branch from 56ba9c7 to 80f8b92 Compare March 18, 2024 17:01
Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 80f8b92

@DrahtBot DrahtBot requested a review from hebasto March 19, 2024 17:44
@fanquake fanquake requested a review from theuni March 20, 2024 14:47
@theuni
Copy link
Member

theuni commented Mar 20, 2024

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
Copy link
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.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@theuni
Copy link
Member

theuni commented Mar 28, 2024

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 fanquake merged commit 948ecf1 into bitcoin:master Apr 1, 2024
16 checks passed
@fanquake fanquake deleted the remove_libbitcoin_consensus branch April 1, 2024 15:54
fanquake added a commit to fanquake/bitcoin that referenced this pull request Apr 2, 2024
We no longer build a lib, so a non-existent dir is causing builds to
fail.
@fanquake
Copy link
Member Author

fanquake commented Apr 2, 2024

Followup in #29787.

fanquake added a commit to fanquake/bitcoin that referenced this pull request Apr 2, 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 Apr 2, 2024
fd8527a guix: remove errant leftover from #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 fd8527a
  hebasto:
    ACK fd8527a.
  TheCharlatan:
    ACK fd8527a

Tree-SHA512: 9175a0de3f95f56939b3eaa3e89dca2cfae4996bcd84ef6b8e2872672bef39cb0550c9f4a79475d887eb8fac92c15dfa8c352648ff167d54a0b736978412226c
davidgumberg pushed a commit to davidgumberg/bitcoin that referenced this pull request Apr 2, 2024
We no longer build a lib, so a non-existent dir is causing builds to
fail.
hebasto added a commit to hebasto/bitcoin that referenced this pull request Apr 3, 2024
hebasto added a commit to hebasto/bitcoin that referenced this pull request Apr 3, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants