tests: Add fuzzing harnesses for various classes/functions in policy/ (CBlockPolicyEstimator, IsRBFOptIn(…), etc.) #18775

practicalswift commented at 8:15 PM on April 26, 2020: contributor

Add fuzzing harnesses for various classes/functions in policy/ (CBlockPolicyEstimator, IsRBFOptIn(…), etc.).

See doc/fuzzing.md for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the Bitcoin Core fuzzing corpus repo.

Happy fuzzing :)

DrahtBot added the label Build system on Apr 26, 2020

DrahtBot added the label Tests on Apr 26, 2020

fanquake removed the label Build system on Apr 26, 2020

DrahtBot commented at 4:33 AM on April 27, 2020: 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:

#18736 (test: Add fuzzing harnesses for various classes/functions in util/ by practicalswift)

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 cross-referenced this on Apr 27, 2020 from issue test: Add fuzzing harnesses for various classes/functions in util/ by practicalswift

in src/test/fuzz/util.h:106 in 9e8bf1c000 outdated

  99 | @@ -97,6 +100,18 @@ NODISCARD inline arith_uint256 ConsumeArithUInt256(FuzzedDataProvider& fuzzed_da
 100 |      return UintToArith256(ConsumeUInt256(fuzzed_data_provider));
 101 |  }
 102 |  
 103 | +NODISCARD inline CTxMemPoolEntry ConsumeTxMemPoolEntry(FuzzedDataProvider& fuzzed_data_provider, const CTransaction& tx) noexcept
 104 | +{
 105 | +    // Avoid:
 106 | +    // policy/feerate.cpp:28:34: runtime error: signed integer overflow: 34873208148477500 * 1000 cannot be represented in type 'long'

MarcoFalke commented at 2:03 PM on April 27, 2020:

how is this possible? ConsumeMoney should be less than 34873208148477500

practicalswift commented at 1:38 PM on April 30, 2020:

I think you're misunderstanding what is happening here: note that it ConsumeMoney is always returning within the money bounds, but the calculation does not take place directly on that amount :)

See this example:

const CAmount fee_paid = 348732081484775;
assert(MoneyRange(fee_paid));
const size_t bytes = 10;
const CFeeRate fee_rate(fee_paid, bytes);
(void)fee_rate.GetFeePerK();
policy/feerate.cpp:28:34: runtime error: signed integer overflow: 34873208148477500 * 1000 cannot be represented in type 'long'

in src/test/fuzz/policy_estimator.cpp:46 in 9e8bf1c000 outdated

  41 | +                }
  42 | +                const CTransaction tx{*mtx};
  43 | +                mempool_entries.push_back(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx));
  44 | +            }
  45 | +            std::vector<const CTxMemPoolEntry*> ptrs;
  46 | +            ptrs.reserve(mempool_entries.size());

MarcoFalke commented at 2:05 PM on April 27, 2020:

does this do anything for the small inputs that the fuzz engine produces by default?

practicalswift commented at 1:12 PM on April 30, 2020:

I don't think the impact (if any) will be large, but it doesn't hurt and it makes clang-tidy happy :)

in src/test/fuzz/policy_estimator.cpp:54 in 9e8bf1c000 outdated

  49 | +            }
  50 | +            block_policy_estimator.processBlock(fuzzed_data_provider.ConsumeIntegral<unsigned int>(), ptrs);
  51 | +            break;
  52 | +        }
  53 | +        case 2: {
  54 | +            (void)block_policy_estimator.removeTx(ConsumeUInt256(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool());

MarcoFalke commented at 2:07 PM on April 27, 2020:

            (void)block_policy_estimator.removeTx(ConsumeUInt256(fuzzed_data_provider), /* inBlock */ fuzzed_data_provider.ConsumeBool());

practicalswift commented at 1:19 PM on April 30, 2020:

Fixed!

in src/test/fuzz/policy_estimator.cpp:31 in 9e8bf1c000 outdated

  26 | +                break;
  27 | +            }
  28 | +            const CTransaction tx{*mtx};
  29 | +            block_policy_estimator.processTransaction(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx), fuzzed_data_provider.ConsumeBool());
  30 | +            if (fuzzed_data_provider.ConsumeBool()) {
  31 | +                (void)block_policy_estimator.removeTx(tx.GetHash(), fuzzed_data_provider.ConsumeBool());

MarcoFalke commented at 2:07 PM on April 27, 2020:

                (void)block_policy_estimator.removeTx(tx.GetHash(), /* inBlock */ fuzzed_data_provider.ConsumeBool());

practicalswift commented at 1:20 PM on April 30, 2020:

Fixed!

MarcoFalke approved

MarcoFalke commented at 2:07 PM on April 27, 2020: member

ACK

DrahtBot added the label Needs rebase on Apr 29, 2020

tests: Add fuzzing harness for CBlockPolicyEstimator 3439c88a5d

tests: Add fuzzing harness for IsRBFOptIn(...) 13c1f6b24f

practicalswift force-pushed on Apr 30, 2020

DrahtBot removed the label Needs rebase on Apr 30, 2020

tests: Clarify how we avoid hitting the signed integer overflow in CFeeRate::GetFeePerK() when fuzzing 2bcc2bd742

practicalswift commented at 2:21 PM on April 30, 2020: contributor

Added a commit which clarifies how the signed integer overflow in CFeeRate::GetFeePerK() is avoided when fuzzing :)

MarcoFalke merged this on Apr 30, 2020

MarcoFalke closed this on Apr 30, 2020

decryp2kanon cross-referenced this on Oct 20, 2020 from issue signed integer overflow: 107374182400000000 * 1000 cannot be represented in type 'long' by decryp2kanon

decryp2kanon referenced this in commit cfe9fc4678 on Oct 20, 2020

practicalswift deleted the branch on Apr 10, 2021

kwvg referenced this in commit 968049d477 on Aug 11, 2022

bitcoin locked this on Aug 16, 2022