From faf6304bdfdf228354b4072b72f4c0ef90fdaade Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 16 May 2025 13:06:03 +0200 Subject: [PATCH 1/3] test: Use mockable time in GuessVerificationProgress Also add a test. --- src/kernel/chainparams.cpp | 6 +++--- src/validation.cpp | 2 +- test/functional/rpc_blockchain.py | 9 +++++++++ 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp index 0a359e6620..f81758879b 100644 --- a/src/kernel/chainparams.cpp +++ b/src/kernel/chainparams.cpp @@ -613,9 +613,9 @@ public: }; chainTxData = ChainTxData{ - 0, - 0, - 0 + .nTime = 0, + .tx_count = 0, + .dTxRate = 0.001, // Set a non-zero rate to make it testable }; base58Prefixes[PUBKEY_ADDRESS] = std::vector(1,111); diff --git a/src/validation.cpp b/src/validation.cpp index 13dbe83f80..a2e1b8c195 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5572,7 +5572,7 @@ double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex) c return 0.0; } - int64_t nNow = time(nullptr); + const int64_t nNow{TicksSinceEpoch(NodeClock::now())}; double fTxTotal; diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py index 2976f9188a..a323d5eebc 100755 --- a/test/functional/rpc_blockchain.py +++ b/test/functional/rpc_blockchain.py @@ -101,6 +101,7 @@ class BlockchainTest(BitcoinTestFramework): self._test_waitforblockheight() self._test_getblock() self._test_getdeploymentinfo() + self._test_verificationprogress() self._test_y2106() assert self.nodes[0].verifychain(4, 0) @@ -280,6 +281,14 @@ class BlockchainTest(BitcoinTestFramework): # calling with an explicit hash works self.check_signalling_deploymentinfo_result(self.nodes[0].getdeploymentinfo(gbci207["bestblockhash"]), gbci207["blocks"], gbci207["bestblockhash"], "started") + def _test_verificationprogress(self): + self.log.info("Check that verificationprogress is less than 1 when the block tip is old") + future = 2 * 60 * 60 + self.nodes[0].setmocktime(self.nodes[0].getblockchaininfo()["time"] + future + 1) + assert_greater_than(1, self.nodes[0].getblockchaininfo()["verificationprogress"]) + self.nodes[0].setmocktime(self.nodes[0].getblockchaininfo()["time"] + future) + assert_greater_than(1, self.nodes[0].getblockchaininfo()["verificationprogress"]) + def _test_y2106(self): self.log.info("Check that block timestamps work until year 2106") self.generate(self.nodes[0], 8)[-1] From fa76b378e4b218fb4853088328b9b488de18dcd2 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 16 May 2025 13:32:23 +0200 Subject: [PATCH 2/3] rpc: Round verificationprogress to exactly 1 for a recent tip This requires a new lock annotation, but all relevant callers already held the lock. --- src/node/interfaces.cpp | 4 +++- src/validation.cpp | 16 +++++++++++++--- src/validation.h | 2 +- test/functional/rpc_blockchain.py | 6 ++++++ 4 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index b5c74a97f6..641589d7cc 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -326,7 +326,8 @@ public: } double getVerificationProgress() override { - return chainman().GuessVerificationProgress(WITH_LOCK(chainman().GetMutex(), return chainman().ActiveChain().Tip())); + LOCK(chainman().GetMutex()); + return chainman().GuessVerificationProgress(chainman().ActiveTip()); } bool isInitialBlockDownload() override { @@ -409,6 +410,7 @@ public: std::unique_ptr handleNotifyBlockTip(NotifyBlockTipFn fn) override { return MakeSignalHandler(::uiInterface.NotifyBlockTip_connect([fn, this](SynchronizationState sync_state, const CBlockIndex* block) { + LOCK(chainman().GetMutex()); fn(sync_state, BlockTip{block->nHeight, block->GetBlockTime(), block->GetBlockHash()}, chainman().GuessVerificationProgress(block)); })); diff --git a/src/validation.cpp b/src/validation.cpp index a2e1b8c195..3ace172552 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5558,10 +5558,9 @@ bool Chainstate::ResizeCoinsCaches(size_t coinstip_size, size_t coinsdb_size) return ret; } -//! Guess how far we are in the verification process at the given block index -//! require cs_main if pindex has not been validated yet (because m_chain_tx_count might be unset) double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex) const { + AssertLockHeld(GetMutex()); const ChainTxData& data{GetParams().TxData()}; if (pindex == nullptr) { return 0.0; @@ -5573,13 +5572,24 @@ double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex) c } const int64_t nNow{TicksSinceEpoch(NodeClock::now())}; + const auto block_time{ + (Assume(m_best_header) && std::abs(nNow - pindex->GetBlockTime()) <= Ticks(2h) && + Assume(m_best_header->nHeight >= pindex->nHeight)) ? + // When the header is known to be recent, switch to a height-based + // approach. This ensures the returned value is quantized when + // close to "1.0", because some users expect it to be. This also + // avoids relying too much on the exact miner-set timestamp, which + // may be off. + nNow - (m_best_header->nHeight - pindex->nHeight) * GetConsensus().nPowTargetSpacing : + pindex->GetBlockTime(), + }; double fTxTotal; if (pindex->m_chain_tx_count <= data.tx_count) { fTxTotal = data.tx_count + (nNow - data.nTime) * data.dTxRate; } else { - fTxTotal = pindex->m_chain_tx_count + (nNow - pindex->GetBlockTime()) * data.dTxRate; + fTxTotal = pindex->m_chain_tx_count + (nNow - block_time) * data.dTxRate; } return std::min(pindex->m_chain_tx_count / fTxTotal, 1.0); diff --git a/src/validation.h b/src/validation.h index 5b488ba4b5..5ff23b6053 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1147,7 +1147,7 @@ public: bool IsInitialBlockDownload() const; /** Guess verification progress (as a fraction between 0.0=genesis and 1.0=current tip). */ - double GuessVerificationProgress(const CBlockIndex* pindex) const; + double GuessVerificationProgress(const CBlockIndex* pindex) const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()); /** * Import blocks from an external file diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py index a323d5eebc..5f6edfd94b 100755 --- a/test/functional/rpc_blockchain.py +++ b/test/functional/rpc_blockchain.py @@ -286,7 +286,13 @@ class BlockchainTest(BitcoinTestFramework): future = 2 * 60 * 60 self.nodes[0].setmocktime(self.nodes[0].getblockchaininfo()["time"] + future + 1) assert_greater_than(1, self.nodes[0].getblockchaininfo()["verificationprogress"]) + + self.log.info("Check that verificationprogress is exactly 1 for a recent block tip") self.nodes[0].setmocktime(self.nodes[0].getblockchaininfo()["time"] + future) + assert_equal(1, self.nodes[0].getblockchaininfo()["verificationprogress"]) + + self.log.info("Check that verificationprogress is less than 1 as soon as a new header comes in") + self.nodes[0].submitheader(self.generateblock(self.nodes[0], output="raw(55)", transactions=[], submit=False, sync_fun=self.no_op)["hex"]) assert_greater_than(1, self.nodes[0].getblockchaininfo()["verificationprogress"]) def _test_y2106(self): From fab1e02086ceebd7d96417b7a386fe61158bfda9 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 20 May 2025 10:46:59 +0200 Subject: [PATCH 3/3] refactor: Pass verification_progress into block tip notifications It is cheap to calculate and the caller does not have to take a lock to calculate it. Also turn pointers that can never be null into references. --- src/bitcoin-chainstate.cpp | 2 +- src/init.cpp | 6 +++--- src/kernel/notifications_interface.h | 2 +- src/node/interface_ui.cpp | 2 +- src/node/interface_ui.h | 2 +- src/node/interfaces.cpp | 6 ++---- src/node/kernel_notifications.cpp | 4 ++-- src/node/kernel_notifications.h | 2 +- src/validation.cpp | 16 +++++++++++++--- 9 files changed, 25 insertions(+), 17 deletions(-) diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index eec3362d4e..47ddfaad8d 100644 --- a/src/bitcoin-chainstate.cpp +++ b/src/bitcoin-chainstate.cpp @@ -74,7 +74,7 @@ int main(int argc, char* argv[]) class KernelNotifications : public kernel::Notifications { public: - kernel::InterruptResult blockTip(SynchronizationState, CBlockIndex&) override + kernel::InterruptResult blockTip(SynchronizationState, CBlockIndex&, double) override { std::cout << "Block tip changed" << std::endl; return {}; diff --git a/src/init.cpp b/src/init.cpp index 78e4a23dec..a8f768741f 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1787,10 +1787,10 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) #if HAVE_SYSTEM const std::string block_notify = args.GetArg("-blocknotify", ""); if (!block_notify.empty()) { - uiInterface.NotifyBlockTip_connect([block_notify](SynchronizationState sync_state, const CBlockIndex* pBlockIndex) { - if (sync_state != SynchronizationState::POST_INIT || !pBlockIndex) return; + uiInterface.NotifyBlockTip_connect([block_notify](SynchronizationState sync_state, const CBlockIndex& block, double /* verification_progress */) { + if (sync_state != SynchronizationState::POST_INIT) return; std::string command = block_notify; - ReplaceAll(command, "%s", pBlockIndex->GetBlockHash().GetHex()); + ReplaceAll(command, "%s", block.GetBlockHash().GetHex()); std::thread t(runCommand, command); t.detach(); // thread runs free }); diff --git a/src/kernel/notifications_interface.h b/src/kernel/notifications_interface.h index ef72d9bdb6..3e97e3b45e 100644 --- a/src/kernel/notifications_interface.h +++ b/src/kernel/notifications_interface.h @@ -37,7 +37,7 @@ class Notifications public: virtual ~Notifications() = default; - [[nodiscard]] virtual InterruptResult blockTip(SynchronizationState state, CBlockIndex& index) { return {}; } + [[nodiscard]] virtual InterruptResult blockTip(SynchronizationState state, CBlockIndex& index, double verification_progress) { return {}; } virtual void headerTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) {} virtual void progress(const bilingual_str& title, int progress_percent, bool resume_possible) {} virtual void warningSet(Warning id, const bilingual_str& message) {} diff --git a/src/node/interface_ui.cpp b/src/node/interface_ui.cpp index c9dcd7a7fb..273e51974e 100644 --- a/src/node/interface_ui.cpp +++ b/src/node/interface_ui.cpp @@ -55,7 +55,7 @@ void CClientUIInterface::NotifyNumConnectionsChanged(int newNumConnections) { re void CClientUIInterface::NotifyNetworkActiveChanged(bool networkActive) { return g_ui_signals.NotifyNetworkActiveChanged(networkActive); } void CClientUIInterface::NotifyAlertChanged() { return g_ui_signals.NotifyAlertChanged(); } void CClientUIInterface::ShowProgress(const std::string& title, int nProgress, bool resume_possible) { return g_ui_signals.ShowProgress(title, nProgress, resume_possible); } -void CClientUIInterface::NotifyBlockTip(SynchronizationState s, const CBlockIndex* i) { return g_ui_signals.NotifyBlockTip(s, i); } +void CClientUIInterface::NotifyBlockTip(SynchronizationState s, const CBlockIndex& block, double verification_progress) { return g_ui_signals.NotifyBlockTip(s, block, verification_progress); } void CClientUIInterface::NotifyHeaderTip(SynchronizationState s, int64_t height, int64_t timestamp, bool presync) { return g_ui_signals.NotifyHeaderTip(s, height, timestamp, presync); } void CClientUIInterface::BannedListChanged() { return g_ui_signals.BannedListChanged(); } diff --git a/src/node/interface_ui.h b/src/node/interface_ui.h index 85c34f5834..7732cf4797 100644 --- a/src/node/interface_ui.h +++ b/src/node/interface_ui.h @@ -103,7 +103,7 @@ public: ADD_SIGNALS_DECL_WRAPPER(ShowProgress, void, const std::string& title, int nProgress, bool resume_possible); /** New block has been accepted */ - ADD_SIGNALS_DECL_WRAPPER(NotifyBlockTip, void, SynchronizationState, const CBlockIndex*); + ADD_SIGNALS_DECL_WRAPPER(NotifyBlockTip, void, SynchronizationState, const CBlockIndex& block, double verification_progress); /** Best header has changed */ ADD_SIGNALS_DECL_WRAPPER(NotifyHeaderTip, void, SynchronizationState, int64_t height, int64_t timestamp, bool presync); diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 641589d7cc..04afc852e4 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -409,10 +409,8 @@ public: } std::unique_ptr handleNotifyBlockTip(NotifyBlockTipFn fn) override { - return MakeSignalHandler(::uiInterface.NotifyBlockTip_connect([fn, this](SynchronizationState sync_state, const CBlockIndex* block) { - LOCK(chainman().GetMutex()); - fn(sync_state, BlockTip{block->nHeight, block->GetBlockTime(), block->GetBlockHash()}, - chainman().GuessVerificationProgress(block)); + return MakeSignalHandler(::uiInterface.NotifyBlockTip_connect([fn](SynchronizationState sync_state, const CBlockIndex& block, double verification_progress) { + fn(sync_state, BlockTip{block.nHeight, block.GetBlockTime(), block.GetBlockHash()}, verification_progress); })); } std::unique_ptr handleNotifyHeaderTip(NotifyHeaderTipFn fn) override diff --git a/src/node/kernel_notifications.cpp b/src/node/kernel_notifications.cpp index 550ffe74c4..4a6f8444da 100644 --- a/src/node/kernel_notifications.cpp +++ b/src/node/kernel_notifications.cpp @@ -48,7 +48,7 @@ static void AlertNotify(const std::string& strMessage) namespace node { -kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state, CBlockIndex& index) +kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state, CBlockIndex& index, double verification_progress) { { LOCK(m_tip_block_mutex); @@ -57,7 +57,7 @@ kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state m_tip_block_cv.notify_all(); } - uiInterface.NotifyBlockTip(state, &index); + uiInterface.NotifyBlockTip(state, index, verification_progress); if (m_stop_at_height && index.nHeight >= m_stop_at_height) { if (!m_shutdown_request()) { LogError("Failed to send shutdown signal after reaching stop height\n"); diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h index f4174381da..10ee3e18d7 100644 --- a/src/node/kernel_notifications.h +++ b/src/node/kernel_notifications.h @@ -35,7 +35,7 @@ public: KernelNotifications(const std::function& shutdown_request, std::atomic& exit_status, node::Warnings& warnings) : m_shutdown_request(shutdown_request), m_exit_status{exit_status}, m_warnings{warnings} {} - [[nodiscard]] kernel::InterruptResult blockTip(SynchronizationState state, CBlockIndex& index) override EXCLUSIVE_LOCKS_REQUIRED(!m_tip_block_mutex); + [[nodiscard]] kernel::InterruptResult blockTip(SynchronizationState state, CBlockIndex& index, double verification_progress) override EXCLUSIVE_LOCKS_REQUIRED(!m_tip_block_mutex); void headerTip(SynchronizationState state, int64_t height, int64_t timestamp, bool presync) override; diff --git a/src/validation.cpp b/src/validation.cpp index 3ace172552..f6ac62a14a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3544,7 +3544,11 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< m_chainman.m_options.signals->UpdatedBlockTip(pindexNewTip, pindexFork, still_in_ibd); } - if (kernel::IsInterrupted(m_chainman.GetNotifications().blockTip(GetSynchronizationState(still_in_ibd, m_chainman.m_blockman.m_blockfiles_indexed), *pindexNewTip))) { + if (kernel::IsInterrupted(m_chainman.GetNotifications().blockTip( + /*state=*/GetSynchronizationState(still_in_ibd, m_chainman.m_blockman.m_blockfiles_indexed), + /*index=*/*pindexNewTip, + /*verification_progress=*/m_chainman.GuessVerificationProgress(pindexNewTip)))) + { // Just breaking and returning success for now. This could // be changed to bubble up the kernel::Interrupted value to // the caller so the caller could distinguish between @@ -3777,7 +3781,10 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde // parameter indicating the source of the tip change so hooks can // distinguish user-initiated invalidateblock changes from other // changes. - (void)m_chainman.GetNotifications().blockTip(GetSynchronizationState(m_chainman.IsInitialBlockDownload(), m_chainman.m_blockman.m_blockfiles_indexed), *to_mark_failed->pprev); + (void)m_chainman.GetNotifications().blockTip( + /*state=*/GetSynchronizationState(m_chainman.IsInitialBlockDownload(), m_chainman.m_blockman.m_blockfiles_indexed), + /*index=*/*to_mark_failed->pprev, + /*verification_progress=*/WITH_LOCK(m_chainman.GetMutex(), return m_chainman.GuessVerificationProgress(to_mark_failed->pprev))); // Fire ActiveTipChange now for the current chain tip to make sure clients are notified. // ActivateBestChain may call this as well, but not necessarily. @@ -4675,7 +4682,10 @@ bool Chainstate::LoadChainTip() // Ensure KernelNotifications m_tip_block is set even if no new block arrives. if (this->GetRole() != ChainstateRole::BACKGROUND) { // Ignoring return value for now. - (void)m_chainman.GetNotifications().blockTip(GetSynchronizationState(/*init=*/true, m_chainman.m_blockman.m_blockfiles_indexed), *pindex); + (void)m_chainman.GetNotifications().blockTip( + /*state=*/GetSynchronizationState(/*init=*/true, m_chainman.m_blockman.m_blockfiles_indexed), + /*index=*/*pindex, + /*verification_progress=*/m_chainman.GuessVerificationProgress(tip)); } return true;