refactor: remove CTxMemPool::queryHashes() #29260

stickies-v commented at 12:57 PM on January 17, 2024: contributor

CTxMemPool::queryHashes() is only used in MempoolToJSON(), where it can just as easily be replaced with the more general CTxMemPool::entryAll(). No behaviour change, just cleans up the code.

DrahtBot commented at 12:57 PM on January 17, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	dergoegge, TheCharlatan, glozow

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Refactoring on Jan 17, 2024

in src/test/fuzz/tx_pool.cpp:106 in 17da80132d outdated

 100 | @@ -101,9 +101,6 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Cha
 101 |      if (!info_all.empty()) {
 102 |          const auto& tx_to_remove = *PickValue(fuzzed_data_provider, info_all).tx;
 103 |          WITH_LOCK(tx_pool.cs, tx_pool.removeRecursive(tx_to_remove, MemPoolRemovalReason::BLOCK /* dummy */));
 104 | -        std::vector<uint256> all_txids;
 105 | -        tx_pool.queryHashes(all_txids);
 106 | -        assert(all_txids.size() < info_all.size());

dergoegge commented at 3:02 PM on January 18, 2024:

Why is it ok to remove this assertion?

glozow commented at 3:13 PM on January 18, 2024:

maybe replace this with a call to something else and keep the test. The assertion is just checking that the size has shrunk after removing something.

stickies-v commented at 9:13 PM on January 18, 2024:

Sorry, I misunderstood the test. Replaced now with:

        assert(tx_pool.size() < info_all.size());

in src/rpc/mempool.cpp:363 in 17da80132d outdated

 360 |              LOCK(pool.cs);
 361 | -            pool.queryHashes(vtxid);
 362 | +            for (const CTxMemPoolEntry& e : pool.entryAll()) {
 363 | +                a.push_back(e.GetTx().GetHash().ToString());
 364 | +            }
 365 |              mempool_sequence = pool.GetSequence();

dergoegge commented at 3:03 PM on January 18, 2024:

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...

stickies-v commented at 9:23 PM on January 18, 2024:

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)

dergoegge commented at 9:30 PM on January 18, 2024:

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...

dergoegge commented at 3:04 PM on January 18, 2024: member

stickies-v force-pushed on Jan 18, 2024

stickies-v commented at 9:21 PM on January 18, 2024: contributor

Force pushed to re-instate the fuzz test, addressing #29260 (review)

refactor: remove CTxMemPool::queryHashes()

Its only usage can easily be replaced with CTxMemPool::entryAll()

282b12ddb0

in src/test/fuzz/tx_pool.cpp:104 in c84c5f2d25 outdated

 100 | @@ -101,9 +101,8 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Cha
 101 |      if (!info_all.empty()) {
 102 |          const auto& tx_to_remove = *PickValue(fuzzed_data_provider, info_all).tx;
 103 |          WITH_LOCK(tx_pool.cs, tx_pool.removeRecursive(tx_to_remove, MemPoolRemovalReason::BLOCK /* dummy */));
 104 | -        std::vector<uint256> all_txids;
 105 | -        tx_pool.queryHashes(all_txids);
 106 | -        assert(all_txids.size() < info_all.size());
 107 | +        const auto new_size{WITH_LOCK(tx_pool.cs, return tx_pool.entryAll().size())};

dergoegge commented at 9:24 PM on January 18, 2024:

could use infoAll to avoid the WITH_LOCK?

stickies-v commented at 9:57 PM on January 18, 2024:

Looks like there's a much simpler tx_pool.size() option too, using that now.

stickies-v force-pushed on Jan 18, 2024

dergoegge approved

dergoegge commented at 9:58 PM on January 18, 2024: member

Code review ACK 282b12ddb0aeb1f0991dd9f45c6b5c5c079652ec

TheCharlatan approved

TheCharlatan commented at 1:08 PM on January 19, 2024: contributor

ACK 282b12ddb0aeb1f0991dd9f45c6b5c5c079652ec

DrahtBot requested review from TheCharlatan on Jan 19, 2024

DrahtBot removed review request from TheCharlatan on Jan 19, 2024

glozow commented at 5:16 PM on January 19, 2024: member

ACK 282b12ddb0aeb1f0991dd9f45c6b5c5c079652ec. Looks like there's no conflicts.

glozow merged this on Jan 22, 2024

glozow closed this on Jan 22, 2024

stickies-v deleted the branch on Jan 22, 2024

bitcoin locked this on Jan 21, 2025