I am confused reading this code block if (!pindexPrev->IsValid(BLOCK_VALID_SCRIPTS)) {...
Then I find bad-prevblk is returned twice. One here and one above if (pindexPrev->nStatus & BLOCK_FAILED_MASK)...
To disambiguate, I change bad-prevblk to bad-prevblk-desc and verify that this is the right error being tested in p2p_unrequested_blocks. This the only place this behavior is tested I think.
How to add Test label to this PR? Do I need permission?
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
I'm not sure what the goal here is, but I think there may be a misunderstanding -- there should be no logical difference between the failure you're changing and the one above it. Note the comment above, specifically:
* In any case D3 will also be marked as BLOCK_FAILED_CHILD at restart
* in LoadBlockIndex.
So I think the code is correct as-is.
Goal is debugging and understanding. I can not determine which section of code is throwing the same error. This is a design flaw I think? If there is no logical difference between this and above, then why duplicate logic?
The goal of the logic you're looking at is to determine whether a new block descends from an invalid one. This is complex due to the intricacies of how block validation occurs.
If the direct parent of a new block is known to be invalid, then that is detected immediately in this chunk of code:
if (pindexPrev->nStatus & BLOCK_FAILED_MASK)
return state.Invalid(ValidationInvalidReason::BLOCK_INVALID_PREV, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk");
However, as explained in the comment directly above the code you're changing, there are situations where the parent of a block may descend from some other known-invalid block, but the parent itself has not yet been validated and so is not yet marked as invalid. This code that loops through m_failed_blocks looking for an invalid ancestor is for this case. Logically, this is no different than the direct parent being invalid but not yet having been marked as such.
My understanding is this: If node has continuous chain of headers A -> ... -> D -> ... -> Z so that D is invalid, all block after D will hit first case. If node does not has continuous chain of headers A and D only and D is invalid blocks after D will hit second case.
Your logic is this: Both cases is bad previous block so treat error message and code the same. This is despite different checks.
Do I have that correct?
Is first case redundant? Just a optimization?
Closing this PR, there's no agreement to change this, it looks more like a question.