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

Bugfix: Don't check the genesis block header before accepting it #6299

Merged
merged 2 commits into from Jun 26, 2015

Conversation

@jtimon
Copy link
Member

@jtimon jtimon commented Jun 17, 2015

This should fix an error that was introduced in #5975 , thanks @sdaftuar for reporting the error.
I will work on a more elegant solution: the genesis block should never be checked at all; it is valid by definition. But it seems that will be more work than I first thought so let's just fix the bug first.

@laanwj
laanwj reviewed Jun 17, 2015
View changes
src/main.cpp Outdated
@@ -2809,6 +2809,7 @@ bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, CBloc
uint256 hash = block.GetHash();
BlockMap::iterator miSelf = mapBlockIndex.find(hash);
CBlockIndex *pindex = NULL;
if (hash != chainparams.GetConsensus().hashGenesisBlock) {
if (miSelf != mapBlockIndex.end()) {

This comment has been minimized.

@laanwj

laanwj Jun 17, 2015
Member

Indentation missing

This comment has been minimized.

@jtimon

jtimon Jun 17, 2015
Author Member

yep, I want to avoid indenting the whole block to reduce the diff and potential for merge conflicts.
After all, at some point we will start running clang-format project wise right before forking for major releases. Is that still the plan @laanwj @sipa ?

This comment has been minimized.

@petertodd

petertodd Jun 18, 2015
Contributor

I'd prefer those merge conflicts to stay merge conflicts, as it forces you to understand if that additional check matters for your merge.

What can I say, I once fixed a bug where a lack of indentation of a newly added bit of code lead to a mistake that could have potentially played a part of getting someone killed, so... :/

This comment has been minimized.

@jtimon

jtimon Jun 18, 2015
Author Member

I'm happy to indent if people think it's better.

This comment has been minimized.

@laanwj

laanwj Jun 18, 2015
Member

Yes, that is still the plan at some point, although I'd say it's very low priority.

Reducing the diff is generally a good reason, but not enough to deviate this much from the coding style IMO. This makes the control flow harder to interpret in an important function.

@laanwj laanwj added the Bug label Jun 17, 2015
@laanwj
Copy link
Member

@laanwj laanwj commented Jun 17, 2015

Can you please cherry-pick #6298 into this, to see if the reindex test passes in travis.

@jtimon
Copy link
Member Author

@jtimon jtimon commented Jun 17, 2015

@laanwj Incorporated #6298

@jtimon jtimon force-pushed the jtimon:5975-quick-fix branch Jun 18, 2015
@jtimon
Copy link
Member Author

@jtimon jtimon commented Jun 18, 2015

Updated with the correct indentation (bigger diff).

@laanwj
Copy link
Member

@laanwj laanwj commented Jun 18, 2015

utACK

@theuni
Copy link
Member

@theuni theuni commented Jun 19, 2015

Btw, for the sake of easier review, you can always do a diff that ignores whitespace via git diff -w.

To see that at github, just append ?w=1 to the url: https://github.com/bitcoin/bitcoin/pull/6299/files?w=1

jtimon and others added 2 commits Jun 17, 2015
This fixes an error triggered when running with -reindex after #5975
This test finishes very quickly, so it should be part of the default set
of tests in rpc-tests.
@jtimon jtimon force-pushed the jtimon:5975-quick-fix branch to 4f40716 Jun 20, 2015
@jtimon
Copy link
Member Author

@jtimon jtimon commented Jun 20, 2015

Needed rebase.
@theuni Thanks, that's useful.

@sdaftuar
Copy link
Member

@sdaftuar sdaftuar commented Jun 24, 2015

Tested ACK

@jtimon
Copy link
Member Author

@jtimon jtimon commented Jun 26, 2015

There's an alternative solution to the bug in #6230.

@laanwj laanwj merged commit 4f40716 into bitcoin:master Jun 26, 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 26, 2015
4f40716 test: Move reindex test to standard tests (Wladimir J. van der Laan)
36c97b4 Bugfix: Don't check the genesis block header before accepting it (Jorge Timón)
@laanwj
Copy link
Member

@laanwj laanwj commented Jun 26, 2015

ACK

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.