net: filteradd message: update bloom filter empty/full flags after adding #16922

1. 
   theStack commented at 9:43 pm on September 19, 2019: member

   fixes #16886, discovered by alexzk1 (slight performance improvement)
2. DrahtBot added the label P2P on Sep 19, 2019
3. 
   promag commented at 9:52 am on September 22, 2019: member

   ACK 9a2eee9082084dee5bf4478d8a6197184b221e3f.

   The suggestion in #16886 (comment) makes sense but it requires a slightly larger refactor but maybe worth the effort?

4. 
   theStack commented at 6:22 pm on September 22, 2019: member

   @promag: Thanks for the review! The suggestion by alexzk1 to make the method private and handle the empty/full flag only internally seemed good to me at first as well, but where would be appropriate places to trigger the update? As you noted, insert() is not an option since it is called in a loop.
5. 
   laanwj commented at 11:07 am on September 30, 2019: member

   (slight performance improvement)

   When claiming performance improvements, please provide a benchmark.

6. laanwj added the label Resource usage on Sep 30, 2019
7. fanquake added the label Waiting for author on Oct 1, 2019
8. 
   theStack commented at 8:15 am on October 6, 2019: member

   (slight performance improvement)

   When claiming performance improvements, please provide a benchmark.

   Okay, will try to craft a benchmark for this within the next days.

9. net: filteradd message: update bloom filter empty/full flags after adding

   fixes #16886, discovered by alexzk1
   (slight performance improvement)

   b1dcc89084
10. theStack force-pushed on Oct 10, 2019
11. 
    theStack commented at 11:39 am on October 10, 2019: member

    (slight performance improvement)

    When claiming performance improvements, please provide a benchmark.

    The following benchmarks (https://github.com/theStack/bitcoin/commit/3da6aac15a91932c8d1a644e9fdf85e043e9f8ef – not part of this PR, for convenience added to rollingbloom.cpp rather than creating a new file bloom.cpp) show how the operations to insert (method .insert()) and check for (method .contains()) elements perform for full bloom filters, once without and once with calling .UpdateEmpyFull() before:

    0$ src/bench/bench_bitcoin "-filter=BloomFull.*"
    1# Benchmark, evals, iterations, total, min, max, median
    2BloomFull_Regular_Contains, 5, 21000000, 11.1822, 1.03667e-07, 1.08973e-07, 1.06582e-07
    3BloomFull_Regular_Insert, 5, 21000000, 11.3052, 1.05405e-07, 1.1052e-07, 1.06237e-07
    4BloomFull_UpdateEmptyFull_Contains, 5, 21000000, 0.478308, 3.78305e-09, 5.22676e-09, 4.73157e-09
    5BloomFull_UpdateEmptyFull_Insert, 5, 21000000, 0.386489, 3.41868e-09, 3.93211e-09, 3.71036e-09
    

    My wording was sloppy – the performance improvement (or in this case, CPU waste minimizing) is actually not ‘slight’, but significant (20-30x), but it only applies for full filters, which is not a regular case.

    Note that the optimization itself is not new, it was added by gmaxwell with commit https://github.com/bitcoin/bitcoin/commit/37c6389c5a0ca63ae3573440ecdfe95d28ad8f07 (“Performance optimization for bloom filters.”). The point is that it was only called on receiving the filterload message, but not after receiving the filteradd message, which is fixed with this PR.

12. fanquake removed the label Waiting for author on Dec 1, 2019
13. 
    fanquake commented at 3:17 am on January 5, 2020: member

    @gmaxwell Would you like to comment here?
14. theStack requested review from kallewoof on Feb 5, 2020
15. 
    kallewoof commented at 7:28 pm on February 5, 2020: member

    I’m confused why I was asked to review this as I haven’t touched this code.
16. 
    theStack commented at 9:54 pm on February 5, 2020: member

    I’m confused why I was asked to review this as I haven’t touched this code.

    For some reason you were in the list of suggested reviewers (don’t know on what criteria github decides this). Feel free to ignore.

17. 
    theStack commented at 6:36 pm on April 15, 2020: member

    At the time of opening this PR I assumed that the reason for the introduction of the CBloomFilter empty/full flag was optimization, as stated in the commit message, but in fact it was a covert fix for CVE-2013-5700 (see gmaxwells comment about commit https://github.com/bitcoin/bitcoin/commit/37c6389c5a0ca63ae3573440ecdfe95d28ad8f07: #18483 (comment)). Hence this PR is closed now.
18. theStack closed this on Apr 15, 2020

    ---

19. theStack deleted the branch on Apr 15, 2020
20. fanquake referenced this in commit 551dc7f664 on May 6, 2020
21. sidhujag referenced this in commit e9d4768e75 on May 12, 2020
22. DrahtBot locked this on Feb 15, 2022

Contributors
theStack promag laanwj fanquake kallewoof

Review Requested
kallewoof

Labels
P2P Resource usage