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

1 files changed, 34 insertions, 0 deletions

diff --git a/src/test/validationinterface_tests.cpp b/src/test/validationinterface_tests.cpp
index 208be9285..d2fc20e62 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: