Replace boost signals with minimal compatible implementation #34495

theuni commented at 9:27 PM on February 3, 2026: member

This drops our dependency on boost::signals2, leaving boost::multi_index as the only remaining boost dependency for bitcoind.

boost::signals2 is 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.

btcsignals adheres to the subset of the boost::signals2 API that we currently use, and thus is a drop-in replacement. Rather than implementing a complex slot tracking class that we never used anyway (and which was much more useful in the days before std::function existed), callbacks are simply wrapped directly in std::functions.

The new tests work with either boost::signals2 or the new btcsignals implementation. Reviewers can verify functional equivalency by running the tests in the commit that introduces them against boost::signals2, then again with btcsignals.

The majority of the commits in this PR are preparation and cleanup. Once boost::signals2 is 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.

signals: Use a lambda to avoid connecting a signal to another signal

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.

fd5e9d9904

signals: Temporarily add boost headers to bitcoind and bitcoin-node builds

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.

2150153f37

DrahtBot commented at 9:28 PM on February 3, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fjahr, w0xlt, maflcko
Concept ACK	fanquake, hebasto
Approach ACK	sedited

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34995 (ci, iwyu: Fix warnings in src/common and treat them as errors by hebasto)
#34806 (refactor: logging: Various API improvements by ajtowns)
#33117 (Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications by D33r-Gee)
#31425 (RFC: Riscv bare metal CI job by sedited)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic 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

theuni force-pushed on Feb 3, 2026

DrahtBot added the label CI failed on Feb 3, 2026

DrahtBot commented at 10:21 PM on February 3, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task 32 bit ARM: https://github.com/bitcoin/bitcoin/actions/runs/21648523714/job/62408637704</sub> <sub>LLM reason (✨ experimental): btcsignals_tests thread_safety test failed (assertion !sig0.empty()), causing the CI job to abort.</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

in src/wallet/wallet.cpp:3554 in fd5e9d9904 outdated

3548 | @@ -3549,7 +3549,9 @@ bool CWallet::HaveCryptedKeys() const
3549 |  void CWallet::ConnectScriptPubKeyManNotifiers()
3550 |  {
3551 |      for (const auto& spk_man : GetActiveScriptPubKeyMans()) {
3552 | -        spk_man->NotifyCanGetAddressesChanged.connect(NotifyCanGetAddressesChanged);
3553 | +        spk_man->NotifyCanGetAddressesChanged.connect([this] {
3554 | +            NotifyCanGetAddressesChanged();
3555 | +        });

davidgumberg commented at 10:35 PM on February 3, 2026:

In https://github.com/bitcoin/bitcoin/pull/34495/changes/fd5e9d990431a6af08dd99b25d1e17c6c9818b4d (signals: Use a lambda to avoid connecting a signal to another signal) Just observation: this fixes ~#26422~ #26442

hebasto commented at 11:00 PM on February 3, 2026:

Just observation: this fixes #26422

#26422 ?

davidgumberg commented at 11:06 PM on February 3, 2026:

Sorry: #26442

in src/btcsignals.h:1 in 91b1ff9eb7 outdated

   0 | @@ -1,15 +1,268 @@
   1 | -// Copyright (c) 2009-2010 Satoshi Nakamoto
   2 | -// Copyright (c) 2009-2021 The Bitcoin Core developers
   3 | +// Copyright (c) The Bitcoin Core developers

davidgumberg commented at 10:42 PM on February 3, 2026:

nit: The copyright notice is introduced in https://github.com/bitcoin/bitcoin/pull/34495/changes/f7ec999db41c54c058ab3b35bf37f52b0292e020 (signals: use forwarding header for boost signals) and then fixed in https://github.com/bitcoin/bitcoin/pull/34495/changes/91b1ff9eb7bde5e0e9d178ab8373d9dff8328000 (signals: Add a simplified boost-compatible implementation)

fanquake commented at 9:32 AM on February 4, 2026: member

Concept ACK

fanquake commented at 9:32 AM on February 4, 2026: member

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'

fanquake commented at 10:47 AM on February 4, 2026: member

This also prunes half a megabyte off the size of a Guix built (stripped) bitcoind. master (1e64aeaaecc2)

/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 bitcoind

This PR (7f7df4102bea867a7826ddfa38955f6ed4697ea6 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

fjahr commented at 11:07 AM on February 4, 2026: contributor

Concept ACK

theuni commented at 3:08 PM on February 4, 2026: member

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'

@hebasto Any experience dealing with MSVC ICE's?

This is frustrating considering how much simpler it is than boost.

l0rinc commented at 4:37 PM on February 4, 2026: contributor

INTERNAL COMPILER ERROR

Can we replace Windows with a minimal compatible implementation as well?

hodlinator commented at 10:07 PM on February 4, 2026: contributor

<details><summary>Re: MSVC Internal Compiler Errors</summary>

I'm able to build 237005aa7579be4488696bdca28253e8216453a9 "signals: add signals tests" fine, but the following commit, 91b1ff9eb7bde5e0e9d178ab8373d9dff8328000 "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:

C:\Users\hodlinator\bitcoin\src\node\interface_ui.cpp(28,18): fatal error C1001: Internal compiler error. [C:\Users\hodlinator\bitcoin\build\src\bitcoin_node.vcxproj]
  (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\Community\VC\Tools\MSVC\14.44.35207\bin\HostX64\x64\CL.exe'
      Please choose the Technical Support command on the Visual C++
      Help menu, or open the Technical Support help file for more information
C:\Users\hodlinator\bitcoin\src\wallet\interfaces.cpp(505,16): error C2664: 'std::unique_ptr<interfaces::Handler,std::default_delete<interfaces::Handler>> interfaces::MakeSignalHandler(btcsignals::connection)': cannot conver
t argument 1 from 'boost::signals2::connection' to 'btcsignals::connection' [C:\Users\hodlinator\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
      C:\Users\hodlinator\bitcoin\src\wallet\interfaces.cpp(505,64):
      No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
      C:\Users\hodlinator\bitcoin\src\interfaces\handler.h(28,26):
      see declaration of 'interfaces::MakeSignalHandler'
      C:\Users\hodlinator\bitcoin\src\wallet\interfaces.cpp(505,16):
      while trying to match the argument list '(boost::signals2::connection)'

C:\Users\hodlinator\bitcoin\src\wallet\interfaces.cpp(509,16): error C2664: 'std::unique_ptr<interfaces::Handler,std::default_delete<interfaces::Handler>> interfaces::MakeSignalHandler(btcsignals::connection)': cannot conver
t argument 1 from 'boost::signals2::connection' to 'btcsignals::connection' [C:\Users\hodlinator\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
      C:\Users\hodlinator\bitcoin\src\wallet\interfaces.cpp(509,64):
      No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
      C:\Users\hodlinator\bitcoin\src\interfaces\handler.h(28,26):
      see declaration of 'interfaces::MakeSignalHandler'
      C:\Users\hodlinator\bitcoin\src\wallet\interfaces.cpp(509,16):
      while trying to match the argument list '(boost::signals2::connection)'

C:\Users\hodlinator\bitcoin\src\wallet\interfaces.cpp(513,16): error C2664: 'std::unique_ptr<interfaces::Handler,std::default_delete<interfaces::Handler>> interfaces::MakeSignalHandler(btcsignals::connection)': cannot conver
t argument 1 from 'boost::signals2::connection' to 'btcsignals::connection' [C:\Users\hodlinator\bitcoin\build\src\wallet\bitcoin_wallet.vcxproj]
      C:\Users\hodlinator\bitcoin\src\wallet\interfaces.cpp(513,71):
      No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
      C:\Users\hodlinator\bitcoin\src\interfaces\handler.h(28,26):
      see declaration of 'interfaces::MakeSignalHandler'
      C:\Users\hodlinator\bitcoin\src\wallet\interfaces.cpp(513,16):
      while trying to match the argument list '(boost::signals2::connection)'

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.

</details>

theuni commented at 10:56 PM on February 4, 2026: member

@hodlinator At 91b1ff9eb7bde5e0e9d178ab8373d9dff8328000, boost is no longer used, so including that header won't help anything. I'm not sure how it's even picking up boost::signals2 references anymore. Is it possible something is stale in your environment? Whatever errors you're seeing without including that header would be the interesting ones.

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 signal or connection. If attempted, the callback will deadlock.

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.

theuni force-pushed on Feb 4, 2026

theuni commented at 11:49 PM on February 4, 2026: member

I realized last night that there's a behavioral break from boost in this branch, in that callbacks are not allowed to modify their signal or connection. If attempted, the callback will deadlock.

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.

Pushed an updated impl of btcsignals which matches boost's callback threading model. Sorry for the noise.

theuni force-pushed on Feb 4, 2026

maflcko commented at 6:10 AM on February 5, 2026: member

@hebasto Any experience dealing with MSVC ICE's?

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)

hodlinator commented at 8:14 AM on February 5, 2026: contributor

Likewise, sorry for the noise, I don't know what happened to my local Windows repo.

hodlinator commented at 10:02 AM on February 5, 2026: contributor

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.cpp

Note that dropping /std:c++20 makes it avoid the ICE.

maflcko commented at 10:12 AM on February 5, 2026: member

Managed to boil down the ICE, would someone else please do the honors of reporting to MS?

Excellent reproducer. I'll report it

maflcko commented at 10:28 AM on February 5, 2026: member

https://developercommunity.visualstudio.com/t/C1001:-Internal-compiler-error-Using-st/11039556

in src/btcsignals.h:245 in 956ea69802

 240 | +            }
 241 | +        }
 242 | +        return true;
 243 | +    }
 244 | +
 245 | +    mutable Mutex m_mutex{};

hodlinator commented at 10:31 AM on February 5, 2026:

I know it's blasphemous but switching to RecursiveMutex/AnnotatedMixin<std::recursive_mutex> avoids the ICE on native Windows for some reason:

    mutable RecursiveMutex m_mutex{};

Found while figuring out the ICE reproducer.

maflcko commented at 10:39 AM on February 5, 2026:

Maybe guard the RecursiveMutex behind #if defined(_MSC_VER)?

hebasto commented at 3:17 PM on February 5, 2026:

Or stop using MSVC?

hebasto referenced this in commit eb97250421 on Feb 5, 2026

hebasto commented at 2:35 PM on February 5, 2026: member

https://developercommunity.visualstudio.com/t/C1001:-Internal-compiler-error-Using-st/11039556

Please vote!

in depends/packages/boost.mk:9 in 956ea69802 outdated

   5 | @@ -6,7 +6,7 @@ $(package)_sha256_hash = 913ca43d49e93d1b158c9862009add1518a4c665e7853b349a6492d
   6 |  $(package)_build_subdir = build
   7 |  
   8 |  define $(package)_set_vars
   9 | -  $(package)_config_opts = -DBOOST_INCLUDE_LIBRARIES="multi_index;signals2;test"
  10 | +  $(package)_config_opts = -DBOOST_INCLUDE_LIBRARIES="multi_index;test"

hebasto commented at 2:53 PM on February 5, 2026:

And from vcpkg:

--- a/vcpkg.json
+++ b/vcpkg.json
@@ -4,7 +4,6 @@
   "builtin-baseline": "120deac3062162151622ca4860575a33844ba10b",
   "dependencies": [
     "boost-multi-index",
-    "boost-signals2",
     "libevent"
   ],
   "default-features": [

hebasto commented at 3:15 PM on February 5, 2026: member

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'

@hebasto Any experience dealing with MSVC ICE's?

This is frustrating considering how much simpler it is than boost.

Let's switch to clang-cl.

The CI log is available here: https://github.com/hebasto/bitcoin/actions/runs/21716329966/job/62633207718.

theuni commented at 4:12 PM on February 5, 2026: member

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.cpp
Note that dropping /std:c++20 makes it avoid the ICE.

Thanks so much!

But.. I don't understand. This is just a vanilla use of our Mutex type. If that's the reproducer, why doesn't this cause an ICE everywhere else in our codebase?

theuni commented at 4:17 PM on February 5, 2026: member

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.cpp
Note that dropping /std:c++20 makes it avoid the ICE.
Thanks so much!

But.. I don't understand. This is just a vanilla use of our Mutex type. If that's the reproducer, why doesn't this cause an ICE everywhere else in our codebase?

Aha! It's the initializer! I added it to make clang-tidy happy, but MSVC apparently doesn't like it. No worries, I'll just remove it.

Bad:

struct ice_signal {
    MutexChild m_mutex{};
};

Good:

struct ice_signal {
    MutexChild m_mutex;
};

signals: use forwarding header for boost signals

For now, including btcsignals.h simply includes boost's signals. A follow-up
commit will replace the implementation.

037e58b57b

signals: remove forward-declare for signals

This eases the transition to a replacement signals implementation

9ade3929aa

signals: use an alias for the boost::signals2 namespace

The next commit will add a real implementation in this namespace.

edc2978058

theuni force-pushed on Feb 5, 2026

theuni commented at 4:26 PM on February 5, 2026: member

In latest push:

Fixed ICE (I hope)
Removed from vcpkg (thanks @hebasto)
Fixed copyright header nit (thanks @davidgumberg)

hebasto commented at 4:38 PM on February 5, 2026: member

Concept ACK.

DrahtBot removed the label CI failed on Feb 5, 2026

fanquake commented at 10:03 PM on February 6, 2026: member

Upstream change has landed: https://github.com/boostorg/signals2/pull/85.

in src/btcsignals.h:245 in 5e088b91b2

 240 | +            }
 241 | +        }
 242 | +        return true;
 243 | +    }
 244 | +
 245 | +    mutable Mutex m_mutex;

w0xlt commented at 12:50 AM on February 13, 2026:

It seems m_mutex and m_connections don't need to be exposed.

private:
    mutable Mutex m_mutex;

theuni commented at 12:59 AM on February 13, 2026:

Whoops, nice catch! This must've gotten lost somewhere along the way. Will fix.

in src/btcsignals.h:151 in 5e088b91b2

 146 | +    /*
 147 | +     * Helper struct for maintaining a callback and its associated connection
 148 | +     */
 149 | +    struct connection_holder {
 150 | +        template <typename Callable>
 151 | +        connection_holder(Callable&& callback) noexcept : m_callback{std::forward<Callable>(callback)}

w0xlt commented at 12:50 AM on February 13, 2026:

nit: Can m_callback (std::function) allocation throw ?

        connection_holder(Callable&& callback) : m_callback{std::forward<Callable>(callback)}

theuni commented at 1:15 AM on February 13, 2026:

No, but looking again, I suppose the callback's move ctor could. I'll make this change to be safe.

theuni commented at 8:07 PM on March 3, 2026:

Fixed in latest push.

in src/test/btcsignals_tests.cpp:20 in 5e088b91b2

  15 | +    moveonly_data(int data) : m_data(data){}
  16 | +    moveonly_data(moveonly_data&&) = default;
  17 | +
  18 | +    moveonly_data& operator=(moveonly_data&&) = delete;
  19 | +    moveonly_data(const moveonly_data&) = delete;
  20 | +    moveonly_data& operator=(const moveonly_data&) = default;

w0xlt commented at 12:53 AM on February 13, 2026:

    moveonly_data& operator=(const moveonly_data&) = delete;

theuni commented at 1:16 AM on February 13, 2026:

Looks right, thanks.

theuni commented at 8:06 PM on March 3, 2026:

Fixed in latest push.

in src/test/btcsignals_tests.cpp:1 in 1260a917b7 outdated

   0 | @@ -0,0 +1,284 @@
   1 | +// Copyright (c) The Bitcoin Core developers

sedited commented at 12:58 PM on March 2, 2026:

In commit 1260a917b7f628db8d036c20dfba32595b270247:

This doesn't seem to compile for me using clang-21:

<details> <summary>Click for error message</summary>

/usr/bin/ccache /usr/bin/clang++-21 -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE -I/home/drgrid/bitcoin/build_dev_mode_clang/src -I/home/drgrid/bitcoin/src -I/home/drgrid/bitcoin/src/univalue/include -I/home/drgrid/bitcoin/src/minisketch/include -I/home/drgrid/bitcoin/src/secp256k1/include -I/home/drgrid/bitcoin/build_dev_mode_clang/src/ipc -I/home/drgrid/bitcoin/src/ipc/libmultiprocess/include -O2 -g -std=c++20 -fPIE -fvisibility=hidden -fmacro-prefix-map=/home/drgrid/bitcoin/src=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wthread-safety-pointer -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wself-assign -Wundef -Wno-unused-parameter -MD -MT src/test/CMakeFiles/test_bitcoin.dir/btcsignals_tests.cpp.o -MF src/test/CMakeFiles/test_bitcoin.dir/btcsignals_tests.cpp.o.d -o src/test/CMakeFiles/test_bitcoin.dir/btcsignals_tests.cpp.o -c /home/drgrid/bitcoin/src/test/btcsignals_tests.cpp 
In file included from /home/drgrid/bitcoin/src/test/btcsignals_tests.cpp:5:
In file included from /home/drgrid/bitcoin/src/btcsignals.h:9:
In file included from /usr/include/boost/signals2/optional_last_value.hpp:15:
In file included from /usr/include/boost/optional.hpp:15:
/usr/include/boost/optional/optional.hpp:410:36: error: call to deleted constructor of 'unqualified_value_type' (aka '(anonymous namespace)::moveonly_data')
  410 |        ::new (m_storage.address()) unqualified_value_type(val) ;
      |                                    ^                      ~~~
/usr/include/boost/optional/optional.hpp:350:12: note: in instantiation of member function 'boost::optional_detail::optional_base<(anonymous namespace)::moveonly_data>::construct' requested here
  350 |       else construct(val);
      |            ^
/usr/include/boost/optional/optional.hpp:396:40: note: in instantiation of member function 'boost::optional_detail::optional_base<(anonymous namespace)::moveonly_data>::assign' requested here
  396 |     void reset ( argument_type val ) { assign(val); }
      |                                        ^
/usr/include/boost/signals2/detail/slot_call_iterator.hpp:110:29: note: in instantiation of member function 'boost::optional_detail::optional_base<(anonymous namespace)::moveonly_data>::reset' requested here
  110 |               cache->result.reset(cache->f(*iter));
      |                             ^
/usr/include/boost/iterator/iterator_facade.hpp:631:20: note: in instantiation of member function 'boost::signals2::detail::slot_call_iterator_t<boost::signals2::detail::variadic_slot_invoker<(anonymous namespace)::moveonly_data, int>, std::_List_iterator<boost::shared_ptr<boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int>>, boost::signals2::slot<(anonymous namespace)::moveonly_data (int)>, boost::signals2::mutex>>>, boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int>>, boost::signals2::slot<(anonymous namespace)::moveonly_data (int)>, boost::signals2::mutex>>::dereference' requested here
  631 |           return f.dereference();
      |                    ^
/usr/include/boost/iterator/iterator_facade.hpp:737:42: note: in instantiation of function template specialization 'boost::iterators::iterator_core_access::dereference<boost::signals2::detail::slot_call_iterator_t<boost::signals2::detail::variadic_slot_invoker<(anonymous namespace)::moveonly_data, int>, std::_List_iterator<boost::shared_ptr<boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int>>, boost::signals2::slot<(anonymous namespace)::moveonly_data (int)>, boost::signals2::mutex>>>, boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int>>, boost::signals2::slot<(anonymous namespace)::moveonly_data (int)>, boost::signals2::mutex>>>' requested here
  737 |             return iterator_core_access::dereference(this->derived());
      |                                          ^
/usr/include/boost/signals2/optional_last_value.hpp:35:21: note: in instantiation of member function 'boost::iterators::detail::iterator_facade_base<boost::signals2::detail::slot_call_iterator_t<boost::signals2::detail::variadic_slot_invoker<(anonymous namespace)::moveonly_data, int>, std::_List_iterator<boost::shared_ptr<boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int>>, boost::signals2::slot<(anonymous namespace)::moveonly_data (int)>, boost::signals2::mutex>>>, boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int>>, boost::signals2::slot<(anonymous namespace)::moveonly_data (int)>, boost::signals2::mutex>>, (anonymous namespace)::moveonly_data, boost::iterators::single_pass_traversal_tag, const (anonymous namespace)::moveonly_data &, long, false, false>::operator*' requested here
   35 |             value = *first;
      |                     ^
/usr/include/boost/signals2/detail/result_type_wrapper.hpp:53:18: note: in instantiation of function template specialization 'boost::signals2::optional_last_value<(anonymous namespace)::moveonly_data>::operator()<boost::signals2::detail::slot_call_iterator_t<boost::signals2::detail::variadic_slot_invoker<(anonymous namespace)::moveonly_data, int>, std::_List_iterator<boost::shared_ptr<boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int>>, boost::signals2::slot<(anonymous namespace)::moveonly_data (int)>, boost::signals2::mutex>>>, boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int>>, boost::signals2::slot<(anonymous namespace)::moveonly_data (int)>, boost::signals2::mutex>>>' requested here
   53 |           return combiner(first, last);
      |                  ^
/usr/include/boost/signals2/detail/signal_template.hpp:242:18: note: in instantiation of function template specialization 'boost::signals2::detail::combiner_invoker<boost::optional<(anonymous namespace)::moveonly_data>>::operator()<boost::signals2::optional_last_value<(anonymous namespace)::moveonly_data>, boost::signals2::detail::slot_call_iterator_t<boost::signals2::detail::variadic_slot_invoker<(anonymous namespace)::moveonly_data, int>, std::_List_iterator<boost::shared_ptr<boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int>>, boost::signals2::slot<(anonymous namespace)::moveonly_data (int)>, boost::signals2::mutex>>>, boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int>>, boost::signals2::slot<(anonymous namespace)::moveonly_data (int)>, boost::signals2::mutex>>>' requested here
  242 |           return detail::combiner_invoker<typename combiner_type::result_type>()
      |                  ^
/usr/include/boost/signals2/detail/signal_template.hpp:722:16: note: in instantiation of member function 'boost::signals2::detail::signal_impl<(anonymous namespace)::moveonly_data (int), boost::signals2::optional_last_value<(anonymous namespace)::moveonly_data>, int, std::less<int>, boost::function<(anonymous namespace)::moveonly_data (int)>, boost::function<(anonymous namespace)::moveonly_data (const boost::signals2::connection &, int)>, boost::signals2::mutex>::operator()' requested here
  722 |         return (*_pimpl)(BOOST_SIGNALS2_SIGNATURE_ARG_NAMES(BOOST_SIGNALS2_NUM_ARGS));
      |                ^
/home/drgrid/bitcoin/src/test/btcsignals_tests.cpp:109:20: note: in instantiation of member function 'boost::signals2::signal<(anonymous namespace)::moveonly_data (int)>::operator()' requested here
  109 |     auto ret = sig0(data);
      |                    ^
/home/drgrid/bitcoin/src/test/btcsignals_tests.cpp:22:5: note: 'moveonly_data' has been explicitly marked deleted here
   22 |     moveonly_data(const moveonly_data&) = delete;
      |     ^
1 error generated.
[10/12] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/wallettests.cpp.o
ninja: build stopped: subcommand failed.

</details>

theuni commented at 5:51 PM on March 2, 2026:

Hmm. I believe recent versions of Boost loosened some of these requirements. What version are you using?

sedited commented at 7:41 PM on March 2, 2026:

Boost 1.83 , our min version is currently 1.74.

theuni commented at 8:06 PM on March 3, 2026:

Fixed in latest push.

sedited commented at 1:05 PM on March 2, 2026: contributor

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?

theuni force-pushed on Mar 3, 2026

theuni commented at 8:09 PM on March 3, 2026: member

In latest push:

addressed @w0xlt's comments
fixed compile failure with older boost::signals2
formatted tests and switched to UpperCamelCase.

fanquake added this to the milestone 32.0 on Mar 3, 2026

sedited requested review from hodlinator on Mar 25, 2026

sedited requested review from w0xlt on Mar 25, 2026

w0xlt commented at 12:11 AM on April 2, 2026: contributor

ACK 90b2ddf80c67c34e1c855b80acadfe4a1efae4c8

DrahtBot requested review from hebasto on Apr 2, 2026

DrahtBot requested review from fanquake on Apr 2, 2026

DrahtBot requested review from sedited on Apr 2, 2026

DrahtBot requested review from fjahr on Apr 2, 2026

in src/test/btcsignals_tests.cpp:99 in 90b2ddf80c outdated

  94 | +    sig0(val);
  95 | +    BOOST_CHECK_EQUAL(val, 6);
  96 | +}
  97 | +
  98 | +/* Check that move-only return types work correctly
  99 | + */

maflcko commented at 8:41 AM on April 2, 2026:

seems harmless to test, but is there any actual code using a move-only return type right now? I think it is all void/bool right now?

theuni commented at 2:56 PM on April 2, 2026:

boost::signals used to needlessly require a move-assignable type. This has been fixed upstream (not yet released): https://github.com/boostorg/signals2/commit/44c4844c94d3888c659a63e6e5782494c366143b

This test exists to ensure that if the implementation ever changes, the constraints remain as tight as boost's.

in src/test/btcsignals_tests.cpp:163 in f2839809d4 outdated

 158 | + * Sanitizers should pick up any buggy data race behavior (if present).
 159 | + */
 160 | +BOOST_AUTO_TEST_CASE(thread_safety)
 161 | +{
 162 | +    btcsignals::signal<void()> sig0;
 163 | +    std::atomic<uint32_t> val{0};

maflcko commented at 9:28 AM on April 2, 2026:

nit in f2839809d48f670fbdbb3c447945753efc246824: I think the test is testing the thread-safety of btcsignals, not of the atomic, so I think it would be fine to have two separate atomics, and stricter checks in the end:

diff --git a/src/test/btcsignals_tests.cpp b/src/test/btcsignals_tests.cpp
index 7b59166a57..b5796c1cbf 100644
--- a/src/test/btcsignals_tests.cpp
+++ b/src/test/btcsignals_tests.cpp
@@ -162,5 +162,6 @@ BOOST_AUTO_TEST_CASE(thread_safety)
     btcsignals::signal<void()> sig0;
-    std::atomic<uint32_t> val{0};
-    auto conn0 = sig0.connect([&val] {
-        val++;
+    std::atomic<uint32_t> val_det{0};
+    std::atomic<uint32_t> val_non_det{0};
+    auto conn0 = sig0.connect([&val_det] {
+        val_det++;
     });
@@ -178,3 +179,3 @@ BOOST_AUTO_TEST_CASE(thread_safety)
 
-    std::thread extra_increment_injector([&conn0, &sig0, &val] {
+    std::thread extra_increment_injector([&conn0, &sig0, &val_non_det] {
         static constexpr size_t num_extra_conns{1000};
@@ -185,4 +186,4 @@ BOOST_AUTO_TEST_CASE(thread_safety)
             BOOST_CHECK(conn0.connected());
-            extra_conns.emplace_back(sig0.connect([&val] {
-                val++;
+            extra_conns.emplace_back(sig0.connect([&val_non_det] {
+                val_non_det++;
             }));
@@ -196,6 +197,11 @@ BOOST_AUTO_TEST_CASE(thread_safety)
 
-    // sig will have been called 2000 times, and at least 1000 of those will
-    // have been executing multiple incrementing callbacks. So while val is
-    // probably MUCH bigger, it's guaranteed to be at least 3000.
-    BOOST_CHECK_GE(val.load(), 3000);
+    // sig will have been called 2000 times, and only the first connection did
+    // increment val_det, so it must be 2000.
+    BOOST_CHECK_EQUAL(val_det.load(), 2000);
+    // The number of connections that increment val_non_det is growing from 1
+    // to 1000, and after each step the connections are called at least once.
+    // However, it is unknown how often the connections have been called
+    // exactly. The 1000th Triangular Number gives a lower estimate.
+    // T_n=n(n+1)/2
+    BOOST_CHECK_GE(val_non_det.load(), 1000*1001/2);
 }

maflcko commented at 7:50 AM on April 9, 2026:

I think you up-voted this, but forgot to push the diff? nbd, either way

maflcko commented at 7:53 PM on April 9, 2026:

Added in #35043 (further modified to disconnect every second connection immediately to check disconnect races better)

in src/test/btcsignals_tests.cpp:205 in f2839809d4 outdated

 200 | +    BOOST_CHECK_GE(val.load(), 3000);
 201 | +}
 202 | +
 203 | +/* Test that connection and disconnection works from within signal
 204 | + * callbacks.
 205 | + */

maflcko commented at 9:36 AM on April 2, 2026:

nit in f283980: Same here: This seems harmless to test, but I think no code in this codebase is (dis)connecting in a callback?

theuni commented at 3:04 PM on April 2, 2026:

One of the primary goals of this re-implementation is maintaining boost's thread-safety guarantees so that the two behave exactly the same way. I thought I had linked that somewhere, but I'm not seeing it now. I'll add it to btcsignals.h.

Arguably the most interesting part of boost::signals over a simple list of callbacks is the ability to disconnect at any given time in a defined way, so it doesn't seem unreasonable to expect that we might take advantage of that functionality in the future.

maflcko commented at 4:04 PM on April 2, 2026:

Yes, right. It seem easier to copy the boost behavior (even if it is unused right now) than to run into thread-safety issues later on, or having to re-invent the wheel later on for a use-case that may be plausible.

All good, and no need to change anything. I just wanted to mention that I think no code is using this currently.

in src/test/btcsignals_tests.cpp:270 in f2839809d4

 265 | + */
 266 | +BOOST_AUTO_TEST_CASE(disconnect_thread_safety)
 267 | +{
 268 | +    btcsignals::connection conn0, conn1, conn2;
 269 | +    btcsignals::signal<void(int&)> sig0;
 270 | +    std::atomic_bool wait1{false};

maflcko commented at 10:09 AM on April 2, 2026:

style nit in f2839809d48f670fbdbb3c447945753efc246824:

Can write the same in less lines of code:

diff --git a/src/test/btcsignals_tests.cpp b/src/test/btcsignals_tests.cpp
index b5796c1cbf..d5e71b27cb 100644
--- a/src/test/btcsignals_tests.cpp
+++ b/src/test/btcsignals_tests.cpp
@@ -9,2 +9,4 @@
 
+#include <semaphore>
+
 #define BOOST_MINOR_VERSION ((BOOST_VERSION / 100) % 1000)
@@ -275,4 +277,4 @@ BOOST_AUTO_TEST_CASE(disconnect_thread_safety)
     btcsignals::signal<void(int&)> sig0;
-    std::atomic_bool wait1{false};
-    std::atomic_bool wait2{false};
+    std::binary_semaphore done1{0};
+    std::binary_semaphore done2{0};
     int val{0};
@@ -281,5 +283,4 @@ BOOST_AUTO_TEST_CASE(disconnect_thread_safety)
         conn1.disconnect();
-        wait1.store(true);
-        wait1.notify_all();
-        wait2.wait(false);
+        done1.release();
+        done2.acquire();
     });
@@ -288,6 +289,5 @@ BOOST_AUTO_TEST_CASE(disconnect_thread_safety)
     std::thread thr([&] {
-        wait1.wait(false);
+        done1.acquire();
         conn2.disconnect();
-        wait2.store(true);
-        wait2.notify_all();
+        done2.release();
     });

theuni commented at 3:05 PM on April 2, 2026:

Neat. Thanks :)

in src/node/interface_ui.h:20 in 3dd105e98b outdated

  14 | @@ -17,6 +15,10 @@ class CBlockIndex;
  15 |  enum class SynchronizationState;
  16 |  struct bilingual_str;
  17 |  
  18 | +namespace btcsignals {
  19 | +    class connection;
  20 | +} // namespace btcsignals

maflcko commented at 11:27 AM on April 2, 2026:

unrelated nit in 3dd105e98bdf0670b684cbc7d9f055d3e4dd3234: I think the new header is so cheap, that commit fa5ce27385bc60cdf6d9a4eeb2d32c916c9e07eb can be reverted. Though, this can be done in a follow-up.

maflcko commented at 7:53 PM on April 9, 2026:

Done in https://github.com/bitcoin/bitcoin/pull/35043

in doc/developer-notes.md:1412 in ed34bc9ff4 outdated

1405 | @@ -1406,16 +1406,6 @@ communication:
1406 |    virtual const CBlockIndex* findBlock(const uint256& hash) = 0;
1407 |    ```
1408 |  
1409 | -  ```c++
1410 | -  // Good: takes plain callback type and returns interface pointer
1411 | -  using TipChangedFn = std::function<void(int block_height, int64_t block_time)>;
1412 | -  virtual std::unique_ptr<interfaces::Handler> handleTipChanged(TipChangedFn fn) = 0;

maflcko commented at 11:29 AM on April 2, 2026:

nit in ed34bc9ff47186198c5ae269f0a1c31b67847fa5: I don't think this is correct. The code still exists. The correct fix would be to just replace below: boost::... -> btcsignals::.

theuni commented at 3:06 PM on April 2, 2026:

Ok, will do.

in src/btcsignals.h:6 in 037e58b57b outdated

   0 | @@ -0,0 +1,12 @@
   1 | +// Copyright (c) The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#ifndef BITCOIN_BTCSIGNALS_H
   6 | +#define BITCOIN_BTCSIGNALS_H

maflcko commented at 11:38 AM on April 2, 2026:

nit in 037e58b57b7304e4a7f013b8768b8a86a09c96e8: Should this header not be in ./common/, or in ./util/, if the goal is to use it broader?

theuni commented at 3:24 PM on April 2, 2026:

Can do. Do you have a preference for which?

maflcko commented at 3:59 PM on April 2, 2026:

For this one I am 50/50 :sweat_smile: . Maybe ./util for now, because it is a stand-alone header? It should be trivial to move later-on, if needed.

theuni commented at 5:15 PM on April 3, 2026:

Actually, I'm afraid that all this movement would make this harder to review. Ok to do as a follow-up?

in src/test/btcsignals_tests.cpp:157 in f2839809d4

 152 | +/* Test the thread-safety of connect/disconnect/empty/connected/callbacks.
 153 | + * Connect sig0 to an incrementor function and loop in a thread.
 154 | + * Meanwhile, in another thread, inject and call new increment callbacks.
 155 | + * Both threads are constantly calling empty/connected.
 156 | + * Though the end-result is undefined due to a non-deterministic number of
 157 | + * total callbacks excecuted, this should all be completely threadsafe.

maflcko commented at 11:41 AM on April 2, 2026:

nit in f2839809d48f670fbdbb3c447945753efc246824: Typo (from the llm)

excecuted -> executed

in src/btcsignals.h:231 in 1a1c8b293c

 230 | +    template <typename Callable>
 231 | +    connection connect(Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
 232 | +    {
 233 | +        LOCK(m_mutex);
 234 | +
 235 | +        // Garbage-collect disconnected signals to prevent unbounded growth

maflcko commented at 11:43 AM on April 2, 2026:

nit in 1a1c8b293c0d343baa1485022ff34eff806af726: Typo signals->connections

maflcko approved

maflcko commented at 11:43 AM on April 2, 2026: member

all looks good. Just some nits, but no blocker.

review ACK 90b2ddf80c67c34e1c855b80acadfe4a1efae4c8 🗣

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 90b2ddf80c67c34e1c855b80acadfe4a1efae4c8 🗣
Ra9lYWzySHW9KIP7wqbBionGmskT4hc7+kiMY3A7poOpwNeE2YU1L2jVsUggmX3Z70MB66oUw4ImPfPBkMwkDQ==

</details>

in src/btcsignals.h:249 in 90b2ddf80c

 244 | +        for (const auto& connection : m_connections) {
 245 | +            if (connection->m_connection.connected()) {
 246 | +                return false;
 247 | +            }
 248 | +        }
 249 | +        return true;

purpleKarrot commented at 4:30 PM on April 2, 2026:

        return std::ranges::none_of(m_connections, [](const auto& connection) {
            return connection->m_connection.connected();
        });

purpleKarrot commented at 4:44 PM on April 2, 2026: contributor

This PR is symptomatic for a fundamental problem that I see with the review process of Bitcoin Core. Since the implementation is already done, the code becomes a kind of gravity well: it draws my attention toward post-hoc discussion of local implementation details (e.g. whether to prefer standard algorithms over manual loops), instead of enabling earlier alignment on structure and tradeoffs before they're encoded.

In practice that biases review toward nitpicks of a finished solution, rather than shaping the solution up front.

maflcko commented at 4:58 PM on April 2, 2026: member

This PR is symptomatic for a fundamental problem that I see with the review process of Bitcoin Core. Since the implementation is already done, the code becomes a kind of gravity well: it draws my attention toward post-hoc discussion of local implementation details (e.g. whether to prefer standard algorithms over manual loops), instead of enabling earlier alignment on structure and tradeoffs before they're encoded.

In practice that biases review toward nitpicks of a finished solution, rather than shaping the solution up front.

I don't see how an implementation prevents anyone from raising structural concerns or other trade-offs. In fact, this is expected to happen, and also does happen. See e.g. (just picking the ones from last week, you can go back yourself further if you want: #34946, #34935, #34932, ... were all re-shaped in another pull request.) In any case, this seems unrelated to this specific pull request, so if you think a broader issue should be raised, it could make sense to do so in a new issue thread.

purpleKarrot commented at 5:45 PM on April 2, 2026: contributor

I don't see how an implementation prevents anyone from raising structural concerns or other trade-offs.

I did not say it prevents raising structural concerns. All I said is that it creates bias. I speak for myself when I say that it draws my attention towards implementation details and I pointed out an explicit example in the code. Showing three PRs that have been closed does not falsify that statement.

theuni commented at 6:49 PM on April 2, 2026: member

@purpleKarrot I'm not sure what you're after here.

#26442 has been open for over 3 years. Our boost removal effort has been ongoing for probably a decade. This PR is a drop-in replacement, meaning that the structural concerns (API) are largely inherited.

This solution migrates us away from boost in a way that's transparent to the outside code. Imo that's strictly better than what we have now. Nothing prevents a followup discussion about a diverging/improved API down the road.

If you would like for me to use more idiomatic (by some measure) code, I'm happy to do so. But the fact is, this is a huge project with lots of contributors and we'll never converge on a single static approach for every intention. The ranges approach you suggest has only become feasible in the last few years, now that libc++/libstdc++ have caught up. Before that it was range-based for. Before that it was iterators. Migrating our entire codebase each time would've caused huge amounts of churn with very little payoff.

I share your frustration. A nice, clean, opinionated, idiomatic codebase would be nice and would make the review process less arbitrary. But it would come at a bureaucratic cost that is (imo) at odds with the spirit of the project.

purpleKarrot commented at 7:46 PM on April 2, 2026: contributor

If you would like for me to use more idiomatic (by some measure) code, I'm happy to do so.

No no, this is not what I am after. In fact, I think this is a distraction and may be the reason why PRs are sitting in the review queue for longer than necessary.

What I would prefer is a process where we review a proposed design before it gets implemented. Once you have a PR that you can point to an ACKed design, code review should be exclusively about whether the implementation matches the design. This does not require in depth knowledge and it is not about agreement. It might be automated, but it is also a great way for others to get introduced into the code base. It should be finished rather quickly.

The problem with the current process is that it is not really clear where to start a review. It seems like every code review requires expert domain knowledge in multiple areas. If I am young and want to contribute, where do I start? I just read the titles in the PR queue to search for a simple one. Open it, try to make sense of the code, close it again without writing a comment. To the author this looks like nobody had a look at it and the PR waits in the queue until one of the dinosaurs finds the time to have a look.

It would be much better if the domain experts concentrated on the design and left the "easy" part to the people that are enthusiastic to learn. But this requires that we decouple the design review from the implementation.

in src/btcsignals.h:239 in 1a1c8b293c outdated

 238 | +        const auto& connection = m_connections.emplace_back(std::make_shared<connection_holder>(std::forward<Callable>(func)));
 239 | +        return connection->m_connection;
 240 | +    }
 241 | +
 242 | +    /*
 243 | +     * Returns true if there is at least one associated and enabled callback.

fjahr commented at 8:32 PM on April 2, 2026:

This should say "Returns false..." to match the code

fjahr commented at 8:37 PM on April 2, 2026: contributor

Code review ACK 90b2ddf80c67c34e1c855b80acadfe4a1efae4c8

Looks very clean, really nice! I would quickly re-review if you address @maflcko 's and my comment.

And I hope the meta discussion can be move to an issue, this is really not the right place and I am biting my tongue to not give an opinion on it here ;)

in src/btcsignals.h:13 in 1a1c8b293c outdated

  12 |  
  13 | -namespace btcsignals = boost::signals2;
  14 | +#include <functional>
  15 | +#include <memory>
  16 | +#include <optional>
  17 | +#include <vector>

fjahr commented at 8:41 PM on April 2, 2026:

mini-nit: missing iwyu includes <atomic>, <type_traits> and <utility>

bitcoin deleted a comment on Apr 2, 2026

in src/btcsignals.h:117 in 1a1c8b293c

 116 | +
 117 | +public:
 118 | +    scoped_connection(connection rhs) noexcept : m_conn{std::move(rhs)} {}
 119 | +
 120 | +    scoped_connection(scoped_connection&&) noexcept = default;
 121 | +    scoped_connection& operator=(scoped_connection&&) noexcept = delete;

maflcko commented at 8:54 AM on April 3, 2026:

nit in 1a1c8b293c0d343baa1485022ff34eff806af726: I think this is a typo. Easy to miss because default and delete look too similar. For reference, boost has it not deleted, see https://www.boost.org/doc/libs/latest/doc/html/boost/signals2/scoped_connection.html#id-1_3_34_6_2_1_1_2_3_6-bb

theuni commented at 3:54 PM on April 3, 2026:

This isn't a typo, it's a purposeful simplification to avoid an anti-pattern.

Though the default behavior just does the right thing, so I suppose it's probably less confusing to just leave it enabled. Will change.

in src/btcsignals.h:114 in 1a1c8b293c outdated

 113 | +class scoped_connection
 114 | +{
 115 | +    connection m_conn;
 116 | +
 117 | +public:
 118 | +    scoped_connection(connection rhs) noexcept : m_conn{std::move(rhs)} {}

maflcko commented at 8:59 AM on April 3, 2026:

nit in 1a1c8b2: I know boost doesn't set this explicit, but it seems harmless to add, and possibly be useful to be explicit that the connection is "stolen"? Just a nit.

Could also require a std::move into this constructor via &&, but given that a connection copy ctor exists (and is required), the benefits seem limited, so up to you. Just a nit.

maflcko commented at 9:19 AM on April 3, 2026:

The diff for explicit would be:

diff --git a/src/btcsignals.h b/src/btcsignals.h
index 941894547e..999b62dd28 100644
--- a/src/btcsignals.h
+++ b/src/btcsignals.h
@@ -113,3 +113,3 @@ class scoped_connection
 public:
-    scoped_connection(connection rhs) noexcept : m_conn{std::move(rhs)} {}
+    explicit scoped_connection(connection rhs) noexcept : m_conn{std::move(rhs)} {}
 
diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index 0b89c605b9..80cd56657e 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -487,5 +487,5 @@ int GuiMain(int argc, char* argv[])
     // Subscribe to global signals from core
-    btcsignals::scoped_connection handler_message_box = ::uiInterface.ThreadSafeMessageBox_connect(noui_ThreadSafeMessageBox);
-    btcsignals::scoped_connection handler_question = ::uiInterface.ThreadSafeQuestion_connect(noui_ThreadSafeQuestion);
-    btcsignals::scoped_connection handler_init_message = ::uiInterface.InitMessage_connect(noui_InitMessage);
+    btcsignals::scoped_connection handler_message_box{::uiInterface.ThreadSafeMessageBox_connect(noui_ThreadSafeMessageBox)};
+    btcsignals::scoped_connection handler_question{::uiInterface.ThreadSafeQuestion_connect(noui_ThreadSafeQuestion)};
+    btcsignals::scoped_connection handler_init_message{::uiInterface.InitMessage_connect(noui_InitMessage)};
 
diff --git a/src/test/btcsignals_tests.cpp b/src/test/btcsignals_tests.cpp
index 7b59166a57..15757fa335 100644
--- a/src/test/btcsignals_tests.cpp
+++ b/src/test/btcsignals_tests.cpp
@@ -132,3 +132,3 @@ BOOST_AUTO_TEST_CASE(return_value)
     {
-        btcsignals::scoped_connection conn0 = sig0.connect(ReturnTrue);
+        btcsignals::scoped_connection conn0{sig0.connect(ReturnTrue)};
         ret = sig0();
@@ -139,4 +139,4 @@ BOOST_AUTO_TEST_CASE(return_value)
     {
-        btcsignals::scoped_connection conn1 = sig0.connect(ReturnTrue);
-        btcsignals::scoped_connection conn0 = sig0.connect(ReturnFalse);
+        btcsignals::scoped_connection conn1{sig0.connect(ReturnTrue)};
+        btcsignals::scoped_connection conn0{sig0.connect(ReturnFalse)};
         ret = sig0();

theuni commented at 3:55 PM on April 3, 2026:

I'd rather not do this, at least not in the PR. One of my goals here was to avoid having to change the user code (with the exception of the first commit). Happy to see it as a follow-up, though.

maflcko commented at 7:54 PM on April 9, 2026:

Done in https://github.com/bitcoin/bitcoin/pull/35043

in src/btcsignals.h:66 in 1a1c8b293c outdated

  65 | +    static constexpr enabled_tag_type enabled_tag{};
  66 | +
  67 | +    /**
  68 | +     * connections have shared_ptr-like copy and move semantics.
  69 | +     */
  70 | +    std::shared_ptr<std::atomic_bool> m_connected{};

maflcko commented at 10:06 AM on April 3, 2026:

nit in 1a1c8b293c0d343baa1485022ff34eff806af726: Not important, but two heap allocation can be avoided by stacking it on top of the other heap allocation and re-using a single control block, at the cost of deferring the std::function destructor to happen at the same time with the connection (in the case where the disconnected connection outlives the signal object). Again, just an unimportant implementation detail, feel free to ignore the diff:

diff --git a/src/btcsignals.h b/src/btcsignals.h
index 999b62dd28..216bdef5fb 100644
--- a/src/btcsignals.h
+++ b/src/btcsignals.h
@@ -55,8 +55,8 @@ class connection
 
-    /*
-     * Tag for the constructor used by signal.
+    /**
+     * Track liveness. Also serves as a tag for the constructor used by signal.
      */
-    struct enabled_tag_type {
+    struct liveness {
+        std::atomic_bool m_connected{true};
     };
-    static constexpr enabled_tag_type enabled_tag{};
 
@@ -65,3 +65,3 @@ class connection
      */
-    std::shared_ptr<std::atomic_bool> m_connected{};
+    std::shared_ptr<liveness> m_state;
 
@@ -70,3 +70,3 @@ class connection
      */
-    explicit connection(enabled_tag_type /*unused*/) : m_connected{std::make_shared<std::atomic_bool>(true)} {}
+    explicit connection(std::shared_ptr<liveness> state) : m_state(std::move(state)) {}
 
@@ -90,4 +90,4 @@ public:
     {
-        if (m_connected) {
-            m_connected->store(false);
+        if (m_state) {
+            m_state->m_connected.store(false);
         }
@@ -101,3 +101,3 @@ public:
     {
-        return m_connected && m_connected->load();
+        return m_state && m_state->m_connected.load();
     }
@@ -148,3 +148,3 @@ class signal
      */
-    struct connection_holder {
+    struct connection_holder : public connection::liveness {
         template <typename Callable>
@@ -154,4 +154,3 @@ class signal
 
-        connection m_connection{connection::enabled_tag};
-        function_type m_callback;
+        const function_type m_callback;
     };
@@ -206,3 +205,3 @@ public:
             for (const auto& connection : connections) {
-                if (connection->m_connection.connected()) {
+                if (connection->m_connected.load()) {
                     connection->m_callback(args...);
@@ -213,3 +212,3 @@ public:
             for (const auto& connection : connections) {
-                if (connection->m_connection.connected()) {
+                if (connection->m_connected.load()) {
                     ret.emplace(connection->m_callback(args...));
@@ -231,6 +230,6 @@ public:
         // Garbage-collect disconnected signals to prevent unbounded growth
-        std::erase_if(m_connections, [](const auto& holder) { return !holder->m_connection.connected(); });
+        std::erase_if(m_connections, [](const auto& holder) { return !holder->m_connected.load(); });
 
-        const auto& connection = m_connections.emplace_back(std::make_shared<connection_holder>(std::forward<Callable>(func)));
-        return connection->m_connection;
+        const auto& holder = m_connections.emplace_back(std::make_shared<connection_holder>(std::forward<Callable>(func)));
+        return connection{holder};
     }
@@ -244,3 +243,3 @@ public:
         for (const auto& connection : m_connections) {
-            if (connection->m_connection.connected()) {
+            if (connection->m_connected.load()) {
                 return false;

theuni commented at 3:04 PM on April 3, 2026:

Haha, I made a very similar change locally after opening the PR, but decided not to push it up for the sake of not nullifying the existing review: https://github.com/theuni/bitcoin/commit/522662c57c745995830b2ba5825e45d3eedb2e52

Like.. shockingly similar :)

I would prefer that as well, and since you came up with the same thing independently, it's apparently a reasonable approach.

What do you think about me pushing that commit on top of the existing ones, that way reviewers can review it as an improvement on top of what they've already reviewed?

maflcko commented at 5:11 PM on April 3, 2026:

Heh, my preference would be to squash, so that new reviewers only have to review the final thing once, and not twice. But anything works for me.

maflcko approved

maflcko commented at 10:08 AM on April 3, 2026: member

left more nits 😅

signals: add signals tests

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.

63c68e2a3f

signals: Add a simplified boost-compatible implementation

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.

e60a0b9a22

signals: remove boost compatibility guards

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.

34eabd77a2

Revert "signals: Temporarily add boost headers to bitcoind and bitcoin-node builds"

This reverts commit 3df1a1405972076384946c71c45bb056cfdf8c41.

This was only necessary as an interim workaround.

9958f4fe49

signals: re-add forward-declares to interface headers

The real includes were only needed temporarily while supporting btcsignals as
an alias for boost::signals2.

091736a153

signals: remove boost includes where possible

In both of these cases, boost was only needed for signals and not for
multi_index/test.

375397ebd9

signals: remove boost::signals2 mentions in linters and docs

The documented example is no longer relevant, so remove it rather than updating
it to mention btcsignals.

a4b1607983

signals: remove boost::signals2 from depends and vcpkg b12f43a0a8

theuni force-pushed on Apr 3, 2026

btcsignals: use a single shared_ptr for liveness and callback

This simplifies the implementation and eliminates an unnecessary shared_ptr.

Suggested by Marco Falke.

<Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

242b0ebb5c

theuni commented at 6:32 PM on April 3, 2026: member

Addressed feedback with the exception of @maflcko's implementation suggestion (will push a new commit for that) and the move to util dir+namespace.

Diff can be seen using: git diff 90b2ddf..b12f43a:


diff --git a/doc/developer-notes.md b/doc/developer-notes.md
index 1929519835e..c30fb509f2b 100644
--- a/doc/developer-notes.md
+++ b/doc/developer-notes.md
@@ -1408,2 +1408,12 @@ communication:

+  ```c++
+  // Good: takes plain callback type and returns interface pointer
+  using TipChangedFn = std::function<void(int block_height, int64_t block_time)>;
+  virtual std::unique_ptr<interfaces::Handler> handleTipChanged(TipChangedFn fn) = 0;
+
+  // Bad: returns btcsignals connection specific to local process
+  using TipChangedFn = std::function<void(int block_height, int64_t block_time)>;
+  virtual btcsignals::scoped_connection connectTipChanged(TipChangedFn fn) = 0;
+  ```
+
 - Interface methods should not be overloaded.
diff --git a/src/btcsignals.h b/src/btcsignals.h
index d917ecfc1da..f4c2530be91 100644
--- a/src/btcsignals.h
+++ b/src/btcsignals.h
@@ -9,2 +9,4 @@

+#include <algorithm>
+#include <atomic>
 #include <functional>
@@ -12,2 +14,4 @@
 #include <optional>
+#include <type_traits>
+#include <utility>
 #include <vector>
@@ -116,3 +120,3 @@ public:
     scoped_connection(scoped_connection&&) noexcept = default;
-    scoped_connection& operator=(scoped_connection&&) noexcept = delete;
+    scoped_connection& operator=(scoped_connection&&) noexcept = default;

@@ -230,3 +234,3 @@ public:

-        // Garbage-collect disconnected signals to prevent unbounded growth
+        // Garbage-collect disconnected connections to prevent unbounded growth
         std::erase_if(m_connections, [](const auto& holder) { return !holder->m_connection.connected(); });
@@ -238,3 +242,3 @@ public:
     /*
-     * Returns true if there is at least one associated and enabled callback.
+     * Returns true if there are no enabled callbacks
      */
@@ -243,8 +247,5 @@ public:
         LOCK(m_mutex);
-        for (const auto& connection : m_connections) {
-            if (connection->m_connection.connected()) {
-                return false;
-            }
-        }
-        return true;
+        return std::ranges::none_of(m_connections, [](const auto& holder) {
+            return holder->m_connection.connected();
+        });
     }
diff --git a/src/test/btcsignals_tests.cpp b/src/test/btcsignals_tests.cpp
index 8468c485fca..b3203ebeb23 100644
--- a/src/test/btcsignals_tests.cpp
+++ b/src/test/btcsignals_tests.cpp
@@ -9,2 +9,4 @@

+#include <semaphore>
+
 namespace {
@@ -142,3 +144,3 @@ BOOST_AUTO_TEST_CASE(return_value)
  * Though the end-result is undefined due to a non-deterministic number of
- * total callbacks excecuted, this should all be completely threadsafe.
+ * total callbacks executed, this should all be completely threadsafe.
  * Sanitizers should pick up any buggy data race behavior (if present).
@@ -255,4 +257,4 @@ BOOST_AUTO_TEST_CASE(disconnect_thread_safety)
     btcsignals::signal<void(int&)> sig0;
-    std::atomic_bool wait1{false};
-    std::atomic_bool wait2{false};
+    std::binary_semaphore done1{0};
+    std::binary_semaphore done2{0};
     int val{0};
@@ -261,5 +263,4 @@ BOOST_AUTO_TEST_CASE(disconnect_thread_safety)
         conn1.disconnect();
-        wait1.store(true);
-        wait1.notify_all();
-        wait2.wait(false);
+        done1.release();
+        done2.acquire();
     });
@@ -268,6 +269,5 @@ BOOST_AUTO_TEST_CASE(disconnect_thread_safety)
     std::thread thr([&] {
-        wait1.wait(false);
+        done1.acquire();
         conn2.disconnect();
-        wait2.store(true);
-        wait2.notify_all();
+        done2.release();
     });

</details>

DrahtBot added the label CI failed on Apr 3, 2026

theuni commented at 6:36 PM on April 3, 2026: member

Pushed an additional commit with @maflcko's suggestion.

Reviewers can first check the squashed changes in the first push, then this new commit independently.

fjahr commented at 7:30 PM on April 3, 2026: contributor

Code review ACK 242b0ebb5ca881053f8c79d1f0ae7a32207f008e

I reviewed the changes in the old commit and confirmed they only address review comments. I also looked at the last, new commit and it seems like a nice simplification. I can quickly re-ack if this is squashed but I don't insist on squashing.

DrahtBot requested review from maflcko on Apr 3, 2026

DrahtBot requested review from w0xlt on Apr 3, 2026

DrahtBot removed the label CI failed on Apr 3, 2026

in src/test/btcsignals_tests.cpp:24 in 63c68e2a3f

  19 | +    MoveOnlyData(MoveOnlyData&&) = default;
  20 | +
  21 | +#if BOOST_MINOR_VERSION <= 85
  22 | +    // Boost::signals2 <= 1.85 required copyable return types
  23 | +    MoveOnlyData(const MoveOnlyData&) = default;
  24 | +    MoveOnlyData& operator=(const MoveOnlyData&) = default;

davidgumberg commented at 10:55 PM on April 8, 2026:

(In https://github.com/bitcoin/bitcoin/pull/34495/changes/63c68e2a3f98d2466a7e766d861ba3a94e92cd20 signals: add signals tests):

Just a remark for other reviewers:

https://github.com/boostorg/signals2/commit/83d10662a494d3cdbeeb30d33026cdc500bf3dd8 in Boost 1.86 (https://github.com/boostorg/signals2/compare/boost-1.85.0...boost-1.86.0) wrapped the assignment to the return value of optional_last_value() in move_if_not_lvalue_reference (~= std::forward)

 template<typename InputIterator>
   optional<T> operator()(InputIterator first, InputIterator last) const
 {
   optional<T> value;
   while (first != last)
   {
     BOOST_TRY
     {
-      value = *first;
+      value = boost::move_if_not_lvalue_reference<T>(*first);
     }
     BOOST_CATCH(const expired_slot &) {}
     BOOST_CATCH_END
     ++first;
   }
   return value;
 }

This means that signal callbacks can return types which have no copy-assignment/copy-ctor and have move assignment defined after 1.86 and conversely that before 1.86 return types must have copy assignment defined.

in src/test/btcsignals_tests.cpp:23 in 63c68e2a3f

  18 | +    MoveOnlyData(int data) : m_data(data) {}
  19 | +    MoveOnlyData(MoveOnlyData&&) = default;
  20 | +
  21 | +#if BOOST_MINOR_VERSION <= 85
  22 | +    // Boost::signals2 <= 1.85 required copyable return types
  23 | +    MoveOnlyData(const MoveOnlyData&) = default;

davidgumberg commented at 10:58 PM on April 8, 2026:

(In https://github.com/bitcoin/bitcoin/pull/34495/changes/63c68e2a3f98d2466a7e766d861ba3a94e92cd20 signals: add signals tests):

~~(nit, not worth changing unless retouching and even then...)~~

~~I haven't tested but if I'm not mistaken the copy constructor can be deleted since only copy assignment is used.~~

~~(https://github.com/boostorg/signals2/blob/50c5b537edda006042c8c9572ed062d5fd083cf6/include/boost/signals2/optional_last_value.hpp#L35)~~

davidgumberg commented at 11:22 PM on April 8, 2026:

Nope, this was totally confused and wrong, I did not understand that if an optional is not initialized, the copy constructor is used, and after initialized, the assignment operator is used, sorry!

<summary> example </summary>

#include <iostream>
#include <optional>

struct Book {
    Book() {}

    Book(const Book&) { 
        std::cout << "copy ctor\n"; 
    }

    Book& operator=(const Book&) { 
        std::cout << "copy assign\n"; 
        return *this; 
    }
};

int main() {
    Book source;
    std::optional<Book> val;

    std::cout << "Uninitialized: ";
    val = source; 

    std::cout << "Initialized: ";
    val = source; 
}

Uninitialized: copy ctor
Initialized: copy assign

</details>

https://godbolt.org/z/T46hYoes9

w0xlt commented at 11:13 PM on April 8, 2026: contributor

reACK 242b0ebb5ca881053f8c79d1f0ae7a32207f008e

in src/test/btcsignals_tests.cpp:29 in 63c68e2a3f

  24 | +    MoveOnlyData& operator=(const MoveOnlyData&) = default;
  25 | +#elif BOOST_MINOR_VERSION <= 90
  26 | +    // Boost::signals2 <= 1.90 required move-assignable return types
  27 | +    MoveOnlyData& operator=(MoveOnlyData&&) = default;
  28 | +    MoveOnlyData(const MoveOnlyData&) = delete;
  29 | +    MoveOnlyData& operator=(const MoveOnlyData&) = delete;

davidgumberg commented at 11:23 PM on April 8, 2026:

(In https://github.com/bitcoin/bitcoin/pull/34495/changes/63c68e2a3f98d2466a7e766d861ba3a94e92cd20 signals: add signals tests):

Just a remark for other reviewers:

https://github.com/boostorg/signals2/commit/80f52e7335f331538e2aec18dc725530b5609971 in Boost 1.91.0 (https://github.com/boostorg/signals2/compare/boost-1.90.0...boost-1.91.0.beta1) changed optional_last_value's to use move construction instead of move assignment:

- cache->result = cache->f(*iter);
+ cache->result.emplace(cache->f(*iter));

A move constructor was already required so this relaxes the requirements to only need a move constructor defined and nothing else.

maflcko commented at 8:09 AM on April 9, 2026: member

lgtm

Looks like one nit was forgotten: #34495 (review) There are also some follow-ups left for a separate later pull request:

re-review ACK 242b0ebb5ca881053f8c79d1f0ae7a32207f008e 🎯

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-review ACK 242b0ebb5ca881053f8c79d1f0ae7a32207f008e 🎯
U8WnA5mH5LC0E0jXtZLDyXgX9ryvRGgk/EWc3uUzlGoAKkKVxrhFn54iBOKNbTHFjmyplqJyl59XmIMAxRRJCA==

</details>

fanquake merged this on Apr 9, 2026

fanquake closed this on Apr 9, 2026

in src/btcsignals.h:34 in 242b0ebb5c

  29 | + */
  30 | +
  31 | +namespace btcsignals {
  32 | +
  33 | +/*
  34 | + * optional_last_value is the default and only supported combiner.

maflcko commented at 7:51 PM on April 9, 2026:

A bit unrelated, but I think this is brittle and confusing, and only works by accident. If the connection order changed, the only place that needs it would silently break, because we don't have any GUI tests.

I think it would be best to remove optional_last_value, to avoid more brittle and confusing code to pop up in the future.

I know it is a pre-existing and unrelated problem, but I wanted to leave a reminder for myself.

This patch is enough to silently break the GUI:

diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index 0b89c605b9..976549470e 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -488,3 +488,2 @@ int GuiMain(int argc, char* argv[])
     btcsignals::scoped_connection handler_message_box = ::uiInterface.ThreadSafeMessageBox_connect(noui_ThreadSafeMessageBox);
-    btcsignals::scoped_connection handler_question = ::uiInterface.ThreadSafeQuestion_connect(noui_ThreadSafeQuestion);
     btcsignals::scoped_connection handler_init_message = ::uiInterface.InitMessage_connect(noui_InitMessage);
@@ -663,2 +662,3 @@ int GuiMain(int argc, char* argv[])
         app.createWindow(networkStyle.data());
+    btcsignals::scoped_connection handler_question = ::uiInterface.ThreadSafeQuestion_connect(noui_ThreadSafeQuestion);
         // Perform base initialization before spinning up initialization/shutdown thread

maflcko commented at 7:52 PM on April 9, 2026:

Removed in https://github.com/bitcoin/bitcoin/pull/35043

hebasto commented at 8:42 AM on April 10, 2026: member

This PR introduced new -Wfree-nonheap-object warnings when cross-compiling for Windows on Ubuntu 24.04 LTS or Ubuntu 26.04 LTS:

$ x86_64-w64-mingw32-g++-posix --version
x86_64-w64-mingw32-g++-posix (GCC) 13-posix
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ gmake -j 16 -C depends HOST=x86_64-w64-mingw32 NO_QT=1
$ cmake -B build --toolchain depends/x86_64-w64-mingw32/toolchain.cmake
$ cmake --build build > /dev/null
In file included from /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/x86_64-w64-mingw32/bits/c++allocator.h:33,
                 from /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/allocator.h:46,
                 from /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/alloc_traits.h:39,
                 from /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/ext/alloc_traits.h:34,
                 from /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/hashtable_policy.h:39,
                 from /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/hashtable.h:35,
                 from /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/unordered_map.h:33,
                 from /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/unordered_map:41,
                 from /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/functional:63,
                 from /bitcoin/src/node/interface_ui.h:10,
                 from /bitcoin/src/node/interface_ui.cpp:5:
In member function 'void std::__new_allocator<_Tp>::deallocate(_Tp*, size_type) [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>]',
    inlined from 'constexpr void std::allocator< <template-parameter-1-1> >::deallocate(_Tp*, std::size_t) [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/allocator.h:210:35,
    inlined from 'static constexpr void std::allocator_traits<std::allocator<_Up> >::deallocate(allocator_type&, pointer, size_type) [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/alloc_traits.h:516:23,
    inlined from 'constexpr void std::_Vector_base<_Tp, _Alloc>::_M_deallocate(pointer, std::size_t) [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>; _Alloc = std::allocator<std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder> >]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/stl_vector.h:387:19,
    inlined from 'constexpr std::_Vector_base<_Tp, _Alloc>::~_Vector_base() [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>; _Alloc = std::allocator<std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder> >]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/stl_vector.h:366:15,
    inlined from 'constexpr std::vector<_Tp, _Alloc>::~vector() [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>; _Alloc = std::allocator<std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder> >]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/stl_vector.h:735:7,
    inlined from 'btcsignals::signal<Signature, Combiner>::result_type btcsignals::signal<Signature, Combiner>::operator()(Args&& ...) const [with Args = {const bilingual_str&, unsigned int&}; Signature = bool(const bilingual_str&, unsigned int); Combiner = btcsignals::optional_last_value<bool>]' at /bitcoin/src/btcsignals.h:225:5,
    inlined from 'bool CClientUIInterface::ThreadSafeMessageBox(const bilingual_str&, unsigned int)' at /bitcoin/src/node/interface_ui.cpp:48:139:
/usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/new_allocator.h:168:33: warning: 'void operator delete(void*, std::size_t)' called on pointer '<unknown>' with nonzero offset [1, 9223372036854775792] [-Wfree-nonheap-object]
  168 |         _GLIBCXX_OPERATOR_DELETE(_GLIBCXX_SIZED_DEALLOC(__p, __n));
      |                                 ^
/usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/new_allocator.h: In member function 'bool CClientUIInterface::ThreadSafeMessageBox(const bilingual_str&, unsigned int)':
/usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/new_allocator.h:147:55: note: returned from 'void* operator new(std::size_t)'
  147 |         return static_cast<_Tp*>(_GLIBCXX_OPERATOR_NEW(__n * sizeof(_Tp)));
      |                                                       ^
In member function 'void std::__new_allocator<_Tp>::deallocate(_Tp*, size_type) [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>]',
    inlined from 'constexpr void std::allocator< <template-parameter-1-1> >::deallocate(_Tp*, std::size_t) [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/allocator.h:210:35,
    inlined from 'static constexpr void std::allocator_traits<std::allocator<_Up> >::deallocate(allocator_type&, pointer, size_type) [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/alloc_traits.h:516:23,
    inlined from 'constexpr void std::_Vector_base<_Tp, _Alloc>::_M_deallocate(pointer, std::size_t) [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>; _Alloc = std::allocator<std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder> >]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/stl_vector.h:387:19,
    inlined from 'constexpr std::_Vector_base<_Tp, _Alloc>::~_Vector_base() [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>; _Alloc = std::allocator<std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder> >]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/stl_vector.h:366:15,
    inlined from 'constexpr std::vector<_Tp, _Alloc>::~vector() [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>; _Alloc = std::allocator<std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder> >]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/stl_vector.h:735:7,
    inlined from 'btcsignals::signal<Signature, Combiner>::result_type btcsignals::signal<Signature, Combiner>::operator()(Args&& ...) const [with Args = {const bilingual_str&, unsigned int&}; Signature = bool(const bilingual_str&, unsigned int); Combiner = btcsignals::optional_last_value<bool>]' at /bitcoin/src/btcsignals.h:225:5,
    inlined from 'bool CClientUIInterface::ThreadSafeMessageBox(const bilingual_str&, unsigned int)' at /bitcoin/src/node/interface_ui.cpp:48:139:
/usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/new_allocator.h:168:33: warning: 'void operator delete(void*, std::size_t)' called on pointer '<unknown>' with nonzero offset [1, 9223372036854775792] [-Wfree-nonheap-object]
  168 |         _GLIBCXX_OPERATOR_DELETE(_GLIBCXX_SIZED_DEALLOC(__p, __n));
      |                                 ^
/usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/new_allocator.h: In member function 'bool CClientUIInterface::ThreadSafeMessageBox(const bilingual_str&, unsigned int)':
/usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/new_allocator.h:147:55: note: returned from 'void* operator new(std::size_t)'
  147 |         return static_cast<_Tp*>(_GLIBCXX_OPERATOR_NEW(__n * sizeof(_Tp)));
      |                                                       ^
In member function 'void std::__new_allocator<_Tp>::deallocate(_Tp*, size_type) [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>]',
    inlined from 'constexpr void std::allocator< <template-parameter-1-1> >::deallocate(_Tp*, std::size_t) [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/allocator.h:210:35,
    inlined from 'static constexpr void std::allocator_traits<std::allocator<_Up> >::deallocate(allocator_type&, pointer, size_type) [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/alloc_traits.h:516:23,
    inlined from 'constexpr void std::_Vector_base<_Tp, _Alloc>::_M_deallocate(pointer, std::size_t) [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>; _Alloc = std::allocator<std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder> >]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/stl_vector.h:387:19,
    inlined from 'constexpr std::_Vector_base<_Tp, _Alloc>::~_Vector_base() [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>; _Alloc = std::allocator<std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder> >]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/stl_vector.h:366:15,
    inlined from 'constexpr std::vector<_Tp, _Alloc>::~vector() [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>; _Alloc = std::allocator<std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder> >]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/stl_vector.h:735:7,
    inlined from 'btcsignals::signal<Signature, Combiner>::result_type btcsignals::signal<Signature, Combiner>::operator()(Args&& ...) const [with Args = {const bilingual_str&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, unsigned int&}; Signature = bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int); Combiner = btcsignals::optional_last_value<bool>]' at /bitcoin/src/btcsignals.h:225:5,
    inlined from 'bool CClientUIInterface::ThreadSafeQuestion(const bilingual_str&, const std::string&, unsigned int)' at /bitcoin/src/node/interface_ui.cpp:49:179:
/usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/new_allocator.h:168:33: warning: 'void operator delete(void*, std::size_t)' called on pointer '<unknown>' with nonzero offset [1, 9223372036854775792] [-Wfree-nonheap-object]
  168 |         _GLIBCXX_OPERATOR_DELETE(_GLIBCXX_SIZED_DEALLOC(__p, __n));
      |                                 ^
/usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/new_allocator.h: In member function 'bool CClientUIInterface::ThreadSafeQuestion(const bilingual_str&, const std::string&, unsigned int)':
/usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/new_allocator.h:147:55: note: returned from 'void* operator new(std::size_t)'
  147 |         return static_cast<_Tp*>(_GLIBCXX_OPERATOR_NEW(__n * sizeof(_Tp)));
      |                                                       ^
In member function 'void std::__new_allocator<_Tp>::deallocate(_Tp*, size_type) [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>]',
    inlined from 'constexpr void std::allocator< <template-parameter-1-1> >::deallocate(_Tp*, std::size_t) [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/allocator.h:210:35,
    inlined from 'static constexpr void std::allocator_traits<std::allocator<_Up> >::deallocate(allocator_type&, pointer, size_type) [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/alloc_traits.h:516:23,
    inlined from 'constexpr void std::_Vector_base<_Tp, _Alloc>::_M_deallocate(pointer, std::size_t) [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>; _Alloc = std::allocator<std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder> >]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/stl_vector.h:387:19,
    inlined from 'constexpr std::_Vector_base<_Tp, _Alloc>::~_Vector_base() [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>; _Alloc = std::allocator<std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder> >]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/stl_vector.h:366:15,
    inlined from 'constexpr std::vector<_Tp, _Alloc>::~vector() [with _Tp = std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder>; _Alloc = std::allocator<std::shared_ptr<btcsignals::signal<bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int), btcsignals::optional_last_value<bool> >::connection_holder> >]' at /usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/stl_vector.h:735:7,
    inlined from 'btcsignals::signal<Signature, Combiner>::result_type btcsignals::signal<Signature, Combiner>::operator()(Args&& ...) const [with Args = {const bilingual_str&, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, unsigned int&}; Signature = bool(const bilingual_str&, const std::__cxx11::basic_string<char>&, unsigned int); Combiner = btcsignals::optional_last_value<bool>]' at /bitcoin/src/btcsignals.h:225:5,
    inlined from 'bool CClientUIInterface::ThreadSafeQuestion(const bilingual_str&, const std::string&, unsigned int)' at /bitcoin/src/node/interface_ui.cpp:49:179:
/usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/new_allocator.h:168:33: warning: 'void operator delete(void*, std::size_t)' called on pointer '<unknown>' with nonzero offset [1, 9223372036854775792] [-Wfree-nonheap-object]
  168 |         _GLIBCXX_OPERATOR_DELETE(_GLIBCXX_SIZED_DEALLOC(__p, __n));
      |                                 ^
/usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/new_allocator.h: In member function 'bool CClientUIInterface::ThreadSafeQuestion(const bilingual_str&, const std::string&, unsigned int)':
/usr/lib/gcc/x86_64-w64-mingw32/13-posix/include/c++/bits/new_allocator.h:147:55: note: returned from 'void* operator new(std::size_t)'
  147 |         return static_cast<_Tp*>(_GLIBCXX_OPERATOR_NEW(__n * sizeof(_Tp)));
      |                                                       ^

maflcko commented at 8:53 AM on April 10, 2026: member

This PR introduced a new -Wfree-nonheap-object warning when cross-compiling for Windows on Ubuntu 24.04:

I presume this false-positive only happens with GCC 13, and is fixed in GCC 14, which is used by CI and Guix?

Also, #35043 moves the code into the header, so it may go away with that?

Otherwise, I think you can ignore it, or report it upstream to GCC.

hebasto commented at 9:06 AM on April 10, 2026: member

This PR introduced a new -Wfree-nonheap-object warning when cross-compiling for Windows on Ubuntu 24.04:

Also, #35043 moves the code into the header, so it may go away with that?

Tested #35043 on Ubuntu 26.04. The warnings have gone.

ryanofsky referenced this in commit 6062981c59 on Apr 10, 2026

ryanofsky referenced this in commit 8ee6b745bb on Apr 10, 2026

maflcko commented at 1:03 PM on April 15, 2026: member

reduced to https://godbolt.org/z/Gac1oeM5P (not sure if this is the same trigger, though)

it goes away with -O1, and I guess it is not strictly a bug, just gcc being too eager to complain.

The cvise script I used:

#!/bin/bash
export LC_ALL=C
COMPILER="/bin/x86_64-w64-mingw32-g++-posix"
FLAGS="-std=c++20 -Wfree-nonheap-object -c"

# 1. MUST be clean at -O1
# If the warning appears at -O1, it might be a "real" bug or a different logic path.
# We want the O2-specific optimizer bug.
if $COMPILER -O1 $FLAGS reduced.ii -o /dev/null 2>&1 | grep -q "Wfree-nonheap-object"; then
  exit 1
fi

# 2. MUST trigger the warning at -O2 with the "impossible" range.
# We grep for a significant part of that massive number.
# Using '9223372036854775' covers the core of the 64-bit max value.
if $COMPILER -O2 $FLAGS reduced.ii -o /dev/null 2>&1 | grep -q -F "with nonzero offset [1, 9223372036854775" ; then
  exit 0
else
  exit 1
fi