Merge #18742: miner: Avoid stack-use-after-return in validationinterface

7777f2a4bb1f9d843bc50a4e35085cfbb2808780 miner: Avoid stack-use-after-return in validationinterface (MarcoFalke)
fa5ceb25fce2200edf6b8ebfa6d4f01ed6774b95 test: Remove UninterruptibleSleep from test and replace it by SyncWithValidationInterfaceQueue (MarcoFalke)
fa770ce7fe67685c43780e219d8232efbee0bb8e validationinterface: Rework documentation, Rename pwalletIn to callbacks (MarcoFalke)
fab6d060ce5f580db538070beec1c5518c8c777c test: Add unregister_validation_interface_race test (MarcoFalke)

Pull request description:

  When a validationinterface has itself unregistered in one thread, but is about to get executed in another thread [1], there is a race:

  * The validationinterface destructing itself
  * The validationinterface getting dereferenced for execution

  [1] https://github.com/bitcoin/bitcoin/blob/64139803f1225dab26197a20314109d37fa87d5f/src/validationinterface.cpp#L82-L83

  This happens in the miner. More generally it happens everywhere where at least one thread is generating notifications and another one is unregistering a validationinterface.

  This issue has been fixed in commit ab31b9d6fe7b39713682e3f52d11238dbe042c16, but the fix has not been applied to the miner.

  Example where this happened in practice: https://travis-ci.org/github/bitcoin/bitcoin/jobs/675322230#L4414

ACKs for top commit:
  promag:
    Code review ACK 7777f2a4bb1f9d843bc50a4e35085cfbb2808780.
  laanwj:
    Code review ACK 7777f2a4bb1f9d843bc50a4e35085cfbb2808780

Tree-SHA512: 8087119243c71ba18a823a63515f3730d127162625d8729024278b447af29e2ff206f4840ee3d90bf84f93a2c5ab73b76c7e7044c83aa93b5b51047a166ec3d3

2 files changed, 40 insertions, 8 deletions

diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp
index c345f1eafb..899f054b83 100644
--- a/src/test/validation_block_tests.cpp
+++ b/src/test/validation_block_tests.cpp
@@ -32,7 +32,7 @@ struct MinerTestingSetup : public RegTestingSetup {
 
 BOOST_FIXTURE_TEST_SUITE(validation_block_tests, MinerTestingSetup)
 
-struct TestSubscriber : public CValidationInterface {
+struct TestSubscriber final : public CValidationInterface {
     uint256 m_expected_tip;
 
     explicit TestSubscriber(uint256 tip) : m_expected_tip(tip) {}
@@ -175,8 +175,8 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
         LOCK(cs_main);
         initial_tip = ::ChainActive().Tip();
     }
-    TestSubscriber sub(initial_tip->GetBlockHash());
-    RegisterValidationInterface(&sub);
+    auto sub = std::make_shared<TestSubscriber>(initial_tip->GetBlockHash());
+    RegisterSharedValidationInterface(sub);
 
     // create a bunch of threads that repeatedly process a block generated above at random
     // this will create parallelism and randomness inside validation - the ValidationInterface
@@ -204,14 +204,12 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
     for (auto& t : threads) {
         t.join();
     }
-    while (GetMainSignals().CallbacksPending() > 0) {
-        UninterruptibleSleep(std::chrono::milliseconds{100});
-    }
+    SyncWithValidationInterfaceQueue();
 
-    UnregisterValidationInterface(&sub);
+    UnregisterSharedValidationInterface(sub);
 
     LOCK(cs_main);
-    BOOST_CHECK_EQUAL(sub.m_expected_tip, ::ChainActive().Tip()->GetBlockHash());
+    BOOST_CHECK_EQUAL(sub->m_expected_tip, ::ChainActive().Tip()->GetBlockHash());
 }
 
 /**
diff --git a/src/test/validationinterface_tests.cpp b/src/test/validationinterface_tests.cpp
index 208be92852..d2fc20e625 100644
--- a/src/test/validationinterface_tests.cpp
+++ b/src/test/validationinterface_tests.cpp
@@ -12,6 +12,40 @@
 
 BOOST_FIXTURE_TEST_SUITE(validationinterface_tests, TestingSetup)
 
+struct TestSubscriberNoop final : public CValidationInterface {
+    void BlockChecked(const CBlock&, const BlockValidationState&) override {}
+};
+
+BOOST_AUTO_TEST_CASE(unregister_validation_interface_race)
+{
+    std::atomic<bool> generate{true};
+
+    // Start thread to generate notifications
+    std::thread gen{[&] {
+        const CBlock block_dummy;
+        const BlockValidationState state_dummy;
+        while (generate) {
+            GetMainSignals().BlockChecked(block_dummy, state_dummy);
+        }
+    }};
+
+    // Start thread to consume notifications
+    std::thread sub{[&] {
+        // keep going for about 1 sec, which is 250k iterations
+        for (int i = 0; i < 250000; i++) {
+            auto sub = std::make_shared<TestSubscriberNoop>();
+            RegisterSharedValidationInterface(sub);
+            UnregisterSharedValidationInterface(sub);
+        }
+        // tell the other thread we are done
+        generate = false;
+    }};
+
+    gen.join();
+    sub.join();
+    BOOST_CHECK(!generate);
+}
+
 class TestInterface : public CValidationInterface
 {
 public: