GuessVerificationProgress: cap the ratio to 1 #17328

pull darosior wants to merge 1 commits into bitcoin:master from darosior:fixup_getblockchaininfo changing 1 files +1 −1

darosior commented at 2:52 PM on October 31, 2019: member

Noticed getblockchaininfo would return a verificationprogress > 1, especially while generating. This caps the verification progress to 1.

Tried to append a check to functional tests but this would pass even without the patch, so it seems better to not add a superfluous check (but this can easily be reproduced by trying to generate blocks in the background and watching getblockchainfo).
fanquake added the label Validation on Oct 31, 2019
MarcoFalke commented at 3:55 PM on October 31, 2019: member

Could add a unit test that fails without this patch? This should only happen in testnet when the block is mined 20 minutes in the future, right?

GuessVerificationProgress: cap the ratio to 1

The getblockchaininfo RPC call could sometime return a
'validationprogress' > 1, but this is absurd.

2f5f7d6b13

in src/validation.cpp:5071 in fb10c0c007 outdated
```
5065 | @@ -5066,6 +5066,8 @@ double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pin
5066 |          fTxTotal = pindex->nChainTx + (nNow - pindex->GetBlockTime()) * data.dTxRate;
5067 |      }
5068 |  
5069 | +    if (pindex->nChainTx > fTxTotal) return 1;
5070 | +
5071 |      return pindex->nChainTx / fTxTotal;
```
laanwj commented at 4:06 PM on October 31, 2019:

please return 1.0 instead of 1 (more clear that it's a double/fraction) or maybe collapse it into one statement

return std::min(pindex->nChainTx / fTxTotal, 1.0);
MarcoFalke commented at 4:10 PM on October 31, 2019:

I think we use std::min<double>(...), so that the type of the args doesn't matter.

laanwj commented at 4:29 PM on October 31, 2019:

that's fine with me too; I just think the return 1; looks strange in a function that returns double

darosior commented at 4:34 PM on October 31, 2019:

Corrected, thanks!
darosior force-pushed on Oct 31, 2019
darosior commented at 4:39 PM on October 31, 2019: member

Could add a unit test that fails without this patch?

I could not manage to make it return verificationprogress > 1 in a clean way. I think launching a mining thread and polling getblockchaininfo would make it occur, but it seems way too overkill for a minor patch like this doesn't it ?

This should only happen in testnet when the block is mined 20 minutes in the future, right?

Hmmm it's true I noticed it on testnet (with the recent difficulty reset), I'll try on regtest.
laanwj commented at 9:22 AM on November 1, 2019: member

With regtest and mocked block times it ought to be possible.
darosior commented at 1:44 PM on November 1, 2019: member

Could not reproduce on regtest, the verificationprogress would always be 1 in all my tests case.
laanwj added the label Waiting for author on Nov 2, 2019
darosior commented at 11:52 AM on November 2, 2019: member

@laanwj why did you add the "Waiting for author" tag ?
laanwj commented at 12:00 PM on November 2, 2019: member

Waiting for you to add a test
darosior commented at 2:44 PM on November 2, 2019: member

@laanwj Finally I think there is something going on that impeach me to test the verificationprogress : it will always be 1, even during IBD (with transactions). Is this due to some regtest tweak I don't know about ? Anyway I think it's out of the scope of this PR and will open an issue to track this behavior.
darosior commented at 8:55 PM on November 4, 2019: member

@laanwj (sorry to ping you again..) is this still "waiting for author" since it seems untestable on regtest for now and that there is an issue to track this specific problem ?
laanwj commented at 9:33 AM on November 6, 2019: member

No, I think this is fine, you should look for more reviewers than me though.
laanwj removed the label Waiting for author on Nov 6, 2019
in src/validation.cpp:5069 in 2f5f7d6b13
```
5065 | @@ -5066,7 +5066,7 @@ double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pin
5066 |          fTxTotal = pindex->nChainTx + (nNow - pindex->GetBlockTime()) * data.dTxRate;
5067 |      }
5068 |  
5069 | -    return pindex->nChainTx / fTxTotal;
5070 | +    return std::min<double>(pindex->nChainTx / fTxTotal, 1.0);
```
promag commented at 10:02 AM on November 6, 2019:

A comment explaining the truncate would be nice.
promag commented at 10:07 AM on November 6, 2019:

An alternative is to truncate where it's needed, in L5064:

fTxTotal = std::max<double>(data.nTxCount + (nNow - data.nTime) * data.dTxRate, pindex->nChainTx);
laanwj commented at 10:39 AM on November 6, 2019:

IMO this is fine as-is

std::min(..., bound) is a common enough way to cap values, it doesn't need explaining on every usage site

it makes sense for "progress" values to never be able to get above 1.0 or below 0.0
promag commented at 10:07 AM on November 6, 2019: member

ACK 2f5f7d6b135e4eab368bbafd9e6e979aa72398de.
laanwj commented at 1:13 PM on November 18, 2019: member

ACK 2f5f7d6b135e4eab368bbafd9e6e979aa72398de
laanwj referenced this in commit 63fac52f31 on Nov 18, 2019
laanwj merged this on Nov 18, 2019
laanwj closed this on Nov 18, 2019
sidhujag referenced this in commit 52e0fb1a55 on Nov 18, 2019
sidhujag referenced this in commit d9b3fbd869 on Nov 10, 2020
darosior deleted the branch on Dec 3, 2020
Fabcien referenced this in commit dc7db5ed93 on Dec 24, 2020
PastaPastaPasta referenced this in commit 839ebcdfce on Jun 27, 2021
PastaPastaPasta referenced this in commit 7b21d1bef8 on Jun 28, 2021
PastaPastaPasta referenced this in commit 8bdf2ccd5a on Jun 29, 2021
PastaPastaPasta referenced this in commit 0bd888c4e4 on Jul 1, 2021
PastaPastaPasta referenced this in commit 391dd0bfae on Jul 1, 2021
PastaPastaPasta referenced this in commit 7e8f553515 on Jul 14, 2021
PastaPastaPasta referenced this in commit 01ca0e38df on Jul 14, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#17352 Verification progress tests