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

The method CBloomFilter::reset() was introduced by commit d2d7ee0e863b286e1c9f9c54659d494fb0a7712d 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.

Hint for reviewers: there are seemingly reset() method calls on CBloomFilter instances, e.g.: https://github.com/bitcoin/bitcoin/blob/661e8df1b63b213d2d5b0d7cba0733869b508af9/src/net_processing.cpp#L3164 but those refer to std::unique_ptr::reset(), not to CBloomFilter::reset()!

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)

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

ACK 9b95bdc89f57bbd6681e66668357edccfba68cdf

ACK 9b95bdc89f57bbd6681e66668357edccfba68cdf

ACK 9b95bdc

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.

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

re-ACK 69ffddc83e0f3e265bf6cf7ae31489ae629fe6be

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

ACK 69ffddc83e0f3e265bf6cf7ae31489ae629fe6be.

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 :)

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