Bugfix: CreateNewBlock: Check that active chain has a valid tip before dereferencing it #5078
pull luke-jr wants to merge 1 commits into bitcoin:master from luke-jr:bugfix_cnb_nullptr changing 1 files +2 −0-
luke-jr commented at 2:28 am on October 12, 2014: memberUnlikely, but might as well check it…
-
Bugfix: CreateNewBlock: Check that active chain has a valid tip before dereferencing it ad9c8920ab
-
TheBlueMatt commented at 2:39 am on October 12, 2014: memberI would just assert this one?
-
luke-jr commented at 2:51 am on October 12, 2014: member@TheBlueMatt I’m not sure under what conditions this might occur. Tip() explicitly returns NULL in some cases… Don’t want running a miner to crash your Bitcoin node.
-
TheBlueMatt commented at 2:54 am on October 12, 2014: memberIt should never return null unless you havent loaded even the genesis block, I think
-
sipa commented at 4:58 am on October 12, 2014: memberCorrect. chainActive.Tip should never be NULL after AppInit. Either LoadBlockIndex or InitBlockIndex fixed that.
-
sipa commented at 5:06 am on October 12, 2014: memberWe use CBlockIndex* variables everywhere as identifiers for blocks, so yes, I would rather not change that. Also, it is effectively useful inside the initialization code to detect the we’re-not-yet-initialized case.
-
laanwj commented at 11:53 am on October 13, 2014: member
Correct. chainActive.Tip should never be NULL after AppInit. Either LoadBlockIndex or InitBlockIndex fixed that.
This post #4995 (comment) in #4995 claims that in the case of -reindex it can be NULL (note: haven’t checked this myself).
-
jgarzik commented at 2:55 pm on October 13, 2014: contributor
This is an assert, not a normal one. Should be impossible for this condition to occur, even with reindex.
reindex always hand-builds the genesis block.
-
sipa commented at 3:40 pm on October 13, 2014: memberOh, no, indeed. If you reindex, InitBlockIndex bypassing rebuilding the genesis block, as the one already on disk will be used.
-
TheBlueMatt commented at 6:09 pm on October 13, 2014: member@laanwj Hmm, would it not make more sense to fix that by making sure Tip() always points to at least genesis, instead of letting it be NULL?
-
sipa commented at 6:34 pm on October 13, 2014: memberWhen you’re reindexing, there is no genesis CBlockIndex before you start the reindex (as you don’t know where it is on disk yet).
-
TheBlueMatt commented at 6:39 pm on October 13, 2014: memberOK, so we should wait to return from AppInit before we at least have the genesis block reindexed?
-
laanwj commented at 10:02 am on October 14, 2014: member
Now that we’ve established that
chainActive.Tip()
can return NULL, I suppose the most straightforward way forward is document this behaviour and make the code robust against it where necessary.Just delaying the return won’t solve anything. @TheBlueMatt To be precise we’d have to wait after starting ThreadImport, before starting the node and RPC threads and internal miner. It would be another possible solution, although a bit hackier than just considering “no chain yet” a legit state, it may have less code impact overall…
-
TheBlueMatt commented at 5:24 pm on October 14, 2014: member@laanwj Yes, I was going for less impact. It certainly seems rather strange to start import in init, and then finish initializing everything else before we have even initialized the genesis block (which should be one of the first blocks imported, no?).
-
laanwj commented at 8:51 am on October 15, 2014: memberOK. Then cleanest would be to split up reindex into a ’locate genesis block’ and ‘reindex chain’ part, the first part being executed from AppInit2. This avoids ugly constructions with waiting for
ThreadImport
. -
laanwj commented at 1:25 pm on October 15, 2014: member
Correct me if wrong: when reindexing, even with headers-first, the genesis block is always the first block in the first block file (blk00000.dat). If not we are reindexing the wrong chain, for example a testnet block chain on a mainnet node or dealing with a corrupted block file. In that case it acceptable to throw an initialization error.
I can’t think of any exceptions to this.
-
sipa commented at 5:45 pm on October 15, 2014: memberThe genesis block should be the first one yes, as it’s not received from network but put there in InitBlockIndex, If it is not, there’s a problem.
-
sipa commented at 5:32 pm on November 6, 2014: memberutACK
-
TheBlueMatt commented at 7:29 am on November 8, 2014: memberIve always considered the chainActive.Tip() never returning NULL to be a rule, so I’d prefer to wait in init for it to be the case (ie #5243)
-
laanwj closed this on Dec 5, 2014
-
MarcoFalke locked this on Sep 8, 2021
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 06:12 UTC
More mirrored repositories can be found on mirror.b10c.me