From 4660170eaed3bdf009efbd533098dd0d9af4ccdc Mon Sep 17 00:00:00 2001 From: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> Date: Sun, 26 Nov 2023 15:10:38 -0600 Subject: [PATCH] Merge pull request #5726 from UdjinM6/bp_18742_28150 backport: bitcoin#18742, #28150 --- src/rpc/mining.cpp | 12 ++++----- src/test/validation_block_tests.cpp | 14 +++++----- src/test/validationinterface_tests.cpp | 36 +++++++++++++++++++++++++- src/validationinterface.cpp | 35 +++++++++++++++---------- src/validationinterface.h | 14 +++++----- 5 files changed, 76 insertions(+), 35 deletions(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 4a4cb503dc..13538afa41 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -955,7 +955,7 @@ static UniValue getblocktemplate(const JSONRPCRequest& request) return result; } -class submitblock_StateCatcher : public CValidationInterface +class submitblock_StateCatcher final : public CValidationInterface { public: uint256 hash; @@ -1016,17 +1016,17 @@ static UniValue submitblock(const JSONRPCRequest& request) } bool new_block; - submitblock_StateCatcher sc(block.GetHash()); - RegisterValidationInterface(&sc); + auto sc = std::make_shared(block.GetHash()); + RegisterSharedValidationInterface(sc); bool accepted = chainman.ProcessNewBlock(Params(), blockptr, /* fForceProcessing */ true, /* fNewBlock */ &new_block); - UnregisterValidationInterface(&sc); + UnregisterSharedValidationInterface(sc); if (!new_block && accepted) { return "duplicate"; } - if (!sc.found) { + if (!sc->found) { return "inconclusive"; } - return BIP22ValidationResult(sc.state); + return BIP22ValidationResult(sc->state); } static UniValue submitheader(const JSONRPCRequest& request) diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index ec19142166..9f6a56c15b 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -40,7 +40,7 @@ static const std::vector V_OP_TRUE{OP_TRUE}; 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) {} @@ -179,8 +179,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(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 @@ -208,14 +208,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..1e935a0ba1 100644 --- a/src/test/validationinterface_tests.cpp +++ b/src/test/validationinterface_tests.cpp @@ -10,7 +10,41 @@ #include #include -BOOST_FIXTURE_TEST_SUITE(validationinterface_tests, TestingSetup) +BOOST_FIXTURE_TEST_SUITE(validationinterface_tests, ChainTestingSetup) + +struct TestSubscriberNoop final : public CValidationInterface { + void BlockChecked(const CBlock&, const BlockValidationState&) override {} +}; + +BOOST_AUTO_TEST_CASE(unregister_validation_interface_race) +{ + std::atomic 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(); + RegisterSharedValidationInterface(sub); + UnregisterSharedValidationInterface(sub); + } + // tell the other thread we are done + generate = false; + }}; + + gen.join(); + sub.join(); + BOOST_CHECK(!generate); +} class TestInterface : public CValidationInterface { diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 1dc40f3aad..ed014cb752 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -94,22 +94,26 @@ public: static CMainSignals g_signals; -void CMainSignals::RegisterBackgroundSignalScheduler(CScheduler& scheduler) { +void CMainSignals::RegisterBackgroundSignalScheduler(CScheduler& scheduler) +{ assert(!m_internals); m_internals.reset(new MainSignalsInstance(&scheduler)); } -void CMainSignals::UnregisterBackgroundSignalScheduler() { +void CMainSignals::UnregisterBackgroundSignalScheduler() +{ m_internals.reset(nullptr); } -void CMainSignals::FlushBackgroundCallbacks() { +void CMainSignals::FlushBackgroundCallbacks() +{ if (m_internals) { m_internals->m_schedulerClient.EmptyQueue(); } } -size_t CMainSignals::CallbacksPending() { +size_t CMainSignals::CallbacksPending() +{ if (!m_internals) return 0; return m_internals->m_schedulerClient.CallbacksPending(); } @@ -119,10 +123,11 @@ CMainSignals& GetMainSignals() return g_signals; } -void RegisterSharedValidationInterface(std::shared_ptr pwalletIn) { - // Each connection captures pwalletIn to ensure that each callback is - // executed before pwalletIn is destroyed. For more details see #18338. - g_signals.m_internals->Register(std::move(pwalletIn)); +void RegisterSharedValidationInterface(std::shared_ptr callbacks) +{ + // Each connection captures the shared_ptr to ensure that each callback is + // executed before the subscriber is destroyed. For more details see #18338. + g_signals.m_internals->Register(std::move(callbacks)); } void RegisterValidationInterface(CValidationInterface* callbacks) @@ -137,24 +142,28 @@ void UnregisterSharedValidationInterface(std::shared_ptr c UnregisterValidationInterface(callbacks.get()); } -void UnregisterValidationInterface(CValidationInterface* pwalletIn) { +void UnregisterValidationInterface(CValidationInterface* callbacks) +{ if (g_signals.m_internals) { - g_signals.m_internals->Unregister(pwalletIn); + g_signals.m_internals->Unregister(callbacks); } } -void UnregisterAllValidationInterfaces() { +void UnregisterAllValidationInterfaces() +{ if (!g_signals.m_internals) { return; } g_signals.m_internals->Clear(); } -void CallFunctionInValidationInterfaceQueue(std::function func) { +void CallFunctionInValidationInterfaceQueue(std::function func) +{ g_signals.m_internals->m_schedulerClient.AddToProcessQueue(std::move(func)); } -void SyncWithValidationInterfaceQueue() { +void SyncWithValidationInterfaceQueue() +{ AssertLockNotHeld(cs_main); // Block until the validation queue drains std::promise promise; diff --git a/src/validationinterface.h b/src/validationinterface.h index 018d858ef2..90a5729239 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -33,20 +33,20 @@ namespace llmq { class CRecoveredSig; } // namespace llmq -// These functions dispatch to one or all registered wallets - -/** Register a wallet to receive updates from core */ -void RegisterValidationInterface(CValidationInterface* pwalletIn); -/** Unregister a wallet from core */ -void UnregisterValidationInterface(CValidationInterface* pwalletIn); -/** Unregister all wallets from core */ +/** Register subscriber */ +void RegisterValidationInterface(CValidationInterface* callbacks); +/** Unregister subscriber. DEPRECATED. This is not safe to use when the RPC server or main message handler thread is running. */ +void UnregisterValidationInterface(CValidationInterface* callbacks); +/** Unregister all subscribers */ void UnregisterAllValidationInterfaces(); // Alternate registration functions that release a shared_ptr after the last // notification is sent. These are useful for race-free cleanup, since // unregistration is nonblocking and can return before the last notification is // processed. +/** Register subscriber */ void RegisterSharedValidationInterface(std::shared_ptr callbacks); +/** Unregister subscriber */ void UnregisterSharedValidationInterface(std::shared_ptr callbacks); /**