#35026 mempool: recalculate stale BIP68 lockpoints in removeForReorg
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35026 mempool: recalculate stale BIP68 lockpoints in removeForReorg | Ready | 2 (+1) | Strong | 1 + 136 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
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.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: re-review from ismaelsadeeq 2026-10-30
Agreement
Strong
substantive support, no open objection (Bortlesboat, ismaelsadeeq, DevaanshPathak, tumutech)
Thread notes: Added comment c:6517025691 from ismaelsadeeq promising an upcoming re-review. No new claims were raised.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| Bicaru20 (contributor) | suggestion, interface | include the genesis-sentinel condition inside TestLockPointValidity | accepted · author explained it breaks an assert invariant in removeForReorg; Bicaru20 accepted | answered | yes | 2026-05-03: 'Since we already call `TestLockPointValidity`, maybe it would be better if this condition: `!(lp.maxInputBlock && lp.maxInputBlock->nHeight == 0 && lp.height > 0))` is included inside of the `TestLock' Status from: 2026-05-13: Bicaru20: 'I see, I also got the same error. Good catch!' |
| Bicaru20 (contributor) | question, approach | why generate another block after block_setup if it does not affect the calculation | fixed · fix pushed · author confirmed it was unnecessary and removed the extra block in a push | answered | yes | 2026-05-03: 'I'm not getting why do we need this. You create a block and immediately invalidate it. `block_setup` is needed to reproduce the bug... However, once that is done, why generate another block?' Status from: 2026-05-10: javierpmateos: 'You're right, verified empirically — the bug reproduces without block_Y. Removed in latest push.' |
| instagibbs (member) | objection, correctness | time-based relative locktime variant is missed if MTP drops post-reorg | fixed · fix pushed · recalculation fix uniformly handles time locks; Bicaru20 and author added functional test coverage | changed | yes | 2026-07-15: 'in addition to what @ismaelsadeeq said it seems it misses time-based variant, if MTP drops post-reorg.' Status from: 2026-08-20: javierpmateos: 'I've added it to the test (dropped the breakpoint(), kept a separate variable for the second setup block, and reset setm' |
| ismaelsadeeq (member) | objection, correctness | transactions spending mixed confirmed and unconfirmed inputs are still falsely evicted along with their descendants | fixed · fix pushed · author adopted the suggested recalculation approach and extended test; ismaelsadeeq approved | changed | 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.' Status from: 2026-07-20: ismaelsadeeq: 'ACK 3813fc8c9d6a70ce2e22fa2295e2ff95837ba67b' |
| ismaelsadeeq (member) | suggestion, safety | capture the time-based variant in the functional test to guard against future regression | fixed · fix pushed · Bicaru20 provided the mocktime setup and author integrated the test case into the branch | answered | yes | 2026-07-20: 'it will be nice to capture the time based variant as well to guard against future regression.' Status from: 2026-08-20: javierpmateos: 'I've added it to the test (dropped the breakpoint(), kept a separate variable for the second setup block, and reset setm' |
Support:
- Bicaru20 (contributor) (ACK, 2026-05-13): (no reason given) [not substantive]
- Bortlesboat (contributor) (2026-05-20) [tested]: regression test catches the stale-lockpoint behavior described in #35007, passing on branch and failing on master
- ismaelsadeeq (member) (ACK, 2026-07-20) [read]: general recalculation fix is sufficient, though capturing the time-based variant would prevent regressions
- Bicaru20 (contributor) (ACK, 2026-08-22): (no reason given) [not substantive]
- Bicaru20 (contributor) (ACK, 2026-08-24): (no reason given) [not substantive]
- DevaanshPathak (none) (2026-10-09) [both]: reproduced #35007 on unpatched master and confirmed that recalculating on CheckSequenceLocksAtTip failure covers mixed inputs better than a genesis-sentinel check
- tumutech (none) (ACK, 2026-10-19) [both: mempool_reorg_bip68_stale_lockpoints.py]: reviewed code and confirmed mempool_reorg_bip68_stale_lockpoints.py passes
Participants:
- instagibbs (member, objection, 2 statements 2026-04-08 to 2026-07-15): objected that the fix missed the time-based relative locktime variant when MTP drops post-reorg
- Bicaru20 (contributor, support, 8 statements 2026-05-03 to 2026-08-24): suggested moving the condition, questioned an extra block in the test, contributed the mocktime test for time locks, and ACKed
- Bortlesboat (contributor, support, 1 statements 2026-05-20 to 2026-05-20): tested regression test against master (fails) and branch (passes)
- ismaelsadeeq (member, objection, 3 statements 2026-07-15 to 2026-10-30): objected that mixed confirmed/unconfirmed inputs still caused false eviction, proposed general recalculation, and approved
- sedited (member, neutral, 1 statements 2026-09-06 to 2026-09-06): commented reminding a user about the project AI policy
- DevaanshPathak (none, support, 1 statements 2026-10-09 to 2026-10-09): independently reproduced the bug on master and supported the PR's recalculation approach
- tumutech (none, support, 1 statements 2026-10-19 to 2026-10-19): reviewed the code and ran the regression test
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
The short text the ranking pass compares across PRs.
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.
Data
thread read through 2026-10-30 (26 statements), head ea2d638bb9
labels: Mempool
Categories
Mempool and policy
P2 · bug fix
Fixes a bug where valid BIP68 transactions with unconfirmed parents and all their descendants are incorrectly evicted from the mempool during a reorg due to stale cached lockpoints. Correct mempool retention and eviction behavior directly impacts transaction propagation and second-layer protocols relying on relative timelocks.
Membership: Touches mempool eviction logic during reorgs in src/validation.cpp and carries the Mempool label.
Factors: security/stability 1, bug 2, performance 0, user value 1, leverage 0