fuzz: wallet, add target for Spend #28236

Ayush170-Future · 2023-08-07T21:25:27Z

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 · 2023-08-07T21:25:30Z

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.

brunoerg · 2023-08-07T22:17:29Z

src/wallet/test/fuzz/spend.cpp

+
+    void InitializeWallet() {
+        auto& node{g_setup->m_node};
+        wallet = CreateSyncedWallet(*node.chain, WITH_LOCK(Assert(node.chainman)->GetMutex(), return node.chainman->ActiveChain()), g_setup->coinbaseKey);


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

void CWallet::SetupDescriptorScriptPubKeyMans() { AssertLockHeld(cs_wallet); if (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { // Make a seed CKey seed_key; seed_key.MakeNewKey(true); CPubKey seed = seed_key.GetPubKey(); assert(seed_key.VerifyPubKey(seed)); // Get the extended key CExtKey master_key; master_key.SetSeed(seed_key); SetupDescriptorScriptPubKeyMans(master_key); }

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.

src/wallet/test/fuzz/spend.cpp

brunoerg · 2023-08-08T00:48:16Z

From lint:

+ test/lint/all-lint.py
This diff appears to have added new lines with trailing whitespace.
The following changes were suspected:

diff --git a/src/wallet/test/fuzz/spend.cpp b/src/wallet/test/fuzz/spend.cpp
@@ -0,0 +1,254 @@
+namespace wallet { 
+ * Singleton wallet class ensures that only one 
+ * instance of CWallet is created and reused as required. 
+ * of creating a new `Descriptor Wallet` each time and deletes 
+CCoinControl GetNewCoinControl(FuzzedDataProvider& fuzzed_data_provider, CWallet& wallet, std::vector<COutput>& coins) 
+    
+    // Because none of the UTXO will ever be used (marked as spent), mining this many should be sufficient. 
+                                        /*nAmount=*/ConsumeMoney(fuzzed_data_provider), 
+                const CTxOut tx_out{ConsumeMoney(fuzzed_data_provider), ConsumeScript(fuzzed_data_provider)}; 
+    
+                // Taking a random sub-sequence from available coins 
+}   
Success: no issues found in 269 source files
^---- failure generated from lint-whitespace.py

Ayush170-Future · 2023-08-10T20:15:56Z

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.

Ayush170-Future · 2023-11-02T18:52:41Z

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

maflcko · 2023-11-02T19:02:35Z

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

Ayush170-Future · 2023-11-04T17:13:15Z

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!

Ayush170-Future · 2023-11-04T19:03:55Z

Force-pushed updating the CRecipient wrt 28246.

All CI checks passed now 💪

dergoegge

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

dergoegge · 2023-11-13T10:05:13Z

src/wallet/test/fuzz/spend.cpp

+        return instance;
+    }
+
+    CWallet& GetWallet()


Suggested change

CWallet& GetWallet()

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)

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.

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?

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.

bitcoin/src/wallet/spend.cpp

Line 1388 in fb85bb2

wallet.LockCoin(txin.prevout);

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.

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.

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.

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

maflcko · 2023-11-29T15:08:00Z

src/wallet/test/fuzz/spend.cpp

+            },
+            [&] {
+                // Occasionally, creating a random Transaction for Fuzzing.
+                auto new_mtx = ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider);


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

Needs rebase

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

maflcko

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.

maflcko · 2023-12-06T10:50:27Z

src/wallet/test/fuzz/spend.cpp

+    CoinsResult wallet_coins;
+
+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 50)


Seems like this should be a separate fuzz target?

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!

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

Ayush170-Future · 2024-01-09T15:16:02Z

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

murchandamus

Hey,

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

murchandamus · 2024-04-17T16:53:28Z

src/wallet/test/fuzz/spend.cpp

+    static auto testing_setup = MakeNoLogFileContext<TestChain100Setup>();
+    g_setup = testing_setup.get();
+
+    // Add 50 spendable UTXO, 50 BTC each, to the wallet (total balance 5000 BTC)


This comment is inconsistent:

Suggested change

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

// 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 · 2024-04-17T16:58:10Z

src/wallet/test/fuzz/spend.cpp

+        return instance;
+    }
+
+    CWallet& GetWallet()


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

murchandamus · 2024-04-17T17:02:35Z

src/wallet/test/fuzz/spend.cpp

+    CoinsResult wallet_coins;
+
+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 50)


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

maflcko · 2024-04-17T18:20:40Z

Are you still working on this?

brunoerg · 2024-04-17T19:54:28Z

src/wallet/test/fuzz/spend.cpp

+        }
+    }
+
+    // Occasionally, some random `CCoutPoint` are also selected.


Suggested change

// Occasionally, some random `CCoutPoint` are also selected.

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

Ayush170-Future · 2024-04-18T05:26:34Z

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 · 2024-05-21T11:06:56Z

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

fanquake · 2024-06-25T15:41:09Z

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

Moved to draft for now.

maflcko · 2024-07-16T19:31:10Z

Picked up in #30461

DrahtBot added the Tests label Aug 7, 2023

brunoerg reviewed Aug 7, 2023

View reviewed changes

src/wallet/test/fuzz/spend.cpp Outdated Show resolved Hide resolved

brunoerg reviewed Aug 7, 2023

View reviewed changes

src/wallet/test/fuzz/spend.cpp Outdated Show resolved Hide resolved

DrahtBot added the CI failed label Aug 8, 2023

This was referenced Aug 8, 2023

fuzz: add target for ScriptPubKeyMan (legacy) #28153

Closed

wallet: Implement independent BDB parser #26606

Merged

wallet: Migrate legacy wallets to descriptor wallets without requiring BDB #26596

Merged

Ayush170-Future force-pushed the fuzz-coverage-spend branch from b812189 to 885b037 Compare August 10, 2023 19:58

DrahtBot removed the CI failed label Aug 10, 2023

DrahtBot added CI failed and removed CI failed labels Sep 4, 2023

DrahtBot mentioned this pull request Oct 24, 2023

Remove the legacy wallet and BDB dependency #28710

Merged

DrahtBot added the CI failed label Oct 25, 2023

Ayush170-Future force-pushed the fuzz-coverage-spend branch from 885b037 to dff1256 Compare November 4, 2023 17:11

fuzz: add target for Spend

17a5905

Ayush170-Future force-pushed the fuzz-coverage-spend branch from dff1256 to 17a5905 Compare November 4, 2023 17:32

DrahtBot removed the CI failed label Nov 4, 2023

dergoegge suggested changes Nov 13, 2023

View reviewed changes

DrahtBot added the CI failed label Nov 16, 2023

maflcko reviewed Nov 29, 2023

View reviewed changes

maflcko reviewed Dec 6, 2023

View reviewed changes

achow101 requested review from achow101 and murchandamus April 9, 2024 15:16

brunoerg mentioned this pull request Apr 17, 2024

Wallet fuzzing tracking issue #29901

Open

13 tasks

murchandamus reviewed Apr 17, 2024

View reviewed changes

brunoerg reviewed Apr 17, 2024

View reviewed changes

DrahtBot mentioned this pull request Apr 22, 2024

fuzz: wallet: add target for CreateTransaction #29936

Merged

DrahtBot added the Needs rebase label May 21, 2024

fanquake marked this pull request as draft June 25, 2024 15:40

brunoerg mentioned this pull request Jul 16, 2024

fuzz: add target for CoinsResult #30461

Closed

maflcko closed this Jul 16, 2024

bitcoin locked and limited conversation to collaborators Jul 16, 2025

		CoinsResult wallet_coins;

		LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 50)

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

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

fuzz: wallet, add target for Spend #28236

fuzz: wallet, add target for Spend #28236

Uh oh!

Conversation

Ayush170-Future commented Aug 7, 2023

Uh oh!

DrahtBot commented Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

brunoerg commented Aug 8, 2023

Uh oh!

Ayush170-Future commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ayush170-Future commented Nov 2, 2023

Uh oh!

maflcko commented Nov 2, 2023

Uh oh!

Ayush170-Future commented Nov 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ayush170-Future commented Nov 4, 2023

Uh oh!

dergoegge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ayush170-Future Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ayush170-Future Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ayush170-Future commented Jan 9, 2024

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ayush170-Future commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

DrahtBot commented Aug 7, 2023 •

edited

Loading

Ayush170-Future commented Aug 10, 2023 •

edited

Loading

Ayush170-Future commented Nov 4, 2023 •

edited

Loading

Ayush170-Future Jan 9, 2024 •

edited

Loading

Ayush170-Future Dec 15, 2023 •

edited

Loading

Ayush170-Future commented Apr 18, 2024 •

edited

Loading