refactor: remove CTxMemPool::queryHashes() #29260
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
| LOCK(pool.cs); | ||
| pool.queryHashes(vtxid); | ||
| for (const CTxMemPoolEntry& e : pool.entryAll()) { | ||
| a.push_back(e.GetTx().GetHash().ToString()); | ||
| } | ||
| mempool_sequence = pool.GetSequence(); |
There was a problem hiding this comment.
Would be cool if we could use infoAll since that doesn't require the lock externally but we need the sequence to be in sync so we can't, oh well...
There was a problem hiding this comment.
entryAll returning just refs has less overhead than infoAll so I think regardless this is a preferred approach? (even if it's not a huge difference)
There was a problem hiding this comment.
That overhead is tiny if any, I'd think. You can benchmark MempoolToJSON and it'd bet it won't even show up. But anyway we can't use it due to the sequence sync requirement.
Actually, maybe it's worth considering removing entryAll too? it's only used in two places and we can work around the sync issue somehow, but not in this PR...
stickies-v
force-pushed
the
2024-01/remove-queryhashes
branch
from
17da801
to
c84c5f2
Compare
|
Force pushed to re-instate the fuzz test, addressing #29260 (comment) |
Its only usage can easily be replaced with CTxMemPool::entryAll()
stickies-v
force-pushed
the
2024-01/remove-queryhashes
branch
from
c84c5f2
to
282b12d
Compare
DrahtBot
requested review from
TheCharlatan
and removed request for
TheCharlatan
CTxMemPool::queryHashes()is only used inMempoolToJSON(), where it can just as easily be replaced with the more generalCTxMemPool::entryAll(). No behaviour change, just cleans up the code.