fuzz: wallet, add target for Spend

Ayush170-Future commented at 9:25 pm on August 7, 2023: contributor

This PR adds fuzz coverage for wallet/spend.

Motivation: Issue 27272

This PR adds Fuzz coverage to the whole concept of Creating a New Transaction as well as other sections of the Spend file. Because CreateTransaction is one of the most frequently used functions in the wallet codebase, merging this PR will significantly improve the wallet codebase’s Fuzz testing!

I also used the Singleton Class concept for creating Wallet instances because it assures that only one instance of it is created during all Fuzz runs, which significantly boosts the file’s exec/sec.

DrahtBot commented at 9:25 pm on August 7, 2023: 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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29936 (fuzz: wallet: add target for CreateTransaction by brunoerg)
#26606 (wallet: Implement independent BDB parser by achow101)
#26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB 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 Aug 7, 2023

in src/wallet/test/fuzz/spend.cpp:72 in b8121896a0 outdated

60+    WalletSingleton(const WalletSingleton&) = delete;
61+    WalletSingleton& operator=(const WalletSingleton&) = delete;
62+
63+    void InitializeWallet() {
64+        auto& node{g_setup->m_node};
65+        wallet = CreateSyncedWallet(*node.chain, WITH_LOCK(Assert(node.chainman)->GetMutex(), return node.chainman->ActiveChain()), g_setup->coinbaseKey);

brunoerg commented at 10:17 pm on August 7, 2023:

I think that CreateSyncedWallet will call SetupDescriptorScriptPubKeyMans() which internally will make a seed.

 0void CWallet::SetupDescriptorScriptPubKeyMans()
 1{
 2    AssertLockHeld(cs_wallet);
 3
 4    if (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) {
 5        // Make a seed
 6        CKey seed_key;
 7        seed_key.MakeNewKey(true);
 8        CPubKey seed = seed_key.GetPubKey();
 9        assert(seed_key.VerifyPubKey(seed));
10
11        // Get the extended key
12        CExtKey master_key;
13        master_key.SetSeed(seed_key);
14
15        SetupDescriptorScriptPubKeyMans(master_key);
16    }

Not sure, but wouldn’t it be non-deterministic? A solution would be to have a fuzz version of CreateSyncedWallet which calls SetupDescriptorScriptPubKeyMans passing a key that we created using buf.

in src/wallet/test/fuzz/spend.cpp:156 in b8121896a0 outdated

151+                for (unsigned int i = 0; i < recipients_size; ++i) {
152+                    recipients.push_back({/*scriptPubKey=*/ConsumeScript(fuzzed_data_provider),
153+                                        /*nAmount=*/ConsumeMoney(fuzzed_data_provider), 
154+                                        /*fSubtractFeeFromAmount=*/fuzzed_data_provider.ConsumeBool()});
155+                }
156+                // Equally likely to chose one of the three values.

brunoerg commented at 10:18 pm on August 7, 2023:

s/chose/choose

in src/wallet/test/fuzz/spend.cpp:184 in b8121896a0 outdated

179+                const CTxOut tx_out{ConsumeMoney(fuzzed_data_provider), ConsumeScript(fuzzed_data_provider)}; 
180+                CalculateMaximumSignedInputSize(tx_out, &wallet, &coin_control);
181+            },
182+            [&] {
183+                CAmount fee;
184+                int random_positions[3] = {-1, fuzzed_data_provider.ConsumeIntegralInRange<int>(0, mtx.vout.size()), fuzzed_data_provider.ConsumeIntegral<int>()};

brunoerg commented at 10:20 pm on August 7, 2023:

I think you could keep same pattern as you had previously:

0std::vector<int> random_positions = {-1, fuzzed_data_provider.ConsumeIntegralInRange<int>(0, recipients_size), fuzzed_data_provider.ConsumeIntegral<int>()};
1int change_pos = PickValue(fuzzed_data_provider, random_positions);

instead of

0int random_positions[3] = {-1, fuzzed_data_provider.ConsumeIntegralInRange<int>(0, mtx.vout.size()), fuzzed_data_provider.ConsumeIntegral<int>()};
1int change_pos = fuzzed_data_provider.PickValueInArray(random_positions);

brunoerg commented at 0:48 am on August 8, 2023: contributor

From lint:

 0+ test/lint/all-lint.py
 1This diff appears to have added new lines with trailing whitespace.
 2The following changes were suspected:
 3
 4diff --git a/src/wallet/test/fuzz/spend.cpp b/src/wallet/test/fuzz/spend.cpp
 5@@ -0,0 +1,254 @@
 6+namespace wallet { 
 7+ * Singleton wallet class ensures that only one 
 8+ * instance of CWallet is created and reused as required. 
 9+ * of creating a new `Descriptor Wallet` each time and deletes 
10+CCoinControl GetNewCoinControl(FuzzedDataProvider& fuzzed_data_provider, CWallet& wallet, std::vector<COutput>& coins) 
11+    
12+    // Because none of the UTXO will ever be used (marked as spent), mining this many should be sufficient. 
13+                                        /*nAmount=*/ConsumeMoney(fuzzed_data_provider), 
14+                const CTxOut tx_out{ConsumeMoney(fuzzed_data_provider), ConsumeScript(fuzzed_data_provider)}; 
15+    
16+                // Taking a random sub-sequence from available coins 
17+}   
18Success: no issues found in 269 source files
19^---- failure generated from lint-whitespace.py

DrahtBot added the label CI failed on Aug 8, 2023

Ayush170-Future force-pushed on Aug 10, 2023

Ayush170-Future commented at 8:15 pm on August 10, 2023: contributor

Thanks, @brunoerg for your reviews! In the recent push, I’ve addressed most of your reviews.

clang-format to fix the lint’s issue.
Changed PickValueFromArray to the PickValue approach.
Fixed one grammar mistake.

This comment is really interesting, but I’m not sure whether the non-determinism in this case might pose a significant issue or not. So, I’ll wait for more people to give their thoughts on this and we can write a new CreateSyncedWallet specific for Fuzzing purposes if felt necessary.

DrahtBot removed the label CI failed on Aug 10, 2023

DrahtBot added the label CI failed on Sep 4, 2023

DrahtBot removed the label CI failed on Sep 5, 2023

DrahtBot added the label CI failed on Oct 25, 2023

Ayush170-Future commented at 6:52 pm on November 2, 2023: contributor

The fuzzer logs are unclear, I’m not sure why it failed suddenly. Maybe some issue with the Fuzzer again?

maflcko commented at 7:02 pm on November 2, 2023: member

You’ll need to rebase on current master in any case

Ayush170-Future force-pushed on Nov 4, 2023

Ayush170-Future commented at 5:13 pm on November 4, 2023: contributor

Force-pushed rebasing on the current master branch.

PS - There is one change in CRecipient that I didn’t know about. Will update this PR against that change soon!

fuzz: add target for Spend 17a59053ef

Ayush170-Future force-pushed on Nov 4, 2023

DrahtBot removed the label CI failed on Nov 4, 2023

Ayush170-Future commented at 7:03 pm on November 4, 2023: contributor

Force-pushed updating the CRecipient wrt 28246.

All CI checks passed now 💪

in src/wallet/test/fuzz/spend.cpp:39 in 17a59053ef

34+    {
35+        static WalletSingleton instance;
36+        return instance;
37+    }
38+
39+    CWallet& GetWallet()

dergoegge commented at 10:05 am on November 13, 2023:

0    const CWallet& GetWallet()

It’s otherwise hard to tell if the global wallet instance is mutated. Same for GetCoins().

If the global instance is mutated, the target is likely to be non-deterministic which hurts fuzzer efficiency. The target should behave exactly the same when given the same input, but that might not be the case if global state is used and modified.

(The suggested change might not compile if wallet or available_coins are used as non-const/mutated)

Ayush170-Future commented at 5:20 pm on November 13, 2023:

Yeah, this should make it more deterministic. As you said, it’s causing some compilation errors with other code. I’ll force-push once I’ve fixed them.

Ayush170-Future commented at 12:10 pm on November 14, 2023:

The CreateTransaction() and FundTransaction() want the CWallet parameter to be non-const. I’m struggling to find a work around but I’m trying.

Just wanted to clear this, I think the CWallet instance inside these functions doesn’t seem to be directly mutated or changed anytime. So, do we really want to make it to const. Am I understanding this correct?

dergoegge commented at 12:27 pm on November 14, 2023:

If CreateTransaction() and FundTransaction() do not mutate the wallet, then they should probably take a const reference instead. But glancing at FundTransaction, it calls CWallet::LockCoin which is not a const method and could probably lead to non-determinism since it appears to be modifying internal state of a wallet. https://github.com/bitcoin/bitcoin/blob/fb85bb277670aad28fef51b7313d4a96cdaa760f/src/wallet/spend.cpp#L1388

Just wanted to clear this, I think the CWallet instance inside these functions doesn’t seem to be directly mutated or changed anytime. So, do we really want to make it to const.

Marking things that should not be mutated as const will make this a compile time check, giving us more confidence that the test is working properly.

You could try to refactor CreateTransaction and/or FundTransaction to take a const CWallet& and see what happens when you compile. The LockCoin issue mentioned above should be one of the things the compiler complains about.

Ayush170-Future commented at 3:30 pm on December 4, 2023:

Yes, I believe the LockCoin function does mutate the global wallet. I’ll need to figure out how to make this deterministic then.

I apologize for the delayed response, I was/am occupied with something else. But, I’m looking forward to brainstorming this and finding a solution as soon as possible.

Ayush170-Future commented at 3:02 pm on January 9, 2024:

I’ve thought about it, and I think a global Singleton might not be the smartest way to save resources. The wallet object definitely mutates during execution. I think I’ve to remove the Singleton pattern, but doing so will make the file very slow, as creating the Descriptor Wallet is really time-consuming.

murchandamus commented at 4:58 pm on April 17, 2024:

If you removed the Singleton, did you perhaps not push the latest state of this PR?

dergoegge changes_requested

dergoegge commented at 10:13 am on November 13, 2023: member

Coverage looks decent: https://dergoegge.github.io/bitcoin-coverage/pr28236/fuzz.coverage/index.html

DrahtBot added the label CI failed on Nov 16, 2023

in src/wallet/test/fuzz/spend.cpp:173 in 17a59053ef

168+                if (!res) return;
169+                mtx = CMutableTransaction(*(res->tx));
170+            },
171+            [&] {
172+                // Occasionally, creating a random Transaction for Fuzzing.
173+                auto new_mtx = ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider);

maflcko commented at 3:08 pm on November 29, 2023:

missing ser-params? See compile failure due to silent merge conflict.

Needs rebase

Ayush170-Future commented at 3:31 pm on December 4, 2023:

Thank you! I’ll rebase in the next force push.

in src/wallet/test/fuzz/spend.cpp:207 in 17a59053ef

202+    }
203+
204+    // Plays with the `CoinsResult`
205+    CoinsResult wallet_coins;
206+
207+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 50)

maflcko commented at 10:50 am on December 6, 2023:

Seems like this should be a separate fuzz target?

Ayush170-Future commented at 11:37 am on December 15, 2023:

Yeah, I can create a separate PR for this immediately. Once it’s ready, we can get it merged, and I can continue working on this. Thanks!

murchandamus commented at 5:02 pm on April 17, 2024:

Agree wtih @maflcko, since the seeds are created per target, it would be better to have a separate target for CoinsResult.

maflcko commented at 10:51 am on December 6, 2023: member

Spend.cpp coverage is not too bad already: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/spend.cpp.gcov.html

It may be good to clarify the new coverage a bit.

Ayush170-Future commented at 3:16 pm on January 9, 2024: contributor

Spend.cpp coverage is not too bad already: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/spend.cpp.gcov.html

It may be good to clarify the new coverage a bit.

I didn’t understand. Are you suggesting that I reconsider what I want to increase coverage of in this file?

Also, I have a question about the blue lines in the coverage reports. Do they represent the line coverage of the code during fuzzing? For instance, if these functions are encountered in the process of running any fuzz file, the fuzz coverage will display them in blue. If I’m understanding this correctly, wouldn’t it still make sense to have a separate fuzz file for Creating Transaction so that we can explore more test cases and code flows? These might not be tested if the coverage is coming from another fuzz file designed for a different purpose or codebase.

https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/spend.cpp.gcov.html

achow101 requested review from achow101 on Apr 9, 2024

achow101 requested review from murchandamus on Apr 9, 2024

in src/wallet/test/fuzz/spend.cpp:126 in 17a59053ef

121+void initialize_spend()
122+{
123+    static auto testing_setup = MakeNoLogFileContext<TestChain100Setup>();
124+    g_setup = testing_setup.get();
125+
126+    // Add 50 spendable UTXO, 50 BTC each, to the wallet (total balance 5000 BTC)

murchandamus commented at 4:53 pm on April 17, 2024:

This comment is inconsistent:

0    // Add 50 spendable UTXO, 50 BTC each, to the wallet (total balance 2500 BTC)

A variable set of available coins might be desirable. Smaller amounts could for example trigger behavior regarding negative effective value or having insufficient funds to create a transaction.

murchandamus commented at 5:03 pm on April 17, 2024: contributor

Hey,

Just did a quick pass. What is the status of this PR?

maflcko commented at 6:20 pm on April 17, 2024: member

Are you still working on this?

in src/wallet/test/fuzz/spend.cpp:108 in 17a59053ef

103+        if (fuzzed_data_provider.ConsumeBool()) {
104+            new_coin_control.Select(coin.outpoint);
105+        }
106+    }
107+
108+    // Occasionally, some random `CCoutPoint` are also selected.

brunoerg commented at 7:54 pm on April 17, 2024:

0    // Occasionally, some random `COutPoint` are also selected.

Ayush170-Future commented at 5:26 am on April 18, 2024: contributor

Are you still working on this?

Sorry I got a little occupied with some other things and would want to continue working on this. I’ve the approach in mind and will force push the update by the end of this month.

Will create a new file for CoinsResult and remove global Singleton from the Spend file.

DrahtBot added the label Needs rebase on May 21, 2024

DrahtBot commented at 11:06 am on May 21, 2024: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

fanquake marked this as a draft on Jun 25, 2024

fanquake commented at 3:41 pm on June 25, 2024: member

I’ve the approach in mind and will force push the update by the end of this month.

Moved to draft for now.

fuzz: wallet, add target for Spend #28236

Code Coverage

Reviews

Conflicts