#35026 mempool: recalculate stale BIP68 lockpoints with mempool parents in removeForReorg
https://github.com/bitcoin/bitcoin/pull/35026 · · +137/-5 in 3 files, 1 commits · labels: Mempool
Goal
- Keep valid transactions with BIP68 relative locktimes in the mempool during chain reorganizations
- Prevent false eviction of valid unconfirmed transaction chains when the chain tip changes
Fixes a bug in `Chainstate::MaybeUpdateMempoolForReorg` where BIP68 transactions with unconfirmed parents are incorrectly evicted when a chain reorg lowers the tip. Cached lockpoints evaluated relative to a previous tip become stale, so the PR recalculates lockpoints via `CalculateLockPointsAtTip` whenever cached validity or sequence checks fail.
Problem: When a BIP68 transaction enters the mempool with unconfirmed inputs, its lockpoints are cached assuming parent confirmation at tip + 1. If a reorg lowers the tip or median time past, the cached lockpoints falsely report unsatisfied sequence locks, causing the transaction and all its descendants to be evicted from the mempool even when still valid.
Category: Mempool and policy (#7 of 10)
P3 · bug fix
- P2 because valid transactions and their descendants are erroneously dropped during chain reorganizations
- It restores relay reliability across chain tip adjustments and block invalidations
Fixes an edge-case mempool bug where valid BIP68 transactions with unconfirmed parents are evicted during reorgs due to stale cached lockpoints. While evicted transactions can be rebroadcast, fixing false eviction maintains mempool consistency across reorgs.
Membership: Changes how mempool reorg handling evaluates sequence locks and evicts transactions.
Factors: security/stability 0, bug 1, performance 0, user value 1, leverage 0
Reviewability: Ready
- Ready for review
- Previous review concerns around mixed inputs and time-based locks have been addressed
The patch is small, cleanly rebased, and CI passes with comprehensive regression test coverage.
Author status: active
Resolved concerns:
- The initial genesis-sentinel check only handled transactions with all-unconfirmed parents and missed mixed confirmed/unconfirmed inputs as well as time-based relative locktimes.
- The test needed a predictable median time past to deterministically test the time-based lockpoint recalculation on reorg.
Agreement: Strong
- Strong consensus on recalculating lockpoints whenever cached checks fail
- Suggested recalculating on cache failure and approved the fix (ismaelsadeeq)
- Co-developed deterministic mocktime tests and approved the fix (Bicaru20)
- Verified regression reproduction on unpatched master (Bortlesboat)
Strong consensus after ismaelsadeeq and Bicaru20 helped refine the fix and test suite
All reviewers who raised concerns are satisfied. The proposed approach was simplified to recalculate on any failed check, covering all input and locktime variants.
- ismaelsadeeq approved the general recalculation approach
- Bicaru20 re-ACKed after contributing the mocktime setup for time-based test coverage
- Bortlesboat tested and confirmed reproduction
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| Bicaru20 | approach | unnecessary caller-side condition if handled inside TestLockPointValidity | resolved | no | yes | 2026-05-03: 'Since we already call TestLockPointValidity, maybe it would be better if this condition... is included inside of the TestLockPointValidity.' Settled: 2026-05-13: 'I see, I also got the same error. Good catch! ACK c281b1ea0b' after author explained it breaks txmempool.cpp assert |
| ismaelsadeeq | correctness | transactions spending mixed confirmed and unconfirmed inputs are still falsely evicted | resolved | yes | yes | 2026-07-15: 'If a transaction spends a mix of confirmed and unconfirmed inputs... the transaction is still falsely evicted along with its descendants.' Settled: 2026-07-20: 'ACK 3813fc8c9d6a70ce2e22fa2295e2ff95837ba67b' after author adopted general recalculation |
| instagibbs | correctness | time-based variant is missed if MTP drops post-reorg | resolved | no | yes | 2026-07-15: 'in addition to what @ismaelsadeeq said it seems it misses time-based variant, if MTP drops post-reorg.' Settled: 2026-07-16: author confirmed general recalculation covers time-based locks and added regression test coverage |
Support:
- ismaelsadeeq: simpler and general fix ensuring valid transactions survive reorgs without relying on genesis sentinel assumptions
- Bicaru20: verified fix, helped develop stable MTP test for time-based variant, and ACKed
- Bortlesboat: verified regression test fails on master and passes on branch
Participants: instagibbs (objection), Bicaru20 (support), Bortlesboat (support), ismaelsadeeq (objection), sedited (neutral)
State derived from the lists: substantive support, no open objection (ismaelsadeeq, Bicaru20, Bortlesboat)
Review verdicts (DrahtBot): 1 (+1)
- ACK: Bicaru20
- Stale ACK: ismaelsadeeq
Files
136 lines under test/bench/ci.
- test/functional/mempool_reorg_bip68_stale_lockpoints.py +135/-0
- src/validation.cpp +1/-5
- test/functional/test_runner.py +1/-0
Card
This PR fixes a bug in removeForReorg where valid BIP68 transactions with unconfirmed parents are incorrectly evicted from the mempool following a chain reorg. Stale cached lockpoints evaluated against the previous higher tip or MTP falsely indicate that relative locktime is unsatisfied. The fix recalculates lockpoints via CalculateLockPointsAtTip whenever the cached check fails, supported by a functional test covering height-based, mixed-input, and time-based cases. Review consensus is strong with multiple ACKs and no open objections.