fuzz: check that ser+unser produces the same AddrMan

[vasild]

fuzz: check that ser+unser produces the same AddrMan

[vasild]

8f3ce9d...ef2edb2: extend the addrman comparison to check also vvNew and vvTried.

[jnewbery]

Thanks @vasild. I'll aim to do a quick review and add to my PR in the next couple of days.

[practicalswift]

Concept ACK: very nice!

Warm welcome as a PR contributor to the src/test/fuzz/ tree @vasild! :)

[vasild]

Running this for a few minutes and it keeps testing ser+unser of an addrman with 0 entries :( The fuzzed data provider runs out of data too soon.

In the few cases where num_sources is some reasonable number (not 1 because no more fuzz data), then ConsumeNetAddr() generates an unroutable address which is skipped by CAddrMan::Add_().

[maflcko]

Did you try -use_value_profile=1, which helps to make "smaller steps"?

[maflcko]

Alternatives could be:

• Modify ConsumeNetAddr (similar to ConsuemWeakEnum) to return valid addresses more likely
• break out of the loop or return early on invalid addresses, as they are skipped anyway
• ... something else

[maflcko]

Is there a reason to not create a new testcase for this?

[vasild]

I appended to the existent test so that addrman is even in a more fancy state, left from the CallOneOf() tests above this. But I see how they are consuming precious fuzz bytes...

[vasild]

706c616...3201be4: tweak in an attempt to fill addrman with more addresses

@MarcoFalke right, generating CNetAddr with bigger likelihood of being routable and quitting as soon as the fuzz data is exhausted helps. However running the current version (3201be4) for a few minutes tests ser+unser, zero times!

Running with -max_len=1048576 -len_control=0 -prefer_small=0 produces some meaningful results:

added 1 times: 59.167.167.167 (routable)
added 6 times: 147.147.147.147 (routable)
added 1 times: 167.135.167.167 (routable)
added 10 times: 167.167.167.167 (routable)
added 1 times: a75b:a7a7:a7a7:a7a7:a7a7:a7a7:a7a7:a7a7 (routable)
added 2 times: a7a7:a7a7:a7a7:a7a7:a7a7:a7a7:a7a7:a7a7 (routable)
added 1 times: sojzhe4tsojzhe4t.onion (routable)
added 1 times: u6t2pj5hu6t2pj5h.onion (routable)
added 1 times: sojzhe4tsojzhe4tsojzhe4tsojzhe4tsojzhe4tsojzhe4tsojyeayd.onion (routable)
added 1 times: u6t2pj5hu6t2pj5hu6tzhe4tsojzhe4tsojzhe4tsojzhe4tsoj3kcad.onion (routable)
fuzz bytes at start: 492, num sources: 17, addresses: 25, addrman size: 10

added 1 times: 0.0.0.0 (not routable)
added 1 times: 0.1.11.11 (not routable)
added 1 times: 34.34.34.34 (routable)
added 1 times: 255.0.0.0 (routable)
added 1 times: aaaaaaaaaaaabwd63dmnrwgy3dmnrwgye3mnrwgy3dmcmirceira2oid.onion (routable)
added 1 times: bmfsycyl3dmcmjzhe7mnqaaaaeanqaaaaaaaaaaagqaaaaaaaaapfeyd.onion (routable)
added 1 times: eircekjcaaaaaaelaefqwcylfqfqwcylbmfsekjcaaxaaaaaaef2iwid.onion (routable)
fuzz bytes at start: 202, num sources: 35, addresses: 7, addrman size: 5

but still far away from filling addrman with 1000s of addresses.

[patch] debug stats print

diff --git i/src/test/fuzz/addrman.cpp w/src/test/fuzz/addrman.cpp
index 01bd2fef1..f307dc07b 100644
--- i/src/test/fuzz/addrman.cpp
+++ w/src/test/fuzz/addrman.cpp
@@ -210,14 +210,17 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman)
     // Check that serialize followed by unserialize produces the same addrman.
 
     if (fuzzed_data_provider.remaining_bytes() < 100) {
         return;
     }
 
+    const auto debug_fuzz_bytes_at_start = fuzzed_data_provider.remaining_bytes();
+
     data_stream.clear();
 
+    std::map<CNetAddr, size_t> debug_added_addresses;
     const size_t num_sources = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, 100);
     for (size_t i = 0; i < num_sources; ++i) {
         if (fuzzed_data_provider.remaining_bytes() < ADDR_TORV3_SIZE) {
             break;
         }
         const auto source = ConsumeLikelyRoutableNetAddr(fuzzed_data_provider);
@@ -231,12 +234,13 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman)
             const auto netaddr = ConsumeLikelyRoutableNetAddr(fuzzed_data_provider);
             const auto port = fuzzed_data_provider.ConsumeIntegralInRange<uint16_t>(
                 1, std::numeric_limits<uint16_t>::max());
             const auto service = CService{netaddr, port};
             const auto flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
             addr = CAddress{service, flags};
+            ++debug_added_addresses[netaddr];
         }
 
         const auto time_penalty = fuzzed_data_provider.ConsumeIntegralInRange<int64_t>(0, 100000000);
         addr_man.Add(addresses, source, time_penalty);
 
         const auto n = fuzzed_data_provider.ConsumeIntegralInRange<uint8_t>(0, 3);
@@ -244,12 +248,22 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman)
             for (size_t j = 0; j < addresses.size(); j += n) {
                 addr_man.Good(addresses[j], false, ConsumeTime(fuzzed_data_provider));
             }
         } // Or leave all in the "new" table if n == 0.
     }
 
+    size_t debug_num_added_addresses = 0;
+    for (auto& [netaddr, count] : debug_added_addresses) {
+        std::cout << "added " << count << " times: " << netaddr.ToString() << " ("
+                  << (netaddr.IsRoutable() ? "" : "not ") << "routable)" << std::endl;
+        debug_num_added_addresses += count;
+    }
+    std::cout << "fuzz bytes at start: " << debug_fuzz_bytes_at_start
+              << ", num sources: " << num_sources << ", addresses: " << debug_num_added_addresses
+              << ", addrman size: " << addr_man.size() << std::endl;
+
     data_stream << addr_man;
     CAddrManDeterministic addr_man_unserialized;
     addr_man_unserialized.m_asmap = addr_man.m_asmap;
     data_stream >> addr_man_unserialized;
     assert(addr_man == addr_man_unserialized);
 }

[vasild]

3201be4...1ff6ceb:

• Fill addrman from a prng (CAddrMan::insecure_rand) which is seeded with fuzz bytes, instead of directly generating addresses from fuzz bytes. This way we have unlimited source of "random" bytes.

• Isolate the new test to a standalone addrman_serdeser.

• Add good/tried addresses directly to vvTried instead of first adding them to vvNew and then moving them. This is not so elegant in the test, compared to just calling Good_() or MakeTried(), but is significantly faster (maybe 100x).

The timing is:

fill with 10038 addresses: 2.62s
serialize: 0.410s
unserialize: 1.29s
compare: 2.56s

[jnewbery]

A couple of questions:

1. Is there any way we can make this test use the addrman's public interface? I worry that reaching directly into addrman's private data structures has a couple of disadvantages:

• it makes the test very implementation-specific (and hence needs maintaining and updating if the internal implementation of addrman changes)
• the test may not reflect realistic usage patters of addrman and the internal invariants may inadvertently be violated by the test updating the private data

2. Does this ever test the same address being added by multiple sources? Losing the additional new bucket entries if the address has been received from multiple sources is the bug that's fixed in part (1) of addrman: Fix new table bucketing during unserialization bitcoin/bitcoin#20557.

[vasild]

A couple of questions:

1. Is there any way we can make this test use the addrman's public interface?

Yes. That would require adding all addresses as new (by Add()) and then marking some, e.g. 30% of them as good (via Good() or MakeTried()). This "marking as good" is very slow - takes a few minutes for addrman with ~10k addresses. I think this is unacceptable. Another option would be to scale down the test to have addrman with a few 100s of addresses.

I worry that reaching directly into addrman's private data structures has a couple of disadvantages:

* it makes the test very implementation-specific (and hence needs maintaining and updating if the internal implementation of addrman changes)

Indeed, that is a concern. Notice however that this is the only code that uses non-public interface (adding directly to vvTried) when filling the addrman:

+                    const CAddrInfo dummy{addr, source};
+                    const int bucket = dummy.GetTriedBucket(nKey, m_asmap);
+                    const int bucket_pos = dummy.GetBucketPosition(nKey, false, bucket);
+                    if (vvTried[bucket][bucket_pos] == -1) {
+                        int id;
+                        CAddrInfo* addr_info = Create(addr, source, &id);
+                        vvTried[bucket][bucket_pos] = id;
+                        addr_info->fInTried = true;
+                        ++nTried;
+                    }

Notice too that CAddrManDeterministic::operator==() also reads private members directly in order to compare the addrmans.

* the test may not reflect realistic usage patters of addrman and the internal invariants may inadvertently be violated by the test updating the private data

Hmm, I don't think that should be the case now - the addrman after Fill() would be in a state as if those addresses were added/made good via the public interface.

1. Does this ever test the same address being added by multiple sources? ...

Yes, it could test that as the sources and addresses are generated randomly. But maybe the chance of this happening is low. Maybe it is worth adding code that deliberately makes sure some addresses are added from more than one source.

I think that there is some threshold at which if a test becomes too big, slow or unmaintainable which would increase the likelihood of bugs in the test itself, then beyond this threshold the test will have negative net impact on the project.

I am not sure where is this test wrt that threshold, but it is close :) I am ~0 to ditch this test.

[jnewbery]

@vasild are you planning to keep working on this and opening a PR against the main repo? If not, I'm happy to work on it at low priority.

[vasild]

Here it is: bitcoin#21129.