fuzz: addrman, add coverage for `network` field in `Select()`, `Size()` and `GetAddr()` #27549

brunoerg commented at 3:05 PM on May 1, 2023: contributor

This PR adds fuzz coverage for network field in Select(), Size() and GetAddr(), there was only call to them without passing a network. https://marcofalke.github.io/b-c-cov/fuzz.coverage/src/addrman.cpp.gcov.html

DrahtBot commented at 3:05 PM on May 1, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	amitiuttarwar, mzumsande, achow101
Concept ACK	pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25284 (net: Use serialization parameters for CAddress serialization 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.

DrahtBot added the label Tests on May 1, 2023

in src/test/fuzz/addrman.cpp:306 in 0d4fe13aa7 outdated

 302 | @@ -303,6 +303,8 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman)
 303 |          /*max_addresses=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
 304 |          /*max_pct=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096),
 305 |          /*network=*/std::nullopt);
 306 | +    const std::optional<Network> network{fuzzed_data_provider.PickValueInArray({Network::NET_IPV4, Network::NET_IPV6, Network::NET_INTERNAL, Network::NET_ONION, Network::NET_I2P})};

maflcko commented at 8:40 AM on May 2, 2023:

Any reason to not use ALL_NETWORKS?

brunoerg commented at 1:42 PM on May 2, 2023:

Well, since it already imports test/fuzz/util/net.h there is no reason, gonna change it!

mzumsande commented at 11:49 AM on May 2, 2023: contributor

Concept ACK. I noticed this too, so a recently added commit from #27213, https://github.com/bitcoin/bitcoin/pull/27213/commits/8f91e5898b3571e0802f2197981c54dda9ee71fa, also includes this - plus coverage for the other network-specific functions (GetAddr(), Size()). I think it's fine to do it in a separate PR too, but while at it, it would be good to also add coverage for these other functions.

fuzz: addrman, add coverage for `network` field in `Select()`, `Size()` and `GetAddr()`

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

35a2175ad8

brunoerg force-pushed on May 2, 2023

brunoerg renamed this:
~~fuzz: addrman, add coverage for `network` field in `Select()`~~
fuzz: addrman, add coverage for `network` field in `Select()`, `Size()` and `GetAddr()`
on May 2, 2023

brunoerg commented at 4:02 PM on May 2, 2023: contributor

Force-pushed addressing #27549#pullrequestreview-1408940288

Added coverage for Size() and GetAddr(), since I got code from https://github.com/bitcoin/bitcoin/commit/8f91e5898b3571e0802f2197981c54dda9ee71fa, I added @mzumsande and @amitiuttarwar as co-authors. PR description has been updated.

amitiuttarwar commented at 7:55 PM on May 24, 2023: contributor

it looks like this PR provides the same coverage as 8f91e5898b3571e0802f2197981c54dda9ee71fa in #27213. I'll remove the commit there so 27213 can focus on discussing the changes to node behavior

amitiuttarwar commented at 7:56 PM on May 24, 2023: contributor

for the record, ACK 35a2175ad8bec92b409ae2202c124e39b2f3f838 - only small changes from the version (previously) proposed in 27213

maflcko requested review from mzumsande on May 25, 2023

maflcko requested review from dergoegge on May 25, 2023

in src/test/fuzz/addrman.cpp:314 in 35a2175ad8

 312 | +        network);
 313 | +    (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), network);
 314 | +    std::optional<bool> in_new;
 315 | +    if (fuzzed_data_provider.ConsumeBool()) {
 316 | +        in_new = fuzzed_data_provider.ConsumeBool();
 317 | +    }

pablomartin4btc commented at 12:06 PM on June 25, 2023:

I was going to propose:

    bool in_new{fuzzed_data_provider.ConsumeBool()};

But then I checked also the definition of theAddrMan::Size()function consuming in_new as and argument and I see in the comments that it would interpret the 3 states:

/**
 * Return size information about addrman.
 *
 * [@param](/bitcoin-bitcoin/contributor/param/)[in] net              Select addresses only from specified network (nullopt = all)
 * [@param](/bitcoin-bitcoin/contributor/param/)[in] in_new           Select addresses only from one table (true = new, false = tried, nullopt = both)
 * [@return](/bitcoin-bitcoin/contributor/return/)                     Number of unique addresses that match specified options.
 */

edit: just thinking, we never use the false alternative here (tried table), perhaps this is not an issue.

in src/test/fuzz/addrman.cpp:305 in 35a2175ad8

 298 | @@ -299,12 +299,20 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman)
 299 |              });
 300 |      }
 301 |      const AddrMan& const_addr_man{addr_man};
 302 | +    std::optional<Network> network;
 303 | +    if (fuzzed_data_provider.ConsumeBool()) {
 304 | +        network = fuzzed_data_provider.PickValueInArray(ALL_NETWORKS);
 305 | +    }

pablomartin4btc commented at 12:17 PM on June 25, 2023:

Do we really need this?

Next line calling AddrMan::GetAddr() function passes std::nullopt as the network argument (meaning ALL_NETWORKS), so as AddrMan::Size() also interprets the same for the network argument and since even ConsumesBool() when returns false it would be std::nullopt (meaning ALL_NETWORKS), why to bother with this, just pass std::nullopt below when you call AddrMan::Size(), unless I'm missing something here.

mzumsande commented at 8:05 PM on June 26, 2023:

Next line calling AddrMan::GetAddr() function passes std::nullopt as the network argument

no, it now passes network as the network argument, which, depending on the fuzzing seed, could be std::nullopt or any network.

The goal of this PR is to give the fuzzer the possibility to reach network-specific code paths. If there was a bug within these, it wouldn't be able to find it before, because they just weren't invoked.

in src/test/fuzz/addrman.cpp:315 in 35a2175ad8

 313 | +    (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), network);
 314 | +    std::optional<bool> in_new;
 315 | +    if (fuzzed_data_provider.ConsumeBool()) {
 316 | +        in_new = fuzzed_data_provider.ConsumeBool();
 317 | +    }
 318 | +    (void)const_addr_man.Size(network, in_new);

pablomartin4btc commented at 12:19 PM on June 25, 2023:

As I explained my reasoning of this above in my first comment on the initialisation of the network variable:

    (void)const_addr_man.Size(/*network=*/std::nullopt, in_new);

pablomartin4btc commented at 1:28 PM on June 25, 2023: member

Tested ACK: FUZZ=addrman ./src/test/fuzz/fuzz

Tried also with a few seeds added last month by @Xekyo: e.g.: FUZZ=addrman ./src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/addrman/1b02be22a08f8e28c192cb1c86f074158d18a691

mzumsande commented at 8:10 PM on June 26, 2023: contributor

Code Review ACK 35a2175ad8bec92b409ae2202c124e39b2f3f838, haven't tested this yet, but I will let the fuzzer run for a while now.

achow101 commented at 11:06 PM on July 13, 2023: member

ACK 35a2175ad8bec92b409ae2202c124e39b2f3f838

achow101 merged this on Jul 13, 2023

achow101 closed this on Jul 13, 2023

sidhujag referenced this in commit cfae9e3c2d on Jul 15, 2023

sidhujag referenced this in commit 2a3c49bf42 on Jul 15, 2023

bitcoin locked this on Jul 12, 2024