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

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();

ACK f5563da

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.

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

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

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

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.

reACK 32953718a3fb8e2f298b99e1a2ee72af1ea5be8c

Changes since my last review:

• Addressed @jonatack's suggestions.

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

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.

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.

Code Review re-ACK 07fdd10

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

re-ACK 07fdd10

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