fuzz: introduce and use `ConsumePrivateKey` helper #28419

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202309-fuzz-add_consumeprivkey_helper changing 7 files +23 −32

theStack commented at 9:20 PM on September 5, 2023: contributor
In the course of reviewing BIP324 related PRs I noticed a frequent pattern of creating private keys (CKey instances) with data consumed from the fuzz data provider:
```
    auto key_data = provider.ConsumeBytes<unsigned char>(32);
    key_data.resize(32);
    CKey key;
    key.Set(key_data.begin(), key_data.end(), /*fCompressedIn=*/true);
```
This PR introduces a corresponding helper ConsumePrivateKey in order to deduplicate code. The compressed flag can either be set to a fixed value, or, if std::nullopt is passed (=default), is also consumed from the fuzz data provider via .ConsumeBool().

Note that this is not a pure refactor, as some of the replaced call-sites previously consumed a random length (ConsumeRandomLengthByteVector) instead of a fixed size of 32 bytes for key data. As far as I can see, there is not much value in using a random size, as in all those cases we can only proceed or do something useful with a valid private key, and key data with sizes other than 32 bytes always lead to invalid keys.
DrahtBot commented at 9:20 PM on September 5, 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 sipa, brunoerg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Tests on Sep 5, 2023
fuzz: introduce and use `ConsumePrivateKey` helper 583af18fd1

Type	Reviewers
ACK	sipa, brunoerg

in src/test/fuzz/util.cpp:201 in 2d05cd2bcb outdated

 192 | @@ -193,6 +193,15 @@ CTxDestination ConsumeTxDestination(FuzzedDataProvider& fuzzed_data_provider) no
 193 |      return tx_destination;
 194 |  }
 195 |  
 196 | +[[nodiscard]] CKey ConsumePrivateKey(FuzzedDataProvider& fuzzed_data_provider, std::optional<bool> compressed) noexcept
 197 | +{
 198 | +    auto key_data = fuzzed_data_provider.ConsumeBytes<uint8_t>(32);
 199 | +    key_data.resize(32);
 200 | +    CKey key;
 201 | +    key.Set(key_data.begin(), key_data.end(), compressed.value_or(fuzzed_data_provider.ConsumeBool()));

maflcko commented at 6:05 AM on September 6, 2023:

Seems wasteful to eat a byte and discard it? You'll have to use the equivalent of https://en.cppreference.com/w/cpp/utility/optional/or_else in C++17, I think

theStack commented at 12:01 PM on September 6, 2023:

Oh indeed, that was not intended. Rewrote by introducing a boolean variable and using the ternary operator.

theStack force-pushed on Sep 6, 2023
brunoerg commented at 4:43 PM on September 6, 2023: contributor

Concept ACK
sipa commented at 8:50 PM on September 6, 2023: member

utACK 583af18fd1d0bda5a6a1d0403ffc498a512a546d
brunoerg approved
brunoerg commented at 2:01 AM on September 7, 2023: contributor

crACK 583af18fd1d0bda5a6a1d0403ffc498a512a546d
fanquake merged this on Sep 7, 2023
fanquake closed this on Sep 7, 2023
theStack deleted the branch on Sep 7, 2023
Frank-GER referenced this in commit bdad066ffd on Sep 8, 2023
bitcoin locked this on Sep 6, 2024

Contributors

Labels

fuzz: introduce and use `ConsumePrivateKey` helper #28419

Reviews