refactor: Properly return from ThreadSafeQuestion signal + btcsignals follow-ups #35043

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

Previously, the ThreadSafeQuestion signal was using btcsignals::optional_last_value<bool>. However, this only worked by accident:

Calling CClientUIInterface::ThreadSafeQuestion did not return an std::optional<bool>, but value_or(false). This makes it hard for callers to differentiate between nullopt and false.
The return value was further influenced by the order in which the connections were done. The noui callbacks would always overwrite the return value with false. This makes the code overall brittle, and confusing.

For example, the following patch that changes the order of connections would break the only and single place where the return value actually matters:

diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index https://github.com/bitcoin/bitcoin/commit/0b89c605b9ce69abcce88ac5a88699c353418078..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

This can be tested by applying the patch and then calling:

(May have to be started twice to trigger the question)

bitcoin-qt -regtest -datadir=/tmp -mocktime=123456789

Before the changes in this commit (on current master), pressing OK would not have any effect and would abort the program.

After the changes in this commit, pressing OK will correctly trigger a -reindex and leave the program running.

So fix that by properly returning from the signal.

Also, remove the then-unused btcsignals::optional_last_value<T> combiner.

Also, other follow-ups from #34495 (comment)

DrahtBot added the label Refactoring on Apr 9, 2026

DrahtBot commented at 7:52 PM on April 9, 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
Concept ACK	hebasto

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:

#34806 (refactor: logging: Various API improvements by ajtowns)
#34435 (refactor: use _MiB/_GiB consistently for byte conversions by l0rinc)
#33117 (Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications by D33r-Gee)
#32489 (wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)

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.

maflcko force-pushed on Apr 9, 2026

DrahtBot added the label CI failed on Apr 9, 2026

maflcko force-pushed on Apr 9, 2026

DrahtBot removed the label CI failed on Apr 9, 2026

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

Concept ACK. This fixes the -Wfree-nonheap-object warnings when cross-compiling for Windows on Ubuntu 24.04 or 26.04.

maflcko added this to the milestone 32.0 on Apr 10, 2026

maflcko added the label DrahtBot Guix build requested on Apr 10, 2026

theuni commented at 10:07 PM on April 10, 2026: member

Thanks for the refactors, you beat me to it.

I don't understand this comment though:

The return value was further influenced by the order in which the connections were done. The noui callbacks would always overwrite the return value with false. This makes the code overall brittle, and confusing.

The optional_last_value combiner returns the value from the last callback. That seems pretty straightforward to me.

If I understand correctly, after these changes, ThreadSafeQuestion gets a bool reference as a parameter. But what happens if there are two registered callbacks which set it? Doesn't it get overwritten the same way? It seems that's just dropping the Combiner concept which is designed to solve this problem, and instead relying on knowing something about how the callers will behave.

I think what we really want is an optional_any_of Combiner, no? See for example (untested): https://github.com/theuni/bitcoin/commit/a168547705e0cfa033e7f009de09f6338bb78d2d

DrahtBot commented at 5:12 AM on April 13, 2026: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit 94f1d35145c534204bf88056bfa249838aac23f1<br>(master)	commit de45b1e3a95133f197a5cd155a0721243c7a01fa<br>(pull/35043/merge)
*-aarch64-linux-gnu-debug.tar.gz	`b31995308142d132...`	`a2f5977e5615010e...`
*-aarch64-linux-gnu.tar.gz	`9ad2ae3bfd52537b...`	`6cc1a2a6448b6213...`
*-arm-linux-gnueabihf-debug.tar.gz	`dae6d49d167f7548...`	`7fd7a98358b80d46...`
*-arm-linux-gnueabihf.tar.gz	`feaf11a4db1c4162...`	`b0073b2480852a4b...`
*-arm64-apple-darwin-codesigning.tar.gz	`4539e3c15975f540...`	`4c318090c194538b...`
*-arm64-apple-darwin-unsigned.tar.gz	`549d74870c75ee00...`	`4fc8ca17af8c762b...`
*-arm64-apple-darwin-unsigned.zip	`888366cd521134af...`	`9ca4e1e441e79ced...`
*-powerpc64-linux-gnu-debug.tar.gz	`8941927ead97e3e5...`	`426ef3d01db46f5e...`
*-powerpc64-linux-gnu.tar.gz	`e84789ed1172ede6...`	`4520c355fd801523...`
*-riscv64-linux-gnu-debug.tar.gz	`3988c0a58b47e82a...`	`fe912c2406a1f1f2...`
*-riscv64-linux-gnu.tar.gz	`33b54f4f61da2a9f...`	`b3a2c1ddb7ce82ff...`
*-win64-codesigning.tar.gz	`968149ac365ad80e...`	`a506702204182964...`
*-win64-debug.zip	`c98d5df06ecf8207...`	`a25439a4edd36642...`
*-win64-setup-unsigned.exe	`034a0791202b2904...`	`d07ea7786017aad9...`
*-win64-unsigned.zip	`f8c491eb9db6352c...`	`896869f5bea34ea8...`
*-x86_64-apple-darwin-codesigning.tar.gz	`9ec52e97b7ededfb...`	`fea40c79051ce0dd...`
*-x86_64-apple-darwin-unsigned.tar.gz	`8f7d3c7471634e30...`	`c398910823fb7aab...`
*-x86_64-apple-darwin-unsigned.zip	`49a44d8fc5f5784e...`	`4f01099d1c39f9d5...`
*-x86_64-linux-gnu-debug.tar.gz	`6735530a39cb4e6f...`	`b4e64bafb879bf21...`
*-x86_64-linux-gnu.tar.gz	`a7c017a693f665b5...`	`e934b1c19676de7e...`
*.tar.gz	`71d8c3029bb8a0e3...`	`63dcc69c96aff3b0...`
SHA256SUMS.part	`c5f9c7ff9ffffbac...`	`0f02aa83a379156e...`
guix_build.log	`0676b9d797868475...`	`2a761d9c9cc75ab2...`
guix_build.log.diff		`3479c68bde712d2b...`

DrahtBot removed the label DrahtBot Guix build requested on Apr 13, 2026

maflcko commented at 5:49 AM on April 13, 2026: member

The optional_last_value combiner returns the value from the last callback. That seems pretty straightforward to me.

Yes, there is no problem with the concept of the combiner itself in isolation. The confusion arises how it is currently used in master.

If I understand correctly, after these changes, ThreadSafeQuestion gets a bool reference as a parameter. But what happens if there are two registered callbacks which set it? Doesn't it get overwritten the same way? It seems that's just dropping the Combiner concept which is designed to solve this problem, and instead relying on knowing something about how the callers will behave.

Yes, but for Bitcoin Core, there is no way to provide a result to a question the daemon asked, because it runs in the background. Really, the only place that could possibly be interactive is the GUI, so I think it is clearer to explicitly document where the return value is meaningless/ignored, instead of "hiding" it inside the signature of the signal.

I think what we really want is an optional_any_of Combiner, no? See for example (untested): theuni@a168547

Yes, that'd be an alternative. Though, I have a hard time seeing a valid use-case for the optional_last_value combiner, since there isn't one in current master, and never has been in Bitcoin Core. So my preference would still be to remove it. Also, the optional_any_of seems a bit too generic, given we only need to ask if a single bool in a single place was set. So I think the combiner should be called any_of, call operator bool and return bool.

Happy to push that, though I still minimally prefer my version, which explicitly documents where the return value is ignored (bitcoind, noui), and documents where it is set (gui).

theuni commented at 3:57 PM on April 16, 2026: member

Yes, that'd be an alternative. Though, I have a hard time seeing a valid use-case for the optional_last_value combiner, since there isn't one in current master, and never has been in Bitcoin Core. So my preference would still be to remove it.

I agree with this. optional_last_value seems pretty useless. The only reason I ported it it to btcsignals is because it's the default for boost.

I think there's a compromise here: how about removing optional_last_value and making a null_value the default, which only works for functions which return void. That way anything that returns void continues to work as-is, and anything that returns non-void has to specify a combiner rather than having a default provided. Imo that's what would've made the most sense for boost as well.

Also, the optional_any_of seems a bit too generic, given we only need to ask if a single bool in a single place was set. So I think the combiner should be called any_of, call operator bool and return bool.

The optional part disambiguates the case where no return values generated because there were no active callbacks. I suppose the only reason we'd need that in this case would be to assert(val) for sanity. I don't feel strongly either way.

I used the generic iterator interface above in order to match boost's: https://www.boost.org/doc/libs/latest/doc/html/signals2/rationale.html#id-1.3.34.9.5

I agree that it's overkill in this case (as it requires us to construct a vector), but I do think we want to expose some sort of generic interface. Maybe we could hard-code and special-case for any_of the way we currently do for optional_last_value? Tbh I'm not worried about the generic version, though. A single vector in the scheme of things is not the end of the world :)

Happy to push that, though I still minimally prefer my version, which explicitly documents where the return value is ignored (bitcoind, noui), and documents where it is set (gui).

Imo the combiner approach is equally explicit: bitcoind/noui return false to the ThreadSafeQuestion unless the answer is true. So I'd prefer that as it keeps the combiner concept intact and useful.

refactor: Make ThreadSafeMessageBox signal void

The message will always return false (a constant) and the return value
is never used.

fae5e1d6fb

maflcko force-pushed on Apr 16, 2026

refactor: Properly return from ThreadSafeQuestion signal

Previously, the signal was using btcsignals::optional_last_value<bool>.
However, this only worked by accident:

The return value was influenced by the order in which the connections
were done. The noui callbacks would always overwrite the return value
with false. This makes the code overall brittle, and confusing.

For example, the following patch that changes the order of connections
would break the only and single place where the return value actually
matters:

```diff
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
```

This can be tested by applying the patch and then calling:

(May have to be started twice to trigger the question)

```
bitcoin-qt -regtest -datadir=/tmp -mocktime=123456789
```

Before the changes in this commit (on current master), pressing `OK`
would not have any effect and would abort the program.

After the changes in this commit, pressing `OK` will correctly trigger a
-reindex and leave the program running.

fac8b5cd31

test: Check btcsignals determinism in thread_safety test case

The test only checked that the single atomic value is greater than 3000.
However, by splitting the atomic into two, one can do one exact check,
and also increase the lower bound on the inexact check.

Also, test disconnect races for every second step, instead of only once
at the end (likely when only one thread is running anyway).

Both changes make the test stricter and may catch non-determinism issues
that are not detected by sanitizers alone.

The test added in this commit should also pass when applied on top of
commit 63c68e2a3f98d2466a7e766d861ba3a94e92cd20, which is still using
the boost implementation.

fa8ef6ba2f

refactor: Make scoped_connection ctor explicit

This ensures that all constructions of scoped connections are explict,
making the intent explicit and review easier. This also follows the
default recommendation from
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c46-by-default-declare-single-argument-constructors-explicit

fa9e24f5a2

mv btcsignals.h to src/util

This is a low-level utility header

fad6c11751

undo "ui: Compile boost:signals2 only once"

commit fa5ce27385bc60cdf6d9a4eeb2d32c916c9e07eb was done to gain a 5%
faster compilation. However, this is no longer needed after the slim
btcsignals implementation.

fabb937fbc

maflcko force-pushed on Apr 16, 2026

DrahtBot added the label CI failed on Apr 16, 2026

maflcko commented at 6:52 PM on April 16, 2026: member

making a null_value the default

thx, done

I suppose the only reason we'd need that in this case would be to assert(val) for sanity.

Ah, interesting. Could consider adding it in the future, if it isn't dead code. (Right now there is only a single line/place that uses the return value, and it is dead code in tests, because there are no gui tests and they have to be done manually anyway.)

I used the generic iterator interface above in order to match boost's

Right, but I left it as it is in current master for now, because it is less code and I don't see a future where we'd need more than than. However, if someone writes the extra code and shares a diff, I am happy to push and squash it here.

Imo the combiner approach is equally explicit: bitcoind/noui return false to the ThreadSafeQuestion unless the answer is true. So I'd prefer that as it keeps the combiner concept intact and useful.

Ok, makes sense. Force pushed this.

DrahtBot removed the label CI failed on Apr 16, 2026