From e058544d0e83aed691903d13d7e5775beb7e678f Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 17 Dec 2024 10:18:36 +0700 Subject: [PATCH 1/2] Make m_tip_block an std::optional This change avoids ambiguity when no tip is connected and it is compared to uint256::ZERO. --- src/init.cpp | 2 +- src/node/interfaces.cpp | 4 +++- src/node/kernel_notifications.h | 2 +- src/test/validation_chainstate_tests.cpp | 3 ++- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 03af3dfb23e..ebb07c8d3a0 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1807,7 +1807,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) { WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock); kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) { - return !kernel_notifications.m_tip_block.IsNull() || ShutdownRequested(node); + return kernel_notifications.m_tip_block || ShutdownRequested(node); }); } diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index e4ae9400e37..03e8028d195 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -973,7 +973,9 @@ class MinerImpl : public Mining { WAIT_LOCK(notifications().m_tip_block_mutex, lock); notifications().m_tip_block_cv.wait_for(lock, timeout, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) { - return (notifications().m_tip_block != current_tip && notifications().m_tip_block != uint256::ZERO) || chainman().m_interrupt; + // We need to wait for m_tip_block to be set AND for the value + // to differ from the current_tip value. + return (notifications().m_tip_block && notifications().m_tip_block != current_tip) || chainman().m_interrupt; }); } // Must release m_tip_block_mutex before locking cs_main, to avoid deadlocks. diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h index 35070b5285d..8a473977eb9 100644 --- a/src/node/kernel_notifications.h +++ b/src/node/kernel_notifications.h @@ -59,7 +59,7 @@ class KernelNotifications : public kernel::Notifications //! The block for which the last blockTip notification was received. //! It's first set when the tip is connected during node initialization. //! Might be unset during an early shutdown. - uint256 m_tip_block GUARDED_BY(m_tip_block_mutex){uint256::ZERO}; + std::optional m_tip_block GUARDED_BY(m_tip_block_mutex); private: const std::function& m_shutdown_request; diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp index c9cca8af04f..0ae0363aaa2 100644 --- a/src/test/validation_chainstate_tests.cpp +++ b/src/test/validation_chainstate_tests.cpp @@ -72,7 +72,8 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) ChainstateManager& chainman = *Assert(m_node.chainman); const auto get_notify_tip{[&]() { LOCK(m_node.notifications->m_tip_block_mutex); - return m_node.notifications->m_tip_block; + BOOST_REQUIRE(m_node.notifications->m_tip_block); + return *m_node.notifications->m_tip_block; }}; uint256 curr_tip = get_notify_tip(); From 81cea5d4ee0e226f7c7145072de85829f4166ec9 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 17 Dec 2024 09:43:22 +0700 Subject: [PATCH 2/2] Ensure m_tip_block is never ZERO To avoid future code changes from reintroducing the ambiguity fixed by the previous commit, mark m_tip_block private and Assume that it's not set to uint256::ZERO. --- src/init.cpp | 2 +- src/node/interfaces.cpp | 2 +- src/node/kernel_notifications.cpp | 8 ++++++++ src/node/kernel_notifications.h | 4 +++- src/test/validation_chainstate_tests.cpp | 4 ++-- 5 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index ebb07c8d3a0..2b046fc2af9 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1807,7 +1807,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) { WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock); kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) { - return kernel_notifications.m_tip_block || ShutdownRequested(node); + return kernel_notifications.TipBlock() || ShutdownRequested(node); }); } diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 03e8028d195..212f0365bb7 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -975,7 +975,7 @@ class MinerImpl : public Mining notifications().m_tip_block_cv.wait_for(lock, timeout, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) { // We need to wait for m_tip_block to be set AND for the value // to differ from the current_tip value. - return (notifications().m_tip_block && notifications().m_tip_block != current_tip) || chainman().m_interrupt; + return (notifications().TipBlock() && notifications().TipBlock() != current_tip) || chainman().m_interrupt; }); } // Must release m_tip_block_mutex before locking cs_main, to avoid deadlocks. diff --git a/src/node/kernel_notifications.cpp b/src/node/kernel_notifications.cpp index a09803165ca..550ffe74c4a 100644 --- a/src/node/kernel_notifications.cpp +++ b/src/node/kernel_notifications.cpp @@ -52,6 +52,7 @@ kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state { { LOCK(m_tip_block_mutex); + Assume(index.GetBlockHash() != uint256::ZERO); m_tip_block = index.GetBlockHash(); m_tip_block_cv.notify_all(); } @@ -99,6 +100,13 @@ void KernelNotifications::fatalError(const bilingual_str& message) m_exit_status, message, &m_warnings); } +std::optional KernelNotifications::TipBlock() +{ + AssertLockHeld(m_tip_block_mutex); + return m_tip_block; +}; + + void ReadNotificationArgs(const ArgsManager& args, KernelNotifications& notifications) { if (auto value{args.GetIntArg("-stopatheight")}) notifications.m_stop_at_height = *value; diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h index 8a473977eb9..f4174381daf 100644 --- a/src/node/kernel_notifications.h +++ b/src/node/kernel_notifications.h @@ -59,12 +59,14 @@ class KernelNotifications : public kernel::Notifications //! The block for which the last blockTip notification was received. //! It's first set when the tip is connected during node initialization. //! Might be unset during an early shutdown. - std::optional m_tip_block GUARDED_BY(m_tip_block_mutex); + std::optional TipBlock() EXCLUSIVE_LOCKS_REQUIRED(m_tip_block_mutex); private: const std::function& m_shutdown_request; std::atomic& m_exit_status; node::Warnings& m_warnings; + + std::optional m_tip_block GUARDED_BY(m_tip_block_mutex); }; void ReadNotificationArgs(const ArgsManager& args, KernelNotifications& notifications); diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp index 0ae0363aaa2..bf8cd819f28 100644 --- a/src/test/validation_chainstate_tests.cpp +++ b/src/test/validation_chainstate_tests.cpp @@ -72,8 +72,8 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup) ChainstateManager& chainman = *Assert(m_node.chainman); const auto get_notify_tip{[&]() { LOCK(m_node.notifications->m_tip_block_mutex); - BOOST_REQUIRE(m_node.notifications->m_tip_block); - return *m_node.notifications->m_tip_block; + BOOST_REQUIRE(m_node.notifications->TipBlock()); + return *m_node.notifications->TipBlock(); }}; uint256 curr_tip = get_notify_tip();