Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: Better explain GetAncestor check for m_failed_blocks in AcceptBlockHeader #13930

Merged
merged 1 commit into from Dec 22, 2018

Conversation

Projects
None yet
6 participants
@Sjors
Copy link
Member

commented Aug 9, 2018

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.

@Sjors Sjors force-pushed the Sjors:2018/08/validation-chainsplain branch Aug 9, 2018

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2018

*
* In the case that we attempted to reorg from E1 to F2, only to find
* C2 to be invalid, we would mark D2, E2, and F2 as BLOCK_FAILED_CHILD
* but NOT D3 (it was not in any of our candidate sets at the time).

This comment has been minimized.

Copy link
@jamesob

jamesob Aug 10, 2018

Member

Is this true because ConnectTip was never called on D3, meaning it was never added to 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.

@jamesob

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

Nice improvement on the doc.

utACK e99f3d9

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2018

Responding here, so it doesn't disappear when I rebase.

@jamesob wrote:

Is this true because ConnectTip was never called on D3, meaning it was never added to 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.

That's how I understand it, and it also took me a few reads :-)

We add stuff to m_failed_blocks here:

In 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

@sdaftuar

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

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

@Sjors Sjors force-pushed the Sjors:2018/08/validation-chainsplain branch to 66e15e8 Sep 4, 2018

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2018

Expanded the comment to take the above into account.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2018

ACK

@MarcoFalke MarcoFalke changed the title Better explain GetAncestor check for m_failed_blocks in AcceptBlockHeader doc: Better explain GetAncestor check for m_failed_blocks in AcceptBlockHeader Dec 22, 2018

@MarcoFalke MarcoFalke merged commit 66e15e8 into bitcoin:master Dec 22, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Dec 22, 2018

Merge #13930: doc: Better explain GetAncestor check for m_failed_bloc…
…ks in AcceptBlockHeader

66e15e8 Explain GetAncestor check for m_failed_blocks in AcceptBlockHeader (Sjors Provoost)

Pull request description:

  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.

Tree-SHA512: 442a319a83290d94697fdf51376463b70454e0f3909d4a45594ddc2e7c26cd19dc703808385a25e26d6d2dddab0aa35ca41722f2e65ee6fe57bbaf62652d3ec8

@Sjors Sjors deleted the Sjors:2018/08/validation-chainsplain branch Dec 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.