fuzz: improve scriptpubkeyman target #30563

1. 
   brunoerg commented at 8:17 pm on July 31, 2024: contributor

   Fixes #30541

   This PR aims to improve scriptpubkeyman target to avoid timeouts. The input provided in #30541 takes too much time to run because it basically calls only MarkUnusedAddresses (300 times * number of spks). The following changes were made to improve it:

   • Reduce keypool size.
   • When calling MarkUnusedAddresses, do it with one of the spks per iteration.
   • Remove the specific AddDescriptorKey call since it is already covered with AddWalletDescriptor.
   • Limit number of iterations to a reasonable value.
2. 
   DrahtBot commented at 8:17 pm on July 31, 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	maflcko, achow101

   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:

   • #30221 (wallet: Ensure best block matches wallet scan state by achow101)
   • #28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively 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.

3. DrahtBot added the label Tests on Jul 31, 2024
4. 
   brunoerg commented at 8:17 pm on July 31, 2024: contributor

   cc: @maflcko
5. in src/wallet/test/fuzz/scriptpubkeyman.cpp:115 in 487580813c outdated

   113+        std::string error;
   114+        if (spk_manager->CanUpdateToWalletDescriptor(wallet_desc->first, error)) {
   115+            auto new_spk_manager{CreateDescriptor(wallet_desc->first, wallet_desc->second, wallet)};
   116+            if (new_spk_manager != nullptr) spk_manager = new_spk_manager;
   117+        }
   118+    }
   

   ---

   ---

   

   maflcko commented at 9:00 am on August 1, 2024:

   nit: Why set or read good_data, when it can never be false? The only code path that sets it to false will return early.

   The bool is only used to break from bad data in nested lambdas, but there are no lambdas here.

   ---

   

   brunoerg commented at 11:16 am on August 1, 2024:

   You’re right, there is no need for good_data here. It was a leftover. Gonna change it.

   ---

   

   brunoerg commented at 12:57 pm on August 1, 2024:

   Done.
6. 
   maflcko commented at 9:03 am on August 1, 2024: member

   calls only MarkUnusedAddresses (300 times * number of spks)

   Are they different spks? IIRC when I looked, they looked all identical, which I am not sure is working as intended.

7. brunoerg force-pushed on Aug 1, 2024
8. 
   brunoerg commented at 1:21 pm on August 1, 2024: contributor

   Are they different spks? IIRC when I looked, they looked all identical, which I am not sure is working as intended.

   For that input, it was calling MarkUnusedAddresses with all spks every iteration. Besides that, every time MarkUnusedAddresses is called, the number of spks increases. That’s why it takes a lot time to run.

   Now, it will just call MarkUnusedAddresses with just one spk (randomly) per iteration. Besides that, the number of iterations was reduced for a reasonable value.

9. in src/wallet/test/fuzz/scriptpubkeyman.cpp:150 in fd7cd3b8b2 outdated

   146@@ -147,26 +147,10 @@ FUZZ_TARGET(scriptpubkeyman, .init = initialize_spkm)
   147                 for (const CScript& spk : spks) {
   148                     if (fuzzed_data_provider.ConsumeBool()) {
   149                         spk_manager->MarkUnusedAddresses(spk);
   150+                        break;
   

   ---

   ---

   

   maflcko commented at 1:46 pm on August 1, 2024:

   If you want to pick one value, you can use PickValue.

   ---

   

   brunoerg commented at 2:11 pm on August 1, 2024:

   Done
10. maflcko approved
11. 
    maflcko commented at 1:46 pm on August 1, 2024: member

    lgtm ACK fd7cd3b8b24ad86dae5c16addbb5acafa18efc0a
12. fuzz: improve scriptpubkeyman target

    The goal of this improvement is to reduce
    TopUp calls which can lead to timeouts.

    401cc4ec70
13. brunoerg force-pushed on Aug 1, 2024
14. 
    brunoerg commented at 2:12 pm on August 1, 2024: contributor

    Force-pushed addressing #30563 (review)
15. 
    maflcko commented at 3:01 pm on August 1, 2024: member

    lgtm ACK 401cc4ec70d67ba2aa0e078d2fab214e1c40742c
16. maflcko added this to the milestone 28.0 on Aug 10, 2024
17. 
    achow101 commented at 6:31 pm on August 12, 2024: member

    ACK 401cc4ec70d67ba2aa0e078d2fab214e1c40742c
18. achow101 merged this on Aug 12, 2024
19. achow101 closed this on Aug 12, 2024

    ---

Contributors
brunoerg DrahtBot maflcko achow101

Labels
Tests

Milestone
28.0