use IsBlockPruned() where appropriate #13430

kallewoof commented at 7:05 AM on June 11, 2018: member

There are cases where a "full" check for block pruning is not done (fHavePruned and nTx > 0 checks), but where the context indicates that pruning is always the reason. This makes the checks more explicit and straightforward, and separates them more from the cases where we are simply checking whether we have a block or not vs whether we had it at one point and it was pruned.

Note that nTx > 0 is supposed to always be equivalent to (pindex->nStatus & BLOCK_HAVE_DATA). (Edit: got this backwards; this is true for the true case, but for pruned nodes, nTx > 0 while (pindex->nStatus & BLOCK_HAVE_DATA) is 0.)

in src/wallet/wallet.cpp:4286 in 106a2a5110 outdated

4276 | @@ -4277,11 +4277,11 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const std::string& name,
4277 |          //We can't rescan beyond non-pruned blocks, stop and throw an error
4278 |          //this might happen if a user uses an old wallet within a pruned node
4279 |          // or if he ran -disablewallet for a longer time, then decided to re-enable
4280 | -        if (fPruneMode)
4281 | -        {
4282 | +        if (fPruneMode) {
4283 |              CBlockIndex *block = chainActive.Tip();
4284 | -            while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA) && block->pprev->nTx > 0 && pindexRescan != block)
4285 | +            while (block && block->pprev && !IsBlockPruned(block->pprev) && pindexRescan != block) {

Empact commented at 7:11 AM on June 11, 2018:

This is slightly different because !IsBlockPruned is true when BLOCK_HAVE_DATA but from the immediate code alone nTx > 0 is not guaranteed. Is it the case that BLOCK_HAVE_DATA blocks are guaranteed to have nTx > 0?

Empact commented at 7:14 AM on June 11, 2018:

Just noting that I see you mention this in your notes: "Note that nTx > 0? is supposed to always be equivalent to (pindex->nStatus & BLOCK_HAVE_DATA)?."

kallewoof commented at 7:24 AM on June 11, 2018:

Yeah. if nTx = 0, it means you do not have data (nStatus & BHD), and if nTx > 0, it means you do have data, so the two checks are supposed to be equivalent, but I think they're done as a safety harness. That said, in this case the old code does block->pprev->nStatus & BLOCK_HAVE_DATA) && block->pprev->nTx > 0 which is identical to the !IsBlockPruned case, isn't it?

Empact commented at 7:45 AM on June 11, 2018:

It's not identical - via De Morgan's law:

IsBlockPruned: !(nStatus & BLOCK_HAVE_DATA) && nTx > 0
!IsBlockPruned: (nStatus & BLOCK_HAVE_DATA) || nTx <= 0

kallewoof commented at 8:15 AM on June 11, 2018:

Hm, you're right. I got the nTx > 0 part backwards... Eesh.

kallewoof commented at 8:26 AM on June 11, 2018:

The more I stare at the original line, the more I think the nTx part is backwards. I think they intended to check if it was pruned, but got the check to instead check if 'they have the data and the nTx > 0'. But if they have the data, nTx should always be > 0.

I.e.

for a pruned block, nTx > 0 but nStatus & BHD == 0
for a non-pruned block, nTx > 0 and nStatus & BHD != 0
for a block the node has not retrieved yet, nTx == 0 and nStatus & BHD == 0

I.e.

nTx > 0 nStatus & BHD != 0 case
======= ================== =====================
false   false              block data never seen
false   true               ?????
true    false              pruned block
true    true               block data available

The code checks that case 2 in the above table (false true) is not the case, but this check is unnecessary.

I think the !IsBlockPruned() replacement here is correct.

Empact commented at 9:06 AM on June 11, 2018:

Right, so my point is that, if the false true ????? case is relevant, then we need to exclude it. Looks like CChainState::CheckBlockIndex excludes that possibility (which is intuitive given coinbase exists at least), so I'm tending to agree with you: https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L4545

kallewoof commented at 9:27 AM on June 11, 2018:

I don't think it's ever possible for that case to happen, so I think !IsBlockPruned() does what we want here. I.e. it's not a relevant case. Note that this is a check that goes back in the blockchain and only stops if it encounters a missing block. That will only ever be the case if pruning is enabled, and will only ever be caused by pruning. Non-pruning nodes never have holes in the blockchain data (i.e. if they have block at height=x, they also have block at height=x-1 for x > 0).

in src/net_processing.cpp:2043 in 106a2a5110 outdated

2039 | @@ -2040,8 +2040,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2040 |              // If pruning, don't inv blocks unless we have on disk and are likely to still have
2041 |              // for some reasonable time window (1 hour) that block relay might require.
2042 |              const int nPrunedBlocksLikelyToHave = MIN_BLOCKS_TO_KEEP - 3600 / chainparams.GetConsensus().nPowTargetSpacing;
2043 | -            if (fPruneMode && (!(pindex->nStatus & BLOCK_HAVE_DATA) || pindex->nHeight <= chainActive.Tip()->nHeight - nPrunedBlocksLikelyToHave))
2044 | -            {
2045 | +            if (fPruneMode && (IsBlockPruned(pindex) || pindex->nHeight <= chainActive.Tip()->nHeight - nPrunedBlocksLikelyToHave)) {

Empact commented at 7:13 AM on June 11, 2018:

nit: Explicit precedence for chainActive.Tip()->nHeight - nPrunedBlocksLikelyToHave would be nice

kallewoof commented at 7:25 AM on June 11, 2018:

You mean ()? I don't think it's unclear that <= takes precedence over - (as opposed to e.g. && and ||)...

promag commented at 12:29 PM on June 11, 2018:

Could drop fPruneMode because fHavePruned is only true if fHavePruned is also true. Same below.

kallewoof commented at 7:13 AM on June 12, 2018:

Done.

Sjors commented at 8:04 AM on June 11, 2018: member

ConceptACK for improved readability, though for the same reason I can't easily tell if it's correct :-)

Empact commented at 9:12 AM on June 11, 2018: member

Concept ACK

laanwj added the label Refactoring on Jun 11, 2018

promag commented at 12:32 PM on June 11, 2018: member

As @sdaftuar pointed out before, the burden in reviewing validation changes may not be worth it, unless this is really incomplete. I don't think this improves readability that much, but I'm in the same position as @Sjors.

kallewoof force-pushed on Jun 12, 2018

kallewoof commented at 7:14 AM on June 12, 2018: member

@promag I feel like this makes it clear which of the two cases "do we have this block yet" versus "is this block pruned", which I believe improves readability enough to warrant the effort. If others think it's more trouble than it's worth I'll grumpily close, though. ;)

in src/wallet/wallet.cpp:4286 in 877c92bb9a outdated

4280 | @@ -4281,11 +4281,11 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const std::string& name,
4281 |          //We can't rescan beyond non-pruned blocks, stop and throw an error
4282 |          //this might happen if a user uses an old wallet within a pruned node
4283 |          // or if he ran -disablewallet for a longer time, then decided to re-enable
4284 | -        if (fPruneMode)
4285 | -        {
4286 | +        if (fPruneMode) {
4287 |              CBlockIndex *block = chainActive.Tip();
4288 | -            while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA) && block->pprev->nTx > 0 && pindexRescan != block)
4289 | +            while (block && block->pprev && !IsBlockPruned(block->pprev) && pindexRescan != block) {

promag commented at 5:47 PM on June 12, 2018:

nit, could drop first condition block && and add assert(block) before the loop, like https://github.com/bitcoin/bitcoin/pull/13430/files#diff-a0c8f511d90e83aa9b5857e819ced344R1243.

kallewoof commented at 11:18 PM on June 12, 2018:

block is updated inside the loop, so that sounds like it could be unsafe. (That said, not sure if there is ever a case where a CBlockIndex goes from being set to being unset, except for genesis block...)

Oh, and it is set to the already tested chainActive.Tip() so it should always start out non-null.

promag commented at 12:02 AM on June 13, 2018:

block is updated to block->pprev which is already tested.

kallewoof commented at 3:09 AM on June 15, 2018:

Ah, yes you're right. Done.

promag commented at 11:27 PM on June 26, 2018:

@sipa see above.

use IsBlockPruned() where appropriate 55e760f547

kallewoof force-pushed on Jun 15, 2018

Sjors commented at 8:36 AM on June 15, 2018: member

Don't worry too much about #13029; I'm waiting to see if #11082 makes it in, in which case that one can be closed.

in src/wallet/wallet.cpp:4286 in 55e760f547

4280 | @@ -4281,11 +4281,11 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(const std::string& name,
4281 |          //We can't rescan beyond non-pruned blocks, stop and throw an error
4282 |          //this might happen if a user uses an old wallet within a pruned node
4283 |          // or if he ran -disablewallet for a longer time, then decided to re-enable
4284 | -        if (fPruneMode)
4285 | -        {
4286 | +        if (fPruneMode) {
4287 |              CBlockIndex *block = chainActive.Tip();
4288 | -            while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA) && block->pprev->nTx > 0 && pindexRescan != block)
4289 | +            while (block->pprev && !IsBlockPruned(block->pprev) && pindexRescan != block) {

sipa commented at 11:20 PM on June 26, 2018:

Why remove block && here?

promag commented at 5:36 PM on July 4, 2018:

@sipa .Tip() is always not null (at this point), and block->pprev is next block value.

kallewoof commented at 10:24 PM on June 27, 2018: member

@sipa Github comment threading is buggy, but yes, see discussion above -- block is known to be non-null because it is checked (1) before entry and (2) at each loop in the form block->pprev.

DrahtBot commented at 8:28 PM on August 3, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14711 (Remove uses of chainActive and mapBlockIndex in wallet code by ryanofsky)
#10973 (Refactor: separate wallet from node by ryanofsky)

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.

DrahtBot closed this on Aug 10, 2018

DrahtBot commented at 12:37 PM on August 10, 2018: member

The last travis run for this pull request was 56 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

DrahtBot reopened this on Aug 10, 2018

MarcoFalke deleted a comment on Aug 10, 2018

kallewoof closed this on Dec 14, 2018

kallewoof deleted the branch on Dec 14, 2018

MarcoFalke commented at 6:30 PM on April 22, 2019: member

Might want to clarify the documentation of IsBlockPruned first? I.e. explain why it returns false for a block that has its data downloaded or why it returns false for a block that is missing its data.

kallewoof commented at 4:35 AM on April 24, 2019: member

@MarcoFalke I kind of closed this out of frustration due to lack of feedback. I can reopen if you think it's worth it to try to get this merged.

DrahtBot locked this on Dec 16, 2021