index: fix wrong assert of current_tip == m_best_block_index #32878

pull HowHsu wants to merge 1 commits into bitcoin:master from HowHsu:rewind changing 1 files +0 −1
  1. HowHsu commented at 3:53 pm on July 4, 2025: none
    In BaseIndex::Sync(), pindex in Rewind(pindex, pindex_next->pprev) isn’t always equal to m_best_block_index since m_best_block_index is updated every SYNC_LOCATOR_WRITE_INTERVAL seconds, during which multiple pindex update could happen. Thus the assert here is wrong.
  2. index: fix wrong assert of current_tip == m_best_block_index
    In BaseIndex::Sync(), pindex in `Rewind(pindex, pindex_next->pprev)` isn't always
    equal to m_best_block_index since m_best_block_index is updated every
    SYNC_LOCATOR_WRITE_INTERVAL seconds, during which multiple pindex update could
    happen. Thus the assert here is wrong.
    
    Signed-off-by: Hao Xu <hao.xu@linux.dev>
    b57eb0a344
  3. DrahtBot added the label UTXO Db and Indexes on Jul 4, 2025
  4. DrahtBot commented at 3:53 pm on July 4, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32878.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK furszy

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

  5. 1440000bytes commented at 8:36 pm on July 4, 2025: none
    Can you write a test that triggers this assert on master branch?
  6. HowHsu commented at 6:39 am on July 5, 2025: none

    Can you write a test that triggers this assert on master branch?

    Sure, I’ll do that.

  7. mzumsande commented at 9:54 pm on July 6, 2025: contributor

    I think that this assert could be hit if a reorg was happening while Sync() was going on:

    1. Sync() syncs some blocks but isn’t finished (doesn’t update m_best_block)

    2. The node undergoes a reorg, so that some of the synced blocks need to be rewinded.

    3. Rewind() is called and the assert fails.

    I could trigger this on regtest with an added sleep in Sync(), freezing the sync so that I could do the reorg. I think it’s going to be hard / impossible to trigger it in a test without that sleep.

  8. HowHsu force-pushed on Jul 7, 2025
  9. DrahtBot added the label CI failed on Jul 7, 2025
  10. DrahtBot commented at 12:37 pm on July 7, 2025: contributor

    🚧 At least one of the CI tasks failed. Task macOS-cross, gui, no tests: https://github.com/bitcoin/bitcoin/runs/45472933675 LLM reason (✨ experimental): The CI failure is caused by compile errors due to unused lambda capture and thread safety analysis violations in the test source code.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  11. HowHsu force-pushed on Jul 7, 2025
  12. HowHsu force-pushed on Jul 7, 2025
  13. furszy commented at 1:51 pm on July 7, 2025: member

    I don’t think the test is useful. The change is being tested against a Sync() mechanism introduced within the test itself and not the real one, which can change over time (see for example #26966 that changes it completely). It would be better, and simpler for you, to share a patch that reproduces the issue (as @mzumsande mentioned, just adding a sleep() call and triggering the reorg is enough to reproduce it. See #32835 PR description for a patch example).

    Other than that, concept ACK.

  14. HowHsu force-pushed on Jul 7, 2025
  15. HowHsu commented at 2:40 pm on July 7, 2025: none

    I don’t think the test is useful. The change is being tested against a Sync() mechanism introduced within the test itself and not the real one, which can change over time (see for example #26966 that changes it completely). It would be better, and simpler for you, to share a patch that reproduces the issue (as @mzumsande mentioned, just adding a sleep() call and triggering the reorg is enough to reproduce it. See #32835 PR description for a patch example).

    Other than that, concept ACK.

    Thanks furszy, you are right, I’ll remove this test.

  16. HowHsu commented at 2:41 pm on July 7, 2025: none

    I think that this assert could be hit if a reorg was happening while Sync() was going on:

    1. Sync() syncs some blocks but isn’t finished (doesn’t update m_best_block)
    2. The node undergoes a reorg, so that some of the synced blocks need to be rewinded.
    3. Rewind() is called and the assert fails.

    I could trigger this on regtest with an added sleep in Sync(), freezing the sync so that I could do the reorg. I think it’s going to be hard / impossible to trigger it in a test without that sleep.

    Thanks mzumsande.

  17. HowHsu force-pushed on Jul 7, 2025
  18. HowHsu commented at 2:46 pm on July 7, 2025: none
    Hi all, I’ve written a test with tweaked Sync() to demonstrate this bug: https://github.com/bitcoin/bitcoin/commit/e9f4ffcd7d8b7211cc049a8ab7632b19c06f7b11
  19. DrahtBot removed the label CI failed on Jul 7, 2025

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: 2025-07-07 21:13 UTC

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