index: Remove unused BlockFilter::Unserialize() #25637

BlockFilter::Unserialize is only used in the test code. Moving it to the tests allows for the removal of the optional sanity check in the GCSFilter and BlockFilter constructor, resulting in simpler code.

This is a follow up to #24832.

ACK

Some nits if you retouch:

• Not sure all the scope brackets in the tests are useful.
• Prefere prefix iterators (faster due to avoiding a copy), see our developer notes
• Avoid unneeded temporaries in loop
• s/insert/emplace/ where it makes sense
• named params and constify where helpful/clearer

<details><summary>suggestions</summary><p>

diff --git a/src/test/blockfilter_tests.cpp b/src/test/blockfilter_tests.cpp
index a17a71bf9f..a20b49e0a7 100644
--- a/src/test/blockfilter_tests.cpp
+++ b/src/test/blockfilter_tests.cpp
@@ -62,7 +62,7 @@ static BlockFilter UnserializeBlockFilter(Stream& s) {
       >> block_hash
       >> encoded_filter;
 
-    BlockFilterType filter_type = static_cast<BlockFilterType>(filter_type_uint8);
+    const BlockFilterType filter_type = static_cast<BlockFilterType>(filter_type_uint8);
     BlockFilter block_filter(filter_type, block_hash, std::move(encoded_filter));
     return block_filter;
 }
diff --git a/src/test/fuzz/blockfilter.cpp b/src/test/fuzz/blockfilter.cpp
index db3788d46f..2a1d81d970 100644
--- a/src/test/fuzz/blockfilter.cpp
+++ b/src/test/fuzz/blockfilter.cpp
@@ -22,23 +22,17 @@ FUZZ_TARGET(blockfilter)
 
     GCSFilter::ElementSet elements;
     size_t num_elements = fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(1, 100000);
-    for (size_t i=0; i < num_elements; i++) {
-        std::vector<unsigned char> element = ConsumeRandomLengthIntegralVector<unsigned char>(fuzzed_data_provider, 32);
-        elements.insert(element);
+    for (size_t i=0; i < num_elements; ++i) {
+        elements.emplace(ConsumeRandomLengthIntegralVector<unsigned char>(fuzzed_data_provider, /*max_vector_size=*/32));
char>(fuzzed_data_provider, 32));
     }
 
     GCSFilter filter({u256->GetUint64(0), u256->GetUint64(1), BASIC_FILTER_P, BASIC_FILTER_M}, elements);
-    BlockFilter block_filter(BlockFilterType::BASIC, u256.value(), filter.GetEncoded());
-    {
-        (void)block_filter.ComputeHeader(ConsumeUInt256(fuzzed_data_provider));
-        (void)block_filter.GetBlockHash();
-        (void)block_filter.GetEncodedFilter();
-        (void)block_filter.GetHash();
-    }
-    {
-        const BlockFilterType block_filter_type = block_filter.GetFilterType();
-        (void)BlockFilterTypeName(block_filter_type);
-    }
+    const BlockFilter block_filter(BlockFilterType::BASIC, u256.value(), filter.GetEncoded());
+    (void)block_filter.ComputeHeader(ConsumeUInt256(fuzzed_data_provider));
+    (void)block_filter.GetBlockHash();
+    (void)block_filter.GetEncodedFilter();
+    (void)block_filter.GetHash();
+    (void)BlockFilterTypeName(block_filter.GetFilterType());
     {
         const GCSFilter gcs_filter = block_filter.GetFilter();
         (void)gcs_filter.GetN();

</p></details>

ACK f5563da6836433a08438893c77bd87028e4b026c

Since it is my first time reviewing a PR concerning filters and streams, I want to detail my understanding of this PR before proceeding with the review. If my account is erroneous, please do correct me. Thank you.

<details> <summary> <b>Notes:</b> </summary>

• Normally, the filters are initialized manually and are not read from a stream. So we can be sure that the encoded_filter always contains the correct number of bits.

Eg.

GCSFilter filter({0, 0, BASIC_FILTER_P, BASIC_FILTER_M}, elements);

• And when the encoded_filter is read from a data stream in a function, a preliminary check is done before passing it through the BlockFilter function.

Eg.

filein >> block_hash >> encoded_filter;
uint256 result;
CHash256().Write(encoded_filter).Finalize(result);
if (result != hash) return error("Checksum mismatch in filter decode.");

• The only function which allows getting the encoded_filter from the data stream and passing it through the GCSFilter() function without any preliminary checks is the Unserialize function.
• Since this function is used only in the testing, it could be moved to the test file, which removes the need for checking encoded_filter correctness in the BlockFilter/GCSFilter functions.
• In the test file, the correctness of encoded_filter is checked by checking if the encoded_filter remains unchanged after moving block_filter in and out of the stream.

stream << block_filter;
stream >> block_filter2;
block_filter2 = UnserializeBlockFilter(stream);
...
BOOST_CHECK(block_filter.GetEncodedFilter() == block_filter2.GetEncodedFilter()); 

</details>

Review:

• I agree with moving the Unserialize function to the test file as it simplifies the code's logic while making it more straightforward.
• The code changes are clean and logical.
• And successful passing of CI shows that the fuzz testing was successful on the PR.
• I would like to suggest addressing @JonAtack suggestions before merging this PR.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26158 (bench: add "priority level" to the benchmark framework by furszy)
• #25296 (Add DataStream without ser-type and ser-version and use it where possible by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

• Not sure all the scope brackets in the tests are useful.
• Prefere prefix iterators (faster due to avoiding a copy), see our developer notes
• Avoid unneeded temporaries in loop
• s/insert/emplace/ where it makes sense
• named params and constify where helpful/clearer

Pushed new code with the suggested changes stated here.

reACK 32953718a3fb8e2f298b99e1a2ee72af1ea5be8c

Changes since my last review:

• Addressed @jonatack's suggestions.

I verified that the update adequately addresses all the suggestions in this comment.

cc @mzumsande @ryanofsky @stickies-v @theStack given you all reviewed #24832.

ACK 32953718a3fb8e2f298b99e1a2ee72af1ea5be8c edit: modulo removing the GCSFilterDecodeSkipCheck() benchmark as pointed out below

Approach ACK 32953718a , with the caveat that I don't understand the fuzzing code very well so count that as unreviewed from my end (even though I left a small nit)

I think GCSFilterDecodeSkipCheck() should be removed before ACK. Besides that, left 2 style comments but none blocking.

Summary: #24832 introduced a new file hash-based check to ensure filter integrity. This hash check (in BlockFilterIndex::ReadFilterFromDisk) can not be used when filters are unserialized from a stream because the hash is unknown, but that unserialization only happened in the test suite. As such, this PR moving unserialization logic to /src/test/ and simplifying the non-test code removing the skip_decode_check parameter and path makes sense.

Code-review ACK 32953718a3fb8e2f298b99e1a2ee72af1ea5be8c (modulo potential benchmark/fuzz test improvements)

Left a nit regarding the fuzz test below. I also agree that the benchmark GCSFilterDecodeSkipCheck can be removed as it's identical to GCSFilterDecode now, as suggested by stickies-v (good catch!).

Just commenting on this PR since I got a review request above. I am -0 on this change. I don't think it's an improvement, but would not object if others want to go ahead with it.

I think this is worse for code organization and debugability. I think serialize and deserialize methods should live next to each other in the code so they can don't get out of sync and stay available for understanding and debugging. If there was a large amount of code that was only called by tests and not used by the application, I'd agree it should be moved or removed. But this is a small amount and IMO this change just moves it someplace harder to find.

I see this similarly (as expressed here), but I think removing the skip_decode_check logic is orthogonal to this and would definitely be an improvement.

code review ACK 1204e3d4d

Verified that since https://github.com/bitcoin/bitcoin/commit/32953718a3fb8e2f298b99e1a2ee72af1ea5be8c the only changes made are incorporating (all) the suggestions made by myself and @theStack

I think serialize and deserialize methods should live next to each other in the code so they can don't get out of sync and stay available for understanding and debugging.

I'm not sufficiently familiar with how (de)serialization is organized across the codebase, so I'll refrain from going beyond a code review ACK, even though otherwise I think this PR is in good shape.

re-ACK 1204e3d4d35a1adf7e2160f29b604bb1ba1ac899

While I agree that to each serialization routine there should also be a deserialization counterpart and it wouldn't be a good idea to remove it completely, I think just having it in the unit tests is fine. If serialization and deserialization went out of sync, the serialization/deserialization roundtrip test would likely detect this, no?

ACK 1204e3d4d35a1adf7e2160f29b604bb1ba1ac899

Some nit comments (feel free to ignore), most of which would be pre-empted by running

git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v

to apply clang formatting to the changes; see contrib/devtools/clang-format-diff.py.

ACK 1204e3d

Some nit comments (feel free to ignore), most of which would be pre-empted by running

git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v

to apply clang formatting to the changes; see contrib/devtools/clang-format-diff.py.

Addressed.

Looks like ae7ae36d311a869b3bda41 added a new check to the Clang-tidy CI (unrelated to changes in the last push) that is red now:

/src/test/fuzz/blockfilter.cpp:34:25: error: the const qualified variable 'gcs_filter'
is copy-constructed from a const reference; consider making it a const reference
[performance-unnecessary-copy-initialization,-warnings-as-errors]
        const GCSFilter gcs_filter = block_filter.GetFilter();
                        ^
                       &

(ACK 9b87854 per git diff 1204e3d 9b87854 otherwise)

Code Review re-ACK 07fdd10

Verified that since 1204e3d changes are limited to clang formatting and declaring gcs_filter as a reference variable.

Diff-only review re-ACK 07fdd10ca1d0bd2bc1d2756f1bc2c069ae97c45a per git diff 1204e3d 07fdd10

cc @theStack for re-ACK?

re-ACK 07fdd10ca1d0bd2bc1d2756f1bc2c069ae97c45a

(verified via git range-diff 1204e3d4...07fdd10c that changes since my last ACK are trivial refactors)

@furszy My understanding is that you are working on implementing something that requires block filter download, which would require having this Unserialize method. If that's the case, then I don't think it really makes sense to merge this PR now as it would mean that already existing/planned future work will just end up reverting this PR.

Yeah, the new feature requires to unserialize block filters from the wire. Would be good to not remove this method, so I don't have to re-introduce it later on.

Thanks for the ping @achow101.

Talked to andrew and fursy and am convinced that the PR would just have to be reverted in a later commit. Closing.