Check pindexBestForkBase for null #5154

ghost commented at 3:17 AM on October 28, 2014: none

This addresses an edge case where pindexBestForkBase is not previously set. Longer term, the logic further upstream in ActivateBestChainStep needs to be reworked.

laanwj added the label Bug on Oct 28, 2014

TheBlueMatt commented at 8:11 PM on October 29, 2014: member

Can you provide a description of how this bug can be triggered? (and have you seen this segfault?)

ghost commented at 3:02 AM on October 30, 2014: none

You'll see it on initializing the daemon, while scanning for better chains in the block chain database, that are not yet connected in the active best chain.

When this does occur, ConnectTip in ActivateBestChainStep returns true. Hence, fInvalidFound remains false, CheckForkWarningConditions is entered instead of CheckForkWarningConditionsOnNewFork where pindexBestForkBase is set. Hence, subsequent refs to pindexBestForkBase fail.

The commit catches this, and produces a "Warning: Found invalid chain at least ~6 blocks longer than our best chain. Chain state database corruption likely." Preferable to what otherwise results in a crash, and, in fact, informative, as reindex-ing does correct it.

TheBlueMatt commented at 5:10 AM on October 30, 2014: member

Huh? The way I read this pull, the only thing it will do is fix a SEGFAULT, not a logic error?

ghost commented at 12:13 PM on October 30, 2014: none

The description is that of the functional impact of the PR.

gmaxwell added this to the milestone 0.10.0 on Nov 21, 2014

gmaxwell added the label Priority High on Nov 21, 2014

gmaxwell added the label Priority Medium on Nov 21, 2014

gmaxwell removed the label Priority High on Nov 21, 2014

gmaxwell commented at 9:55 AM on November 21, 2014: contributor

I'm pretty sure this is actually triggerable exactly as 21E14 describes, e.g. when restarting a node which is had previously had an invalid block in the last execution. It's easily triggerable with Pieter's invalidate block patch.

The fix given here does not appear to be sufficient as pindexBestForkBase is also dereferenced in the next block below, so I cannot ACK. I would bludgeon Matt to clean the whole thing up, but he's travling and a minimal change for 0.10 would be preferable.

21E14, can you please extend the guards so that all the deferences in this function are protected? I think that would be the simple and conservative move right now rather than trying to reason about what invarients hold for these globals. Having potential crash bugs for a corner case nag message isn't something that excites me.

Check pindexBestForkBase for null 730b1ed1a0

ghost commented at 5:46 AM on November 22, 2014: none

Updated as suggested. Please note that, unlike the first block, I have not observed the second block being entered in this instance. Nevertheless, extending the guards seems prudent until the globals are reconsidered more systematically.

gmaxwell commented at 8:26 AM on November 22, 2014: contributor

Thanks. ACK.

sipa commented at 6:12 PM on November 23, 2014: member

utACK

luke-jr commented at 8:23 PM on November 23, 2014: member

Wouldn't we want warnings even if !pindexBestForkBase? Or am I missing something?

gmaxwell commented at 8:29 PM on November 23, 2014: contributor

@luke-jr It still gives a warning, just a less specific one, see the other prints in that block.

laanwj merged this on Nov 24, 2014

laanwj closed this on Nov 24, 2014

laanwj referenced this in commit 1ee685f984 on Nov 24, 2014

laanwj referenced this in commit 74f29c2737 on Feb 18, 2015

MarcoFalke locked this on Sep 8, 2021