rpc: Fix dumptxoutset rollback with competing forks #33444

1. 
   enirox001 commented at 1:45 pm on September 20, 2025: contributor

   Fixes dumptxoutset rollback functionality when competing forks exist.

   Problem: dumptxoutset rollback fails when competing forks are present at the target height, even though it should work on the active chain regardless of forks.

   Solution: When competing forks exist, the current implementation’s InvalidateBlock call causes a reorg to the competing fork instead of rolling back to the desired main chain block.

   This PR fixes the issue by enhancing the TemporaryRollback class to:

   • Identify all competing fork blocks at heights above the target
   • Invalidate fork blocks first to prevent reorg during main chain invalidation
   • Then invalidate the main chain block for clean rollback
   • Restore all invalidated blocks in the destructor

   Changes:

   • Fix TemporaryRollback class in src/rpc/blockchain.cpp to handle competing forks
   • Add comprehensive functional test rpc_dumptxoutset_forks.py that verifies the fix
   • Update test to confirm dumptxoutset works correctly with competing forks present

   Testing: The test creates competing forks and verifies that dumptxoutset rollback now works correctly, creating snapshots from the intended main chain block rather than failing or using the wrong chain.

   Fixes #32817.

2. DrahtBot added the label RPC/REST/ZMQ on Sep 20, 2025
3. 
   DrahtBot commented at 1:45 pm on September 20, 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/33444.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #33477 (Rollback for dumptxoutset without invalidating blocks by fjahr)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

4. test: Add functional test for dumptxoutset rollback with competing forks

   Add test coverage for dumptxoutset rollback behavior when competing
   forks exist at the target rollback height. The test verifies that
   the current implementation fails in this scenario, establishing
   a baseline for future fixes.

   d615fbfa93
5. enirox001 force-pushed on Sep 20, 2025
6. rpc: Fix dumptxoutset rollback with competing forks

   When dumptxoutset rollback is used and competing forks exist at the target
   height, the InvalidateBlock call would cause a reorg to the competing fork
   instead of rolling back to the desired block on the main chain.
   
   Fix by invalidating all competing fork blocks first before invalidating
   the main chain block. This prevents reorgs during rollback and ensures
   the snapshot is created from the correct main chain block.
   
   The fix enhances the TemporaryRollback class to:
   - Identify competing fork blocks at heights above the target
   - Invalidate all fork blocks before main chain invalidation
   - Restore all invalidated blocks in the destructor

   aeb22c4361
7. 
   enirox001 commented at 10:16 pm on September 20, 2025: contributor

   @fjahr @mzumsande Would appreciate your feedback on this implementation when you get a chance. Thanks!
8. enirox001 marked this as ready for review on Sep 23, 2025
9. 
   fjahr commented at 5:04 pm on September 24, 2025: contributor

   This seems to implement the same logic that I used in #31117 but this turned out to be too fragile to be seriously considered for merging there. In this context it may work well enough for mainnet because there are not a lot of forks but we have been discussing an alternative approach for a while that does not rely on invalidateblock/reconsiderblock and this would solve the competing forks problem as well. I have worked on this idea for #31117 primarily but I will open a PR with that approach for dumptxoutset so it can be evaluated side-by-side since I am not sure which one is better yet.

   Also, it seems like the test you added is failing.

10. 
    enirox001 commented at 5:47 pm on September 24, 2025: contributor

    Thanks for the review and context @fjahr . I wasn’t aware of that prior work.

    You’re right that this approach has some fragility concerns. I’d be very interested to see your alternative approach that avoids invalidateblock/reconsiderblock altogether, as that does sound more robust.

    Regarding the test failure - let me investigate and fix that. Would you prefer I:

    1. Fix the test and keep this PR open for comparison with your upcoming alternative approach, or
    2. Close this PR and wait for your alternative solution?

    I’m happy to help review/test your approach when it’s ready, and I appreciate you taking the time to work on a cleaner solution to this issue.

11. 
    fjahr commented at 9:20 pm on September 24, 2025: contributor

    Regarding the test failure - let me investigate and fix that. Would you prefer I:

    I have opened #33477 now, as you can see there I am describing that there are some trade-offs between the approach and master which this PR would then be applied to. Let’s see what reviewers think about those trade-offs. You should keep it open and ideally fix the test. If the CI is failing that usually signals to reviewers that the PR isn’t ready for consideration since the code will need to change anyway. So reviewers might hold off from taking a look at the code.

Contributors
enirox001 DrahtBot fjahr

Labels
RPC/REST/ZMQ