By separating a new CheckIndexAgainstCheckpoint() from ContextualCheckBlockHeader(). @theuni you will probably be interested in this since you were doing some work on the checkpoints. I hope it doesn't conflict with what you've done.
The checkpoints shouldn't be used to check a new block header. Instead, they should be used to check the index (the chain) that it's going to be used to check new block header.
fixed after a discussion with @theuni on IRC Now CheckIndexAgainstCheckpoint() takes CChainParams instead of uint256& hashGenesisBlock as parameter to avoid conflicts with work that he is doing in parallel related to checkpoints. Ready for squash. #5968 and #5970 probably need to be adapted to CheckIndexAgainstCheckpoint depending on CChainParams.
2641 | @@ -2640,6 +2642,8 @@ bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, CBloc 2642 | if (pindexPrev->nStatus & BLOCK_FAILED_MASK) 2643 | return state.DoS(100, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk"); 2644 | } 2645 | + if (!CheckIndexAgainstCheckpoint(pindexPrev, state, params, mapBlockIndex))
Shouldn't these just return false? state is already set in CheckIndexAgainstCheckpoint().
2 more fixes, ready to squash
Needed rebase.
Somehow I missed this one. Renamed params to chainparams like everywhere else.
2539 | @@ -2540,19 +2540,30 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo 2540 | return true; 2541 | } 2542 | 2543 | -bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, CBlockIndex * const pindexPrev) 2544 | +static bool CheckIndexAgainstCheckpoint(const CBlockIndex* pindexPrev, CValidationState& state, const CChainParams& chainparams, const BlockMap& mapBlockIndex)
nit: mapBlockIndex shadows the global, will be confusing when it's used.
ut ack other than the nit
nit solved
Thanks, looks good.
2547 | - uint256 hash = block.GetHash(); 2548 | - if (hash == consensusParams.hashGenesisBlock) 2549 | - return true; 2550 | - 2551 | assert(pindexPrev); 2552 | + if (*pindexPrev->phashBlock == chainparams.GetConsensus().hashGenesisBlock)
I'd rather keep comparisons with the genesis block out of the "checkpoints" logic. That's purely philosophical, but we may get rid of checkpoints at some point, though the genesis block will always remain part of consensus.
Well, the question is, should we validate the genesis block? I don't think so, the genesis block is hardcoded, no need to check it redundantly from many computers. So this is more moving that check out of consensus than "moving it to the checkpoints logic".
But of course, if your the tip of the chain index you're using (your pindexPrev) is the genesis block (that is, if you're validating the second block), you can skip the other checkpoint tests.
What you want to check is that your chain index contains the genesis block. Or with checkpoints, you may even want to just check that your chain index contains the previous checkpoint (you have to be certain that all your checkpoints contain the genesis block when your hardcode them). In that sense you can see the genesis block just as the oldest checkpoint possible.
But thanks for bringing this up, this is the only change in logic and what needs discussion.
Needed rebase
@jtimon I think #6055 was a big improvement in readability for the checkpoint functions because it moved the "what if checkpoints aren't enabled" case out of the actual checkpoint logic. This would undo some of that.
How about removing the early return from CheckIndexAgainstCheckpoint() and instead calling into it like if (fCheckpointsEnabled && !CheckIndexAgainstCheckpoint(...)) ?
Sorry, that was poorly worded. It wouldn't undo any of #6055, but it would negate one of the beneficial side-effects of it.
As for not removing the assert, I don't follow. pindexPrev is used whether checkpoints are enabled or not, so relying on the assert in CheckIndexAgainstCheckpoint() even if checkpoints are disabled makes no sense to me.
I still disagree that it is removing one of the beneficial side effects of it, I don't think the negated condition is less readable...
Anyway, what I mean is that if I want to maintain it almost equivalent (is not completely equivalent functionally because I'm changing the genesis check as discussed in the outdated comment by @sipa), I can't do just
if (fCheckpointsEnabled && !CheckIndexAgainstCheckpoint(...))
It would need to be something like
assert(pindexPrev);
if (fCheckpointsEnabled && !CheckIndexAgainstCheckpoint(...))
because pindexPrev shouldn't arrive NULL to ContextualCheckBlockHeader. But I'm fine with that, should I change it to that, then?
Yes, that looks good. Thanks.
2711 | @@ -2715,6 +2712,9 @@ bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, CBloc 2712 | if (pindexPrev->nStatus & BLOCK_FAILED_MASK) 2713 | return state.DoS(100, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk"); 2714 | } 2715 | + assert(pindexPrev);
Any reason why the checkpoints check is moved?
I think it's harmless to do so, but I was wondering if there is a reason.
@sipa if you're referring to fCheckpointsEnabled moving out of CheckIndexAgainstCheckpoint(), I requested that to reduce the side-effects of CheckIndexAgainstCheckpoint(). Imo if possible the checkpoint functions should be side-effect free so that we know it's consensus-safe to simply not call them at all.
I'm referring to the fCheckpointsEnabled conditional. Just to the fact that the checkpoints-checking point in this function moving.
@sipa I'm still not sure what you mean. The checkpoints checks are moved out of ContextualCheckBlockHeader to keep them out of consensus. Then is moved up in the functions, because you should have your index checked before checking a new block, maybe they can be moved upper (and earlier in time) later. I suspect the index is checked in this way redundantly. If this is an issue I can maintain the calls to CheckIndexAgainstCheckpoint right before the calls to ContextualCheckBlockHeader.
Sorry, I missed this was in AcceptBlockHeader already, and not in ContextualCheckBlockHeader anymore. Looks good.
2823 | @@ -2824,8 +2824,11 @@ bool ProcessNewBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, CDis 2824 | 2825 | bool TestBlockValidity(CValidationState &state, const CBlock& block, CBlockIndex * const pindexPrev, bool fCheckPOW, bool fCheckMerkleRoot) 2826 | { 2827 | + const CChainParams& chainparams = Params(); 2828 | AssertLockHeld(cs_main); 2829 | - assert(pindexPrev == chainActive.Tip()); 2830 | + assert(pindexPrev && pindexPrev == chainActive.Tip()); 2831 | + if (fCheckpointsEnabled && !CheckIndexAgainstCheckpoint(pindexPrev, state, chainparams, block.GetHash(), mapBlockIndex)) 2832 | + return error("%s: CheckIndexAgainstCheckpoint(): %s", __func__, state.GetRejectReason().c_str());
No need to print an error in this case; it's perfectly legitimate that this fails.
EDIT: Hmm, the rest of the checks prints stuff too. Never mind then, but I think we should fix that independently (there are probably already issues about that).
Yes I'm trying to move the errors up (because logging errrors shouldnt be done in consensus, but one step at a time. That is the kind of change that is better done after the moveonly, hopefully, one day...
Untested ACK
Updated without the unused blockIndexMap parameter. It may take long until it gets used so better to just introduce it at that point.
No worries, it was an easy rebase.
This appears to have broken running bitcoind with -reindex(!); it appears the assert(pindexPrev) in AcceptBlockHeader can be reached when processing the genesis block from disk. Can be reproduced by running qa/rpc-tests/reindex.py.