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

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

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

Some calls to the function GuessVerificationProgress remain unchanged pending comments on this proposal.

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
Concept 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

Reviewers, this pull request conflicts with the following ones:

#31393 (refactor: Move GuessVerificationProgress into ChainstateManager by maflcko)
#31346 (Set notifications m_tip_block in LoadChainTip() by Sjors)

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.

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

rpc, logging: move estimate progress from based on time to based on best header chain 95e6ae16ac

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: none

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:5581 in 95e6ae16ac

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:5571 in 95e6ae16ac

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 2:23 pm on December 13, 2024: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

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

Code Coverage & Benchmarks

Reviews

Conflicts