Merge bitcoin/bitcoin#32528: rpc: Round verificationprogress to 1 for a recent tip

achow101 · achow101 · commit 88b22acc3d6f · 2025-05-27T16:45:23.000-07:00
fab1e02 refactor: Pass verification_progress into block tip notifications (MarcoFalke) fa76b37 rpc: Round verificationprogress to exactly 1 for a recent tip (MarcoFalke) faf6304 test: Use mockable time in GuessVerificationProgress (MarcoFalke) Pull request description: Some users really seem to care about this. While it shouldn't matter much, the diff is so trivial that it is probably worth doing. Fixes #31127 One could also consider to split the field into two dedicated ones (bitcoin/bitcoin#28847 (comment)), but this is left for a more involved follow-up and may also be controversial. ACKs for top commit: achow101: ACK fab1e02 pinheadmz: ACK fab1e02 sipa: utACK fab1e02 Tree-SHA512: a3c24e3c446d38fbad9399c1e7f1ffa7904490a3a7d12623b44e583b435cc8b5f1ba83b84d29c7ffaf22028bc909c7cec07202b825480449c6419d2a190938f5
diff --git 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
@@ -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/chainparams.cpp b/src/kernel/chainparams.cpp
@@ -613,9 +613,9 @@ class CRegTestParams : public CChainParams
         };
 
         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<unsigned char>(1,111);
diff --git 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
@@ -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
@@ -103,7 +103,7 @@ class CClientUIInterface
     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
@@ -326,7 +326,8 @@ class NodeImpl : public Node
     }
     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
     {
@@ -408,9 +409,8 @@ class NodeImpl : public Node
     }
     std::unique_ptr<Handler> handleNotifyBlockTip(NotifyBlockTipFn fn) override
     {
-        return MakeSignalHandler(::uiInterface.NotifyBlockTip_connect([fn, this](SynchronizationState sync_state, const CBlockIndex* block) {
-            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<Handler> handleNotifyHeaderTip(NotifyHeaderTipFn fn) override
diff --git 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
@@ -35,7 +35,7 @@ class KernelNotifications : public kernel::Notifications
     KernelNotifications(const std::function<bool()>& shutdown_request, std::atomic<int>& 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
@@ -3557,7 +3557,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
@@ -3790,7 +3794,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.
@@ -4688,7 +4695,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;
@@ -5572,10 +5582,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;
@@ -5586,14 +5595,25 @@ double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex) c
         return 0.0;
     }
 
-    int64_t nNow = time(nullptr);
+    const int64_t nNow{TicksSinceEpoch<std::chrono::seconds>(NodeClock::now())};
+    const auto block_time{
+        (Assume(m_best_header) && std::abs(nNow - pindex->GetBlockTime()) <= Ticks<std::chrono::seconds>(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<double>(pindex->m_chain_tx_count / fTxTotal, 1.0);
diff --git a/src/validation.h b/src/validation.h
@@ -1148,7 +1148,7 @@ class ChainstateManager
     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
@@ -101,6 +101,7 @@ def run_test(self):
         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,20 @@ def _test_getdeploymentinfo(self):
         # 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.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):
         self.log.info("Check that block timestamps work until year 2106")
         self.generate(self.nodes[0], 8)[-1]

Original file line number	Diff line number	Diff line change
`@@ -74,7 +74,7 @@ int main(int argc, char* argv[])`
`74`	`74`	`class KernelNotifications : public kernel::Notifications`
`75`	`75`	`{`
`76`	`76`	`public:`
`77`		`- kernel::InterruptResult blockTip(SynchronizationState, CBlockIndex&) override`
	`77`	`+ kernel::InterruptResult blockTip(SynchronizationState, CBlockIndex&, double) override`
`78`	`78`	`{`
`79`	`79`	`std::cout << "Block tip changed" << std::endl;`
`80`	`80`	`return {};`
Original file line number	Diff line number	Diff line change
`@@ -326,7 +326,8 @@ class NodeImpl : public Node`
`326`	`326`	`}`
`327`	`327`	`double getVerificationProgress() override`
`328`	`328`	`{`
`329`		`- return chainman().GuessVerificationProgress(WITH_LOCK(chainman().GetMutex(), return chainman().ActiveChain().Tip()));`
	`329`	`+ LOCK(chainman().GetMutex());`
	`330`	`+ return chainman().GuessVerificationProgress(chainman().ActiveTip());`
`330`	`331`	`}`
`331`	`332`	`bool isInitialBlockDownload() override`
`332`	`333`	`{`
`@@ -408,9 +409,8 @@ class NodeImpl : public Node`
`408`	`409`	`}`
`409`	`410`	`std::unique_ptr<Handler> handleNotifyBlockTip(NotifyBlockTipFn fn) override`
`410`	`411`	`{`
`411`		`- return MakeSignalHandler(::uiInterface.NotifyBlockTip_connect([fn, this](SynchronizationState sync_state, const CBlockIndex* block) {`
`412`		`- fn(sync_state, BlockTip{block->nHeight, block->GetBlockTime(), block->GetBlockHash()},`
`413`		`- chainman().GuessVerificationProgress(block));`
	`412`	`+ return MakeSignalHandler(::uiInterface.NotifyBlockTip_connect([fn](SynchronizationState sync_state, const CBlockIndex& block, double verification_progress) {`
	`413`	`+ fn(sync_state, BlockTip{block.nHeight, block.GetBlockTime(), block.GetBlockHash()}, verification_progress);`
`414`	`414`	`}));`
`415`	`415`	`}`
`416`	`416`	`std::unique_ptr<Handler> handleNotifyHeaderTip(NotifyHeaderTipFn fn) override`
Original file line number	Diff line number	Diff line change
`@@ -48,7 +48,7 @@ static void AlertNotify(const std::string& strMessage)`
`48`	`48`
`49`	`49`	`namespace node {`
`50`	`50`
`51`		`-kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state, CBlockIndex& index)`
	`51`	`+kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state, CBlockIndex& index, double verification_progress)`
`52`	`52`	`{`
`53`	`53`	`{`
`54`	`54`	`LOCK(m_tip_block_mutex);`
`@@ -57,7 +57,7 @@ kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state`
`57`	`57`	`m_tip_block_cv.notify_all();`
`58`	`58`	`}`
`59`	`59`
`60`		`- uiInterface.NotifyBlockTip(state, &index);`
	`60`	`+ uiInterface.NotifyBlockTip(state, index, verification_progress);`
`61`	`61`	`if (m_stop_at_height && index.nHeight >= m_stop_at_height) {`
`62`	`62`	`if (!m_shutdown_request()) {`
`63`	`63`	`LogError("Failed to send shutdown signal after reaching stop height\n");`