rpc: Fix dumptxoutset rollback with competing forks #33444
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33444. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
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.
enirox001
force-pushed
the
20_09_25_test_dumptxoutset_forks
branch
from
0b1c1d3 to
d615fbf
Compare
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
|
@fjahr @mzumsande Would appreciate your feedback on this implementation when you get a chance. Thanks! |
|
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 Also, it seems like the test you added is failing. |
|
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:
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. |
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. |
Couldn't this just be a contrib script or client-side bitcoin-cli feature? ie: which first looks up that block hash's height N, then iterates through each chain tip whose height is at least N, getting that tip's ancestor at height N+1 (optimise this using the |
FWIW, the rollback part used to be a contrib script and I integrated it into |
4 participants
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
InvalidateBlockcall 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
TemporaryRollbackclass to:Changes:
TemporaryRollbackclass insrc/rpc/blockchain.cppto handle competing forksrpc_dumptxoutset_forks.pythat verifies the fixdumptxoutsetworks correctly with competing forks presentTesting:
The test creates competing forks and verifies that
dumptxoutset rollbacknow works correctly, creating snapshots from the intended main chain block rather than failing or using the wrong chain.Fixes #32817.