refactor: remove CTxMemPool::queryHashes() #29260

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

3. DrahtBot added the label Refactoring on Jan 17, 2024
4. 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:

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

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

6. 
   dergoegge commented at 3:04 pm on January 18, 2024: member
7. stickies-v force-pushed on Jan 18, 2024
8. 
   stickies-v commented at 9:21 pm on January 18, 2024: contributor

   Force pushed to re-instate the fuzz test, addressing #29260 (review)
9. refactor: remove CTxMemPool::queryHashes()

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

   282b12ddb0
10. 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.
11. stickies-v force-pushed on Jan 18, 2024
12. dergoegge approved
13. 
    dergoegge commented at 9:58 pm on January 18, 2024: member

    Code review ACK 282b12ddb0aeb1f0991dd9f45c6b5c5c079652ec
14. TheCharlatan approved
15. 
    TheCharlatan commented at 1:08 pm on January 19, 2024: contributor

    ACK 282b12ddb0aeb1f0991dd9f45c6b5c5c079652ec
16. DrahtBot requested review from TheCharlatan on Jan 19, 2024
17. DrahtBot removed review request from TheCharlatan on Jan 19, 2024
18. 
    glozow commented at 5:16 pm on January 19, 2024: member

    ACK 282b12ddb0aeb1f0991dd9f45c6b5c5c079652ec. Looks like there’s no conflicts.
19. glozow merged this on Jan 22, 2024
20. glozow closed this on Jan 22, 2024

    ---

21. stickies-v deleted the branch on Jan 22, 2024

Contributors
stickies-v DrahtBot dergoegge glozow TheCharlatan

Labels
Refactoring