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

1. 
   pinheadmz commented at 8:25 pm on April 6, 2023: 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!

2. 
   DrahtBot commented at 8:25 pm on April 6, 2023: contributor

   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.

3. DrahtBot added the label Validation on Apr 6, 2023
4. pinheadmz force-pushed on Apr 6, 2023
5. validation: implement MaybeInvalidateFork() and call from rpc getchaintips 447dc0bca4
6. pinheadmz force-pushed on Apr 7, 2023
7. pinheadmz marked this as ready for review on Apr 8, 2023
8. rpc: fix "orphan" comments and variable names in getchaintips 9bae704096
9. 
   pinheadmz commented at 7:30 pm on April 12, 2023: member

   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 (fa0dfdf447d5b84a1849dc823d8508463600136a) and @mrbandrews (87049e832d97d4f2808c0b479b21fc7b16c86934)

10. 
    luke-jr commented at 1:14 am on June 23, 2023: member

    Approach NACK. RPC getters shouldn’t be changing internal state. Let’s just mark them invalid when they’re found to be so?
11. 
    pinheadmz commented at 12:28 pm on June 23, 2023: member

    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.

12. pinheadmz closed this on Jun 23, 2023

    ---

13. bitcoin locked this on Jun 22, 2024

Contributors
pinheadmz DrahtBot luke-jr

Labels
Validation