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 (https://github.com/bitcoin/bitcoin/issues/28847#issuecomment-1807115357), but this is left for a more involved follow-up and may also be controversial.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32528.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | jonatack, jasonribble |
| Stale ACK | polespinasa, pinheadmz |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
ACK 11116aa19f5173792f757276c8a58b279f18a199
Built and tested on macos/arm64. Reviewed the code. Grudgingly I accept that this is a user complaint that will never go away and since the PR only updates the UI I’m ok with it after all.
guessVerificationProgress() will now return 1 if the timestamp in the chain tip is within the last two hours. (Probably should have a release note?)
That gives the following result in the log when I finished syncing my mainnet full node (at about 13:40:00 UTC):
0height=896951 date='2025-05-16T09:17:23Z' progress=0.999967
1height=896952 date='2025-05-16T09:27:05Z' progress=0.999969
2height=896953 date='2025-05-16T09:57:42Z' progress=0.999976
3height=896954 date='2025-05-16T10:02:10Z' progress=0.999977
4height=896955 date='2025-05-16T10:13:23Z' progress=0.999980
5height=896956 date='2025-05-16T10:14:26Z' progress=0.999980
6height=896957 date='2025-05-16T10:18:47Z' progress=0.999981
7height=896958 date='2025-05-16T10:42:15Z' progress=0.999986
8height=896959 date='2025-05-16T11:52:58Z' progress=1.000000
9height=896960 date='2025-05-16T11:54:12Z' progress=1.000000
10height=896961 date='2025-05-16T11:59:16Z' progress=1.000000
11height=896962 date='2025-05-16T11:59:17Z' progress=1.000000
12height=896963 date='2025-05-16T12:10:47Z' progress=1.000000
13height=896964 date='2025-05-16T12:13:21Z' progress=1.000000
14height=896965 date='2025-05-16T12:38:33Z' progress=1.000000
15height=896966 date='2025-05-16T12:38:43Z' progress=1.000000
16height=896967 date='2025-05-16T12:45:46Z' progress=1.000000
17height=896968 date='2025-05-16T12:47:09Z' progress=1.000000
18height=896969 date='2025-05-16T13:07:02Z' progress=1.000000
19height=896970 date='2025-05-16T13:19:45Z' progress=1.000000
The only other place the “guess” is called is during wallet rescan. I tried to fill up a wallet with descriptors and start a rescan to try and catch a “progress: 100%” in the logs but couldn’t nail it after a few attempts, which is a good thing. Those log messages look like this:
Still rescanning. At block 896884. Progress=0.999814Rescan interrupted by shutdown request at block 896735. Progress=0.999468I also tried a rescan in the GUI but its just a progress bar, no integer percentage and the last ~12 blocks are processed fast enough that I think even seeing a complete-looking status for a few extra seconds is worth that glowing smile on the user’s face ;-)
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA256
2
3ACK 11116aa19f5173792f757276c8a58b279f18a199
4-----BEGIN PGP SIGNATURE-----
5
6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmgnRqEACgkQ5+KYS2KJ
7yToGIQ//Ysxl/euuLbz/Wq/F2abHwOb/jOcKVHe8kZjtYokr2yCENPWA+RfqNtKP
8KKJZbsilvUa8YAFl683opJj0m92M7G+dU3m2Opq/+VwuVlrfF8AHHxYgV2MK3E+U
9csK4QjeHBrXsWsMKv31sEL7ogG/oQ67EMiuFPjTzQpNsnZDiRUPwRE4bSefmQ4Jw
10eS2m/Jp3bVxCjCweFamsUVg/WtNeIDPcxa95cIeY9WarWOsFqmZR2wGsirICDVvj
11K74AD6zq5YV7jMI8DVcYHGfd95/stydV+J9CA3XVeQQDHv1NtVcZQBGFpubT7k4O
12KlUVUKmFF1TfExAwfJYgY98u6NoXQ811XvTQgZh9WC1OmxWucyUdpVWsZbMMdFz2
13MhA46Z5lpgb23rPW57+U/MzczMFWKSS9jWCDF/9GA0PwZnox+73Wlrtk/A+Sgs1D
14HSv+6OYRrrw19rbzflEz1KN+ON0jxBVrtW5kl0xNmZmrJO5ON9jJS0aojZTT3kDT
152ON34yQjS9ySFVsxPQdsQ+JuNd0mfK/DZz0v5VbirEp/gSv3xczHDkfxSQz9cldY
16m01cq+jIaczpCKSvIQneO61f+Pfw4SixQm3TUv3NVSte4syAU811pyC74NLBxtDj
17qwMimuHlydKd4D0jPbqO3/lj85b6nuytIIenResP5aZmiLuh9f8=
18=Ub9B
19-----END PGP SIGNATURE-----
pinheadmz’s public key is on openpgp.org
progress=1.000000 before the last block as well?
I find that more confusing than 0.999999 for the last one.
Will this show
progress=1.000000before the last block as well?
How do you define “last block”? Or, more to the point, how does your full node “know” there are no more blocks out there? That’s always been the UX issue, it’s not accurate to ever claim we are 100% certain we are 100% synced.
After this PR, it will mean your chain tip’s timestamp is within 2 hours of now.
I think that’s a worse cure than the disease. In case we know there are unprocessed blocks in the chain (because we have their headers), we shouldn’t say progress: 1.
After this PR, it will mean your chain tip’s timestamp is within 2 hours of now.
I think that’s a worse cure than the disease. In case we know there are unprocessed blocks in the chain (because we have their headers), we shouldn’t say progress: 1.
I don’t think this is making anything worse. This is already possible on current master, when a miner sets the timestamp to a future one (compared to your time), even if there are still blocks ahead.
Edit: In such cases we’d even return a value larger than 1. This was fixed in 2f5f7d6b135e4eab368bbafd9e6e979aa72398de
5578+ // value does not matter anyway in relation to the full chain age. Thus it
5579+ // is fine to add a random offset, because some users really want to see a
5580+ // verificationprogress of exactly "1.0" once they are close to the tip.
5581+ // The progress is clamped before returning to at most "1.0", even if the
5582+ // miner set a timestamp off from the real time.
5583+ const auto block_time{pindex->GetBlockTime() + Ticks<std::chrono::seconds>(2h)};
In case we know there are unprocessed blocks in the chain (because we have their headers), we shouldn’t say progress: 1
I agree we should not return 1 if we didn’t verify the last block we know (we don’t want Core to lie to users 😉)
What if we do something like:
0const auto block_time{(pindex->nHeight == m_best_header->nHeight && pindex->nChainWork == m_best_header->nChainWork)
1 ? pindex->GetBlockTime() + Ticks<std::chrono::seconds>(2h)
2 : pindex->GetBlockTime()};
So we return 1 if we are in the best known header and inside the two hours offset.
02025-05-19T22:32:55Z UpdateTip: new best=000000000000000000007de21c2c10adec3376d6f0c69f24ba143dc572497b87 height=897462 version=0x202c2000 log2_work=95.617541 tx=1192584903 date='2025-05-19T22:05:52Z' progress=0.999994 cache=111.4MiB(825445txo)
12025-05-19T22:32:55Z UpdateTip: new best=00000000000000000001efdca54c71c76ae3c06b54598ff50fc20b48a01149ab height=897463 version=0x292e4000 log2_work=95.617554 tx=1192586948 date='2025-05-19T22:07:05Z' progress=1.000000 cache=111.9MiB(829042txo)
0$ bitcoin-cli getblockchaininfo
1 "chain": "main",
2 "blocks": 897463,
3 "headers": 897463,
4....
5 "verificationprogress": 1,
6...
What’s the benefit of doing it this way? /curi
By checking if the last block verified is the one matching the last best header known, we avoid previous blocks inside the 2h offset return verificationprogress=1.
But doesn’t the current code already solve the problem?
No, maflcko proposal returns verificationprogress=1 when the last verified block is within a 2h offset of our local time.
lie to users
Again, this can happen on current master already. Also, it can happen with your suggested diff. The only reliable way to not rely on the exact value of remotely miner-set timestamps would be to not use them, but that is a more involved patch.
Also, it can happen with your suggested diff
Oh, you’re right if the miner sets the timestamp to a future one. Maybe we could just limit the maximum value to something like 0.999... until we verified the last block. That’s hard coding a bit the values, but as it’s only for a greater UX I don’t see much problem doing that.
That could probably be done by using the local_time - 1 if block_time >= local_time and best_header_hash != block_hash. Block timestamps should not be trusted at all as they are kinda flexible, so I guess it’s ok to “manipulate” the values a bit.
Also add a test.
This requires a new lock annotation, but all relevant callers already
held the lock.
5583+ // When the header is known to be recent, switch to a height-based
5584+ // approach. This ensures the returned value is quantized when
5585+ // close to "1.0", because some users expect it to be. This also
5586+ // avoids relying too much on the exact miner-set timestamp, which
5587+ // may be off.
5588+ nNow - (m_best_header->nHeight - pindex->nHeight) * GetConsensus().nPowTargetSpacing :
I guess GetConsensus().nPowTargetSpacing returns 10min?
If so, could we use this even if the block being validated is not within the 2h range? I think we could just never rely on miner-set timestamps.
I wanted to avoid long-range estimates based on block height, due to dTxRate being based on time (number per second), but I guess everything is an estimate anyway and dTxRate could be changed to be a number of txs per block if it mattered.
However, I don’t see the benefit of your suggestion. If miner-set timestamps are off long-range, this debug stat is the least thing to worry about. Also, the fallback time-based code will have to be kept for IBD-catch-up scenarios.
tACK fa53098472521de9d5b3cc42983043c240b7c321
Note: The failing test rpc_signer.py is working for me locally
Left a small comment
3766@@ -3766,6 +3767,9 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde
3767 }
3768
3769 InvalidChainFound(to_mark_failed);
3770+ if (pindex_was_in_chain) {
3771+ failed_prev_verification_progress = m_chainman.GuessVerificationProgress(to_mark_failed->pprev);
3772+ }
fa53098472521de9d5b3cc42983043c240b7c321
Why is this here and not in the next block that is already guarded by pindex_was_in_chain? to_mark_failed and m_chainman are still in scope.
One thing that’s a little weird about this is that when I open regtest after a few days…
0--> bccli -regtest -getinfo
1Chain: regtest
2Blocks: 202
3Headers: 202
4Verification progress: ▒▒▒▒▒▒░░░░░░░░░░░░░░░ 27.7195%
Of course after generating 1 block at time=now, it snaps to 100%
409@@ -409,6 +410,7 @@ class NodeImpl : public Node
410 std::unique_ptr<Handler> handleNotifyBlockTip(NotifyBlockTipFn fn) override
411 {
412 return MakeSignalHandler(::uiInterface.NotifyBlockTip_connect([fn, this](SynchronizationState sync_state, const CBlockIndex* block) {
413+ LOCK(chainman().GetMutex());
handleNotifyBlockTip() taking a lock now where it didn’t before?
Also this is somewhat pathalogical but…
0
1--> bccli -regtest setmocktime 1719861124 # July 1, 2024
2--> bccli -regtest -getinfo
3Chain: regtest
4Blocks: 203
5Headers: 203
6Verification progress: -0.7451%
when I open regtest after a few days…
Maybe we should force this code to work only in mainnet? It doesn’t make sense the time considerations for regtest. Maybe testnet and signet could still use it.
Approach ACK fa53098472 (still reviewing code changes around blocktip notifications)
With fresh brain this morning I think this is the best possible approach if we make any change at all. The regtest stuff I observed is expected behavior - if there’s no block for 2 days then yes you are 288 blocks behind and progress should estimate that. The new chainparams define regtest as having an expected 0.001 tx/s which is approximately equal to one (coinbase) tx every ten minutes.
Example catching up on mainnet looks good:
0
1date='2025-05-22T12:45:54Z' progress=0.999979
2date='2025-05-22T12:47:48Z' progress=0.999981
3date='2025-05-22T12:51:57Z' progress=0.999984
4date='2025-05-22T13:01:53Z' progress=0.999986
5date='2025-05-22T13:17:07Z' progress=0.999988
6date='2025-05-22T13:31:46Z' progress=0.999991
7date='2025-05-22T13:39:06Z' progress=0.999993
8date='2025-05-22T13:46:58Z' progress=0.999995
9date='2025-05-22T13:56:12Z' progress=0.999998
10date='2025-05-22T14:01:13Z' progress=1.000000
ACK fa53098472521de9d5b3cc42983043c240b7c321
Tested and reviewed code. Might warrant a release note: “Verification progress will be 100% if all known blocks are verified and timestamped within the last two hours when previously a stale tip might result in progress < 100%”.
In addition to this UI change, block tip signal callers now compute the estimated progress as opposed to the signal handlers, and CBlockIndex are being passed by reference instead of raw pointer through the signal-handler chain.
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA256
2
3ACK fa53098472521de9d5b3cc42983043c240b7c321
4-----BEGIN PGP SIGNATURE-----
5
6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmgvclwACgkQ5+KYS2KJ
7yTrUsA//QL0jGfEKqsy8PhRtfS2CqyFQMS+VTDeSeBofJHv0El1Tuwq5fMiLNG5U
8O3twA1SIvCSGJOjFlsDxvgb4PnxqnR5ioHH31aN1uZK1+wN/GW5k8mDYg5ul2kWP
9nfZ031w/z93yftyuB+APqqKaZsdVzhVKzw97yscvaPCIOO8kzUN1DoQySfVX6L/V
10WVHGaAbXk8eE1qWReWeux/AWy8PwDZMjCfNZkWggpexFyRPVNn8JFPGvG7xrUjWe
114qtsrK3juuw9UljForsm2ibGe7QtXw7jTPh/OTCyYQvJoNDgN/2/Z9I00TI8l2bK
12TX9DpXUXyxDglEDRvDXybHmmPJrsJquDvqQPIr6zv//PsSM/WKs6jwX5p0J8DB5b
13v+cWGstX4rlvlEWHx5djk8+NKL3Eb377kq/Rn2EpNXpvtsLvlTIG4OS3TkmkTReX
14WGvFfbGabk+CxeD0Sntg5pmmDMdFem2YRIekasmjB7M5AJQrn51YL8PyeMaseQHG
15EBToUfNzXR4o3pJtorso3cIG5QTAeL9yvvQC7+VfmFyxYLv9TOThMBROHvabjotr
16a4H9GPITfJwBluKfA6qqgFg2j1g2bOLB6gpkvzwGFTQI83O+myQxZexmvUzD419b
17C6uMVolX85w26iTY+YuUB6cCXzkl4/O21vj6Oe1Wqgy0N7QDVOo=
18=qaQr
19-----END PGP SIGNATURE-----
pinheadmz’s public key is on openpgp.org
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.
Might warrant a release note: “Verification progress will be 100% if all known blocks are verified and timestamped within the last two hours when previously a stale tip might result in progress < 100%”.
I consider the changes here only stylistic (similar to changing the bitcoin-cli -color=... colours). No one should be relying on the informational-only value anyway.
One thing that’s a little weird about this is that when I open regtest after a few days…
0--> bccli -regtest -getinfo 1Chain: regtest 2Blocks: 202 3Headers: 202 4Verification progress: ▒▒▒▒▒▒░░░░░░░░░░░░░░░ 27.7195%Of course after generating 1 block at time=now, it snaps to 100%
Maybe instead of not using this new approach on regtest (as I suggested here #32528 (comment)) we could just add a message so users can understand why this happens?
The actual result can be really confusing if you don’t know how the % is calculated.