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
  1. luke-jr commented at 2:28 am on October 12, 2014: member
    Unlikely, but might as well check it…
  2. Bugfix: CreateNewBlock: Check that active chain has a valid tip before dereferencing it ad9c8920ab
  3. TheBlueMatt commented at 2:39 am on October 12, 2014: member
    I would just assert this one?
  4. 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.
  5. TheBlueMatt commented at 2:54 am on October 12, 2014: member
    It should never return null unless you havent loaded even the genesis block, I think
  6. sipa commented at 4:58 am on October 12, 2014: member
    Correct. chainActive.Tip should never be NULL after AppInit. Either LoadBlockIndex or InitBlockIndex fixed that.
  7. luke-jr commented at 5:01 am on October 12, 2014: member
    @sipa Would you object to making Tip() return a reference rather than a pointer, then?
  8. sipa commented at 5:06 am on October 12, 2014: member
    We 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.
  9. 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).

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

  11. sipa commented at 3:40 pm on October 13, 2014: member
    Oh, no, indeed. If you reindex, InitBlockIndex bypassing rebuilding the genesis block, as the one already on disk will be used.
  12. 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?
  13. sipa commented at 6:34 pm on October 13, 2014: member
    When you’re reindexing, there is no genesis CBlockIndex before you start the reindex (as you don’t know where it is on disk yet).
  14. TheBlueMatt commented at 6:39 pm on October 13, 2014: member
    OK, so we should wait to return from AppInit before we at least have the genesis block reindexed?
  15. 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…

  16. 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?).
  17. laanwj commented at 8:51 am on October 15, 2014: member
    OK. 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.
  18. 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.

  19. sipa commented at 5:45 pm on October 15, 2014: member
    The 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.
  20. sipa commented at 5:32 pm on November 6, 2014: member
    utACK
  21. TheBlueMatt commented at 7:29 am on November 8, 2014: member
    Ive 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)
  22. laanwj commented at 10:20 am on December 5, 2014: member
    Closing in favor of #5243.
  23. laanwj closed this on Dec 5, 2014

  24. MarcoFalke locked this on Sep 8, 2021

github-metadata-mirror

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

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me