rpc, cli: return "verificationprogress" of 1 when up to date #31135
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. |
when the block height equals the headers height
when the block height equals the headers height, and update the verificationprogress help to that of getblockchaininfo.
f6472d6 to
b27e20d
Compare
| data.pushKV("bestblockhash", tip->GetBlockHash().GetHex()); | ||
| data.pushKV("difficulty", GetDifficulty(*tip)); | ||
| data.pushKV("verificationprogress", GuessVerificationProgress(Params().TxData(), tip)); | ||
| data.pushKV("verificationprogress", height == headers ? 1 : GuessVerificationProgress(Params().TxData(), tip)); |
There was a problem hiding this comment.
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.
|
i think doing this is fine, it's reported often enough--however, wouldn't it make sense to implement the functionality in |
maflcko
left a comment
There was a problem hiding this comment.
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?
| data.pushKV("bestblockhash", tip->GetBlockHash().GetHex()); | ||
| data.pushKV("difficulty", GetDifficulty(*tip)); | ||
| data.pushKV("verificationprogress", GuessVerificationProgress(Params().TxData(), tip)); | ||
| data.pushKV("verificationprogress", height == headers ? 1 : GuessVerificationProgress(Params().TxData(), tip)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
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.
I believe this avoids the need for any special casing for "1" (and can even get rid of the @maflcko I think this would address the problems you point out? |
|
Maybe #31127 might be marked as Good First Issue? |
|
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 |
|
Hi, I tried to go with a proposal on this based on the comments given here #31177 |
in getblockchaininfo/-getinfo and getchainstates, as requested in issues #31127 and #26433. Verification progress estimates in the debug logging remain unchanged.