doc: Better explain GetAncestor check for m_failed_blocks in AcceptBlockHeader #13930
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
026adec to
e99f3d9
Compare
|
jamesob
reviewed
Aug 10, 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). |
There was a problem hiding this comment.
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.
|
Nice improvement on the doc. utACK e99f3d9 |
|
Responding here, so it doesn't disappear when I rebase. @jamesob wrote:
That's how I understand it, and it also took me a few reads :-) We add stuff to
In both case that's only the block itself, so The only two other places where 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). |
e99f3d9 to
66e15e8
Compare
|
Expanded the comment to take the above into account. |
|
ACK |
maflcko
pushed a commit
that referenced
this pull request
Dec 22, 2018
…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
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jun 27, 2021
…ed_blocks in AcceptBlockHeader 66e15e8 Explain GetAncestor check for m_failed_blocks in AcceptBlockHeader (Sjors Provoost) Pull request description: Salvaged (but slightly modified) from bitcoin#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
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jun 28, 2021
…ed_blocks in AcceptBlockHeader 66e15e8 Explain GetAncestor check for m_failed_blocks in AcceptBlockHeader (Sjors Provoost) Pull request description: Salvaged (but slightly modified) from bitcoin#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
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jun 29, 2021
…ed_blocks in AcceptBlockHeader 66e15e8 Explain GetAncestor check for m_failed_blocks in AcceptBlockHeader (Sjors Provoost) Pull request description: Salvaged (but slightly modified) from bitcoin#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
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jul 1, 2021
…ed_blocks in AcceptBlockHeader 66e15e8 Explain GetAncestor check for m_failed_blocks in AcceptBlockHeader (Sjors Provoost) Pull request description: Salvaged (but slightly modified) from bitcoin#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
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jul 1, 2021
…ed_blocks in AcceptBlockHeader 66e15e8 Explain GetAncestor check for m_failed_blocks in AcceptBlockHeader (Sjors Provoost) Pull request description: Salvaged (but slightly modified) from bitcoin#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
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jul 1, 2021
…ed_blocks in AcceptBlockHeader 66e15e8 Explain GetAncestor check for m_failed_blocks in AcceptBlockHeader (Sjors Provoost) Pull request description: Salvaged (but slightly modified) from bitcoin#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
UdjinM6
pushed a commit
to UdjinM6/dash
that referenced
this pull request
Jul 5, 2021
…ed_blocks in AcceptBlockHeader 66e15e8 Explain GetAncestor check for m_failed_blocks in AcceptBlockHeader (Sjors Provoost) Pull request description: Salvaged (but slightly modified) from bitcoin#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
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jul 8, 2021
…ed_blocks in AcceptBlockHeader 66e15e8 Explain GetAncestor check for m_failed_blocks in AcceptBlockHeader (Sjors Provoost) Pull request description: Salvaged (but slightly modified) from bitcoin#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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 theBLOCK_VALID_SCRIPTSlevel yet. So in that case the existing text "previous block index isn't valid" is wrong.