[bugfix] update lockpoints correctly during reorg

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.
commit bedf246 introduced a bug by separating those two loops.

Author: glozow

src/txmempool.cpp

@@ -639,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/validation.cpp

@@ -378,6 +378,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;
     };