CBloomFilter: Missing updateemptyfull? #16886

1. 
   alexzk1 commented at 4:31 pm on September 16, 2019: none

   On processing command filteradd (if (strCommand == NetMsgType::FILTERADD)) in net_processing.cpp it does not call pfilter->UpdateEmptyFull() while command filterload do that. Is it bug?
2. alexzk1 added the label Bug on Sep 16, 2019
3. fanquake added the label P2P on Sep 16, 2019
4. 
   theStack commented at 12:51 pm on September 19, 2019: member

   It’s not a bug, but rather a missed performance improvement: the purpose of CBloomFilter::UpdateEmptyFull() is to set the flags isEmpty (filter is all zero-bits) and isFull (filter is all one-bits) which in turn are checked in the in-set function CBloomFilter::contains() to avoid unnecessarily wasting time for those two corner-cases. Since the CBloomFilter::insert() operation could lead to a full bloom filter, it makes in my opinion perfect sense to update the isFull flag after via calling UpdateEmptyFull().
5. MarcoFalke removed the label Bug on Sep 19, 2019
6. MarcoFalke renamed this:
   Missing updateemptyfull?
   CBloomFilter: Missing updateemptyfull?
   on Sep 19, 2019
7. theStack referenced this in commit 9a2eee9082 on Sep 19, 2019
8. 
   alexzk1 commented at 6:39 pm on September 20, 2019: none

   Guess, UpdateEmptyFull must be private method and called from insert/add/copy filter from inside class CBloomFilter. That could make impossible such “forgot”.
9. 
   promag commented at 9:50 am on September 22, 2019: member

   @alexzk1 note that insert() is called in a loop so UpdateEmptyFull would be called unnecessarily.
10. 
    alexzk1 commented at 7:35 pm on September 22, 2019: none

    @alexzk1 note that insert() is called in a loop so UpdateEmptyFull would be called unnecessarily.

    Shouldn’t it be for each insert ? As I recall inserting into full filter is invalid, so any way must check if full prior next insert. But I can be wrong about.

11. 
    theStack commented at 9:01 pm on September 22, 2019: member

    @alexzk1 note that insert() is called in a loop so UpdateEmptyFull would be called unnecessarily.

    Shouldn’t it be for each insert ? As I recall inserting into full filter is invalid, so any way must check if full prior next insert. But I can be wrong about.

    Note that explicitely keeping track of whether a filter is full or empty is not necessary for correctness, but is only an optimization (“to avoid wasting cpu”, as the comment for the update function in bloom.h puts it), introduced with commit 37c6389c5a0ca63ae3573440ecdfe95d28ad8f07. Inserting into a full filter is not invalid, but rather a waste of time – since all bits are already set, setting any bit again doesn’t change the state.

12. theStack referenced this in commit b1dcc89084 on Oct 10, 2019
13. 
    theStack commented at 3:04 pm on April 28, 2020: member

    Considering that the primary point of introducing those empty/full-flags was not optimization, but a covert fix for CVE-2013-5700 (see gmaxwells comments about commit 37c6389: #18483 (comment) and #9060 (comment)), I think this issue can be closed.
14. MarcoFalke closed this on Apr 28, 2020

    ---

15. fanquake referenced this in commit 551dc7f664 on May 6, 2020
16. sidhujag referenced this in commit e9d4768e75 on May 12, 2020
17. MarcoFalke locked this on Feb 15, 2022

Contributors
alexzk1 theStack promag

Labels
P2P