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

jonatack commented at 3:20 pm on October 22, 2024: member

in getblockchaininfo/-getinfo and getchainstates, as requested in issues #31127 and #26433. Verification progress estimates in the debug logging remain unchanged.

DrahtBot commented at 3:20 pm on October 22, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

laanwj added the label RPC/REST/ZMQ on Oct 22, 2024

rpc, cli: in getblockchaininfo/-getinfo, allow verificationprogress of 1

when the block height equals the headers height

47fab0b391

in src/rpc/blockchain.cpp:3113 in f6472d6a77 outdated

3109@@ -3109,7 +3110,7 @@ return RPCHelpMan{
3110         data.pushKV("blocks",                (int)chain.Height());
3111         data.pushKV("bestblockhash",         tip->GetBlockHash().GetHex());
3112         data.pushKV("difficulty", GetDifficulty(*tip));
3113-        data.pushKV("verificationprogress",  GuessVerificationProgress(Params().TxData(), tip));
3114+        data.pushKV("verificationprogress", validated ? 1 : GuessVerificationProgress(Params().TxData(), tip));

jonatack commented at 5:30 pm on October 22, 2024:

Note to self, using the “validated” field isn’t a good check here, best to use blockheight equals headers as in getblockchaininfo. Edit: done.

jonatack marked this as a draft on Oct 22, 2024

rpc: in getchainstates, allow verificationprogress of 1

when the block height equals the headers height, and update the
verificationprogress help to that of getblockchaininfo.

f9a04e7a75

doc: release note for RPC/CLI "verificationprogress" of 1 b27e20d1ee

jonatack force-pushed on Oct 22, 2024

jonatack marked this as ready for review on Oct 22, 2024

in src/rpc/blockchain.cpp:3115 in b27e20d1ee

3112-        data.pushKV("blocks",                (int)chain.Height());
3113+        data.pushKV("blocks", height);
3114         data.pushKV("bestblockhash",         tip->GetBlockHash().GetHex());
3115         data.pushKV("difficulty", GetDifficulty(*tip));
3116-        data.pushKV("verificationprogress",  GuessVerificationProgress(Params().TxData(), tip));
3117+        data.pushKV("verificationprogress", height == headers ? 1 : GuessVerificationProgress(Params().TxData(), tip));

sipa commented at 12:27 pm on October 23, 2024:

Would it make sense to pass chainman.m_best_header->nTime to GuessVerificationProgress() (which then uses that instead of the current time)? That would make the result always consistently report progress w.r.t. the headers tip, instead of w.r.t. the current time.

maflcko commented at 1:47 pm on October 23, 2024:

Not sure about this. Hard-coding the value to 1 when height == headers means that the value may inconsistently jump from 1 to 0.xxxx (in a loop), when headers pre-sync is disabled and the node is fed blocks one-by-one (header before block).

Also, if the node is eclipsed (intentionally, or just accidentally due to a network config error), this may also return 1, when the node is far behind the real network.

laanwj commented at 5:38 am on October 24, 2024:

it should probably only do the “round last bit” thing when it’s actually within some small delta of 1, not like from 0.5 to 1

laanwj commented at 1:02 pm on October 23, 2024: member

i think doing this is fine, it’s reported often enough–however, wouldn’t it make sense to implement the functionality in GuessVerificationProgress itself so that other places (like the GUI and logging) consistently show the same value?

maflcko commented at 1:48 pm on October 23, 2024: member

Not sure about the current fix. It seems to “fix” one style issue in a value that is meant as an estimate, but it may be adding two new issues at the same time?

sipa commented at 2:31 pm on October 23, 2024: member

So overall, this is effectively changing from estimating progress based on time, to estimating progress based on comparing with the best headers chain. I think that’s a reasonable thing to do, but I would suggest doing it more completely and cleanly, and add some safeguards.

Guess an “end of chain timestamp” as the maximum of:
- The timestamp of the active chain tip
- The best headers tip’s timestamp if it has more work than minchainwork; the current timestamp otherwise. There could also be a condition that the headers tip timestamp must not be more than 1 day old or so.
Use that timestamp in the progress estimation formula in GuessVerificationProgress (instead of nNow).
Do as much of that as possible inside GuessVerificationProgress (could pass it pointers to both active tip and headers tip), so that whatever we believe is the best way of estimating progress gets used everywhere.

I believe this avoids the need for any special casing for “1” (and can even get rid of the std::min inside GuessVerificationProgress to clamp at 1.0. @maflcko I think this would address the problems you point out?

jonatack commented at 7:03 pm on October 24, 2024: member

I agree with the approaches suggested by @sipa and @laanwj above and will be happy to review them. However, I am not confident of garnering sufficient ACKs if I implement those approaches here myself, so leaving it up for grabs.

jonatack closed this on Oct 24, 2024

laanwj added the label Up for grabs on Oct 24, 2024

edilmedeiros commented at 7:46 pm on October 24, 2024: contributor

Maybe #31127 might be marked as Good First Issue?

laanwj commented at 8:28 am on October 25, 2024: member

maybe… im not sure it’s a super good first issue, implementing it properly isn’t super complicated but does require some knowledge of how various parts work and interact

fanquake removed the label Up for grabs on Oct 29, 2024

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

Hi, I tried to go with a proposal on this based on the comments given here https://github.com/bitcoin/bitcoin/pull/31177

rpc, cli: return “verificationprogress” of 1 when up to date #31135

Code Coverage

Reviews