rpc, logging: return "verificationprogress" of 1 when up to date

polespinasa commented at 12:21 pm on October 29, 2024: contributor

getblockchaininfo never reaches 1.0 as reported in issue #31127. This PR is based on the reviews given on #31135.

DrahtBot commented at 12:21 pm on October 29, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31177.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Approach ACK	jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

polespinasa marked this as a draft on Oct 29, 2024

DrahtBot added the label CI failed on Oct 29, 2024

DrahtBot commented at 12:29 pm on October 29, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32216509210

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

jonatack commented at 2:31 pm on October 29, 2024: member

Concept ACK, some ideas to encourage review:

make sure your proposal compiles in your local environment, with passing unit and functional tests, so updating all the callers of GuessVerificationProgress. This makes it easier for reviewers to build and test your code and see if any test coverage needs to be updated or possibly added. Some reviewers may also wait until your pull has a green CI as enough proof of work before reviewing it more closely.
organize your commits into final form
each separate commit needs to build cleanly with passing tests
you might need a release note (see #31135)
the debug logging is now changed as well, so perhaps prefix the PR title with rpc, logging:

This article might be helpful.

polespinasa renamed this:
~~rpc, cli: return "verificationprogress" of 1 when up to date~~
rpc, login: return "verificationprogress" of 1 when up to date
on Oct 29, 2024

polespinasa renamed this:
~~rpc, login: return "verificationprogress" of 1 when up to date~~
rpc, logging: return "verificationprogress" of 1 when up to date
on Oct 29, 2024

polespinasa force-pushed on Oct 30, 2024

polespinasa force-pushed on Oct 31, 2024

DrahtBot removed the label CI failed on Oct 31, 2024

polespinasa commented at 6:09 pm on October 31, 2024: contributor

Some calls to the function have duplicated the last argument to pass the tests, but it is necessary to see how to correctly pass the best_header.

in src/validation.cpp:5642 in 95e6ae16ac outdated

5586     } else {
5587-        fTxTotal = pindex->m_chain_tx_count + (nNow - pindex->GetBlockTime()) * data.dTxRate;
5588+        fTxTotal = pindex->m_chain_tx_count + (end_of_chain_timestamp - pindex->GetBlockTime()) * data.dTxRate;
5589     }
5590-
5591-    return std::min<double>(pindex->m_chain_tx_count / fTxTotal, 1.0);

maflcko commented at 6:25 pm on October 31, 2024:

Why is this clamping removed? Blocks can have a time in the future, and the changes in this pull don’t change it, so it still seems possible to be hit? If not, it would be good explain why, or a unit test could be added to show that a block time in the future doesn’t lead to a value larger than 1.

polespinasa commented at 6:40 pm on November 6, 2024:

You are right. If data.nTime is bigger than end_of_chain_timestamp we could go into the case where fTxTotal is smaller than pindex->m_chain_tx_count .

Maybe @sipa can explain why thinks that after the changes this clamp could be removed? As mentioned in #31135 (comment).

For the moment, I will add it again. Thanks for the observation!

in src/validation.cpp:5632 in 95e6ae16ac outdated

5567@@ -5568,17 +5568,23 @@ double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pin
5568         return 0.0;
5569     }
5570 
5571-    int64_t nNow = time(nullptr);
5572+    int64_t end_of_chain_timestamp = pindex->GetBlockTime();

maflcko commented at 6:27 pm on October 31, 2024:

Why is this fallback reasonable? It would be good to explain why, or remove it (require the argument to be passed). An alternative would be to make the function a member method on chainman. This way the call site doesn’t have to pass hindex every time. If hindex is supposed to be different for different calls, it would be good to explain why.

polespinasa commented at 12:56 pm on November 8, 2024:

Hi, I’m not sure if I understood you correctly, but I guess you say it because it’s possible to call the function with hindex = nullptr.

To solve that I propose this changes:

 0double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex* pindex, const CBlockIndex* hindex) {
 1    if (pindex == nullptr || hindex == nullptr)
 2        return 0.0;
 3...
 4    int64_t end_of_chain_timestamp;
 5
 6    if (hindex->nChainWork > pindex->nChainWork) {
 7        int64_t header_age = time(nullptr) - hindex->GetBlockTime();
 8        end_of_chain_timestamp = header_age < 24 * 60 * 60 ? std::max(pindex->GetBlockTime(), hindex->GetBlockTime()) : pindex->GetBlockTime();
 9    } else {
10        end_of_chain_timestamp = pindex -> GetBlockTime();
11    }
12...

Please let me know your thoughts

maflcko commented at 1:21 pm on November 8, 2024:

Hi, I’m not sure if I understood you correctly, but I guess you say it because it’s possible to call the function with hindex = nullptr.

I am asking why it needs to be passed at all. Shouldn’t it be the same for all callers?

polespinasa commented at 1:54 pm on November 8, 2024:

Oh, understood, yes, I think so. But it’s the way to not move the function inside chainman and keep the actual structure. Do you think is more reasonable move it inside?

DrahtBot added the label Needs rebase on Dec 13, 2024

DrahtBot commented at 10:21 am on January 27, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36213790312

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label CI failed on Jan 27, 2025

DrahtBot removed the label Needs rebase on Jan 27, 2025

polespinasa force-pushed on Jan 27, 2025

polespinasa commented at 11:20 am on January 27, 2025: contributor

This was refactored to work inside Chainman (https://github.com/bitcoin/bitcoin/pull/31393) thanks to @maflcko work.

polespinasa marked this as ready for review on Jan 27, 2025

polespinasa requested review from jonatack on Jan 27, 2025

polespinasa requested review from maflcko on Jan 27, 2025

DrahtBot removed the label CI failed on Jan 27, 2025

rpc, logging: move estimate progress from based on time to based on best header chain 6d877a19f9

polespinasa force-pushed on Jan 28, 2025

polespinasa commented at 11:24 pm on January 29, 2025: contributor

@sipa and @laanwj pinging you here as this PR is based on your comments on #31135

This has been tested on mainnet and seems to be working with the solution proposed:

 0bitcoin-cli -rpcport=8332 getblockchaininfo
 1{
 2  "chain": "main",
 3  "blocks": 881415,
 4  "headers": 881415,
 5...
 6  "verificationprogress": 1,
 7  "initialblockdownload": false,
 8...
 9  "warnings": [
10    "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
11  ]
12}

jonatack commented at 3:53 am on February 1, 2025: member

Thanks for updating – will review.

in src/validation.cpp:5621 in 6d877a19f9 outdated

5617+//! Guess how far we are in the verification process at the given block index and the best headers chain
5618 //! require cs_main if pindex has not been validated yet (because m_chain_tx_count might be unset)
5619 double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex) const
5620 {
5621     const ChainTxData& data{GetParams().TxData()};
5622+    LOCK(::cs_main);

jonatack commented at 8:53 pm on February 3, 2025:

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

 0diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
 1index 73ce927f712..46fe9e10b0f 100644
 2--- a/src/node/interfaces.cpp
 3+++ b/src/node/interfaces.cpp
 4@@ -325,7 +325,8 @@ public:
 5     }
 6     double getVerificationProgress() override
 7     {
 8-        return chainman().GuessVerificationProgress(WITH_LOCK(chainman().GetMutex(), return chainman().ActiveChain().Tip()));
 9+        LOCK(chainman().GetMutex());
10+        return chainman().GuessVerificationProgress(chainman().ActiveChain().Tip());
11     }
12     bool isInitialBlockDownload() override
13     {
14@@ -409,7 +410,7 @@ public:
15     {
16         return MakeSignalHandler(::uiInterface.NotifyBlockTip_connect([fn, this](SynchronizationState sync_state, const CBlockIndex* block) {
17             fn(sync_state, BlockTip{block->nHeight, block->GetBlockTime(), block->GetBlockHash()},
18-               chainman().GuessVerificationProgress(block));
19+               WITH_LOCK(chainman().GetMutex(), return chainman().GuessVerificationProgress(block)));
20         }));
21     }
22     std::unique_ptr<Handler> handleNotifyHeaderTip(NotifyHeaderTipFn fn) override
23diff --git a/src/validation.cpp b/src/validation.cpp
24index 5dca359aedf..54e85e07a6d 100644
25--- a/src/validation.cpp
26+++ b/src/validation.cpp
27@@ -5613,12 +5613,8 @@ bool Chainstate::ResizeCoinsCaches(size_t coinstip_size, size_t coinsdb_size)
28     return ret;
29 }
30 
31-//! Guess how far we are in the verification process at the given block index and the best headers chain
32-//! require cs_main if pindex has not been validated yet (because m_chain_tx_count might be unset)
33 double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex) const
34 {
35-    const ChainTxData& data{GetParams().TxData()};
36-    LOCK(::cs_main);
37     if (pindex == nullptr) {
38         return 0.0;
39     }
40@@ -5638,7 +5634,7 @@ double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex) c
41     }
42 
43     double fTxTotal;
44-
45+    const ChainTxData& data{GetParams().TxData()};
46     if (pindex->m_chain_tx_count <= data.tx_count) {
47         fTxTotal = data.tx_count + (end_of_chain_timestamp - data.nTime) * data.dTxRate;
48     } else {
49diff --git a/src/validation.h b/src/validation.h
50index 9e4fdbe6809..2223dffc249 100644
51--- a/src/validation.h
52+++ b/src/validation.h
53@@ -1149,7 +1149,7 @@ public:
54     bool IsInitialBlockDownload() const;
55 
56     /** Guess verification progress (as a fraction between 0.0=genesis and 1.0=current tip). */
57-    double GuessVerificationProgress(const CBlockIndex* pindex) const;
58+    double GuessVerificationProgress(const CBlockIndex* pindex) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
59 
60     /**
61      * Import blocks from an external file

polespinasa commented at 10:08 pm on February 3, 2025:

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

jonatack commented at 8:54 pm on February 3, 2025: member

Approach ACK

cleanup and locking improvement ef15351b76

use buid-in time functionality 317ddca8e6

in src/validation.cpp:5635 in 6d877a19f9 outdated

5628@@ -5629,16 +5629,22 @@ double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex) c
5629         return 0.0;
5630     }
5631 
5632-    int64_t nNow = time(nullptr);
5633+    int64_t end_of_chain_timestamp = pindex->GetBlockTime();
5634+
5635+    if (m_best_header && m_best_header->nChainWork > pindex->nChainWork) {
5636+        int64_t header_age = time(nullptr) - m_best_header->GetBlockTime();

sipa commented at 9:17 pm on February 3, 2025:

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

polespinasa commented at 9:42 pm on February 3, 2025:

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

laanwj requested review from laanwj on Feb 4, 2025

in src/validation.cpp:4735 in 317ddca8e6

4731@@ -4732,7 +4732,6 @@ bool Chainstate::LoadChainTip()
4732         // Ignoring return value for now.
4733         (void)m_chainman.GetNotifications().blockTip(GetSynchronizationState(/*init=*/true, m_chainman.m_blockman.m_blockfiles_indexed), *pindex);
4734     }
4735-

laanwj commented at 9:37 am on February 4, 2025:

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

in src/validation.cpp:5617 in 317ddca8e6

5612@@ -5614,11 +5613,8 @@ bool Chainstate::ResizeCoinsCaches(size_t coinstip_size, size_t coinsdb_size)
5613     return ret;
5614 }
5615 
5616-//! Guess how far we are in the verification process at the given block index

laanwj commented at 9:38 am on February 4, 2025:

~~Why delete this comment?~~ Edit: okay i think it’s fine, you’re adding EXCLUSIVE_LOCKS_REQUIRED(::cs_main) in the header and this description is already in the header too.

jonatack commented at 2:12 pm on February 4, 2025:

Yes, I suggested these in #31177 (review).

in src/validation.cpp:5629 in 317ddca8e6

5624@@ -5629,16 +5625,21 @@ double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex) c
5625         return 0.0;
5626     }
5627 
5628-    int64_t nNow = time(nullptr);
5629+    int64_t end_of_chain_timestamp = TicksSinceEpoch<std::chrono::seconds>(NodeClock::time_point{std::chrono::seconds{pindex->GetBlockTime()}});
5630+    if (m_best_header && m_best_header->nChainWork > pindex->nChainWork) {

laanwj commented at 9:44 am on February 4, 2025:

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?

polespinasa commented at 9:26 am on February 5, 2025:

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:

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

@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?

laanwj commented at 9:57 am on February 5, 2025:

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.

jonatack commented at 1:14 pm on February 5, 2025:

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.

0Blocks: 882404
1Headers: 882404
2Verification progress: 100.0000%

maflcko commented at 1:19 pm on February 5, 2025:

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 (review)

polespinasa commented at 2:47 pm on February 5, 2025:

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.

polespinasa commented at 3:04 pm on February 5, 2025:

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:

0const CBlockIndex* active_tip = ActiveChain().Tip();
1    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)

in src/validation.h:1152 in 317ddca8e6

1148@@ -1149,7 +1149,7 @@ class ChainstateManager
1149     bool IsInitialBlockDownload() const;
1150 
1151     /** Guess verification progress (as a fraction between 0.0=genesis and 1.0=current tip). */
1152-    double GuessVerificationProgress(const CBlockIndex* pindex) const;
1153+    double GuessVerificationProgress(const CBlockIndex* pindex) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

jonatack commented at 1:25 pm on February 5, 2025:

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.

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

polespinasa commented at 5:06 pm on February 5, 2025:

NoteToSelf: Check this before next push

ajtowns commented at 3:51 pm on February 5, 2025: contributor

Given that we do headers sync first these days, would it make sense to switch to reporting:

verificationprogress = just blocks/headers
headerrecency = max(0, now() - timestamp of best header)

Normally, verificationprogress will be 1.0; dropping to ~0.9999988 briefly when a new header comes in, and headerrecency will tend to be in the ~10 minute range. OTOH, if you’re in IBD, then verificationprogress will report how far through validating the blocks you have headers for; and if you’ve been disconnected from the network, verificationprogress will sit at 1.0 but headerrecency will tell you it’s been days since you’ve seen a new header instead of minutes. When your node comes online after being down for a day, verificationprogress will initially be 1, but headerrecency will be 24 hours; as you sync headers, that will change to 0.9998 and ~10 minutes, then as you validate blocks it’ll move to 1.0 and ~10 minutes.

That seems a lot easier to calculate, and maybe also easier to interpret?

polespinasa commented at 5:05 pm on February 5, 2025: contributor

verificationprogress = just blocks/headers

I think the problem doing this is that not all blocks “are worth the same”. So when doing IBD we would go fast to high % and then slow down a lot because newer blocks have more transactions. If we want the % to be accurate by the time we should still measure it by txs. E.g. The first 50k blocks would be around 5% using this approach, but it takes just 1-2min to sync them as they are empty. This could lead to a fake feeling of fast sync. Also, we’re already reporting blocks and headers so right now the user can see the progress using that metric.

headerrecency = max(0, now() - timestamp of best header)

I like this approach and it’s more or less what I’m trying to take into account in this other comment #31177 (review)

maflcko commented at 5:52 pm on February 5, 2025: member

verificationprogress = just blocks/headers

I think the problem doing this is that not all blocks “are worth the same”

I think the suggestion is to keep the “tx weight” estimation, not change it. The suggestion is basically to split the “future headers estimation” from the verification progress estimation of a block in a known headers chain and return two floats.

(I wanted to suggest the same, because I was certain that this was suggested earlier already, but I couldn’t find an earlier mention, so I gave up)

ajtowns commented at 2:49 am on February 6, 2025: contributor

verificationprogress = just blocks/headers

I think the problem doing this is that not all blocks “are worth the same”

I think the suggestion is to keep the “tx weight” estimation, not change it. The suggestion is basically to split the “future headers estimation” from the verification progress estimation of a block in a known headers chain and return two floats.

Sure, let’s pretend that’s what I said :) That would be:

t = txs in blocks we’ve verified
h = height of best header
b = height of tip
r = expected tx rate per block
if t > data.tx_count
- total = t + (h-b)*r,
- else total = data.tx_count + (max(h, data.tx_height)-data.tx_height)*r)
progress = t / total
recency = now - min(now, best_header_timestamp)

maflcko commented at 10:12 am on February 6, 2025: member

On a second thought, the GUI uses the derivative (progress increase per hour), so whenever progress decreases, the GUI will calculate a negative increase. Then, likely the same people that complain about the progress not being 1 will complain about the increase being negative (or zero).

I guess this scenario can already happen post-minchainwork-ibd, but it will probably happen more often when progress is just based on available headers, given that it is faster to download headers than to verify blocks (even early in ibd).

So taking a step back, as the progress relies on the block time in the edge case scenario where users want the progress being reported as 1, and that block time is set by a miner and may be off anyway, I wonder if a simpler fix would be to just take a constant offset of the block time of a few hours. Obviously this fix is arbitrary, but it seems nice, because:

It documents for the purposes of this function that the exact time can’t be trusted and doesn’t matter.
It fixes the issue with a simple one-line fix.

Also, I can’t see any downsides?

polespinasa commented at 11:31 am on February 6, 2025: contributor

@maflcko I’m not sure I understand your proposal correctly. You’re proposing this offset on the original function version or the “new” one?

Would it get to 1.0 if we set a constant offset on all blocks?

I think this idea is kinda similar to this one: #31177 (review) but in this case I was thinking of applying it when the block time is far behind the local time.

maflcko commented at 12:13 pm on February 6, 2025: member

Just a simple one-line patch to offset GetBlockTime with a constant number of hours on top of the code in current master. I don’t think there needs to be any special casing?

polespinasa commented at 3:45 pm on May 5, 2025: contributor

I will close this PR for the moment as I don’t think I will get enough ACKs with the current approach. Will be happy to review new approaches to solve this.

polespinasa closed this on May 5, 2025

maflcko commented at 11:27 am on May 16, 2025: member

Just a simple one-line patch to offset GetBlockTime with a constant number of hours on top of the code in current master. I don’t think there needs to be any special casing?

Went ahead and just implemented that in https://github.com/bitcoin/bitcoin/pull/32528

polespinasa deleted the branch on Jun 9, 2025

rpc, logging: return “verificationprogress” of 1 when up to date #31177

Code Coverage & Benchmarks

Reviews

Conflicts