refactor: Remove unused methods CBloomFilter::reset()/clear() #18670

theStack · 2020-04-16T15:19:59Z

The method CBloomFilter::reset() was introduced by commit d2d7ee0 in 2015, but was never ever used, as far as I could find. As discovered by MarcoFalke, the method clear() is also unused outside of unit tests and is hence also removed.

theStack · 2020-04-16T15:25:20Z

Hint for reviewers: there are seemingly reset() method calls on CBloomFilter instances, e.g.:

bitcoin/src/net_processing.cpp

Line 3164 in 661e8df

pfrom->m_tx_relay->pfilter.reset(new CBloomFilter(filter));

but those refer to std::unique_ptr::reset(), not to CBloomFilter::reset()!

maflcko · 2020-04-16T15:39:34Z

ACK. Please also remove clear. clear is unused outside of tests and the bip explicitly states that the filter should be discarded completely instead of cleared when a filterclear message is sent. I don't understand why we implement features in Bitcoin Core that are unused outside of tests.

diff --git a/src/test/bloom_tests.cpp b/src/test/bloom_tests.cpp
index 4a7ad9b38b..bcf2e8ccff 100644
--- a/src/test/bloom_tests.cpp
+++ b/src/test/bloom_tests.cpp
@@ -27,6 +27,7 @@ BOOST_AUTO_TEST_CASE(bloom_create_insert_serialize)
 {
     CBloomFilter filter(3, 0.01, 0, BLOOM_UPDATE_ALL);
 
+    BOOST_CHECK_MESSAGE( !filter.contains(ParseHex("99108ad8ed9bb6274d3980bab5a85c048f0950c8")), "Bloom filter should be empty!");
     filter.insert(ParseHex("99108ad8ed9bb6274d3980bab5a85c048f0950c8"));
     BOOST_CHECK_MESSAGE( filter.contains(ParseHex("99108ad8ed9bb6274d3980bab5a85c048f0950c8")), "Bloom filter doesn't contain just-inserted object!");
     // One bit different in first byte
@@ -50,8 +51,6 @@ BOOST_AUTO_TEST_CASE(bloom_create_insert_serialize)
     BOOST_CHECK_EQUAL_COLLECTIONS(stream.begin(), stream.end(), expected.begin(), expected.end());
 
     BOOST_CHECK_MESSAGE( filter.contains(ParseHex("99108ad8ed9bb6274d3980bab5a85c048f0950c8")), "Bloom filter doesn't contain just-inserted object!");
-    filter.clear();
-    BOOST_CHECK_MESSAGE( !filter.contains(ParseHex("99108ad8ed9bb6274d3980bab5a85c048f0950c8")), "Bloom filter should be empty!");
 }
 
 BOOST_AUTO_TEST_CASE(bloom_create_insert_serialize_with_tweak)

theStack · 2020-04-16T15:53:59Z

Updated commit, PR title and description to reflect the change of also removing CBloomFilter::clear().

maflcko · 2020-04-16T16:10:16Z

ACK 9b95bdc

laanwj · 2020-04-16T16:16:46Z

ACK 9b95bdc

jonatack · 2020-04-16T18:57:43Z

ACK 9b95bdc

maflcko · 2020-04-16T22:47:56Z

test/fuzz/bloom_filter.cpp:60:26: error: no member named 'clear' in 'CBloomFilter'

            bloom_filter.clear();

            ~~~~~~~~~~~~ ^

test/fuzz/bloom_filter.cpp:63:26: error: no member named 'reset' in 'CBloomFilter'

            bloom_filter.reset(fuzzed_data_provider.ConsumeIntegral<unsigned int>());

            ~~~~~~~~~~~~ ^

2 errors generated.

Co-authored-by: MarcoFalke <falke.marco@gmail.com>

theStack · 2020-04-16T23:14:58Z

Force-pushed commit, now also removes the two methods from the bloom filter fuzz test.

maflcko · 2020-04-16T23:20:22Z

re-ACK 69ffddc

jonatack · 2020-04-16T23:21:04Z

ACK 69ffddc, code review, compiled a fuzz build and started the bloom_filter fuzz test as a sanity check.

promag · 2020-04-17T08:52:45Z

ACK 69ffddc.

practicalswift · 2020-04-20T09:21:28Z

Post-merge ACK 69ffddc

@theStack

Thanks for removing CBloomFilter::reset()/clear().

If you want to look at adding tests for unused functions (or remove unused functions that are unlikely to be used in the near future) then this list might be helpful:

Unused: bitcoinconsensus_version
Unused: BlockFilter::BlockFilter(BlockFilterType, uint256 const&, std::vector<unsigned char>)
Unused: CBlock::ToString() const
Unused: Join(std::vector<std::string, std::allocator<std::string>> const&, std::string const&)::{lambda(std::string const&)#1}::_FUN(std::string const&)
Unused: PartiallySignedTransaction::IsNull() const
Unused: PSBTInput::IsNull() const
Unused: PSBTOutput::IsNull() const
Unused outside of tests: base_blob<160u>::SetHex(std::string const&)
Unused outside of tests: base_blob<160u>::ToString() const
Unused outside of tests: base_uint<256u>::base_uint(std::string const&)
Unused outside of tests: base_uint<256u>::ToString() const
Unused outside of tests: bitcoinconsensus_verify_script
Unused outside of tests: bitcoinconsensus_verify_script_with_amount
Unused outside of tests: BlockFilter::BlockFilter(BlockFilterType, CBlock const&, CBlockUndo const&)
Unused outside of tests: BlockFilter::ComputeHeader(uint256 const&) const
Unused outside of tests: CBloomFilter::CBloomFilter(unsigned int, double, unsigned int, unsigned char)
Unused outside of tests: CBloomFilter::insert(uint256 const&)
Unused outside of tests: CKey::Negate()
Unused outside of tests: CKey::SignCompact(uint256 const&, std::vector<unsigned char>&) const
Unused outside of tests: CPubKey::RecoverCompact(uint256 const&, std::vector<unsigned char> const&)
Unused outside of tests: DecodeBase58(std::string const&, std::vector<unsigned char>&, int)
Unused outside of tests: GCSFilter::MatchAny(std::unordered_set<std::vector<unsigned char>, ByteVectorHash, std::equal_to<std::vector<unsigned char>>, std::allocator<std::vector<unsigned char>>> const&) const
Unused outside of tests: GCSFilter::Match(std::vector<unsigned char> const&) const
Unused outside of tests: SignSignature(SigningProvider const&, CTransaction const&, CMutableTransaction&, unsigned int, int)

But as said: please note that at least some of these have been added recently and might be intentionally unused for the time being but has concrete plans for near future use in an open PR (GCSFilter is such an example IIRC). All of them should have tests though :)

Edit: List is not entirely correct. Example: CPubKey::RecoverCompact is in the list but is used also outside of tests. See this as a candidate list :)

theStack · 2020-04-20T15:55:14Z

@practicalswift: Thanks for your valuable input, this wanders directly onto my Bitcoin Core TODO list. For some functions, e.g. the constructors of CBloomFilter() and BlockFilter() it can be quite confusing to the reader that they are never actually used anywhere in bitcoind, only for test purposes. For those cases it would make sense to add a comment like "only for testing".

Summary: Co-authored-by: MarcoFalke <falke.marco@gmail.com> > The method CBloomFilter::reset() was introduced by commit d2d7ee0 in 2015, but was never ever used, as far as I could find. As discovered by MarcoFalke, the method clear() is also unused outside of unit tests and is hence also removed. This is a backport of Core [[bitcoin/bitcoin#18670 | PR18670]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8937

Backport bloom filter improvements Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7113 - bitcoin/bitcoin#7818 - Only the second commit (to resolve conflicts). - bitcoin/bitcoin#7934 - bitcoin/bitcoin#8655 - Partial backport to help resolve conflicts. - bitcoin/bitcoin#9060 - bitcoin/bitcoin#9223 - bitcoin/bitcoin#9644 - Partial backport to help resolve conflicts. - bitcoin/bitcoin#9916 - bitcoin/bitcoin#9750 - bitcoin/bitcoin#13176 - bitcoin/bitcoin#13948 - bitcoin/bitcoin#16073 - bitcoin/bitcoin#18670 - bitcoin/bitcoin#18806 - Reveals upstream's covert fix for CVE-2013-5700. - bitcoin/bitcoin#19968

theStack force-pushed the 20200416-refactor-remove-unused-bloom-filter-reset branch from cfbc5a5 to 9b95bdc Compare April 16, 2020 15:52

theStack changed the title ~~refactor: Remove unused method CBloomFilter::reset()~~ refactor: Remove unused methods CBloomFilter::reset()/clear() Apr 16, 2020

DrahtBot added Refactoring Tests labels Apr 16, 2020

refactor: Remove unused methods CBloomFilter::reset()/clear()

69ffddc

Co-authored-by: MarcoFalke <falke.marco@gmail.com>

theStack force-pushed the 20200416-refactor-remove-unused-bloom-filter-reset branch from 9b95bdc to 69ffddc Compare April 16, 2020 23:10

maflcko merged commit 4a71c46 into bitcoin:master Apr 17, 2020

practicalswift mentioned this pull request May 3, 2020

qt: function filterAcceptsRow seems to be unused #18860

Closed

theStack deleted the 20200416-refactor-remove-unused-bloom-filter-reset branch December 1, 2020 09:55

str4d mentioned this pull request Mar 5, 2021

Backport bloom filter improvements zcash/zcash#5026

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Remove unused methods CBloomFilter::reset()/clear() #18670

refactor: Remove unused methods CBloomFilter::reset()/clear() #18670

Uh oh!

theStack commented Apr 16, 2020 •

edited

Loading

Uh oh!

theStack commented Apr 16, 2020

Uh oh!

maflcko commented Apr 16, 2020

Uh oh!

theStack commented Apr 16, 2020

Uh oh!

maflcko commented Apr 16, 2020

Uh oh!

laanwj commented Apr 16, 2020

Uh oh!

jonatack commented Apr 16, 2020

Uh oh!

maflcko commented Apr 16, 2020

Uh oh!

theStack commented Apr 16, 2020

Uh oh!

maflcko commented Apr 16, 2020

Uh oh!

jonatack commented Apr 16, 2020

Uh oh!

promag commented Apr 17, 2020

Uh oh!

practicalswift commented Apr 20, 2020 •

edited

Loading

Uh oh!

theStack commented Apr 20, 2020

Uh oh!

Uh oh!

refactor: Remove unused methods CBloomFilter::reset()/clear() #18670

refactor: Remove unused methods CBloomFilter::reset()/clear() #18670

Uh oh!

Conversation

theStack commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theStack commented Apr 16, 2020

Uh oh!

maflcko commented Apr 16, 2020

Uh oh!

theStack commented Apr 16, 2020

Uh oh!

maflcko commented Apr 16, 2020

Uh oh!

laanwj commented Apr 16, 2020

Uh oh!

jonatack commented Apr 16, 2020

Uh oh!

maflcko commented Apr 16, 2020

Uh oh!

theStack commented Apr 16, 2020

Uh oh!

maflcko commented Apr 16, 2020

Uh oh!

jonatack commented Apr 16, 2020

Uh oh!

promag commented Apr 17, 2020

Uh oh!

practicalswift commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theStack commented Apr 20, 2020

Uh oh!

Uh oh!

theStack commented Apr 16, 2020 •

edited

Loading

practicalswift commented Apr 20, 2020 •

edited

Loading