fuzz: Check banman roundtrip #22322

proposed docs if you like

-        BanMan ban_man{banlist_file, nullptr, offset};
+        BanMan ban_man{banlist_file, /* client_interface */ nullptr, /* default_ban_time */ offset};
         while (--limit_max_ops >= 0 && fuzzed_data_provider.ConsumeBool()) {
             CallOneOf(
                 fuzzed_data_provider,
@@ -100,15 +100,15 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
             SetMockTime(ConsumeTime(fuzzed_data_provider));
             banmap_t banmap;
             ban_man.GetBanned(banmap);
-            BanMan ban_man_read{banlist_file, nullptr, offset};
+            BanMan ban_man_read{banlist_file, /* client_interface */ nullptr, /* default_ban_time */ offset};
             banmap_t banmap_read;
             ban_man_read.GetBanned(banmap_read);
             assert(banmap.size() == banmap_read.size());
             auto it1{banmap.begin()};
             auto it2{banmap_read.begin()};
             for (; it1 != banmap.end(); ++it1, ++it2) {
-                assert(it1->first == it2->first);
-                assert(it1->second.ToJson().write() == it2->second.ToJson().write());
+                assert(it1->first == it2->first); // CSubNet
+                assert(it1->second.ToJson().write() == it2->second.ToJson().write()); // CBanEntry
             }
         }
     }

Thanks, added comments

Unrelated to this PR, why is this called "offset"? It is actually "ban_time" - number of seconds to keep the ban entry after the misbehavior occurred.

There is some "offset" involved in BanMan::Ban(), but that is completely internal to BanMan.

Removed offset

The dumped data may end up containing more entries than banmap and banmap_read because SweepBanned() may leave them when called from DumpBanlist() but when called from GetBanned() after SetMockTime() it may remove "expired" entries.

It is ok since both banmap and banmap_read will contain the same, but if somebody ends up debugging this and looking at the file, then it may be confusing. Maybe swap lines 99 and 100.

This was done on purpose to check SweepBanned is called on read

Hmm, then maybe add one entry that will certainly expire? This will also make the intention more explicit:

diff --git i/src/test/fuzz/banman.cpp w/src/test/fuzz/banman.cpp
index 6aa95b484f..b3b10da084 100644
--- i/src/test/fuzz/banman.cpp
+++ w/src/test/fuzz/banman.cpp
@@ -93,16 +93,23 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
                 },
                 [&] {
                     ban_man.Discourage(ConsumeNetAddr(fuzzed_data_provider));
                 });
         }
         if (!force_read_and_write_to_err) {
+            const auto mock_time = ConsumeTime(fuzzed_data_provider);
+            SetMockTime(mock_time);
+            // Add one ban entry that will expire to check that on read expired entries are removed.
+            ban_man.Ban(ConsumeNetAddr(fuzzed_data_provider), mock_time + 5, true);
             ban_man.DumpBanlist();
-            SetMockTime(ConsumeTime(fuzzed_data_provider));
+            banmap_t banmap_unexpired;
+            ban_man.GetBanned(banmap_unexpired);
+            SetMockTime(mock_time + 10);
             banmap_t banmap;
             ban_man.GetBanned(banmap);
+            assert(banmap.size() < banmap_unexpired.size()); // At least one should have expired.
             BanMan ban_man_read{banlist_file, nullptr, offset};
             banmap_t banmap_read;
             ban_man_read.GetBanned(banmap_read);
             assert(banmap.size() == banmap_read.size());
             auto it1{banmap.begin()};
             auto it2{banmap_read.begin()};

This may brick if banmap_t is changed from std::map to std::unordered_map. It should be changed because the latter is faster and we don't need an order into this container.

Maybe add something like static_assert(std::is_base_of<std::map, banmap_t>::value) to ensure this gets adjusted if banmap_t is changed in the future?

Or a slower, but more robust variant: for each entry in banmap ensure that it can be found in banmap_read and that both maps have the same size.

             assert(banmap.size() == banmap_read.size());
-            auto it1{banmap.begin()};
-            auto it2{banmap_read.begin()};
-            for (; it1 != banmap.end(); ++it1, ++it2) {
-                assert(it1->first == it2->first);
-                assert(it1->second.ToJson().write() == it2->second.ToJson().write());
+            for (const auto& [subnet, ban_entry] : banmap) {
+                const auto it = banmap_read.find(subnet);
+                assert(it != banmap_read.end());
+                assert(it->second.ToJson().write() == ban_entry.ToJson().write());
             }

Or even better:

diff --git i/src/test/fuzz/banman.cpp w/src/test/fuzz/banman.cpp
index 6aa95b484f..15ccd9df3c 100644
--- i/src/test/fuzz/banman.cpp
+++ w/src/test/fuzz/banman.cpp
@@ -29,12 +29,18 @@ int64_t ConsumeBanTimeOffset(FuzzedDataProvider& fuzzed_data_provider) noexcept
 
 void initialize_banman()
 {
     static const auto testing_setup = MakeNoLogFileContext<>();
 }
 
+static bool operator==(const CBanEntry& lhs, const CBanEntry& rhs)
+{
+    return lhs.nVersion == rhs.nVersion && lhs.nCreateTime == rhs.nCreateTime &&
+           lhs.nBanUntil == rhs.nBanUntil;
+}
+
 FUZZ_TARGET_INIT(banman, initialize_banman)
 {
     // The complexity is O(N^2), where N is the input size, because each call
     // might call DumpBanlist (or other methods that are at least linear
     // complexity of the input size).
     int limit_max_ops{300};
@@ -100,18 +106,12 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
             SetMockTime(ConsumeTime(fuzzed_data_provider));
             banmap_t banmap;
             ban_man.GetBanned(banmap);
             BanMan ban_man_read{banlist_file, nullptr, offset};
             banmap_t banmap_read;
             ban_man_read.GetBanned(banmap_read);
-            assert(banmap.size() == banmap_read.size());
-            auto it1{banmap.begin()};
-            auto it2{banmap_read.begin()};
-            for (; it1 != banmap.end(); ++it1, ++it2) {
-                assert(it1->first == it2->first);
-                assert(it1->second.ToJson().write() == it2->second.ToJson().write());
-            }
+            assert(banmap_read == banmap);
         }
     }
     fs::remove(banlist_file.string() + ".dat");
     fs::remove(banlist_file.string() + ".json");
 }

Made it a compile failure to change the type without touching this file

This is now addressed in another way. Just out of curiosity - why not the assert(banmap_read == banmap); variant? It is shorter and works for both map and unordered_map.

The 3rd argument offset is only used by BanMan::Ban() which we never call on ban_man_read. So this can be

            BanMan ban_man_read{banlist_file, nullptr, 0};

and then the offset variable is not necessary.

Thanks, done

Errors in CI job (https://cirrus-ci.com/task/6055643191705600?logs=ci#L2769):

fuzz: test/fuzz/banman.cpp:112: void banman_fuzz_target(FuzzBufferType): Assertion `banmap == banmap_read' failed.

Is there a way to retrieve ./crash-0671aac15e619e99522e2119487eaa9cc97e5a34 from the CI machine or make the fuzzer print it in base64?

Latest master (d67330d11245b11fbdd5e2dd5343ee451186931e) has been changed to:

https://github.com/bitcoin/bitcoin/blob/d67330d11245b11fbdd5e2dd5343ee451186931e/src/test/fuzz/banman.cpp#L111-L113