mempool: Clear vTxHashes when mapTx is cleared #24145

pull Bushstar wants to merge 1 commits into bitcoin:master from Bushstar:patch-8 changing 1 files +1 −0

Bushstar commented at 9:34 AM on January 25, 2022: contributor

vTxHashes is a vector of all entries in mapTx, if you clear one you should clear the other, lest someone try to use the txiter in vTxHashes which would result in a segfault.
fanquake added the label Mempool on Jan 25, 2022
glozow commented at 11:05 AM on January 25, 2022: member

light code review ACK 2902780

Perhaps someone with more historical insight should look at this as well. I can't find a reason for vTxHashes to not be cleared. It looks like it was introduced this way in 811902649d6aaddd886cb39b83aa69adf7b441bd.
MarcoFalke commented at 3:10 PM on January 26, 2022: member

It might be better to remove the whole function instead. See #20030 and then #19909
glozow commented at 4:01 PM on January 26, 2022: member

It might be better to remove the whole function instead. See #20030 and then #19909

Oh, agree. Is #19909 up for grabs?
MarcoFalke commented at 4:04 PM on January 26, 2022: member

#19909 is the trivial part, #20030 is the tougher one to review even though it only changes on line. I can reopen it later today.
MarcoFalke cross-referenced this on Jan 28, 2022 from issue bug: Reindex started via `ThreadSafeQuestion` will always fail by dongcarl
luke-jr commented at 10:54 PM on January 29, 2022: member

Regardless of removing the whole function later, this seems to be a good idea and should be reviewed/merged for backport benefit.
in src/txmempool.cpp:707 in 2902780093 outdated
```
 704 | @@ -705,6 +705,7 @@ void CTxMemPool::removeForBlock(const std::vector<CTransactionRef>& vtx, unsigne
 705 |  void CTxMemPool::_clear()
 706 |  {
 707 |      mapTx.clear();
 708 | +    vTxHashes.clear();
```
luke-jr commented at 10:54 PM on January 29, 2022:

Just for the sake of not having invalid iterators even temporarily, I prefer this before mapTx is cleared.

Bushstar commented at 7:53 AM on January 31, 2022:

Updated to move vTxHashes clear before mapTx clear.
luke-jr approved
luke-jr commented at 10:55 PM on January 29, 2022: member

utACK, 1 nit
Clear vTxHashes when mapTx is cleared 9d65ad365c
Bushstar force-pushed on Jan 31, 2022
luke-jr approved
luke-jr commented at 8:15 AM on January 31, 2022: member

re-utACK
MarcoFalke cross-referenced this on Feb 9, 2022 from issue validation, refactor: UnloadBlockIndex and ChainstateManager::Reset thread safety cleanups by jonatack
laanwj commented at 12:00 PM on April 6, 2022: member

Code review ACK 9d65ad365c539557e969a35d22723d92e0b422fe This makes sense for consistency. As long as there is a clear function it makes sense to clear everything and not forget this. Also agree on the long-run goal of removing it in favor of safer approaches.
laanwj renamed this:
~~Clear vTxHashes when mapTx is cleared~~
mempool: Clear vTxHashes when mapTx is cleared
on Apr 6, 2022
laanwj merged this on Apr 6, 2022
laanwj closed this on Apr 6, 2022
sidhujag referenced this in commit d29293f6aa on Apr 6, 2022
MarcoFalke cross-referenced this on Apr 20, 2022 from issue validation: Remove useless call to mempool->clear() by MarcoFalke
MarcoFalke cross-referenced this on Oct 10, 2022 from issue refactor: Remove unused CTxMemPool::clear() helper by MarcoFalke
glozow referenced this in commit 03254c2229 on Jan 4, 2023
bitcoin locked this on Apr 6, 2023

Contributors

Labels

Linked (view graph)

#19909 refactor: Remove unused CTxMemPool::clear() helper #20030 validation: Remove useless call to mempool->clear()#22964 bug: Reindex started via `ThreadSafeQuestion` will always fail #24299 validation, refactor: UnloadBlockIndex and ChainstateManager::Reset thread safety cleanups