Chainparams: Don't check the genesis block #6597

jtimon · 2015-08-28T23:35:11Z

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 · 2015-08-29T01:28:35Z

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 · 2015-08-29T01:36:13Z

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 · 2015-08-29T01:40:29Z

In any case, benchmarking welcomed.

jtimon · 2015-11-11T22:59:09Z

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 · 2015-11-11T23:13:00Z

utACK

sipa · 2015-11-12T15:40:24Z

src/txdb.cpp

@@ -203,7 +204,8 @@ bool CBlockTreeDB::LoadBlockIndexGuts()
                pindexNew->nStatus        = diskindex.nStatus;
                pindexNew->nTx            = diskindex.nTx;

-                if (!CheckProofOfWork(pindexNew->GetBlockHash(), pindexNew->nBits, Params().GetConsensus()))
+                if (!CheckProofOfWork(pindexNew->GetBlockHash(), pindexNew->nBits, consensusParams) &&
+                    pindexNew->GetBlockHash() != consensusParams.hashGenesisBlock)


Is the genesis block expected to fail PoW?

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

Fair enough.

sipa · 2015-11-12T23:19:51Z

ACK

gmaxwell · 2015-11-17T01:34:15Z

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

jtimon · 2015-11-17T03:06:10Z

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

gmaxwell · 2015-11-17T06:02:04Z

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

jtimon · 2015-11-17T11:27:43Z

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.

jtimon · 2015-11-17T13:54:56Z

Updated with @gmaxwell 's nit solved.

sipa · 2015-11-28T13:08:35Z

Still ACK

jtimon · 2015-12-18T05:18:49Z

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

dcousens · 2015-12-18T23:30:48Z

re-ACK e4037dd, @jtimon test sounds good too 👍

sipa · 2016-01-06T19:36:20Z

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.

Suggested fix: see sipa@f7a1a47

jtimon · 2016-01-07T12:47:36Z

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 jtimon@98923c3 it seems like the testing would be much more than the change itself.

laanwj · 2016-01-18T15:58:07Z

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

sipa · 2016-01-18T16:10:37Z

@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 · 2016-01-19T11:28:32Z

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.

sipa · 2016-01-21T12:26:05Z

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

jtimon · 2016-01-21T12:44:41Z

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?

…id (HF for bitcoin) - Like in bitcoin#6597 (bitcoin), the genesis block shouldn't be checked because it is a consensus rule in itself - On the other hand, some elements like deterministic peg (because the 21M reserve of pegged-coins needs to be created at some point) and multi-assets (because one cannot define new assets without existing utxos to be used as input for the asset definition transactions)

…s asvalid (HF for bitcoin) - Like in bitcoin#6597 (bitcoin), the genesis block shouldn't be checked because it is a consensus rule in itself - On the other hand, some elements like deterministic peg (because the 21M reserve of pegged-coins needs to be created at some point) and multi-assets (because one cannot define new assets without existing utxos to be used as input for the asset definition transactions)

Rebased by Pieter Wuille. Cleanup by Matt Corallo.

jtimon mentioned this pull request Aug 29, 2015

DEPENDENT: Chainparams: Introduce N testnet chains to test different block sizes #6382

Closed

12 tasks

laanwj added the Refactoring label Sep 4, 2015

jtimon force-pushed the chainparams-genesis-no-check-0.12.99 branch from 13c5755 to b95a2f9 Compare November 11, 2015 22:52

sipa reviewed Nov 12, 2015
View reviewed changes

Testchains: Don't check the genesis block

e4037dd

jtimon force-pushed the chainparams-genesis-no-check-0.12.99 branch from b95a2f9 to e4037dd Compare November 17, 2015 13:54

jtimon closed this Dec 18, 2015

jtimon reopened this Dec 18, 2015

jtimon closed this Jan 19, 2016

sipa reopened this Jan 21, 2016

jtimon closed this Jan 28, 2016

jtimon referenced this pull request in sipa/bitcoin Apr 6, 2016

Testchains: Don't check the genesis block

1cdb9c8

Rebased by Pieter Wuille. Cleanup by Matt Corallo.

jtimon mentioned this pull request Nov 8, 2016

Really don't validate genesis block #9102

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Chainparams: Don't check the genesis block #6597

Chainparams: Don't check the genesis block #6597

Uh oh!

Conversation

jtimon commented Aug 28, 2015

Uh oh!

dcousens commented Aug 29, 2015

Uh oh!

jtimon commented Aug 29, 2015

Uh oh!

jtimon commented Aug 29, 2015

Uh oh!

jtimon commented Nov 11, 2015

Uh oh!

dcousens commented Nov 11, 2015

Uh oh!

sipa Nov 12, 2015

Choose a reason for hiding this comment

Uh oh!

jtimon Nov 12, 2015

Choose a reason for hiding this comment

Uh oh!

sipa Nov 12, 2015

Choose a reason for hiding this comment

Uh oh!

sipa commented Nov 12, 2015

Uh oh!

gmaxwell commented Nov 17, 2015

Uh oh!

jtimon commented Nov 17, 2015

Uh oh!

gmaxwell commented Nov 17, 2015

Uh oh!

jtimon commented Nov 17, 2015

Uh oh!

jtimon commented Nov 17, 2015

Uh oh!

sipa commented Nov 28, 2015

Uh oh!

jtimon commented Dec 18, 2015

Uh oh!

dcousens commented Dec 18, 2015

Uh oh!

sipa commented Jan 6, 2016

Uh oh!

jtimon commented Jan 7, 2016

Uh oh!

laanwj commented Jan 18, 2016

Uh oh!

sipa commented Jan 18, 2016 via email

Uh oh!

jtimon commented Jan 19, 2016

Uh oh!

sipa commented Jan 21, 2016

Uh oh!

jtimon commented Jan 21, 2016

Uh oh!

Uh oh!