Chainparams: Don't check the genesis block

jtimon commented at 11:35 pm on August 28, 2015: contributor

The genesis block is the truth: don’t check it. Apart from being a minimal optimization, this removes the necessity of “mining the genesis block” when new testchains are created. This is a necessary step for #6382 (which creates std::numeric_limits<uint64_t>::max() new testchains).

dcousens commented at 1:28 am on August 29, 2015: contributor

Apart from being a minimal optimization

How is this an optimization? You are now comparing the blocks hash against the genesis block every time ConnectBlock is called?

jtimon commented at 1:36 am on August 29, 2015: contributor

How is this an optimization?

By saving some checks for the genesis block.

You are now comparing the blocks hash against the genesis block every time ConnectBlock is called?

The check in ConnectBlock was already there, now CheckBlock is not called if the block being checked is the genesis block (ie the checks has just been moved below the genesis block check).

jtimon commented at 1:40 am on August 29, 2015: contributor

In any case, benchmarking welcomed.

laanwj added the label Refactoring on Sep 4, 2015

jtimon force-pushed on Nov 11, 2015

jtimon commented at 10:59 pm on November 11, 2015: contributor

Rebased and simplified. Looking at it again, @dcousens was right that many of the changes would have affected performance negatively. Besides they were (or at least they currently seem to be) unnecessary for #6382 (or for facilitating the creating of testchains in general by not requiring tester to “mine” the genesis block of their testchains: the genesis block is correct by definition). Now it should be clear that we’re just optimizing the genesis block case in ConnectBlock() without adding any work to the other cases, and we’re only affecting negatively the performance of CBlockTreeDB::LoadBlockIndexGuts by comparing two hashes right before returning and error (only if CheckProofOfWork had previously failed), which I think is very acceptable. So I don’t think benchmarking this makes sense anymore.

dcousens commented at 11:13 pm on November 11, 2015: contributor

utACK

in src/txdb.cpp: in b95a2f959f outdated

203@@ -203,7 +204,8 @@ bool CBlockTreeDB::LoadBlockIndexGuts()
204                 pindexNew->nStatus        = diskindex.nStatus;
205                 pindexNew->nTx            = diskindex.nTx;
206 
207-                if (!CheckProofOfWork(pindexNew->GetBlockHash(), pindexNew->nBits, Params().GetConsensus()))
208+                if (!CheckProofOfWork(pindexNew->GetBlockHash(), pindexNew->nBits, consensusParams) &&
209+                    pindexNew->GetBlockHash() != consensusParams.hashGenesisBlock)

sipa commented at 3:40 pm on November 12, 2015:

Is the genesis block expected to fail PoW?

jtimon commented at 5:19 pm on November 12, 2015:

Not usually, but this is useful for creating new testchains (whether those testchains are merged in master or not) without having to spend costly PoW to “mine” a genesis block to hardcode. For example, #6382 introduces std::numeric_limits<uint64_t>::max() new testchains whose genesis blocks are created dynamically (but not mined dynamically in chainparams.cpp) and will therefore fail PoW. For any production chain, mining the genesis block is a meaningless cost, but for a testnet (specially regtest-like testchains) it is utterly absurd. Even for mainchain, Satoshi didn’t need to mine the genesis block because it is correct by definition (or more accurately, it is part of the definition of how valid chains must be). Just in the same way, he could have decided to make the genesis subsidy spendable but he didn’t (but changing that now would be a hardfork). The creator of the system worked in mysterious ways…

sipa commented at 11:18 pm on November 12, 2015:

Fair enough.

sipa commented at 11:19 pm on November 12, 2015: member

ACK

gmaxwell commented at 1:34 am on November 17, 2015: contributor

Care to pass in the params from above like #6986?

jtimon commented at 3:06 am on November 17, 2015: contributor

@gmaxwell you mean passing it explicitly to CBlockTreeDB::LoadBlockIndexGuts()?

gmaxwell commented at 6:02 am on November 17, 2015: contributor

@jtimon Right. Do you agree it would be the correct and consistent thing to do?

jtimon commented at 11:27 am on November 17, 2015: contributor

Sure, using the local variable is in preparation for that at some point later (see #5970), but I’m happy to do it here directly.

Testchains: Don't check the genesis block e4037dd51b

jtimon force-pushed on Nov 17, 2015

jtimon commented at 1:54 pm on November 17, 2015: contributor

Updated with @gmaxwell ’s nit solved.

sipa commented at 1:08 pm on November 28, 2015: member

Still ACK

jtimon commented at 5:18 am on December 18, 2015: contributor

still ping, I’ve been thinking about closing this and reopen it whith a test that makes sure a new non-mined chain wold work (that means another chain in master),

jtimon closed this on Dec 18, 2015

dcousens commented at 11:30 pm on December 18, 2015: contributor

re-ACK e4037dd, @jtimon test sounds good too :+1:

jtimon reopened this on Dec 18, 2015

sipa commented at 7:36 pm on January 6, 2016: member

From experience elsewhere: if you introduce a new chain with an “invalid” genesis block and this change, it will fail to reindex, as the check is still done in that case.

jtimon commented at 12:47 pm on January 7, 2016: contributor

Mhmm, why #6382 seems to be working just fine with reindex then? Have you actually reproduced the error you claim would appear?

This shouldn’t be necessary, certainly not in CheckBlockHeader(). In AcceptBlockHeader, it won’t be called for the genesis block, see https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L3071 Maybe it is required on TestBlockValidity() or something else that calls CheckBlock() (which calls CheckBlockHeader()), but I would prefer to leave the genesis block check out of the consensus function CheckBlockHeader().

Probably a good test for this would be to create a new regtest-like chain with a genesis block that doesn’t satisfy the proof of work and run it for a few blocks both with and without reindex from the python tests. But without #6907 and something like https://github.com/jtimon/bitcoin/commit/98923c381eca9874f135f08297218f3f8fff5221 it seems like the testing would be much more than the change itself.

laanwj commented at 3:58 pm on January 18, 2016: member

Let’s try to get agreement here @sipa @jtimon

sipa commented at 4:10 pm on January 18, 2016: member

@jtimon Because Bitcoin’s genesis block does actually pass PoW checks.

What I mean is that this patch is effectively removing the invariant that genesis blocks have to be valid. However, if you actually try with an invalid genesis block, reindex fails.

Technically not a concern for Bitcoin at this point, so this is not a blocker for merge. Just some advice that if we’d actually want to start creating invalid genesis blocks (which makes creating test chains much easier), this patch isn’t enough.

jtimon commented at 11:28 am on January 19, 2016: contributor

Genesis blocks are valid by definition and should never be checked (whether they pass pow or not). In any case, IIRC last time I tested #6382 it worked just fine even when reindexing the chain, therefore I disagree with sipa that this patch isn’t enough. As said the very fact that we disagree on whether or not this is enough for its stated goal, means IMO that this should be introduced with tests that verify this is enough and will keep being enough in the future (make sure nobody undoes this without noticing). I believe the best way to test this is through a new chain whose genesis block doesn’t pass pow (regtest2 ?) And test boh with and without reindex.

Does that make sense?

Addionally, I don’t plan to create a new chain without #6907 (and generic selection with -chain, also part of #6382 ),and that’s why I’m closing this for now. Anyone feel free to take over this if you can’t wait for the factory. I’ve personally been wanting this factory for almost two years, I can wait more for #6907 to be merged (was supposed to happen before forking 0.12, I really hope it will happen before forking 0.13.

jtimon closed this on Jan 19, 2016

sipa reopened this on Jan 21, 2016

sipa commented at 12:26 pm on January 21, 2016: member

I’m not against merging this! I was simply giving a suggestion for further work towards its stated goal.

jtimon commented at 12:44 pm on January 21, 2016: contributor

And I’m saying I believe this is enough for fulfilling its stated goal, but the fact that we disagree means we should introduce tests here to make sure the goal is achieved (and later merges don’t ruin it). But as said I don’t plan to write those tests until #6907. This was not merged before forking 0.12 so I’m not in a hurry anyway. As long as I reopen it before 0.13 is forked I’m happy closing it for now. I can leave this opened as incomplete, but I wanted people interested in making new testchains easier (most of what #6382 was about) to focus on #6907 first. #6907 was supposed to be merged before 0.12 and is also more costly to maintain rebased, can we please do that first?

jtimon closed this on Jan 28, 2016

jtimon referenced this in commit c8a7ae8a65 on Mar 11, 2016

jtimon referenced this in commit c3cf9e45da on Mar 11, 2016

jtimon referenced this in commit 84365ed1ad on Mar 11, 2016

jtimon referenced this in commit 1cdb9c8b5f on Apr 6, 2016

MarcoFalke locked this on Sep 8, 2021

Chainparams: Don’t check the genesis block #6597