Don't do mempool lookups for "mempool" command without a filter #7174

1. 
   TheBlueMatt commented at 9:46 AM on December 5, 2015: member

   No description provided.

2. Don't do mempool lookups for "mempool" command without a filter 96918a2f09
3. 
   pstratem commented at 9:51 AM on December 5, 2015: contributor

   utACK

4. 
   pstratem commented at 10:20 AM on December 5, 2015: contributor

   This performance regression was introduced in 319b11607f8592d7ef67ec82fa73545ad7430974

5. 
   petertodd commented at 11:38 AM on December 5, 2015: contributor

   utACK

6. 
   jgarzik commented at 4:15 PM on December 5, 2015: contributor

   utACK

7. 
   gmaxwell commented at 11:29 PM on December 5, 2015: contributor

   ACK. Good catch, it makes a huge performance difference with a large mempool. The problem still exists with a filter set, but it's a nice step forward. This change also makes the code simpler and clearer, which is always a bonus. I think we should take this back to 0.12 as well.

8. gmaxwell merged this on Dec 5, 2015
9. gmaxwell closed this on Dec 5, 2015

   ---

10. gmaxwell referenced this in commit 075faaebf2 on Dec 5, 2015
11. gmaxwell added this to the milestone 0.12.0 on Dec 6, 2015
12. 
    dcousens commented at 12:55 AM on December 7, 2015: contributor

    utACK

13. 
    laanwj commented at 11:27 AM on December 7, 2015: member

    Posthumous ACK on the code change. But this should have had a description and motivation in the commit message and PR description.

14. laanwj referenced this in commit 82aff880d3 on Dec 7, 2015
15. 
    luke-jr commented at 11:36 AM on December 7, 2015: member

    Agreed, I was confused as to its behaviour for a while. The description given sounds like "mempool" wouldn't work at all with non-filter connections.

16. 
    jgarzik commented at 11:50 AM on December 7, 2015: contributor

    Da. The description confused several devs...

17. 
    gmaxwell commented at 11:22 PM on December 7, 2015: contributor

    Ah, I didn't see the potential for confusion it all. Though the description was complete and accurate, what "mempool lookup" means and is ambigious I could see it how you might extract that from the description without reading the code.

    For short patches I try to read the code before the description, so as to not be biased by what it says it does and fail to see what it actually does. I would have expected with three acks from regulars that someone would have pointed out something like that earlier. Live and learn.

18. 
    dcousens commented at 1:50 AM on December 8, 2015: contributor

    Title made sense to me, I can understand how the misconception could exist, but, upon reading the code it was clear that it was just moving the branching logic around such that mempool.lookup(...) is only being done when necessary instead of pre-emptively.

19. in src/main.cpp:None in 96918a2f09

    5003 | +            if (pfrom->pfilter) {
    5004 | +                CTransaction tx;
    5005 | +                bool fInMemPool = mempool.lookup(hash, tx);
    5006 | +                if (!fInMemPool) continue; // another thread removed since queryHashes, maybe...
    5007 | +                if (!pfrom->pfilter->IsRelevantAndUpdate(tx)) continue;
    5008 | +            }
    

    ---

    ---

    

    dcousens commented at 1:53 AM on December 8, 2015:

    There is 1 other change that wasn't described here, which is !IsRelevantAndUpdate and vInv.size() === MAX_INV_SZ would previously cause a "inv" message to be pushed. This does not occur now.

    However, given the logic that vInv.size() is only increased if isRelevantAndUpdate was true anyway, means AFAIK this shouldn't be an issue.

    I guess, that is a minor performance optimization then :+1: :smiley:

20. zkbot referenced this in commit f6d09aa822 on Apr 5, 2018
21. furszy referenced this in commit 1f010c0969 on Jan 23, 2021
22. DrahtBot locked this on Sep 8, 2021

Milestone
0.12.0