mempool: recalculate stale BIP68 lockpoints with mempool parents in removeForReorg #35026

pull javierpmateos wants to merge 1 commits into bitcoin:master from javierpmateos:fix-bip68-stale-lockpoints-clean changing 3 files +71 −1

javierpmateos commented at 8:13 PM on April 7, 2026: none

When a transaction with nSequence=0 (BIP68 relative locktime 0) enters the mempool with an unconfirmed parent, CalculatePrevHeights assigns nCoinHeight=tip+1 and the resulting LockPoints are cached with maxInputBlock=genesis. After invalidateblock lowers the tip, TestLockPointValidity returns true for genesis (always on chain), so the stale cached lockpoints are reused. CheckSequenceLocksAtTip then incorrectly determines the lock is not satisfied and removes the transaction along with all its descendants.

The fix detects the genesis sentinel (maxInputBlock->nHeight==0 with lp.height>0) in filter_final_and_mature, which indicates lockpoints computed with mempool parent inputs, and forces fresh recalculation via CalculateLockPointsAtTip instead of using the stale cache.

Added mempool_reorg_bip68_stale_lockpoints.py which verifies that both BIP68-disabled and BIP68-enabled children with mempool parents survive a multi-block reorg via invalidateblock.

Reproducer for unpatched nodes: https://gist.github.com/javierpmateos/c55d365973adbf488a852dc5e0b77dec

Same class of issue fixed for TRUC in #33504.

Closes #35007
DrahtBot added the label Mempool on Apr 7, 2026
DrahtBot commented at 8:13 PM on April 7, 2026: 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/35026.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK Bicaru20

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
javierpmateos force-pushed on Apr 7, 2026
javierpmateos force-pushed on Apr 7, 2026
javierpmateos force-pushed on Apr 7, 2026
javierpmateos force-pushed on Apr 8, 2026
javierpmateos force-pushed on Apr 8, 2026
javierpmateos force-pushed on Apr 8, 2026
instagibbs commented at 1:15 PM on April 8, 2026: member

thanks for the PR. From my recollection this code is pretty complicated sadly. Will take a look soon hopefully to see if I can grasp it
DrahtBot added the label CI failed on Apr 9, 2026
DrahtBot removed the label CI failed on Apr 9, 2026
in src/validation.cpp:353 in 9dc0642ac5 outdated
```
 349 | @@ -350,7 +350,7 @@ void Chainstate::MaybeUpdateMempoolForReorg(
 350 |          const LockPoints& lp = it->GetLockPoints();
 351 |          // CheckSequenceLocksAtTip checks if the transaction will be final in the next block to be
 352 |          // created on top of the new chain.
 353 | -        if (TestLockPointValidity(m_chain, lp)) {
 354 | +        if (TestLockPointValidity(m_chain, lp) && !(lp.maxInputBlock && lp.maxInputBlock->nHeight == 0 && lp.height > 0)) {
```
Bicaru20 commented at 12:39 PM on May 3, 2026:

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 TestLockPointValidity. We don't have to pass any extra parameters and that way the condition on the if is cleaner.
javierpmateos commented at 5:42 PM on May 10, 2026:

Thanks for the suggestion. I attempted this refactor but it breaks the invariant asserted in CTxMemPool::removeForReorg() at txmempool.cpp:390:

assert(TestLockPointValidity(chain, it->GetLockPoints()));

When filter_final_and_mature recalculates lockpoints for a tx with mempool parents, CalculateLockPointsAtTip ignores mempool inputs (height == tip+1) when computing max_input_height, leaving it at 0. The resulting maxInputBlock is tip->GetAncestor(0) = genesis. So the freshly recalculated lockpoints still have maxInputBlock=genesis after UpdateLockPoints.

If the genesis-sentinel check were inside TestLockPointValidity, those legitimately-recalculated lockpoints would fail the post-removal assert in removeForReorg, crashing bitcoind (verified empirically — tested locally and the assertion fires).

Keeping the check at the caller site preserves TestLockPointValidity's broader semantics (it answers "is maxInputBlock still on chain?") while addressing the specific stale-lockpoints case in filter_final_and_mature where lp.height>0 combined with the genesis sentinel signals lockpoints from a previous tip.
Bicaru20 commented at 8:57 AM on May 13, 2026:

I see, I also got the same error. Good catch!

Type	Reviewers
ACK	Bicaru20

in test/functional/mempool_reorg_bip68_stale_lockpoints.py:60 in 9dc0642ac5

  55 | +            node, output=self.wallet.get_address(), transactions=[],
  56 | +        )["hash"]
  57 | +
  58 | +        node.invalidateblock(block_Y)
  59 | +        node.invalidateblock(block_setup)
  60 | +        assert node.getblockcount() == H - 1

Bicaru20 commented at 12:42 PM on May 3, 2026:

Maybe use assert_equal

        assert_equal(node.getblockcount(), H - 1)

javierpmateos commented at 5:42 PM on May 10, 2026:

Done in latest push.

in test/functional/mempool_reorg_bip68_stale_lockpoints.py:58 in 9dc0642ac5
```
  53 | +
  54 | +        block_Y = self.generateblock(
  55 | +            node, output=self.wallet.get_address(), transactions=[],
  56 | +        )["hash"]
  57 | +
  58 | +        node.invalidateblock(block_Y)
```
Bicaru20 commented at 7:39 PM on May 3, 2026:

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, since the LockPoints were calculated with this blockchain height. However, once that is done, why generate another block? If I am not mistaken this last block does not affect at all on the calculation.

I've tried the test without this block and the behavior is the same.

javierpmateos commented at 5:42 PM on May 10, 2026:

You're right, verified empirically — the bug reproduces without block_Y. Removed in latest push.

mempool: recalculate stale BIP68 lockpoints with mempool parents in removeForReorg

When a transaction with nSequence=0 (BIP68 relative locktime 0) enters
the mempool with an unconfirmed parent, CalculatePrevHeights assigns
nCoinHeight=tip+1 and the resulting LockPoints are cached with
maxInputBlock=genesis. After invalidateblock lowers the tip,
TestLockPointValidity returns true for genesis (always on chain),
so the stale cached lockpoints are reused. CheckSequenceLocksAtTip
then incorrectly determines the lock is not satisfied and removes
the transaction along with all its descendants.

Fix: in filter_final_and_mature, detect the genesis sentinel
(maxInputBlock->nHeight==0 with lp.height>0) which indicates
lockpoints computed with mempool parent inputs, and force fresh
recalculation via CalculateLockPointsAtTip.

This is the same class of issue fixed for TRUC in #33504.

Closes #35007

c281b1ea0b

javierpmateos force-pushed on May 10, 2026
Bicaru20 commented at 9:00 AM on May 13, 2026: none

ACK c281b1ea0b

Contributors

Labels

Linked (view graph)

#33504 Mempool: Do not enforce TRUC checks on reorg #35007 removeForReorg incorrectly evicts valid BIP68 transactions with mempool parents after invalidateblock

mempool: recalculate stale BIP68 lockpoints with mempool parents in removeForReorg #35026

Code Coverage & Benchmarks

Reviews