fuzz: set `nMaxOutboundLimit` in connman target #29172

brunoerg commented at 8:15 PM on January 3, 2024: contributor

Setting nMaxOutboundLimit (-maxuploadtarget) will make fuzz to reach more coverage in connman target. This value is used in GetMaxOutboundTimeLeftInCycle, OutboundTargetReached and GetOutboundTargetBytesLeft.

DrahtBot commented at 8:15 PM on January 3, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	dergoegge, jonatack
Stale ACK	maflcko

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:

#28584 (Fuzz: extend CConnman tests by vasild)

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 Jan 3, 2024

brunoerg commented at 8:41 PM on January 3, 2024: contributor

CI failure is unrelated to this PR.

maflcko commented at 9:06 AM on January 4, 2024: member

lgtm ACK 46d7113ec1389eb78d7cd44425ecc22dda9b67bf

brunoerg commented at 7:53 PM on January 4, 2024: contributor

friendly ping: @dergoegge

in src/test/fuzz/connman.cpp:43 in 46d7113ec1 outdated

  37 | @@ -38,6 +38,10 @@ FUZZ_TARGET(connman, .init = initialize_connman)
  38 |                       *g_setup->m_node.netgroupman,
  39 |                       Params(),
  40 |                       fuzzed_data_provider.ConsumeBool()};
  41 | +
  42 | +    const uint64_t max_outbound_limit{fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral<uint64_t>()};
  43 | +    connman.SetMaxOutboundLimit(max_outbound_limit);

dergoegge commented at 11:24 AM on January 5, 2024:

Maybe call CConnan::Init instead of introducing this new test only setter?

brunoerg commented at 12:49 PM on January 5, 2024:

No problem on calling CConnman::Init instead. With SetMaxOutboundLimit we ensure we're only messing with nMaxOutboundLimit and perhaps we avoid other locks (?).

dergoegge commented at 1:53 PM on January 5, 2024:

Init is called in the connman constructor: https://github.com/bitcoin/bitcoin/blob/c80f57ba575af96890f185765a53a62ef58ef2c8/src/net.cpp#L3117-L3118

Calling it again and only changing nMaxOutboundLimit achieves the same as you have here with less new code.

brunoerg commented at 3:45 PM on January 5, 2024:

I agree, just addressed it.

in src/test/fuzz/connman.cpp:42 in 46d7113ec1 outdated

  37 | @@ -38,6 +38,10 @@ FUZZ_TARGET(connman, .init = initialize_connman)
  38 |                       *g_setup->m_node.netgroupman,
  39 |                       Params(),
  40 |                       fuzzed_data_provider.ConsumeBool()};
  41 | +
  42 | +    const uint64_t max_outbound_limit{fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral<uint64_t>()};

dergoegge commented at 11:26 AM on January 5, 2024:

    const uint64_t max_outbound_limit{fuzzed_data_provider.ConsumeIntegral<uint64_t>()};

this will result in 0 some of the time.

brunoerg commented at 12:41 PM on January 5, 2024:

The ConsumeBool is to ensure that the default value will be used in most cases.

dergoegge commented at 1:50 PM on January 5, 2024:

The ConsumeBool is to ensure that the default value will be used in most cases.

How does it ensure that and why is that useful?

brunoerg commented at 3:51 PM on January 5, 2024:

The idea was to have more runs with the default value, but just tested here and in practice it didn't have so much effect. Just changed it to your suggestion.

fuzz: set `nMaxOutboundLimit` in connman target e5b9ee0221

brunoerg force-pushed on Jan 5, 2024

brunoerg commented at 3:52 PM on January 5, 2024: contributor

Thanks, @dergoegge for your review. Force-pushed addressing: #29172 (review) and #29172 (review).

dergoegge approved

dergoegge commented at 5:24 PM on January 5, 2024: member

utACK e5b9ee0221ec8aa238fb5720fcd6faa01b09fe46

DrahtBot requested review from maflcko on Jan 5, 2024

jonatack commented at 10:19 PM on January 8, 2024: member

ACK e5b9ee0221ec8aa238fb5720fcd6faa01b09fe46

fanquake merged this on Jan 9, 2024

fanquake closed this on Jan 9, 2024

achow101 commented at 6:09 PM on January 9, 2024: member

New compiler warning, using gcc 13.2.1

test/fuzz/connman.cpp: In function ‘void connman_fuzz_target(FuzzBufferType)’:
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::vSeedNodes’ [-Werror=missing-field-initializers]
   43 |     connman.Init({ .nMaxOutboundLimit = max_outbound_limit });
      |     ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::vWhitelistedRange’ [-Werror=missing-field-initializers]
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::vWhiteBinds’ [-Werror=missing-field-initializers]
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::vBinds’ [-Werror=missing-field-initializers]
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::onion_binds’ [-Werror=missing-field-initializers]
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::bind_on_any’ [-Werror=missing-field-initializers]
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::vSeedNodes’ [-Werror=missing-field-initializers]
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::vWhitelistedRange’ [-Werror=missing-field-initializers]
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::vWhiteBinds’ [-Werror=missing-field-initializers]
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::vBinds’ [-Werror=missing-field-initializers]
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::onion_binds’ [-Werror=missing-field-initializers]
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::bind_on_any’ [-Werror=missing-field-initializers]
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::m_specified_outgoing’ [-Werror=missing-field-initializers]
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::m_added_nodes’ [-Werror=missing-field-initializers]
test/fuzz/connman.cpp:43:17: error: missing initializer for member ‘CConnman::Options::m_i2p_accept_incoming’ [-Werror=missing-field-initializers]

brunoerg commented at 6:13 PM on January 9, 2024: contributor

New compiler warning, using gcc 13.2.1

I'm checking it atm.

achow101 referenced this in commit 507dbe4ca2 on Jan 10, 2024

PastaPastaPasta referenced this in commit 303f75cf9e on Oct 24, 2024

PastaPastaPasta referenced this in commit fec901a36d on Oct 24, 2024

PastaPastaPasta referenced this in commit 572298859d on Oct 24, 2024

PastaPastaPasta referenced this in commit 4037ad6d75 on Oct 24, 2024

PastaPastaPasta referenced this in commit 4cdd1a8a5d on Oct 24, 2024

PastaPastaPasta referenced this in commit b091329599 on Oct 24, 2024

PastaPastaPasta referenced this in commit a67319351a on Oct 24, 2024

bitcoin locked this on Jan 8, 2025