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

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 and AI policy for information on the review process.

Type	Reviewers
Stale ACK	Bicaru20, ismaelsadeeq

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 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!

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.

javierpmateos force-pushed on May 10, 2026

Bicaru20 commented at 9:00 AM on May 13, 2026: none

ACK c281b1ea0b

Bortlesboat commented at 9:45 PM on May 20, 2026: contributor

Tested c281b1ea0b17a969b29f12665c12e1b2f8aab664 on Ubuntu 24.04 WSL2, GCC 13.3, debug build with GUI and IPC disabled.

build/test/functional/test_runner.py mempool_reorg_bip68_stale_lockpoints.py --jobs=1 passes on this branch.

I also ran the same test file against master 489da5df60f797a13e162e8e6ab89c0a7d9f5b2f, where it fails with:

AssertionError: child_B (BIP68 enabled) must survive after fix

So the regression test catches the stale-lockpoint behavior described in #35007.

sedited requested review from ismaelsadeeq on Jul 6, 2026

ismaelsadeeq commented at 2:43 PM on July 15, 2026: member

Good find, but I think this only handles the case where all of the transaction's inputs are unconfirmed that's the only way maxInputBlock ends up at genesis.

If a transaction spends a mix of confirmed and unconfirmed inputs, the cached lp.height still carries the tip+1 assumption from the unconfirmed input, but maxInputBlock points to the confirmed input's block.

If that block survives the reorg, TestLockPointValidity passes, the genesis check added here doesn't fire, and CheckSequenceLocksAtTip below fails against the lowered tip, the transaction is still falsely evicted along with its descendants.

The new test doesn't catch this because both children spend only unconfirmed parents; it fails with this adjustment.

diff --git a/test/functional/mempool_reorg_bip68_stale_lockpoints.py b/test/functional/mempool_reorg_bip68_stale_lockpoints.py
index 8c73f5e5d1..a066b8be8d 100755
--- a/test/functional/mempool_reorg_bip68_stale_lockpoints.py
+++ b/test/functional/mempool_reorg_bip68_stale_lockpoints.py
@@ -23,6 +23,13 @@ class MempoolReorgBip68StaleLocksTest(BitcoinTestFramework):
         self.wallet = MiniWallet(node)
         self.generate(self.wallet, 200)

+        # Confirm a utxo one block BELOW the block we will invalidate, so that
+        # the mixed spend below keeps a confirmed input through the reorg.
+        mixed_funding = self.wallet.send_self_transfer(
+            from_node=node, confirmed_only=True,
+        )
+        self.generate(node, 1)
+
         funding = self.wallet.send_self_transfer_multi(
             from_node=node, num_outputs=2, confirmed_only=True,
         )
@@ -49,8 +56,22 @@ class MempoolReorgBip68StaleLocksTest(BitcoinTestFramework):
             sequence=SEQ_BIP68_ZERO,
         )

+        child_mixed = self.wallet.create_self_transfer_multi(
+            utxos_to_spend=[mixed_funding["new_utxo"], child_B["new_utxo"]],
+            sequence=SEQ_BIP68_ZERO,
+        )
+        node.sendrawtransaction(child_mixed["hex"])
+
         self.log.info("child_A seq=0xFFFFFFFE: %s", child_A["txid"][:16])
         self.log.info("child_B seq=0x00000000: %s", child_B["txid"][:16])
+        self.log.info("child_mixed confirmed+unconfirmed inputs, seq=0x00000000: %s", child_mixed["txid"][:16])

         node.invalidateblock(block_setup)
         assert_equal(node.getblockcount(), H - 1)
@@ -59,9 +80,11 @@ class MempoolReorgBip68StaleLocksTest(BitcoinTestFramework):

         assert child_A["txid"] in mp, "child_A (BIP68 disabled) must survive"
         assert child_B["txid"] in mp, "child_B (BIP68 enabled) must survive after fix"
+        assert child_mixed["txid"] in mp, "child_mixed (confirmed + unconfirmed inputs) must survive after fix"

         self.log.info("child_A (BIP68 disabled): SURVIVED")
         self.log.info("child_B (BIP68 enabled):  SURVIVED (stale lockpoints recalculated)")
+        self.log.info("child_mixed (mixed inputs): SURVIVED (stale lockpoints recalculated)")
         self.log.info("PASS: removeForReorg correctly recalculates BIP68 lockpoints")

A simpler fix is to never evict on the cached values alone: recalculate whenever the lockpoints are stale, or the cached check fails, and evict only if the fresh result also fails. Of course, this has a tradeoff, we may recalculate when we don't need to.

diff --git a/src/validation.cpp b/src/validation.cpp
index fac537f99e..adf4fc6e47 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -350,11 +350,7 @@ void Chainstate::MaybeUpdateMempoolForReorg(
         const LockPoints& lp = it->GetLockPoints();
         // CheckSequenceLocksAtTip checks if the transaction will be final in the next block to be
         // created on top of the new chain.
-        if (TestLockPointValidity(m_chain, lp) && !(lp.maxInputBlock && lp.maxInputBlock->nHeight == 0 && lp.height > 0)) {
-            if (!CheckSequenceLocksAtTip(m_chain.Tip(), lp)) {
-                return true;
-            }
-        } else {
+        if (!TestLockPointValidity(m_chain, lp) || !CheckSequenceLocksAtTip(m_chain.Tip(), lp)) {
             const CCoinsViewMemPool view_mempool{&CoinsTip(), *m_mempool};
             const std::optional<LockPoints> new_lock_points{CalculateLockPointsAtTip(m_chain.Tip(), view_mempool, tx)};
             if (new_lock_points.has_value() && CheckSequenceLocksAtTip(m_chain.Tip(), *new_lock_points)) {

instagibbs commented at 3:11 PM on July 15, 2026: member

in addition to what @ismaelsadeeq said it seems it misses time-based variant, if MTP drops post-reorg. his fix seems to work more generally and less brittle?

javierpmateos force-pushed on Jul 16, 2026

javierpmateos commented at 9:34 AM on July 16, 2026: none

Good find, but I think this only handles the case where all of the transaction's inputs are unconfirmed that's the only way maxInputBlock ends up at genesis.

If a transaction spends a mix of confirmed and unconfirmed inputs, the cached lp.height still carries the tip+1 assumption from the unconfirmed input, but maxInputBlock points to the confirmed input's block.

If that block survives the reorg, TestLockPointValidity passes, the genesis check added here doesn't fire, and CheckSequenceLocksAtTip below fails against the lowered tip, the transaction is still falsely evicted along with its descendants.

The new test doesn't catch this because both children spend only unconfirmed parents; it fails with this adjustment.

diff --git a/test/functional/mempool_reorg_bip68_stale_lockpoints.py b/test/functional/mempool_reorg_bip68_stale_lockpoints.py
index 8c73f5e5d1..a066b8be8d 100755
--- a/test/functional/mempool_reorg_bip68_stale_lockpoints.py
+++ b/test/functional/mempool_reorg_bip68_stale_lockpoints.py
@@ -23,6 +23,13 @@ class MempoolReorgBip68StaleLocksTest(BitcoinTestFramework):
         self.wallet = MiniWallet(node)
         self.generate(self.wallet, 200)

+        # Confirm a utxo one block BELOW the block we will invalidate, so that
+        # the mixed spend below keeps a confirmed input through the reorg.
+        mixed_funding = self.wallet.send_self_transfer(
+            from_node=node, confirmed_only=True,
+        )
+        self.generate(node, 1)
+
         funding = self.wallet.send_self_transfer_multi(
             from_node=node, num_outputs=2, confirmed_only=True,
         )
@@ -49,8 +56,22 @@ class MempoolReorgBip68StaleLocksTest(BitcoinTestFramework):
             sequence=SEQ_BIP68_ZERO,
         )

+        child_mixed = self.wallet.create_self_transfer_multi(
+            utxos_to_spend=[mixed_funding["new_utxo"], child_B["new_utxo"]],
+            sequence=SEQ_BIP68_ZERO,
+        )
+        node.sendrawtransaction(child_mixed["hex"])
+
         self.log.info("child_A seq=0xFFFFFFFE: %s", child_A["txid"][:16])
         self.log.info("child_B seq=0x00000000: %s", child_B["txid"][:16])
+        self.log.info("child_mixed confirmed+unconfirmed inputs, seq=0x00000000: %s", child_mixed["txid"][:16])

         node.invalidateblock(block_setup)
         assert_equal(node.getblockcount(), H - 1)
@@ -59,9 +80,11 @@ class MempoolReorgBip68StaleLocksTest(BitcoinTestFramework):

         assert child_A["txid"] in mp, "child_A (BIP68 disabled) must survive"
         assert child_B["txid"] in mp, "child_B (BIP68 enabled) must survive after fix"
+        assert child_mixed["txid"] in mp, "child_mixed (confirmed + unconfirmed inputs) must survive after fix"

         self.log.info("child_A (BIP68 disabled): SURVIVED")
         self.log.info("child_B (BIP68 enabled):  SURVIVED (stale lockpoints recalculated)")
+        self.log.info("child_mixed (mixed inputs): SURVIVED (stale lockpoints recalculated)")
         self.log.info("PASS: removeForReorg correctly recalculates BIP68 lockpoints")

A simpler fix is to never evict on the cached values alone: recalculate whenever the lockpoints are stale, or the cached check fails, and evict only if the fresh result also fails. Of course, this has a tradeoff, we may recalculate when we don't need to.

diff --git a/src/validation.cpp b/src/validation.cpp
index fac537f99e..adf4fc6e47 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -350,11 +350,7 @@ void Chainstate::MaybeUpdateMempoolForReorg(
         const LockPoints& lp = it->GetLockPoints();
         // CheckSequenceLocksAtTip checks if the transaction will be final in the next block to be
         // created on top of the new chain.
-        if (TestLockPointValidity(m_chain, lp) && !(lp.maxInputBlock && lp.maxInputBlock->nHeight == 0 && lp.height > 0)) {
-            if (!CheckSequenceLocksAtTip(m_chain.Tip(), lp)) {
-                return true;
-            }
-        } else {
+        if (!TestLockPointValidity(m_chain, lp) || !CheckSequenceLocksAtTip(m_chain.Tip(), lp)) {
             const CCoinsViewMemPool view_mempool{&CoinsTip(), *m_mempool};
             const std::optional<LockPoints> new_lock_points{CalculateLockPointsAtTip(m_chain.Tip(), view_mempool, tx)};
             if (new_lock_points.has_value() && CheckSequenceLocksAtTip(m_chain.Tip(), *new_lock_points)) {

Thanks for the thorough review — you're right on both counts. I've adopted your approach of recalculating whenever the cached validity check or the sequence-lock check fails, rather than detecting the genesis sentinel specifically. It's simpler and covers the mixed confirmed/unconfirmed input case that my original check missed.

I've also added your child_mixed case to the regression test (confirmed input below block_setup + unconfirmed input), which fails on the previous genesis-only approach and passes with the general recalculation.

Confirmed locally: the extended test passes with this fix, and mempool_reorg.py / feature_bip68_sequence.py still pass.

javierpmateos commented at 9:34 AM on July 16, 2026: none

in addition to what @ismaelsadeeq said it seems it misses time-based variant, if MTP drops post-reorg. his fix seems to work more generally and less brittle?

Good point on the time-based variant. The recalculation approach handles it uniformly ... CheckSequenceLocksAtTip evaluates both height and time locks, so any stale cache (height or MTP-based) triggers recalculation the same way.

I tried adding a time-based case to the functional test but reproducing it reliably in regtest is brittle: a time-based relative lock over an unconfirmed parent is rejected as non-BIP68-final on mempool entry (the 512s lock isn't satisfied against tip+1), and forcing the MTP to drop post-reorg requires careful timestamp choreography that would make the test flaky. Given the fix covers height and time uniformly by construction, I've kept the test to the height-based and mixed-input cases that reproduce deterministically. Open to suggestions if you think a robust time-based case is worth adding.

ismaelsadeeq approved

ismaelsadeeq commented at 12:39 PM on July 20, 2026: member

ACK 3813fc8c9d6a70ce2e22fa2295e2ff95837ba67b

nit: commit message is too verbose, and does not capture the time variant indicated by @instagibbs #35026 (comment), this test is sufficient, but it will be nice to capture the time based variant as well to guard against future regression.

DrahtBot requested review from Bicaru20 on Jul 20, 2026

mempool: recalculate stale BIP68 lockpoints in removeForReorg

BIP68 transactions entering the mempool with unconfirmed parents cache
LockPoints relative to the current tip (nCoinHeight=tip+1). After
invalidateblock lowers the tip, these cached lockpoints are stale, but
TestLockPointValidity may still return true, so filter_final_and_mature
evicts the transaction and its descendants based on the stale cache.

Recalculate via CalculateLockPointsAtTip whenever the cached validity
check or the sequence-lock check fails, and only evict if the fresh
result also fails. This handles all-unconfirmed, mixed, and time-based
relative locktimes uniformly.

Co-authored-by: Abubakar Sadiq Ismail <abubakarsadiqismail@proton.me>

Closes #35007

c71aa2db62

javierpmateos force-pushed on Jul 20, 2026

javierpmateos commented at 7:11 PM on July 20, 2026: none

ACK 3813fc8

nit: commit message is too verbose, and does not capture the time variant indicated by @instagibbs #35026 (comment), this test is sufficient, but it will be nice to capture the time based variant as well to guard against future regression.

Thanks! ... shortened the commit message and added the Co-authored-by trailer.

On the time-based variant: I spent some time trying to build a deterministic functional test for it and I don't think one exists cleanly. The height-based mixed case works as a regression test because lp.height caches an artificially high tip+1 from the unconfirmed input while the confirmed input stays valid at the new tip — so the tx is genuinely valid post-reorg but the stale cache marks it invalid.

For the time-based case, when the reorg lowers the MTP, the transaction tends to become genuinely invalid (the relative time hasn't actually elapsed on the reorged chain), so eviction is correct rather than a bug. I instrumented several scenarios (varying lock magnitude and reorg depth) and confirmed the eviction result is identical on this branch and on master — i.e. not a regression the test could catch.

The fix still covers the time-based path by construction, since the recalculation branch runs whenever CheckSequenceLocksAtTip fails regardless of whether the lock is height- or time-based. If you know of a timestamp arrangement that produces a genuinely-valid-but-stale time-based case, I'm happy to add it — otherwise I've kept the test to the cases that reproduce deterministically.

Note: the only change since your ACK is the commit message and the Co-authored-by trailer — the code (validation.cpp and the test) is byte-identical to 3813fc8.