fuzz: add target for `ScriptPubKeyMan` (legacy) #28153

brunoerg commented at 8:03 PM on July 25, 2023: contributor

This PR adds target for {Legacy}ScriptPubKeyMan. I'm working on a descriptor one and will do it in a separate file. I tried to focus here on functions that we use directly and we may have in some unit tests.

DrahtBot commented at 8:03 PM on July 25, 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
Concept ACK	Ayush170-Future

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:

#28236 (fuzz: wallet, add target for Spend by Ayush170-Future)
#26606 (wallet: Implement independent BDB parser by achow101)

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 Jul 25, 2023

brunoerg marked this as ready for review on Jul 25, 2023

DrahtBot added the label CI failed on Jul 25, 2023

in src/wallet/test/fuzz/scriptpubkeyman.cpp:39 in e951d31735 outdated

  34 | +    {
  35 | +        LOCK(wallet.cs_wallet);
  36 | +        wallet.SetLastBlockProcessed(chainstate->m_chain.Height(), chainstate->m_chain.Tip()->GetBlockHash());
  37 | +    }
  38 | +
  39 | +    LegacyScriptPubKeyMan& legacy_spkm{*wallet.GetOrCreateLegacyScriptPubKeyMan()};

maflcko commented at 6:11 AM on July 26, 2023:

fuzz targets should be deterministic based on the fuzz input. If you use a global wallet and spkm, the global state from the previous fuzz inputs will leak into the current one.

brunoerg commented at 10:02 AM on July 26, 2023:

Yea, I just realized it. I'm changing it!

brunoerg marked this as a draft on Jul 26, 2023

brunoerg force-pushed on Jul 26, 2023

in src/wallet/test/fuzz/scriptpubkeyman.cpp:30 in c2e197a7c5 outdated

  25 | +FUZZ_TARGET(scriptpubkeyman, .init = initialize_spkm)
  26 | +{
  27 | +    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
  28 | +    const auto& node{g_setup->m_node};
  29 | +    Chainstate* chainstate = &node.chainman->ActiveChainstate();
  30 | +    std::unique_ptr<CWallet> wallet{std::make_unique<CWallet>(node.chain.get(), "", CreateMockableWalletDatabase())};

maflcko commented at 2:28 PM on July 26, 2023:

Is this faster than the setup in ./src/wallet/test/fuzz/notifications.cpp? If yes, could switch that one over as well?

brunoerg commented at 4:23 PM on July 26, 2023:

I think the contexts are different. notifications is working with descriptor and here is legacy, I believe I'll have something better when I finish the DescriptorScriptPubKey target. Also, I believe the FuzzedWallet in notifications is disabled for spkm.

brunoerg marked this as ready for review on Jul 26, 2023

brunoerg commented at 4:24 PM on July 26, 2023: contributor

CI failure seems unrelated.

brunoerg force-pushed on Jul 26, 2023

brunoerg commented at 4:27 PM on July 26, 2023: contributor

Pushed to re-run CI.

DrahtBot removed the label CI failed on Jul 26, 2023

Ayush170-Future approved

Ayush170-Future commented at 8:27 PM on July 26, 2023: contributor

ACK

I support the idea of having separate files for Legacy and Descriptor because I think the Wallet implementation would differ. Overall, the code looks great to me.

in src/wallet/test/fuzz/scriptpubkeyman.cpp:45 in dbe54b1336 outdated

  40 | +    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) {
  41 | +        CallOneOf(
  42 | +            fuzzed_data_provider,
  43 | +            [&] {
  44 | +                CKey key;
  45 | +                key.MakeNewKey(fuzzed_data_provider.ConsumeBool());

maflcko commented at 6:07 AM on July 27, 2023:

I don't think this is deterministic, is it? You can set the key from the fuzz input buffer instead.

brunoerg commented at 8:30 PM on July 27, 2023:

Yea, just changed the approach to use buffer.

in src/wallet/test/fuzz/scriptpubkeyman.cpp:61 in dbe54b1336 outdated

  56 | +            [&] {
  57 | +                CScript script{ConsumeScript(fuzzed_data_provider)};
  58 | +                (void)legacy_spkm.CanProvide(script, data);
  59 | +            },
  60 | +            [&] {
  61 | +                CKeyID address(ConsumeUInt160(fuzzed_data_provider));

maflcko commented at 6:10 AM on July 27, 2023:

Seems impossible that this is ever hit in the happy case, since a fuzz engine can not predict the 160-bit hash of a key in the wallet. Wouldn't it be better to move this symbol outside this scope and occasionally set it to a valid hash?

brunoerg commented at 8:31 PM on July 27, 2023:

Yes, just changed it.

brunoerg force-pushed on Jul 27, 2023

in src/wallet/test/fuzz/scriptpubkeyman.cpp:69 in f67d8ecca3 outdated

  64 | +            [&] {
  65 | +                CScript script{ConsumeScript(fuzzed_data_provider)};
  66 | +                (void)legacy_spkm.CanProvide(script, data);
  67 | +            },
  68 | +            [&] {
  69 | +                key = keys[fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, num_keys - 1)];

maflcko commented at 6:01 AM on July 28, 2023:

nit: Could use PickValue?

brunoerg commented at 2:55 PM on July 28, 2023:

Yea, more elegant. Pushed changing it

maflcko approved

maflcko commented at 6:04 AM on July 28, 2023: member

lgtm

fuzz: add target for `ScriptPubKeyMan` (legacy) eb4d8b38f0

brunoerg force-pushed on Jul 28, 2023

fanquake requested review from achow101 on Aug 1, 2023

achow101 commented at 5:19 PM on September 20, 2023: member

Closing as there is little support for working on fixing non-critical issues in legacy wallets.

achow101 closed this on Sep 20, 2023

maflcko commented at 5:33 PM on September 20, 2023: member

Would be nice to have a descriptor wallet target? Alternatively, I think your plan was to speed up wallet_notifications IIRC?

brunoerg commented at 8:57 AM on September 21, 2023: contributor

Would be nice to have a descriptor wallet target? Alternatively, I think your plan was to speed up wallet_notifications IIRC?

Yes, I'm working on speeding up wallet_notifications.

bitcoin locked this on Sep 20, 2024