-
Notifications
You must be signed in to change notification settings - Fork 0
Merge bitcoin/bitcoin#29189: RFC: Deprecate libconsensus #1082
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
base: backport-auto-20250821
Are you sure you want to change the base?
Merge bitcoin/bitcoin#29189: RFC: Deprecate libconsensus #1082
Conversation
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 bitcoin#29123 And here: bitcoin#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
WalkthroughThe PR introduces documentation updates announcing the deprecation of libdashconsensus library. A new release notes file describes the deprecation rationale, removal target version (v28), and notes that libdashkernel will provide future functionality. An existing documentation file is updated with a deprecation notice. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (2)doc/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-07-28T20:34:29.061ZApplied to files:
📚 Learning: 2025-07-31T01:14:55.631ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Backport Verification SuccessfulNo issues found! This backport is complete and faithful to the original Bitcoin change. Original Bitcoin commit: Verification Details:
Reviewer Feedback Analysis:PastaPastaPasta comments reviewed: 0 All human comments addressed: No reviewer feedback requiring attention. Technical Analysis:This PR correctly implements the deprecation of libdashconsensus (Bitcoin PR bitcoin#29189), not the removal (which was a separate Bitcoin PR bitcoin#29648). The backport includes:
Summary:This is a textbook example of faithful Bitcoin backporting - preserving the original intent while making appropriate Dash-specific adaptations. This PR is ready for merge. ✅ |
knst
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NACK: it's too early. It's not v28 yet and we haven't created libdashkernel (mentioned in release notes) yet
- Complete sentence fragment in release notes (added 'this is' before 'its final consensus update') - Properly adapts Bitcoin's Taproot reference removal for Dash
Verification Agent - Fixes AppliedSummaryApplied a minor grammatical fix to the release notes. The backport is technically correct and faithful to Bitcoin PR bitcoin#29189. Changes Made
Verification Results✅ Bitcoin Commit: Notes
Commit: 47b3e0c |
✅ Verification CompleteStatus: APPROVED (Technical Implementation) SummaryThis PR correctly backports Bitcoin Core PR bitcoin#29189, which adds a deprecation notice for libdashconsensus. The implementation is faithful to the original Bitcoin changes with appropriate Dash-specific adaptations. CI Status
Backport AnalysisBitcoin Commit: Changes:
Dash Adaptations:
Validation Fix ReviewA previous verification run applied a grammatical fix:
Size AnalysisThe comparison tool initially flagged size discrepancy because it compared against the wrong Bitcoin commit ( Human Review Note@knst raised a valid policy concern about timing - whether this deprecation should be announced before Dash v28 and before From a pure technical correctness standpoint, this backport is implemented correctly and passes all automated checks. Automated verification by Claude Code |
Backports bitcoin#29189
Original commit: 5b8c597
Backported from Bitcoin Core v0.27
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.