May I suggest the following cleanup and locking improvement:

• Use Clang thread safety annotation with GuessVerificationProgress to ensure ::cs_main is held by callers

• Drop unneeded lock already held by most callers

• Remove redundant and out of date doxygen; the convention is to document the declaration

• Move const ChainTxData& data{GetParams().TxData()}; to where it is used

diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index 73ce927f712..46fe9e10b0f 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -325,7 +325,8 @@ public:
     }
     double getVerificationProgress() override
     {
-        return chainman().GuessVerificationProgress(WITH_LOCK(chainman().GetMutex(), return chainman().ActiveChain().Tip()));
+        LOCK(chainman().GetMutex());
+        return chainman().GuessVerificationProgress(chainman().ActiveChain().Tip());
     }
     bool isInitialBlockDownload() override
     {
@@ -409,7 +410,7 @@ public:
     {
         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));
+               WITH_LOCK(chainman().GetMutex(), return chainman().GuessVerificationProgress(block)));
         }));
     }
     std::unique_ptr<Handler> handleNotifyHeaderTip(NotifyHeaderTipFn fn) override
diff --git a/src/validation.cpp b/src/validation.cpp
index 5dca359aedf..54e85e07a6d 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -5613,12 +5613,8 @@ 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 and the best headers chain
-//! 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
 {
-    const ChainTxData& data{GetParams().TxData()};
-    LOCK(::cs_main);
     if (pindex == nullptr) {
         return 0.0;
     }
@@ -5638,7 +5634,7 @@ double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex) c
     }
 
     double fTxTotal;
-
+    const ChainTxData& data{GetParams().TxData()};
     if (pindex->m_chain_tx_count <= data.tx_count) {
         fTxTotal = data.tx_count + (end_of_chain_timestamp - data.nTime) * data.dTxRate;
     } else {
diff --git a/src/validation.h b/src/validation.h
index 9e4fdbe6809..2223dffc249 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -1149,7 +1149,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(::cs_main);
 
     /**
      * Import blocks from an external file

thanks for reviewing. Suggestions applied here ef15351.
Will squash once other reviews are finished.

Use our built-in time functionality in util/time.h, which is type-safe (using the standard library's chrono types) and mockable.

Thanks for reviewing.
I'm not sure about using it as requested. Is this what you're referring to? 317ddca

Unnecessary empty line removal, please keep the changes contained to the scope of your PR

Please add a descriptive comment what this code fragment does and why; we know, but the code isn't super easy to follow if you don't know the context (so for future maintainers).

eg "Estimate the time of the most recent block. This is either the time of the block at the tip of the chain index, or, if we know of a recent enough header with more work than this block, the time of that header."

Edit: wait, this assumes that pindex is the tip of the chain, but it's the block to estimate the progress of, which might not be the tip. i think i'm missing something. Won't this always return 1.0 if there is no good header known?

I think you're right, this seems like a rare edge case.
If I'm understanding correctly, the issue only arises if pindex is an old block (not the actual tip) and we also have no good headers available. In that scenario, the function would return 1.0 due to a lack of visibility beyond pindex.

I think the only way this happens is if a node just reconnected after being offline or is somehow isolated from the network. In that case, it might briefly estimate 1.0 until it receives newer headers.

Not sure how problematic this is in practice, but a possible fix could be to use m_best_header when available and, if not, fall back to Now<NodeSeconds>() as an estimate of where the chain should be.
Could look something similar to this:

int64_t end_of_chain_timestamp = TicksSinceEpoch<std::chrono::seconds>(NodeClock::time_point{std::chrono::seconds{pindex->GetBlockTime()}});
if (m_best_header && m_best_header->nChainWork > pindex->nChainWork) {
        int64_t header_age = TicksSinceEpoch<std::chrono::seconds>(Now<NodeSeconds>()) - m_best_header->GetBlockTime();
        if (header_age < 24 * 60 * 60) {
            end_of_chain_timestamp = std::max(end_of_chain_timestamp, m_best_header->GetBlockTime());
        }
    } else {
        // If no good best header is available, estimate where the chain should be
        end_of_chain_timestamp = std::max(end_of_chain_timestamp, TicksSinceEpoch<std::chrono::seconds>(Now<NodeSeconds>()));
    }

@sipa tagging you since this was originally your proposal.

Edit: I think with this we will be again on the case where we never reach 1.0. Is it actually possible to reach 1.0 if we don't assume that pindex is the tip if we don't know about any other best header?

if pindex is an old block (not the actual tip) and we also have no good headers available. In that scenario, the function would return 1.0 due to a lack of visibility beyond pindex.

Maybe that's how it is used at the moment, but the documentation of the function implies that the estimate can be used on any block, not that pindex necessarily is the latest block.

I think the only way this happens is if a node just reconnected after being offline or is somehow isolated from the network. In that case, it might briefly estimate 1.0 until it receives newer headers.

That seems bad to me. Being isolated from the network shouldn't make it report values that far off! The old current-time-based estimate might have edge cases around 1, but apart from that it handles this better.

i think, if the purpose here is to essentially "round up the result a bit when almost 1 (and we're sure we're at the tip)" that's what we should do and not change the entire way estimates are done.

Haven't looked into this yet, but just finished a 3-week IBD. The past few days was switching between this branch at 6d877a1 and master, with stops and starts of several hours as internet access was intermittent, and this branch seemed to work well -- it returned the same value or very close each time. I did not see any 100% values before reaching the tip.

Blocks: 882404
Headers: 882404
Verification progress: 100.0000%

I think the only way this happens is if a node just reconnected after being offline or is somehow isolated from the network. In that case, it might briefly estimate 1.0 until it receives newer headers.

That seems bad to me.

I agree. It doesn't seem worth it to fix one edge case and introduce another. This reminds me of #31135 (comment)

it returned the same value or very close each time. I did not see any 100% values before reaching the tip.

It makes sense bc your node is not isolated, but when you turn it on before having any peer I think it returns 1.0 and then it goes back to 0.x when it receives some header.

Maybe that's how it is used at the moment, but the documentation of the function implies that the estimate can be used on any block, not that pindex necessarily is the latest block.

As long as we have good headers using any block should not be a problem and should return a reasonable estimate. We could adjust it with something like this:

const CBlockIndex* active_tip = ActiveChain().Tip();
    int64_t end_of_chain_timestamp = TicksSinceEpoch<std::chrono::seconds>(NodeClock::time_point{std::chrono::seconds{active_tip ? active_tip->GetBlockTime() : pindex->GetBlockTime()}});

But I'm quite sure that the behavior will be the same. If pindex is not the ActiveChain().Tip() at least the best header will be the tip one, right?

That seems bad to me. Being isolated from the network shouldn't make it report values that far off! The old current-time-based estimate might have edge cases around 1, but apart from that it handles this better.

A possible solution could be to keep the new estimation method, which allows us to reach 1.0 naturally without rounding. However, to handle cases where the node is isolated and lacks up-to-date headers, we could fall back to a time-based estimation when:

• we don't have good headers
• pindex is older than N hours (e.g. 2h)

This ensures that an isolated node doesn’t immediately assume it's fully synced but still allows reasonable progress estimation when there is no better data available.
(I implemented this already, will shut down a node and wait to be some blocks behind, will turn it on again and see if it shows 1.0. or estimates 0.x before being connected to any peer. I will edit this comment with the results)

When you next update, please add the corresponding run-time lock assertion in the definition (see doc/developer-notes.md::L1003) that I overlooked in my diff.

diff --git a/src/validation.cpp b/src/validation.cpp
index bee19f917ce..ccc05c8054d 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -5615,6 +5615,8 @@ bool Chainstate::ResizeCoinsCaches(size_t coinstip_size, size_t coinsdb_size)
 
 double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex) const
 {
+    AssertLockHeld(::cs_main);
+
     if (pindex == nullptr) {

NoteToSelf: Check this before next push