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.
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))
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)
2547- uint256 hash = block.GetHash();
2548- if (hash == consensusParams.hashGenesisBlock)
2549- return true;
2550-
2551 assert(pindexPrev);
2552+ if (*pindexPrev->phashBlock == chainparams.GetConsensus().hashGenesisBlock)
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.
@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
0if (fCheckpointsEnabled && !CheckIndexAgainstCheckpoint(...))
It would need to be something like
0assert(pindexPrev);
1if (fCheckpointsEnabled && !CheckIndexAgainstCheckpoint(...))
because pindexPrev shouldn’t arrive NULL to ContextualCheckBlockHeader. But I’m fine with that, should I change it to that, then?
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.
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).
assert(pindexPrev) in AcceptBlockHeader can be reached when processing the genesis block from disk. Can be reproduced by running qa/rpc-tests/reindex.py.