mirrors/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-21 14:34:49 +01:00
Code Issues Projects Releases Wiki Activity

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

Browse source 
b4adc5ad67 [bugfix] update lockpoints correctly during reorg (glozow)
b6002b07a3 MOVEONLY: update_lock_points to txmempool.h (glozow)

Pull request description:

  I introduced a bug in #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 b4adc5ad67
  vasild:
    ACK b4adc5ad67
  mzumsande:
    Code Review ACK b4adc5ad67
  hebasto:
    ACK b4adc5ad67
  MarcoFalke:
    re-ACK b4adc5ad67 🏁

Tree-SHA512: 16b59f6ff8140d0229079ca1c6b04f2f4a00a2e49931275150e4f3fe5ac4ec109698b083fa6b223ba9511f328271cc1ab081263669d5da020af7fee83c13e401
This commit is contained in:
MarcoFalke 2022-01-03 14:34:06 +01:00
commit 75a227e39e
No known key found for this signature in database
GPG key ID: CE2B75697E69A548
3 changed files with 13 additions and 14 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

15
src/txmempool.cpp
View file

@ -64,16 +64,6 @@ private:
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()));
}
}

10
src/txmempool.h
View file

@ -312,6 +312,16 @@ public:
}
};
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 {};

2
src/validation.cpp
View file

@ -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;
};