-
Notifications
You must be signed in to change notification settings - Fork 0
Merge bitcoin/bitcoin#27724: build: disable boost multi index safe mode in debug mode #805
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-0.26-batch-602
Are you sure you want to change the base?
Merge bitcoin/bitcoin#27724: build: disable boost multi index safe mode in debug mode #805
Conversation
…bug mode 59c8944 build: disable boost multi index safe mode (willcl-ark) Pull request description: Fixes bitcoin#27586 Disable boost multi index safe mode by default when configuring with --enable-debug. This option can cause transactions to take a long time to be accepted into the mempool under certain conditions; iterator destruction takes O(n) time vs O(1) as they are stored in a singly linked list. See 27586 and the [boost docs](https://www.boost.org/doc/libs/1_58_0/boost/multi_index/detail/safe_mode.hpp) for more information. Re-enable it on the CI builds which previously had it enabled. Re-enable it on the msan fuzz task so that we have fuzz tasks testing with it enabled and disabled in this repo. ACKs for top commit: hebasto: ~ACK 59c8944~ fanquake: ACK 59c8944 Tree-SHA512: ed654f63dbebdd02e4414d1f81147d92a4d490dbb5a2e0376858e3129097645f3a2df45191d6b40c410a76e803b0d28796d1a01c1d2fd995b94e8b7eb3949027
WalkthroughThe change relocates the Boost Multi-Index safe mode flag from conditional logic in the main build configuration to specific CI test environment setup scripts. The flag Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 📥 CommitsReviewing files that changed from the base of the PR and between 9ab5da6 and 1378765dfd59e92fa6e8de4f4bbcf26d17a8200b. 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📓 Common learnings⏰ 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). (12)
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:
Technical Analysis:Bitcoin changes: Modified 4 CI configuration files to re-enable boost multi-index safe mode for debug builds
Reviewer Feedback Analysis:PastaPastaPasta comments reviewed: 0 Size ratio (46.2%): Appropriate due to architectural differences between Bitcoin and Dash CI configurations. This PR is ready for merge. ✅ |
- Add BOOST_MULTI_INDEX_ENABLE_SAFE_MODE to native_fuzz CI config - Add BOOST_MULTI_INDEX_ENABLE_SAFE_MODE to native_fuzz_with_valgrind CI config - Add BOOST_MULTI_INDEX_ENABLE_SAFE_MODE to native_multiprocess CI config These changes ensure that all CI jobs using --enable-debug or --enable-fuzz explicitly enable boost multi index safe mode, matching Bitcoin's intent in PR bitcoin#27724 which removed automatic enabling in configure.ac and re-enabled it in specific CI jobs.
Verification - Fixes AppliedAnalysisBitcoin PR bitcoin#27724 makes changes to 4 files:
Fixes AppliedWhile the two missing files don't exist in Dash, the principle of the change is clear: any CI job using Applied fixes to Dash's equivalent CI jobs:
All three files use either CI Status
Next StepsThe fixes have been committed and pushed. A future verification run will validate these changes after CI completes. |
Re-Verification: Additional Fix AppliedI've reviewed the previously applied validation fixes (commit 9ab5da6) and found they were correct: ✅ Correct fixes from 9ab5da6:
However, I found an issue in the original merge commit (853821b): ❌ Issue found in merge commit: Fix applied in commit 1378765dfd:
This makes the backport faithful to Bitcoin's intent. CI Status:
The PR now correctly implements Bitcoin PR bitcoin#27724. |
The merge commit incorrectly added BOOST_MULTI_INDEX_ENABLE_SAFE_MODE to native_qt5.sh. This is not faithful to the Bitcoin backport because: - Bitcoin's native_qt5.sh has --enable-debug, so it previously had safe mode enabled - Dash's native_qt5.sh does NOT have --enable-debug, so it never had safe mode enabled - The Bitcoin PR only re-enabled safe mode on builds that previously had it The previous validation fixes (9ab5da6) correctly added safe mode to the 3 Dash CI files that use --enable-debug or --enable-fuzz. This fix removes the incorrect addition to native_qt5, making the backport faithful to Bitcoin's intent.
1378765 to
65aa83d
Compare
Re-Verification Complete ✅Changes AppliedFixed the issues from the previous validation attempt:
Final StateThe backport now correctly:
Bitcoin Backport Comparison
This backport is now correct and ready for merge. |
Backports bitcoin#27724
Original commit: fb4f047
Backported from Bitcoin Core v0.26
This change disables boost multi index safe mode by default when configuring with --enable-debug to improve performance. The safe mode can cause transactions to take a long time to be accepted into the mempool under certain conditions due to iterator destruction taking O(n) time vs O(1).
The change:
Note: Two CI test files modified in Bitcoin (00_setup_env_i686_multiprocess.sh and 00_setup_env_native_fuzz_with_msan.sh) don't exist in Dash, so those changes were skipped.
Summary by CodeRabbit
Note: These are internal build and testing infrastructure changes with no impact on end-user functionality.
✏️ Tip: You can customize this high-level summary in your review settings.