cc @MarcoFalke, thanks for catching my bug 🐛

utACK 28b60ce

The bug is pretty scary, good thing we found it this soon, and the fix is straightforward.
The test looks correct.

Too bad this can't properly be tested on regtest. I guess the only way is via a pre-mined main-chain?

Ah no. I don't think this is possible to test at all. The lockpoints are just a cache and if it is outdated, evaluating it will always return false, in which case the lockpoints are re-evaluated. So this is not fixing a behaviour bug, but potentially improving performance a bit?

Too bad this can't properly be tested on regtest. I guess the only way is via a pre-mined main-chain?

Ah no. I don't think this is possible to test at all. The lockpoints are just a cache and if it is outdated, evaluating it will always return false, in which case the lockpoints are re-evaluated. So this is not fixing a behaviour bug, but potentially improving performance a bit?

The "natural reorg to a new chain that is shorter (but has more work)" is probably not possible without significant changes to regtest chainparams, I think. Since this PR adds assertions for the lockpoints, making sure they're updated is testable. And true, I suppose outdated lockpoints are not that bad, though now I'm wondering if it's worth adding the assertions if it's relatively harmless.

Since this PR adds assertions for the lockpoints

I was hoping there was a way to test this on current master without modifying the code or looking at the process internal memory, but yeah...

Concept ACK a64078e

Ah no. I don't think this is possible to test at all. The lockpoints are just a cache and if it is outdated, evaluating it will always return false, in which case the lockpoints are re-evaluated. So this is not fixing a behaviour bug, but potentially improving performance a bit?

Yes, I don't think this is a bug introducing invalid transactions in the mempool. Just a perf regression.

If you have a reorg from chain H to chain H'' and the lockpoints are not valid anymore on H'', the cache is not going be updated accordingly.

Let's say you have one more reorg from chain H'' to chain H'''. If H''' includes the max CBlockIndex (tested in TestLockPointValidity), the lockpoints are anew valid. If H''' does not include the max CBlockIndex, the lockpoints are going to be recomputed by CheckSequenceLocks to determine if the transaction is final.

So if this reasoning is correct, I also agree that caching lockpoints is a perf improvement, in the sense they save few coins fetches and block index walkes ?

cc @MarcoFalke, thanks for catching my bug

How did you catch this? It looks pretty obscure to me.

cc @MarcoFalke, thanks for catching my bug

How did you catch this? It looks pretty obscure to me.

Through code review, basically. Though, it was a bit tricky because there was never an externally observable logic bug, at most the performance difference. For a few hours I even thought the code could be removed (#23660) 😅

Change looks good to me. I agree it fixes the lockpoint invalidity bug introduced in #22677.

A few suggestions for the check_final_and_mature lambda:

• A contributing factor to the the bug being introduced was misleading naming (of update_lock_points()) and a lack of documentation. Therefore, we should be extra explicit about what's going on in these functions by adding plenty of code documentation.
• the return value is the inverse of what I'd expect. I'd expect a check_blah() function to return true if the check passes and false if it fails.
• the local should_remove variable seems unnecessary. The function can just return immediately if one of the checks fails.

I've included a suggested patch below. Feel free to take whatever you think is an improvement.

diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 26cb2ff4b4..c742548875 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -631,7 +631,7 @@ void CTxMemPool::removeForReorg(CChain& chain, std::function<bool(txiter)> check
 
     setEntries txToRemove;
     for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) {
-        if (check_final_and_mature(it)) txToRemove.insert(it);
+        if (!check_final_and_mature(it)) txToRemove.insert(it);
     }
     setEntries setAllRemoves;
     for (txiter it : txToRemove) {
diff --git a/src/validation.cpp b/src/validation.cpp
index f53641e786..8da2e80510 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -350,21 +350,38 @@ void CChainState::MaybeUpdateMempoolForReorg(
     // the disconnectpool that were added back and cleans up the mempool state.
     m_mempool->UpdateTransactionsFromBlock(vHashUpdate);
 
-    const auto check_final_and_mature = [this, flags=STANDARD_LOCKTIME_VERIFY_FLAGS](CTxMemPool::txiter it)
+    // Check whether the transaction is still final/mature, and update
+    // the transaction's cached lockpoints if necessary.
+    // If this returns false, the mempool *must* remove the transaction, since
+    // it is no longer valid for inclusion in the next block (and its cached
+    // lockpoints may be invalid).
+    const auto check_final_and_mature = [this, flags=STANDARD_LOCKTIME_VERIFY_FLAGS](CTxMemPool::txiter it) -> bool
         EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs, ::cs_main) {
-        bool should_remove = false;
         AssertLockHeld(m_mempool->cs);
         AssertLockHeld(::cs_main);
         const CTransaction& tx = it->GetTx();
+
+        // Check transaction finality
+        if (!CheckFinalTx(m_chain.Tip(), tx, flags)) {
+            return false;
+        }
+
+        // Check sequence locks
         LockPoints lp = it->GetLockPoints();
         const bool validLP{TestLockPointValidity(m_chain, lp)};
         CCoinsViewMemPool view_mempool(&CoinsTip(), *m_mempool);
-        if (!CheckFinalTx(m_chain.Tip(), tx, flags)
-            || !CheckSequenceLocks(m_chain.Tip(), view_mempool, tx, flags, &lp, validLP)) {
-            // Note if CheckSequenceLocks fails the LockPoints may still be invalid
-            // So it's critical that we remove the tx and not depend on the LockPoints.
-            should_remove = true;
-        } else if (it->GetSpendsCoinbase()) {
+
+        if (!CheckSequenceLocks(m_chain.Tip(), view_mempool, tx, flags, &lp, validLP)) {
+            // Sequence locks fail. The mempool *must* remove this transaction since
+            // we're not going to update its cached lockpoints.
+            return false;
+        } else if (!validLP) {
+            // CheckSequenceLocks has modified lp. Update the cached lockpoints.
+             m_mempool->mapTx.modify(it, update_lock_points(lp));
+        }
+
+        // Check coinbase spend maturity
+        if (it->GetSpendsCoinbase()) {
             for (const CTxIn& txin : tx.vin) {
                 auto it2 = m_mempool->mapTx.find(txin.prevout.hash);
                 if (it2 != m_mempool->mapTx.end())
@@ -373,18 +390,19 @@ void CChainState::MaybeUpdateMempoolForReorg(
                 assert(!coin.IsSpent());
                 const auto mempool_spend_height{m_chain.Tip()->nHeight + 1};
                 if (coin.IsSpent() || (coin.IsCoinBase() && mempool_spend_height - coin.nHeight < COINBASE_MATURITY)) {
-                    should_remove = true;
-                    break;
+                    // Coinbase spend is now immature. Remove tx from mempool.
+                    return false;
                 }
             }
         }
-        // CheckSequenceLocks updates lp. Update the mempool entry LockPoints.
-        if (!validLP) m_mempool->mapTx.modify(it, update_lock_points(lp));
-        return should_remove;
+
+        // Transaction is still final and mature. Cached lockpoints have been updated.
+        return true;
     };
 
-    // We also need to remove any now-immature transactions
+    // Remove any transactions that are no longer final/mature, and update cached lockpoints.
     m_mempool->removeForReorg(m_chain, check_final_and_mature);
+
     // Re-limit mempool size, in case we added any transactions
     LimitMempoolSize(
         *m_mempool,

Notice - the ACKed commit is the HEAD~ of this PR, without the topmost commit which adds a test which I do not think is harmful and I do not object. But I don't see what value it adds

I've removed the test. Perhaps a followup dedicated to test improvements can convert the invalidateblocks to natural reorgs.

A few suggestions for the check_final_and_mature lambda:

Thanks, I've applied (most of) your diff. I see what you mean by the return results being unexpected for something called "check." My wish is to follow the convention of a unary predicate that returns true if something needs to be removed. I've renamed check to filter.

Sorry for the churn. I had wanted to incorporate the suggestions in #23683 (comment) for the lambda, but I now think it's best saved for a different PR.

Sorry for the churn. I had wanted to incorporate the suggestions in #23683 (comment) for the lambda, but I now think it's best saved for a different PR.

I think it's very important that the comments are updated to clarify what this function is doing. A bug was introduced in this code because the code was unclear, so we should take the opportunity to clarify it to prevent a similar regression being added again.

This branch does fix the recent regression, but I think it's significantly worse than the previous version since good tests and clarifying code comments have been removed.

Concept ACK.

ACK b4adc5a

But please do update the comments and add a test in a follow-up.