Merge bitcoin/bitcoin#34495: Replace boost signals with minimal compatible implementation

242b0ebb5c btcsignals: use a single shared_ptr for liveness and callback (Cory Fields)
b12f43a0a8 signals: remove boost::signals2 from depends and vcpkg (Cory Fields)
a4b1607983 signals: remove boost::signals2 mentions in linters and docs (Cory Fields)
375397ebd9 signals: remove boost includes where possible (Cory Fields)
091736a153 signals: re-add forward-declares to interface headers (Cory Fields)
9958f4fe49 Revert "signals: Temporarily add boost headers to bitcoind and bitcoin-node builds" (Cory Fields)
34eabd77a2 signals: remove boost compatibility guards (Cory Fields)
e60a0b9a22 signals: Add a simplified boost-compatible implementation (Cory Fields)
63c68e2a3f signals: add signals tests (Cory Fields)
edc2978058 signals: use an alias for the boost::signals2 namespace (Cory Fields)
9ade3929aa signals: remove forward-declare for signals (Cory Fields)
037e58b57b signals: use forwarding header for boost signals (Cory Fields)
2150153f37 signals: Temporarily add boost headers to bitcoind and bitcoin-node builds (Cory Fields)
fd5e9d9904 signals: Use a lambda to avoid connecting a signal to another signal (Cory Fields)

Pull request description:

  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::function`s.

  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.

ACKs for top commit:
  fjahr:
    Code review ACK 242b0ebb5c
  maflcko:
    re-review ACK 242b0ebb5c 🎯
  w0xlt:
    reACK 242b0ebb5c

Tree-SHA512: 9a472afa4f655624fa44493774a63b57509ad30fb61bf1d89b6d0b52000cb9a1409a5b8d515a99c76e0b26b2437c30508206c29a7dd44ea96eb1979d572cd4d4

src/test/CMakeLists.txt

@@ -25,6 +25,7 @@ add_executable(test_bitcoin
   blockmanager_tests.cpp
   bloom_tests.cpp
   bswap_tests.cpp
+  btcsignals_tests.cpp
   caches_tests.cpp
   chain_tests.cpp
   chainstate_write_tests.cpp

src/test/btcsignals_tests.cpp

@@ -0,0 +1,280 @@
+// Copyright (c) The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <btcsignals.h>
+#include <test/util/setup_common.h>
+
+#include <boost/test/unit_test.hpp>
+
+#include <semaphore>
+
+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++;
+}
+void SquareCallback(int& val)
+{
+    val *= val;
+}
+
+bool ReturnTrue()
+{
+    return true;
+}
+bool ReturnFalse()
+{
+    return false;
+}
+
+} // anonymous namespace
+
+BOOST_FIXTURE_TEST_SUITE(btcsignals_tests, BasicTestingSetup)
+
+/* Callbacks should always be executed in the order in which they were added
+ */
+BOOST_AUTO_TEST_CASE(callback_order)
+{
+    btcsignals::signal<void(int&)> sig0;
+    sig0.connect(IncrementCallback);
+    sig0.connect(SquareCallback);
+    int val{3};
+    sig0(val);
+    BOOST_CHECK_EQUAL(val, 16);
+    BOOST_CHECK(!sig0.empty());
+}
+
+BOOST_AUTO_TEST_CASE(disconnects)
+{
+    btcsignals::signal<void(int&)> sig0;
+    auto conn0 = sig0.connect(IncrementCallback);
+    auto conn1 = sig0.connect(SquareCallback);
+    conn1.disconnect();
+    BOOST_CHECK(!sig0.empty());
+    int val{3};
+    sig0(val);
+    BOOST_CHECK_EQUAL(val, 4);
+
+    BOOST_CHECK(!sig0.empty());
+    conn0.disconnect();
+    BOOST_CHECK(sig0.empty());
+    sig0(val);
+    BOOST_CHECK_EQUAL(val, 4);
+
+    conn0 = sig0.connect(IncrementCallback);
+    conn1 = sig0.connect(IncrementCallback);
+    BOOST_CHECK(!sig0.empty());
+    sig0(val);
+    BOOST_CHECK_EQUAL(val, 6);
+    conn1.disconnect();
+
+    BOOST_CHECK(conn0.connected());
+    {
+        btcsignals::scoped_connection scope(conn0);
+    }
+    BOOST_CHECK(!conn0.connected());
+    BOOST_CHECK(sig0.empty());
+    sig0(val);
+    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)
+{
+    btcsignals::signal<bool()> sig0;
+    decltype(sig0)::result_type ret;
+    ret = sig0();
+    BOOST_CHECK(!ret);
+    {
+        btcsignals::scoped_connection conn0 = sig0.connect(ReturnTrue);
+        ret = sig0();
+        BOOST_CHECK(ret && *ret == true);
+    }
+    ret = sig0();
+    BOOST_CHECK(!ret);
+    {
+        btcsignals::scoped_connection conn1 = sig0.connect(ReturnTrue);
+        btcsignals::scoped_connection conn0 = sig0.connect(ReturnFalse);
+        ret = sig0();
+        BOOST_CHECK(ret && *ret == false);
+        conn0.disconnect();
+        ret = sig0();
+        BOOST_CHECK(ret && *ret == true);
+    }
+    ret = sig0();
+    BOOST_CHECK(!ret);
+}
+
+/* Test the thread-safety of connect/disconnect/empty/connected/callbacks.
+ * Connect sig0 to an incrementor function and loop in a thread.
+ * Meanwhile, in another thread, inject and call new increment callbacks.
+ * Both threads are constantly calling empty/connected.
+ * Though the end-result is undefined due to a non-deterministic number of
+ * total callbacks executed, this should all be completely threadsafe.
+ * Sanitizers should pick up any buggy data race behavior (if present).
+ */
+BOOST_AUTO_TEST_CASE(thread_safety)
+{
+    btcsignals::signal<void()> sig0;
+    std::atomic<uint32_t> val{0};
+    auto conn0 = sig0.connect([&val] {
+        val++;
+    });
+
+    std::thread incrementor([&conn0, &sig0] {
+        for (int i = 0; i < 1000; i++) {
+            sig0();
+        }
+        // Because these calls are purposely happening on both threads at the
+        // same time, these must be asserts rather than BOOST_CHECKs to prevent
+        // a race inside of BOOST_CHECK itself (writing to the log).
+        assert(!sig0.empty());
+        assert(conn0.connected());
+    });
+
+    std::thread extra_increment_injector([&conn0, &sig0, &val] {
+        static constexpr size_t num_extra_conns{1000};
+        std::vector<btcsignals::scoped_connection> extra_conns;
+        extra_conns.reserve(num_extra_conns);
+        for (size_t i = 0; i < num_extra_conns; i++) {
+            BOOST_CHECK(!sig0.empty());
+            BOOST_CHECK(conn0.connected());
+            extra_conns.emplace_back(sig0.connect([&val] {
+                val++;
+            }));
+            sig0();
+        }
+    });
+    incrementor.join();
+    extra_increment_injector.join();
+    conn0.disconnect();
+    BOOST_CHECK(sig0.empty());
+
+    // 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);
+}
+
+/* Test that connection and disconnection works from within signal
+ * callbacks.
+ */
+BOOST_AUTO_TEST_CASE(recursion_safety)
+{
+    btcsignals::connection conn0, conn1, conn2;
+    btcsignals::signal<void()> sig0;
+    bool nonrecursive_callback_ran{false};
+    bool recursive_callback_ran{false};
+
+    conn0 = sig0.connect([&] {
+        BOOST_CHECK(!sig0.empty());
+        nonrecursive_callback_ran = true;
+    });
+    BOOST_CHECK(!nonrecursive_callback_ran);
+    sig0();
+    BOOST_CHECK(nonrecursive_callback_ran);
+    BOOST_CHECK(conn0.connected());
+
+    nonrecursive_callback_ran = false;
+    conn1 = sig0.connect([&] {
+        nonrecursive_callback_ran = true;
+        conn1.disconnect();
+    });
+    BOOST_CHECK(!nonrecursive_callback_ran);
+    BOOST_CHECK(conn0.connected());
+    BOOST_CHECK(conn1.connected());
+    sig0();
+    BOOST_CHECK(nonrecursive_callback_ran);
+    BOOST_CHECK(conn0.connected());
+    BOOST_CHECK(!conn1.connected());
+
+    nonrecursive_callback_ran = false;
+    conn1 = sig0.connect([&] {
+        conn2 = sig0.connect([&] {
+            BOOST_CHECK(conn0.connected());
+            recursive_callback_ran = true;
+            conn0.disconnect();
+            conn2.disconnect();
+        });
+        nonrecursive_callback_ran = true;
+        conn1.disconnect();
+    });
+    BOOST_CHECK(!nonrecursive_callback_ran);
+    BOOST_CHECK(!recursive_callback_ran);
+    BOOST_CHECK(conn0.connected());
+    BOOST_CHECK(conn1.connected());
+    BOOST_CHECK(!conn2.connected());
+    sig0();
+    BOOST_CHECK(nonrecursive_callback_ran);
+    BOOST_CHECK(!recursive_callback_ran);
+    BOOST_CHECK(conn0.connected());
+    BOOST_CHECK(!conn1.connected());
+    BOOST_CHECK(conn2.connected());
+    sig0();
+    BOOST_CHECK(recursive_callback_ran);
+    BOOST_CHECK(!conn0.connected());
+    BOOST_CHECK(!conn1.connected());
+    BOOST_CHECK(!conn2.connected());
+}
+
+/* Test that disconnection from another thread works in real time
+ */
+BOOST_AUTO_TEST_CASE(disconnect_thread_safety)
+{
+    btcsignals::connection conn0, conn1, conn2;
+    btcsignals::signal<void(int&)> sig0;
+    std::binary_semaphore done1{0};
+    std::binary_semaphore done2{0};
+    int val{0};
+
+    conn0 = sig0.connect([&](int&) {
+        conn1.disconnect();
+        done1.release();
+        done2.acquire();
+    });
+    conn1 = sig0.connect(IncrementCallback);
+    conn2 = sig0.connect(IncrementCallback);
+    std::thread thr([&] {
+        done1.acquire();
+        conn2.disconnect();
+        done2.release();
+    });
+    sig0(val);
+    thr.join();
+    BOOST_CHECK_EQUAL(val, 0);
+}
+
+
+BOOST_AUTO_TEST_SUITE_END()