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

validation: implement MaybeInvalidateFork() and call from rpc getchaintips #27434

Closed
wants to merge 2 commits into from

Conversation

pinheadmz
Copy link
Member

Closes #8050

If we discover an invalid block on a blockchain branch with otherwise valid headers, all the child headers can be marked as INVALID_CHILD. When invalidating mainchain blocks we can easily mark all the invalid children but on a headers-only branch we just stop on the invalid block and search for the next best tip and branch to validate. On restart, during LoadBlockIndex() we iterate through all the headers we know about sorted by height, and that is when we normally mark these child-headers of invalid ancestors with INVALID_CHILD.

Issue #8050 illustrates a discrepancy where a headers-only branch is not marked as invalid until the node is restarted. What we do in this PR is check for invalid ancestors while we are scanning the block index for chain tips during rpc getchaintips itself.

I had to remove a bunch of const qualifiers from getchaintips which makes me think that modifying the block index during an RPC is just bad separation of concerns -- but it is a solution!

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 6, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
Approach NACK luke-jr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@pinheadmz pinheadmz marked this pull request as ready for review April 8, 2023 14:07
@pinheadmz
Copy link
Member Author

Since I'm already here, I added a commit that fixes the incorrect usage of the term "orphan blocks" in rpc getchaintips code

cc: @MarcoFalke (fa0dfdf) and @mrbandrews (87049e8)

@luke-jr
Copy link
Member

luke-jr commented Jun 23, 2023

Approach NACK. RPC getters shouldn't be changing internal state. Let's just mark them invalid when they're found to be so?

@pinheadmz
Copy link
Member Author

Approach NACK. RPC getters shouldn't be changing internal state. Let's just mark them invalid when they're found to be so?

Yeah I agree. I'll close this PR for now and take another stab at the issue later. I am happy about the test and fixing the "orphan" phrasing though. Hopefully can reintegrate those in a new PR.

@pinheadmz pinheadmz closed this Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getchaintips doesn't mark headers-only chain as invalid
3 participants