validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates #30479

pull mzumsande wants to merge 1 commits into bitcoin:master from mzumsande:202407_fix_resetfailure changing 1 files +2 −12
  1. mzumsande commented at 5:55 pm on July 18, 2024: contributor

    When we call reconsiderblock for some block,  Chainstate::ResetBlockFailureFlags puts the descendants of that block into setBlockIndexCandidates (if they meet the criteria, i.e. have more work than the tip etc.), but never put any ancestors into the set even though we do clear their failure flags.

    I think that this is wrong, because setBlockIndexCandidates should always contain all eligible indexes that have at least as much work as the current tip, which can include ancestors of the reconsidered block. This is being checked by CheckBlockIndex(), which could fail if it was invoked after ActivateBestChain connects a block and releases cs_main:

     0diff --git a/src/validation.cpp b/src/validation.cpp
     1index 7b04bd9a5b..ff0c3c9f58 100644
     2--- a/src/validation.cpp
     3+++ b/src/validation.cpp
     4@@ -3551,6 +3551,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
     5             }
     6         }
     7         // When we reach this point, we switched to a new tip (stored in pindexNewTip).
     8+        m_chainman.CheckBlockIndex();
     9 
    10         if (exited_ibd) {
    11             // If a background chainstate is in use, we may need to rebalance our
    

    makes rpc_invalidateblock.py fail on master.

    Even though we don’t currently have a CheckBlockIndex() in that place, after cs_main is released other threads could invoke it, which is happening in the rare failures of #16444 where an invalid header received from another peer could trigger a CheckBlockIndex() call that would fail.

    Fix this by adding eligible ancestors to setBlockIndexCandidates in Chainstate::ResetBlockFailureFlags (also simplifying that function a bit).

    Fixes #16444

  2. validation: Add ancestors of reconsiderblock to setBlockIndexCandidates
    When we call reconsiderblock for some block, ResetBlockFailureFlags puts the descendants of that block
    into setBlockIndexCandidates (if they meet the criteria, i.e. have more work than the tip etc.)
    We also clear the failure flags of the ancestors, but we never put any of those into setBlockIndexCandidates
    this is wrong and could lead to failures in CheckBlockIndex().
    4a76a8300f
  3. DrahtBot commented at 5:55 pm on July 18, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK TheCharlatan

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

  4. DrahtBot added the label Validation on Jul 18, 2024
  5. TheCharlatan approved
  6. TheCharlatan commented at 3:33 pm on August 23, 2024: contributor

    ACK 4a76a8300f295e8eab0ea8ab1ea1580450f131c8

    This has been the behaviour since reconsiderblock was introduced ~10 years ago https://github.com/bitcoin/bitcoin/commit/9b0a8d3152b43b63c99878d0223a1681993ad608#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R2176 .

  7. DrahtBot added the label CI failed on Sep 10, 2024
  8. DrahtBot removed the label CI failed on Sep 14, 2024
  9. achow101 requested review from sr-gi on Oct 15, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-18 04:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me