Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Oct 25, 2019

GuessVerificationProgress for a header (not a block) is always 0 because the number of txs in the block can not be determined from the header alone. Anyway, this result was never used, so we can optimize this call by hardcoding 0.

This is the next commit in a series of changes toward removing nChainTx (see #14863, #13875)

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, this result was never used

Isn't progress used here even for an header notification

progressBar->setValue(nVerificationProgress * 1000000000.0 + 0.5);

is always 0 because the number of txs in the block can not be determined from the header alone

Right, nChainTx is not known, but I fail to see how it's zero:

bitcoin/src/validation.cpp

Lines 5061 to 5076 in be50469

double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pindex) {
if (pindex == nullptr)
return 0.0;
int64_t nNow = time(nullptr);
double fTxTotal;
if (pindex->nChainTx <= data.nTxCount) {
fTxTotal = data.nTxCount + (nNow - data.nTime) * data.dTxRate;
} else {
fTxTotal = pindex->nChainTx + (nNow - pindex->GetBlockTime()) * data.dTxRate;
}
return pindex->nChainTx / fTxTotal;
}

@maflcko
Copy link
Member Author

maflcko commented Oct 27, 2019

Right, nChainTx is not known, but I fail to see how it's zero:

It is initialized to zero: https://dev.visucore.com/bitcoin/doxygen/class_c_block_index.html#af3c6d6dd8a7579e5ce516d94b98d2db5 and only set when we have the block downloaded. Without having the transactions downloaded, it is currently impossible to know how many txs there are in the block.

@maflcko
Copy link
Member Author

maflcko commented Oct 27, 2019

Then GuessVerificationProgress will return zero, because zero divided by something is still zero

@promag
Copy link
Contributor

promag commented Oct 27, 2019

It is initialized to zero

Code review ACK fa39809, missed that.

@laanwj
Copy link
Member

laanwj commented Oct 28, 2019

ACK fa39809

laanwj added a commit that referenced this pull request Oct 28, 2019
…yHeaderTip

fa39809 Avoid unused call to GuessVerificationProgress in NotifyHeaderTip (MarcoFalke)

Pull request description:

  `GuessVerificationProgress` for a header (not a block) is always 0 because the number of txs in the block can not be determined from the header alone. Anyway, this result was never used, so we can optimize this call by hardcoding 0.

  This is the next commit in a series of changes toward removing nChainTx (see #14863, #13875)

ACKs for top commit:
  promag:
    Code review ACK fa39809, missed that.
  laanwj:
    ACK fa39809

Tree-SHA512: 11016f8dbb1af1cf75241948d1ad35eac0c79d1311cd0db8c6ec806df2a9e3dc5f998dbd66ccbad5d84564e6cec7fe21ce7a2a13c2b34c746e2d3b31aa1db53a
@laanwj laanwj merged commit fa39809 into bitcoin:master Oct 28, 2019
@maflcko maflcko deleted the 1910-NoWrongGuess branch November 1, 2019 21:45
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 30, 2020
Summary:
> GuessVerificationProgress for a header (not a block) is always 0 because the number of txs in the block can not be determined from the header alone. Anyway, this result was never used, so we can optimize this call by hardcoding 0.
>
> This is the next commit in a series of changes toward removing nChainTx (see #14863, #13875)

This is a backport of Core [[bitcoin/bitcoin#17250 | PR17250]]

Test Plan: `ninja && ninja check-all`

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D8201
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants