Merge bitcoin#23683: bug fix: valid but different LockPoints after a reorg

b4adc5a [bugfix] update lockpoints correctly during reorg (glozow)
b6002b0 MOVEONLY: update_lock_points to txmempool.h (glozow)

Pull request description:

  I introduced a bug in bitcoin#22677 (sorry! 😅)

  Mempool entries cache `LockPoints`, containing the first height/blockhash/`CBlockIndex*` at which the transaction becomes valid. During a reorg, we re-check timelocks on all mempool entries using `CheckSequenceLocks(useExistingLockPoints=false)` and remove any now-invalid entries. `CheckSequenceLocks()` also mutates the `LockPoints` passed in, and we update valid entries' `LockPoints` using `update_lock_points`. Thus, `update_lock_points(lp)` needs to be called right after `CheckSequenceLocks(lp)`, otherwise we lose the data in `lp`. I incorrectly assumed they could be called in separate loops.

  The incorrect behavior introduced is: if we have a reorg in which a timelocked mempool transaction is still valid but becomes valid at a different block, the cached `LockPoints` will be incorrect.

  This PR fixes the bug, adds a test, and adds an assertion at the end of `removeForReorg()` to check that all mempool entries' lockpoints are valid. You can reproduce the bug by running the test added in the [test] commit on the code before the [bugfix] commit.

ACKs for top commit:
  jnewbery:
    ACK b4adc5a
  vasild:
    ACK b4adc5a
  mzumsande:
    Code Review ACK b4adc5a
  hebasto:
    ACK b4adc5a
  MarcoFalke:
    re-ACK b4adc5a 🏁

Tree-SHA512: 16b59f6ff8140d0229079ca1c6b04f2f4a00a2e49931275150e4f3fe5ac4ec109698b083fa6b223ba9511f328271cc1ab081263669d5da020af7fee83c13e401

Author: MarcoFalke

src/txmempool.cpp

@@ -64,16 +64,6 @@ struct update_fee_delta
     int64_t feeDelta;
 };
 
-struct update_lock_points
-{
-    explicit update_lock_points(const LockPoints& _lp) : lp(_lp) { }
-
-    void operator() (CTxMemPoolEntry &e) { e.UpdateLockPoints(lp); }
-
-private:
-    const LockPoints& lp;
-};
-
 bool TestLockPointValidity(CChain& active_chain, const LockPoints& lp)
 {
     AssertLockHeld(cs_main);
@@ -649,10 +639,7 @@ void CTxMemPool::removeForReorg(CChain& chain, std::function<bool(txiter)> check
     }
     RemoveStaged(setAllRemoves, false, MemPoolRemovalReason::REORG);
     for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) {
-        const LockPoints lp{it->GetLockPoints()};
-        if (!TestLockPointValidity(chain, lp)) {
-            mapTx.modify(it, update_lock_points(lp));
-        }
+        assert(TestLockPointValidity(chain, it->GetLockPoints()));
     }
 }
 

src/txmempool.h

@@ -312,6 +312,16 @@ class CompareTxMemPoolEntryByAncestorFee
     }
 };
 
+struct update_lock_points
+{
+    explicit update_lock_points(const LockPoints& _lp) : lp(_lp) { }
+
+    void operator() (CTxMemPoolEntry &e) { e.UpdateLockPoints(lp); }
+
+private:
+    const LockPoints& lp;
+};
+
 // Multi_index tag names
 struct descendant_score {};
 struct entry_time {};

src/validation.cpp

@@ -375,6 +375,8 @@ void CChainState::MaybeUpdateMempoolForReorg(
                 }
             }
         }
+        // CheckSequenceLocks updates lp. Update the mempool entry LockPoints.
+        if (!validLP) m_mempool->mapTx.modify(it, update_lock_points(lp));
         return should_remove;
     };