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

[theStack]

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

[promag]

ACK 9a2eee9.

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

[theStack]

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

[laanwj]

(slight performance improvement)

When claiming performance improvements, please provide a benchmark.

[theStack]

(slight performance improvement)

When claiming performance improvements, please provide a benchmark.

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

[theStack]

(slight performance improvement)

When claiming performance improvements, please provide a benchmark.

The following benchmarks (theStack@3da6aac -- 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:

$ src/bench/bench_bitcoin "-filter=BloomFull.*"
# Benchmark, evals, iterations, total, min, max, median
BloomFull_Regular_Contains, 5, 21000000, 11.1822, 1.03667e-07, 1.08973e-07, 1.06582e-07
BloomFull_Regular_Insert, 5, 21000000, 11.3052, 1.05405e-07, 1.1052e-07, 1.06237e-07
BloomFull_UpdateEmptyFull_Contains, 5, 21000000, 0.478308, 3.78305e-09, 5.22676e-09, 4.73157e-09
BloomFull_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 37c6389 ("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.

[fanquake]

@gmaxwell Would you like to comment here?

[kallewoof]

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

[theStack]

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.

[theStack]

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 37c6389: #18483 (comment)). Hence this PR is closed now.