From fa86e5dba94efbdc05212e70c991d4fdd281e984 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 9 Apr 2026 21:48:42 +0200 Subject: [PATCH] refactor: Properly return from ThreadSafeQuestion signal Previously, the signal was using btcsignals::optional_last_value. 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. --- src/btcsignals.h | 40 +++++++++++++++------------ src/node/interface_ui.cpp | 4 +-- src/test/btcsignals_tests.cpp | 52 ++++++++--------------------------- 3 files changed, 35 insertions(+), 61 deletions(-) diff --git a/src/btcsignals.h b/src/btcsignals.h index 94625edd3cf..e0df8ae8adb 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 is undefined, void must be special-cased. - */ - -template -class optional_last_value +/// The default combiner, which only returns void. +class null_value { public: - using result_type = std::conditional_t, void, std::optional>; + using result_type = void; }; -template ::result_type>> +/// A combiner, which checks if at least one callback returned true. +class any_of +{ +public: + // 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 class signal; /* @@ -150,8 +152,6 @@ class signal { using function_type = std::function; - static_assert(std::is_same_v>, "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) { + static_assert(std::is_same_v, + "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, + "only the any_of combiner is supported and hard-coded into this functor."); + static_assert(std::is_same_v, + "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 54b267c80b0..c80c2902efd 100644 --- a/src/node/interface_ui.cpp +++ b/src/node/interface_ui.cpp @@ -14,7 +14,7 @@ CClientUIInterface uiInterface; struct UISignals { btcsignals::signal ThreadSafeMessageBox; - btcsignals::signal> ThreadSafeQuestion; + btcsignals::signal ThreadSafeQuestion; btcsignals::signal InitMessage; btcsignals::signal InitWallet; btcsignals::signal 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 b3203ebeb23..7d7970bdb59 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) +BOOST_AUTO_TEST_CASE(any_of_combiner) { - btcsignals::signal 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) -{ - btcsignals::signal sig0; + btcsignals::signal 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.