fuzz: add mempool_utils.cpp #26250

fanquake commented at 2:49 PM on October 4, 2022: member

Moving the heavy (Boost) mempool code out of fuzz/util.h. Means that (for ex) a crypto_common fuzz unit doesn't need to care about seeing endless Boost headers. This results in a ~10% speedup (for me) when compiling the fuzz tests. Your results may vary.

fanquake added the label Tests on Oct 4, 2022

in src/Makefile.test_fuzz.include:20 in 53366df01e outdated

  16 | @@ -17,5 +17,6 @@ libtest_fuzz_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BOOST_CPPFLAGS)
  17 |  libtest_fuzz_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
  18 |  libtest_fuzz_a_SOURCES = \
  19 |    test/fuzz/fuzz.cpp \
  20 | +  test/fuzz/mempool_utils.cpp \

maflcko commented at 3:06 PM on October 4, 2022:

I wonder if we can nest more folders. Maybe src/test/fuzz/util/mempool.cpp to differentiate utils from tests?

fanquake commented at 4:53 PM on October 4, 2022:

Added some nesting.

in src/test/fuzz/mempool_utils.cpp:20 in 53366df01e outdated

  15 | +{
  16 | +    // Avoid:
  17 | +    // policy/feerate.cpp:28:34: runtime error: signed integer overflow: 34873208148477500 * 1000 cannot be represented in type 'long'
  18 | +    //
  19 | +    // Reproduce using CFeeRate(348732081484775, 10).GetFeePerK()
  20 | +    const CAmount fee = std::min<CAmount>(ConsumeMoney(fuzzed_data_provider), std::numeric_limits<CAmount>::max() / static_cast<CAmount>(100000));

maflcko commented at 3:08 PM on October 4, 2022:

unrelated:

    const CAmount fee{ConsumeMoney(fuzzed_data_provider, /*max=*/std::numeric_limits<CAmount>::max() / CAmount{100'000})};

It should be possible to just pass the max. (Can do in a separate commit, if you want)

fanquake commented at 4:53 PM on October 4, 2022:

Added another commit for this.

maflcko commented at 3:08 PM on October 4, 2022: member

lgtm

fanquake force-pushed on Oct 4, 2022

fuzz: add util/mempool/h.cpp

Moving the mempool code (Boost) out of util.h, results in a ~10% speedup
(for me) when compiling the fuzz tests.

eb15569280

fuzz: pass max fee into ConsumeTxMemPoolEntry 8a6b6dfcd8

fanquake force-pushed on Oct 4, 2022

DrahtBot commented at 8:23 PM on October 4, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)

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.

in src/test/fuzz/util/mempool.h:9 in eb15569280 outdated

   6 | -#define BITCOIN_TEST_FUZZ_MEMPOOL_UTILS_H
   7 | +#ifndef BITCOIN_TEST_FUZZ_UTIL_MEMPOOL_H
   8 | +#define BITCOIN_TEST_FUZZ_UTIL_MEMPOOL_H
   9 |  
  10 | +#include <primitives/transaction.h>
  11 | +#include <test/fuzz/FuzzedDataProvider.h>

maflcko commented at 8:27 AM on October 5, 2022:

those two includes could be fwd-decls, but it doesn't matter because everywhere where this header is included, the full includes already exist

maflcko approved

maflcko commented at 8:28 AM on October 5, 2022: member

review ACK 8a6b6dfcd8d26b62c3a13beba72440635fcc5e19 🍮

<details><summary>Show signature</summary>

Signature:

</details>

maflcko merged this on Oct 5, 2022

maflcko closed this on Oct 5, 2022

glozow commented at 8:41 AM on October 5, 2022: member

post-merge concept ACK

sidhujag referenced this in commit 05b8fd05f9 on Oct 5, 2022

fanquake deleted the branch on Oct 10, 2022

bitcoin locked this on Oct 10, 2023