Replace boost signals with minimal compatible implementation#34495
Replace boost signals with minimal compatible implementation#34495theuni wants to merge 13 commits intobitcoin:masterfrom
Conversation
This is undocumented and unspecified Boost behavior that happens to work as intended for now, but could break at any point in the future. See the boost discussion here: https://groups.google.com/g/boost-list/c/So4i8JXneJ0 It also complicates a potential replacement of Boost::signals2.
…uilds The current code forward-declares boost::signals2, which avoids the need for these includes. An upcoming commit will (temporarily) include boost headers directly instead. A follow-up commit will then replace boost with an internal signals implementation, which will allow this commit to be reverted.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste 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. LLM Linter (✨ experimental)Possible typos and grammar issues:
2026-03-03 20:06:50 |
0b5c40f to
7f7df41
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
Concept ACK |
|
https://github.com/bitcoin/bitcoin/actions/runs/21650096364/job/62412240316?pr=34495#step:11:581: [80](https://github.com/bitcoin/bitcoin/actions/runs/21650096364/job/62412240316?pr=34495#step:11:581)
D:\a\bitcoin\bitcoin\src\node\interface_ui.cpp(28,18): fatal error C1001: Internal compiler error. [D:\a\bitcoin\bitcoin\build\src\bitcoin_node.vcxproj]
bitcoin_wallet.vcxproj -> D:\a\bitcoin\bitcoin\build\lib\Release\bitcoin_wallet.lib
(compiler file 'msc1.cpp', line 1589)
To work around this problem, try simplifying or changing the program near the locations listed above.
If possible please provide a repro here: https://developercommunity.visualstudio.com/
Please choose the Technical Support command on the Visual C++
Help menu, or open the Technical Support help file for more information
INTERNAL COMPILER ERROR in 'C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\bin\HostX64\x64\CL.exe' |
|
This also prunes half a megabyte off the size of a Guix built (stripped) /bitcoin/guix-build-1e64aeaaecc2/output/x86_64-linux-gnu/bitcoin-1e64aeaaecc2/bin# ls -lah *
1.9M bitcoin
2.5M bitcoin-cli
39.3M bitcoin-qt
4.6M bitcoin-tx
4.2M bitcoin-util
8.8M bitcoin-wallet
15.4M bitcoindThis PR (7f7df41 rebased on master): /bitcoin/guix-build-75e0b3349c5d/output/x86_64-linux-gnu/bitcoin-75e0b3349c5d/bin# ls -lah *
1.9M bitcoin
2.5M bitcoin-cli
38.8M bitcoin-qt
4.6M bitcoin-tx
4.2M bitcoin-util
8.6M bitcoin-wallet
14.9M bitcoind |
|
Concept ACK |
@hebasto Any experience dealing with MSVC ICE's? This is frustrating considering how much simpler it is than boost. |
Can we replace Windows with a minimal compatible implementation as well? |
Re: MSVC Internal Compiler ErrorsI'm able to build 237005a "signals: add signals tests" fine, but the following commit, 91b1ff9 "signals: Add a simplified boost-compatible implementation" has a lot of failures. Some can be fixed by adding #include <boost/signals2/signal.hpp>to /src/wallet/scriptpubkeyman.h. But after fixing that, ICE's show up together with more easily actionable issues: Would focus on fixing the non-ICE issues in that commit. I think there has been some work with failing faster, hopefully you still see the other errors on CI. |
|
@hodlinator At 91b1ff9, boost is no longer used, so including that header won't help anything. I'm not sure how it's even picking up But don't poke too hard just yet. I realized last night that there's a behavioral break from boost in this branch, in that callbacks are not allowed to modify their I've updated the approach to match boost's model on this branch: https://github.com/theuni/bitcoin/tree/replace-signals-recursive . I'm just waiting to see how the c-i looks before force-pushing it here. Apologies to anyone who's reviewed so far, hopefully nobody has looked into it too deeply. I've added some extra tests to demonstrate what would've deadlocked with the previous approach. |
7f7df41 to
f698c1c
Compare
Pushed an updated impl of btcsignals which matches boost's callback threading model. Sorry for the noise. |
f698c1c to
956ea69
Compare
I think they try to fix them within the next two releases, if reported. Minimizing will likely help, but I guess creduce doesn't work on Windows, so someone would have to do this manually. Maybe an LLM could be taught to behave like creduce? (I tried that once, but gave up rather quickly) |
|
Likewise, sorry for the noise, I don't know what happened to my local Windows repo. |
|
Managed to boil down the ICE, would someone else please do the honors of reporting to MS? C++-file: #include <mutex>
struct MutexChild : public std::mutex {
// No ICE: ~MutexChild() = default;
~MutexChild() {}
};
struct ice_signal {
MutexChild m_mutex{};
};
ice_signal s;Command line: "C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.44.35207\bin\HostX64\x64\CL.exe" /std:c++20 path_to_file.cppNote that dropping |
Excellent reproducer. I'll report it |
src/btcsignals.h
Outdated
| return true; | ||
| } | ||
|
|
||
| mutable Mutex m_mutex{}; |
There was a problem hiding this comment.
I know it's blasphemous but switching to RecursiveMutex/AnnotatedMixin<std::recursive_mutex> avoids the ICE on native Windows for some reason:
| mutable Mutex m_mutex{}; | |
| mutable RecursiveMutex m_mutex{}; |
Found while figuring out the ICE reproducer.
There was a problem hiding this comment.
Maybe guard the RecursiveMutex behind #if defined(_MSC_VER)?
a50d0b6 build: don't pass on boost dependency to kernel consumers (Cory Fields) Pull request description: This is unnecessary now that the kernel now exports a (boost-less) API. Noticed while slimming down boost dependencies in #34495. ACKs for top commit: stickies-v: ACK a50d0b6 hebasto: ACK a50d0b6, I have reviewed the code and it looks OK. I tested it by applying the Boost-specifc commits from #34143 and building with depends. Tree-SHA512: e2d12356f41dd51dd729362121a33bd4f395821d53dd9a0bb0d5d6a53aba2ca2064e0709d9799dc6751b3d61ea576d2efc0e28296fdba26f2809dbcb0feabe44
Let's switch to clang-cl. The CI log is available here: https://github.com/hebasto/bitcoin/actions/runs/21716329966/job/62633207718. |
This eases the transition to a replacement signals implementation
The next commit will add a real implementation in this namespace.
956ea69 to
5e088b9
Compare
|
In latest push:
|
|
Upstream change has landed: boostorg/signals2#85. |
sedited
left a comment
There was a problem hiding this comment.
Approach ACK
This looks very promising, will look at this in more detail later. Can you use UpperCamelCase for the few new function and class names outside of the btcsignals namespace and run clang-format-diff over it?
These tests are compatible with boost::signals2 as well as the replacement implementation that will be introduced in the next commit. This is intended to demonstrate some equivalency between the implementations.
This re-implements the tiny portion of boost::signals2 that we currently use. It is enough to be useful as a generic multicast callback mechanism.
These were necessary to work around unnecessary constraints that have been fixed in the (upcoming) boost::signals2 version 1.91. Our implementation's constraints match those of that version.
…n-node builds" This reverts commit 3df1a14. This was only necessary as an interim workaround.
The real includes were only needed temporarily while supporting btcsignals as an alias for boost::signals2.
In both of these cases, boost was only needed for signals and not for multi_index/test.
The documented example is no longer relevant, so remove it rather than updating it to mention btcsignals.
5e088b9 to
90b2ddf
Compare
|
In latest push:
|
This drops our dependency on
boost::signals2, leavingboost::multi_indexas the only remaining boost dependency for bitcoind.boost::signals2is a complex beast, but we only use a small portion of it. Namely: it's a way for multiple subscribers to connect to the same event, and the ability to later disconnect individual subscribers from that event.btcsignalsadheres to the subset of theboost::signals2API that we currently use, and thus is a drop-in replacement. Rather than implementing a complexslottracking class that we never used anyway (and which was much more useful in the days before std::function existed), callbacks are simply wrapped directly instd::functions.The new tests work with either
boost::signals2or the newbtcsignalsimplementation. Reviewers can verifyfunctional equivalency by running the tests in the commit that introduces them against
boost::signals2, then again withbtcsignals.The majority of the commits in this PR are preparation and cleanup. Once
boost::signals2is no longer needed, it is removed from depends. Additionally, a few CMake targets no longer need boost includes as they were previously only required for signals.I think this is actually pretty straightforward to review. I kept things simple, including keeping types unmovable/uncopyable where possible rather than trying to define those semantics. In doing so, the new implementation has even fewer type requirements than boost, which I believe is due to a boost bug. I've opened a PR upstream for that to attempt to maintain parity between the implementations.
See individual commits for more details.
Closes #26442.