Salvaged (but slightly modified) from #12138, the comment there was really helpful to wrap my head around that part of the code.
In addition, a naive reader like yours truly will first think IsValid(BLOCK_VALID_SCRIPTS) means the previous block was invalid. But IIUC that’s not what it means. Instead, it means the block hasn’t been checked for validity at the BLOCK_VALID_SCRIPTS level yet. So in that case the existing text “previous block index isn’t valid” is wrong.
3399+ * \
3400+ * B1 - C1 - D1 - E1
3401+ *
3402+ * In the case that we attempted to reorg from E1 to F2, only to find
3403+ * C2 to be invalid, we would mark D2, E2, and F2 as BLOCK_FAILED_CHILD
3404+ * but NOT D3 (it was not in any of our candidate sets at the time).
m_failed_blocks by way of InvalidBlockFound? If that is the case, it may be worth making a more detailed note of that on this line; took me a few minutes of digging in the code to evaluate this claim.
Nice improvement on the doc.
utACK https://github.com/bitcoin/bitcoin/pull/13930/commits/e99f3d9b0c2ba7f787eab1ae3724e738ff050e01
Responding here, so it doesn’t disappear when I rebase. @jamesob wrote:
Is this true because
ConnectTipwas never called on D3, meaning it was never added tom_failed_blocksby way ofInvalidBlockFound? If that is the case, it may be worth making a more detailed note of that on this line; took me a few minutes of digging in the code to evaluate this claim.
That’s how I understand it, and it also took me a few reads :-)
We add stuff to m_failed_blocks here:
InvalidBlockFound: https://github.com/bitcoin/bitcoin/blob/e99f3d9b0c2ba7f787eab1ae3724e738ff050e01/src/validation.cpp#L1309InvalidateBlock: https://github.com/bitcoin/bitcoin/blob/e99f3d9b0c2ba7f787eab1ae3724e738ff050e01/src/validation.cpp#L2838In both case that’s only the block itself, so C2, children aren’t touched. Presumably they just remain BLOCK_VALID_TREE.
The only two other places where BLOCK_FAILED_CHILD is set are LoadBlockIndex (only called at launch) and FindMostWorkChain, which only traverses from the candidate block down to the bad block via ->pprev.
If we learned about D3 after E1 was connected, it has less work than E1 - assuming equal difficulty - and therefore we would not have tried to connect it. Then when F2 comes in it does have more work, so now we try to connect it, which is when we find out C2 is invalid. So then we mark F2…C2 invalid, still ignoring D3.
I think we should use the block height as the number in this chart, to emphasize relative difficulty
utACK, thanks for improving this comment.
If you want to explain things even further, you could note that the reason we bypass this logic when pindexPrev has been validated up to BLOCK_VALID_SCRIPTS is just a performance optimization, so that in the common case of adding a new block to the tip, we don’t have to iterate over the failed blocks list.
Also to be totally pedantic, while D3 won’t be marked BLOCK_FAILED_CHILD since we never tried to connect it, it should be marked as such the next time we restart bitcoind (in LoadBlockIndex).
Labels
Docs
Validation