Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consensus: Decouple ContextualCheckBlockHeader from checkpoints #5975

Merged
merged 1 commit into from Jun 10, 2015

Conversation

@jtimon
Copy link
Member

jtimon commented Apr 6, 2015

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.

@laanwj laanwj added the Refactoring label Apr 7, 2015
@jtimon
Copy link
Member Author

jtimon commented Apr 8, 2015

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.

@@ -2640,6 +2642,8 @@ bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, CBloc
if (pindexPrev->nStatus & BLOCK_FAILED_MASK)
return state.DoS(100, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk");
}
if (!CheckIndexAgainstCheckpoint(pindexPrev, state, params, mapBlockIndex))

This comment has been minimized.

Copy link
@theuni

theuni Apr 8, 2015

Member

Shouldn't these just return false? state is already set in CheckIndexAgainstCheckpoint().

@jtimon
Copy link
Member Author

jtimon commented Apr 8, 2015

2 more fixes, ready to squash

@jtimon jtimon force-pushed the jtimon:consensus_checkpoints branch 4 times, most recently from 9c38309 to 54f2e9d Apr 8, 2015
@jtimon jtimon force-pushed the jtimon:consensus_checkpoints branch from 54f2e9d to 55b98a2 Apr 15, 2015
@jtimon
Copy link
Member Author

jtimon commented Apr 15, 2015

Needed rebase.

@jtimon jtimon force-pushed the jtimon:consensus_checkpoints branch from 55b98a2 to e55ad48 Apr 19, 2015
@jtimon
Copy link
Member Author

jtimon commented Apr 19, 2015

Somehow I missed this one. Renamed params to chainparams like everywhere else.

@jtimon
Copy link
Member Author

jtimon commented Apr 22, 2015

ping @theuni

@@ -2540,19 +2540,30 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo
return true;
}

bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, CBlockIndex * const pindexPrev)
static bool CheckIndexAgainstCheckpoint(const CBlockIndex* pindexPrev, CValidationState& state, const CChainParams& chainparams, const BlockMap& mapBlockIndex)

This comment has been minimized.

Copy link
@theuni

theuni Apr 22, 2015

Member

nit: mapBlockIndex shadows the global, will be confusing when it's used.

@theuni
Copy link
Member

theuni commented Apr 22, 2015

ut ack other than the nit

@jtimon jtimon force-pushed the jtimon:consensus_checkpoints branch from e55ad48 to b00583b Apr 22, 2015
@jtimon
Copy link
Member Author

jtimon commented Apr 22, 2015

nit solved

@theuni
Copy link
Member

theuni commented Apr 23, 2015

Thanks, looks good.

assert(pindexPrev);
if (*pindexPrev->phashBlock == chainparams.GetConsensus().hashGenesisBlock)

This comment has been minimized.

Copy link
@sipa

sipa Apr 24, 2015

Member

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.

This comment has been minimized.

Copy link
@jtimon

jtimon Apr 25, 2015

Author Member

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
Copy link
Member Author

jtimon commented May 6, 2015

Needed rebase

@theuni
Copy link
Member

theuni commented May 6, 2015

@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(...)) ?

@jtimon
Copy link
Member Author

jtimon commented May 6, 2015

I disagree that this undoes part of #6055 but I'm ok with your proposed alternative. I thought about it too but I didn't wanted to take the assert(pindexPrev); out of the function as well.

@theuni
Copy link
Member

theuni commented May 6, 2015

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.

@jtimon
Copy link
Member Author

jtimon commented May 6, 2015

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?

@theuni
Copy link
Member

theuni commented May 6, 2015

Yes, that looks good. Thanks.

@jtimon jtimon force-pushed the jtimon:consensus_checkpoints branch from 6e55325 to 102b09b May 6, 2015
@jtimon
Copy link
Member Author

jtimon commented May 6, 2015

Done, changed as suggested by @theuni

@@ -2715,6 +2712,9 @@ bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, CBloc
if (pindexPrev->nStatus & BLOCK_FAILED_MASK)
return state.DoS(100, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk");
}
assert(pindexPrev);

This comment has been minimized.

Copy link
@sipa

sipa May 7, 2015

Member

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.

This comment has been minimized.

Copy link
@jtimon

jtimon May 8, 2015

Author Member

Apparently having the check of the global boolean outside of the function is more readable according to @theuni.

This comment has been minimized.

Copy link
@theuni

theuni May 9, 2015

Member

@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.

This comment has been minimized.

Copy link
@sipa

sipa via email May 9, 2015

Member

This comment has been minimized.

Copy link
@jtimon

jtimon May 10, 2015

Author Member

@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.

This comment has been minimized.

Copy link
@sipa

sipa May 10, 2015

Member

Sorry, I missed this was in AcceptBlockHeader already, and not in ContextualCheckBlockHeader anymore. Looks good.

assert(pindexPrev == chainActive.Tip());
assert(pindexPrev && pindexPrev == chainActive.Tip());
if (fCheckpointsEnabled && !CheckIndexAgainstCheckpoint(pindexPrev, state, chainparams, block.GetHash(), mapBlockIndex))
return error("%s: CheckIndexAgainstCheckpoint(): %s", __func__, state.GetRejectReason().c_str());

This comment has been minimized.

Copy link
@sipa

sipa May 8, 2015

Member

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).

This comment has been minimized.

Copy link
@jtimon

jtimon May 8, 2015

Author Member

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...

@jtimon
Copy link
Member Author

jtimon commented May 8, 2015

@sipa I answered to your worries on checking the genesis block. You never answered back so I assume you were satisfied by my answer.

@sipa
Copy link
Member

sipa commented May 10, 2015

Untested ACK

@jtimon jtimon force-pushed the jtimon:consensus_checkpoints branch from 102b09b to 8d834ec May 27, 2015
@jtimon
Copy link
Member Author

jtimon commented May 27, 2015

Updated without the unused blockIndexMap parameter.
It may take long until it gets used so better to just introduce it at that point.

@laanwj
Copy link
Member

laanwj commented Jun 10, 2015

Sorry: needs rebase after #5927, will merge after that.

@jtimon jtimon force-pushed the jtimon:consensus_checkpoints branch from 8d834ec to 425c3a8 Jun 10, 2015
@jtimon
Copy link
Member Author

jtimon commented Jun 10, 2015

No worries, it was an easy rebase.

@laanwj laanwj merged commit 425c3a8 into bitcoin:master Jun 10, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Jun 10, 2015
425c3a8 Consensus: Separate CheckIndexAgainstCheckpoint() from ContextualCheckBlockHeader (Jorge Timón)
@sdaftuar
Copy link
Member

sdaftuar commented Jun 16, 2015

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.

@jtimon
Copy link
Member Author

jtimon commented Jun 17, 2015

@sdaftuar created #6299 to fix the bug.

jtimon added a commit to jtimon/bitcoin that referenced this pull request Jun 20, 2015
This fixes an error triggered when running with -reindex after bitcoin#5975
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.