refactor: Properly return from ThreadSafeQuestion signal
What the developer wrote
Authored by MarcoFalke
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.
What changed, and why it matters
This commit fixes a brittle signal-handling bug in Bitcoin Core's user-interface code. Previously, when the program asked the user a yes/no question (for example, whether to rebuild the database), the answer could be ignored depending on the order in which internal callbacks were connected. The commit changes the signal system so that a 'yes' answer from any relevant handler is respected, rather than being overwritten by a later handler. The commit message explicitly notes that, before the fix, clicking 'OK' could abort the program instead of performing the requested recovery.
Treat this as a reliability fix with possible denial-of-service or recovery-failure implications. Users and node operators should upgrade to a release containing this commit, especially those running bitcoin-qt, because the bug can prevent automatic database recovery from proceeding when the GUI is used. Reviewers should verify that no other signals rely on the removed optional_last_value semantics.
Security signals we found
Logic bug in signal combiner caused return value to depend on connection order
GUI 'OK' response to a recovery question could be discarded, aborting instead of reindexing
Non-interactive callback silently overrode interactive callback's return value
Fix changes combiner semantics from last-value-wins to any-true-wins
Commit message describes a reproducible failure mode (-regtest -mocktime scenario)
Evidence from the diff
The patch refactors the custom btcsignals signal/slot implementation. It removes the optional_last_value combiner (which returned the last callback’s result, or std::nullopt) and replaces it with two explicit combiners: null_value for void signals and any_of for bool signals. The ThreadSafeQuestion signal is switched to any_of, so the overall result is the logical OR of all connected callbacks. CClientUIInterface::ThreadSafeQuestion no longer falls back to false on nullopt. Unit tests are updated to assert OR semantics. The commit message states the old behavior was order-dependent and that the noui (non-interactive) callback would overwrite the GUI callback’s true return with false, breaking the only caller that depends on the return value.
Changed components
src/btcsignals.hsrc/node/interface_ui.cppsrc/test/btcsignals_tests.cppCClientUIInterface::ThreadSafeQuestionBitcoin-Qt GUI initialization flowInspect captured patch +34 / −60
diff --git a/src/btcsignals.h b/src/btcsignals.h
index 94625edd..e0df8ae8 100644
--- a/src/btcsignals.h
+++ b/src/btcsignals.h
@@ -30,21 +30,23 @@
namespace btcsignals {
-/*
- * optional_last_value is the default and only supported combiner.
- * As such, its behavior is embedded into the signal functor.
- *
- * Because optional<void> is undefined, void must be special-cased.
- */
+/// The default combiner, which only returns void.
+class null_value
+{
+public:
+ using result_type = void;
+};
-template <typename T>
-class optional_last_value
+/// A combiner, which checks if at least one callback returned true.
+class any_of
{
public:
- using result_type = std::conditional_t<std::is_void_v<T>, void, std::optional<T>>;
+ // This is the only supported combiner with a non-void return type. As
+ // such, its behavior is embedded into the signal functor.
+ using result_type = bool;
};
-template <typename Signature, typename Combiner = optional_last_value<typename std::function<Signature>::result_type>>
+template <typename Signature, typename Combiner = null_value>
class signal;
/*
@@ -150,8 +152,6 @@ class signal
{
using function_type = std::function<Signature>;
- static_assert(std::is_same_v<Combiner, optional_last_value<typename function_type::result_type>>, "only the optional_last_value combiner is supported");
-
/*
* Helper struct for maintaining a callback and its associated connection liveness
*/
@@ -184,9 +184,7 @@ public:
/*
* Execute all enabled callbacks for the signal. Rather than allowing for
- * custom combiners, the behavior of optional_last_value is hard-coded
- * here. Return the value of the last executed callback, or nullopt if none
- * were executed.
+ * custom combiners, the behavior of any_of is hard-coded here.
*
* Callbacks which return void require special handling.
*
@@ -208,16 +206,22 @@ public:
connections = m_connections;
}
if constexpr (std::is_void_v<result_type>) {
+ static_assert(std::is_same_v<result_type, typename function_type::result_type>,
+ "Callback result type must be equal to the combiner result type (void).");
for (const auto& connection : connections) {
if (connection->connected()) {
connection->m_callback(args...);
}
}
} else {
- result_type ret{std::nullopt};
+ static_assert(std::is_same_v<Combiner, any_of>,
+ "only the any_of combiner is supported and hard-coded into this functor.");
+ static_assert(std::is_same_v<result_type, typename function_type::result_type>,
+ "Callback result type must be equal to the combiner result type (bool).");
+ result_type ret{false};
for (const auto& connection : connections) {
if (connection->connected()) {
- ret.emplace(connection->m_callback(args...));
+ ret |= connection->m_callback(args...);
}
}
return ret;
diff --git a/src/node/interface_ui.cpp b/src/node/interface_ui.cpp
index 54b267c8..c80c2902 100644
--- a/src/node/interface_ui.cpp
+++ b/src/node/interface_ui.cpp
@@ -14,7 +14,7 @@ CClientUIInterface uiInterface;
struct UISignals {
btcsignals::signal<CClientUIInterface::ThreadSafeMessageBoxSig> ThreadSafeMessageBox;
- btcsignals::signal<CClientUIInterface::ThreadSafeQuestionSig, btcsignals::optional_last_value<bool>> ThreadSafeQuestion;
+ btcsignals::signal<CClientUIInterface::ThreadSafeQuestionSig, btcsignals::any_of> ThreadSafeQuestion;
btcsignals::signal<CClientUIInterface::InitMessageSig> InitMessage;
btcsignals::signal<CClientUIInterface::InitWalletSig> InitWallet;
btcsignals::signal<CClientUIInterface::NotifyNumConnectionsChangedSig> NotifyNumConnectionsChanged;
@@ -46,7 +46,7 @@ ADD_SIGNALS_IMPL_WRAPPER(NotifyHeaderTip);
ADD_SIGNALS_IMPL_WRAPPER(BannedListChanged);
void CClientUIInterface::ThreadSafeMessageBox(const bilingual_str& message, unsigned int style) { return g_ui_signals.ThreadSafeMessageBox(message, style); }
-bool CClientUIInterface::ThreadSafeQuestion(const bilingual_str& message, const std::string& non_interactive_message, unsigned int style) { return g_ui_signals.ThreadSafeQuestion(message, non_interactive_message, style).value_or(false);}
+bool CClientUIInterface::ThreadSafeQuestion(const bilingual_str& message, const std::string& non_interactive_message, unsigned int style) { return g_ui_signals.ThreadSafeQuestion(message, non_interactive_message, style);}
void CClientUIInterface::InitMessage(const std::string& message) { return g_ui_signals.InitMessage(message); }
void CClientUIInterface::InitWallet() { return g_ui_signals.InitWallet(); }
void CClientUIInterface::NotifyNumConnectionsChanged(int newNumConnections) { return g_ui_signals.NotifyNumConnectionsChanged(newNumConnections); }
diff --git a/src/test/btcsignals_tests.cpp b/src/test/btcsignals_tests.cpp
index b3203ebe..7d7970bd 100644
--- a/src/test/btcsignals_tests.cpp
+++ b/src/test/btcsignals_tests.cpp
@@ -12,22 +12,6 @@
namespace {
-struct MoveOnlyData {
- MoveOnlyData(int data) : m_data(data) {}
- MoveOnlyData(MoveOnlyData&&) = default;
-
- MoveOnlyData& operator=(MoveOnlyData&&) = delete;
- MoveOnlyData(const MoveOnlyData&) = delete;
- MoveOnlyData& operator=(const MoveOnlyData&) = delete;
-
- int m_data;
-};
-
-MoveOnlyData MoveOnlyReturnCallback(int val)
-{
- return {val};
-}
-
void IncrementCallback(int& val)
{
val++;
@@ -97,44 +81,30 @@ BOOST_AUTO_TEST_CASE(disconnects)
BOOST_CHECK_EQUAL(val, 6);
}
-/* Check that move-only return types work correctly
- */
-BOOST_AUTO_TEST_CASE(moveonly_return)
-{
- btcsignals::signal<MoveOnlyData(int)> sig0;
- sig0.connect(MoveOnlyReturnCallback);
- int data{3};
- auto ret = sig0(data);
- BOOST_CHECK_EQUAL(ret->m_data, 3);
-}
-
-/* The result of the signal invocation should always be the result of the last
- * enabled callback.
- */
-BOOST_AUTO_TEST_CASE(return_value)
+BOOST_AUTO_TEST_CASE(any_of_combiner)
{
- btcsignals::signal<bool()> sig0;
+ btcsignals::signal<bool(), btcsignals::any_of> sig0;
decltype(sig0)::result_type ret;
ret = sig0();
- BOOST_CHECK(!ret);
+ BOOST_CHECK_EQUAL(ret, false);
{
- btcsignals::scoped_connection conn0 = sig0.connect(ReturnTrue);
+ btcsignals::scoped_connection conn0{sig0.connect(ReturnTrue)};
ret = sig0();
- BOOST_CHECK(ret && *ret == true);
+ BOOST_CHECK_EQUAL(ret, true);
}
ret = sig0();
- BOOST_CHECK(!ret);
+ BOOST_CHECK_EQUAL(ret, false);
{
- btcsignals::scoped_connection conn1 = sig0.connect(ReturnTrue);
- btcsignals::scoped_connection conn0 = sig0.connect(ReturnFalse);
+ btcsignals::scoped_connection conn0{sig0.connect(ReturnTrue)};
+ btcsignals::scoped_connection conn1{sig0.connect(ReturnFalse)};
ret = sig0();
- BOOST_CHECK(ret && *ret == false);
+ BOOST_CHECK_EQUAL(ret, true);
conn0.disconnect();
ret = sig0();
- BOOST_CHECK(ret && *ret == true);
+ BOOST_CHECK_EQUAL(ret, false);
}
ret = sig0();
- BOOST_CHECK(!ret);
+ BOOST_CHECK_EQUAL(ret, false);
}
/* Test the thread-safety of connect/disconnect/empty/connected/callbacks.
Why this scored 51/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.