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.

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.
- */
-
-template <typename T>
-class optional_last_value
+/// The default combiner, which only returns void.
+class null_value
 {
 public:
-    using result_type = std::conditional_t<std::is_void_v<T>, void, std::optional<T>>;
+    using result_type = void;
 };
 
-template <typename Signature, typename Combiner = optional_last_value<typename std::function<Signature>::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 <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;

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); }

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<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)
-{
-    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.