[tests] New fuzz target wallet

kevkevinpal commented at 11:42 pm on August 1, 2024: contributor

This change adds a new fuzz target wallet_rpc, this change was suggested in #29901

This change use some of the functionality of the fuzz target rpc to also target the wallet rpc’s

DrahtBot commented at 11:42 pm on August 1, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30570.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	maflcko, dergoegge, brunoerg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

kevkevinpal force-pushed on Aug 1, 2024

in src/test/fuzz/rpc.cpp:207 in f94ae1cbfb outdated

202+const std::vector<std::string> WALLET_RPC_COMMANDS_NOT_SAFE_FOR_FUZZING{
203+    "importwallet",
204+    "loadwallet",
205+};
206+// RPC commands which are safe for fuzzing.
207+const std::vector<std::string> WALLET_RPC_COMMANDS_SAFE_FOR_FUZZING{

kevkevinpal commented at 11:45 pm on August 1, 2024:

I will need to move some of these over due to the rpc accessing the disk or network activity

kevkevinpal force-pushed on Aug 1, 2024

kevkevinpal commented at 11:48 pm on August 1, 2024: contributor

This is a screenshot of the coverage I was able to generate so far for ./src/wallet/rpc

Screenshot 2024-08-01 at 6 47 13 PM

DrahtBot added the label CI failed on Aug 2, 2024

DrahtBot commented at 0:57 am on August 2, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/28242205421

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

maflcko commented at 8:13 am on August 2, 2024: member

Concept ACK. To fix the CI failure, you’ll have to move the new file to src/wallet/test/fuzz/.

dergoegge commented at 8:36 am on August 2, 2024: member

Concept ACK

brunoerg commented at 12:23 pm on August 2, 2024: contributor

Concept ACK

kevkevinpal force-pushed on Aug 31, 2024

in src/Makefile.test.include:205 in c994331bdc outdated

201@@ -202,6 +202,7 @@ BITCOIN_TESTS += wallet/test/db_tests.cpp
202 endif
203 
204 FUZZ_WALLET_SRC = \
205+ wallet/test/fuzz/rpc.cpp \

hebasto commented at 3:12 am on August 31, 2024:

This change should be replaced with the following diff:

0--- a/src/wallet/test/fuzz/CMakeLists.txt
1+++ b/src/wallet/test/fuzz/CMakeLists.txt
2@@ -10,6 +10,7 @@ target_sources(fuzz
3     fees.cpp
4     $<$<BOOL:${USE_SQLITE}>:${CMAKE_CURRENT_LIST_DIR}/notifications.cpp>
5     parse_iso8601.cpp
6+    rpc.cpp
7     $<$<BOOL:${USE_SQLITE}>:${CMAKE_CURRENT_LIST_DIR}/scriptpubkeyman.cpp>
8     wallet_bdb_parser.cpp
9 )

kevkevinpal commented at 3:00 am on September 4, 2024:

Thanks!

Updated in 2c8cb2b

DrahtBot removed the label CI failed on Aug 31, 2024

DrahtBot added the label Needs rebase on Sep 2, 2024

kevkevinpal force-pushed on Sep 4, 2024

kevkevinpal changed the base branch on Sep 4, 2024

kevkevinpal force-pushed on Sep 4, 2024

DrahtBot removed the label Needs rebase on Sep 4, 2024

DrahtBot added the label CI failed on Sep 8, 2024

DrahtBot removed the label CI failed on Sep 9, 2024

DrahtBot added the label CI failed on Sep 13, 2024

DrahtBot commented at 6:05 am on September 13, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30049600088

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

kevkevinpal force-pushed on Sep 13, 2024

brunoerg commented at 5:23 pm on September 24, 2024: contributor

What is the state of this?

kevkevinpal commented at 3:19 am on September 25, 2024: contributor

What is the state of this?

I have been busy for the last couple of weeks I can take a look at it and finish up to make it ready for review in the next week or so, sorry for the delay

kevkevinpal force-pushed on Sep 25, 2024

test: new fuzz target wallet_rpc

By reusing some of the logic for the rpc fuzz target this new fuzz
target wallet_rpc adds coverage to the wallet rpc calls.

26591de46c

kevkevinpal force-pushed on Sep 25, 2024

kevkevinpal marked this as ready for review on Sep 25, 2024

kevkevinpal commented at 1:16 pm on September 25, 2024: contributor

@brunoerg I think this should be ready for review now, I think I just need to currate the list of WALLET_RPC_COMMANDS_SAFE_FOR_FUZZING and WALLET_RPC_COMMANDS_NOT_SAFE_FOR_FUZZING

DrahtBot removed the label CI failed on Sep 25, 2024

in src/wallet/test/fuzz/rpc.cpp:42 in 26591de46c

37+        return supported_rpc_commands;
38+    }
39+};
40+
41+RPCWalletFuzzTestingSetup* rpc_wallet_testing_setup = nullptr;
42+std::string g_limit_to_rpc_command_wallet;

brunoerg commented at 10:43 am on October 25, 2024:

In 26591de46c93a0c73df8c9133af69dcc7f4db569: g_limit_to_rpc_command_wallet is not being set, so LIMIT_TO_RPC_COMMAND has no effect in this target. When using LIMIT_TO_RPC_COMMAND for this target, it’s probably setting g_limit_to_rpc_command in fuzz/rpc.cpp.

brunoerg commented at 10:44 am on October 25, 2024: contributor

@brunoerg I think this should be ready for review now, I think I just need to currate the list of WALLET_RPC_COMMANDS_SAFE_FOR_FUZZING and WALLET_RPC_COMMANDS_NOT_SAFE_FOR_FUZZING

Did you advance on curating the list? Is this the final list?

in src/test/fuzz/rpc.cpp:353 in 26591de46c

356+            std::cerr << "Error: RPC command \"" << rpc_command << "\" not found in rpc_commands_safe_for_fuzzing or RPC_COMMANDS_NOT_SAFE_FOR_FUZZING. Please update " << __FILE__ << ".\n";
357             std::terminate();
358         }
359         if (safe_for_fuzzing && not_safe_for_fuzzing) {
360-            std::cerr << "Error: RPC command \"" << rpc_command << "\" found in *both* RPC_COMMANDS_SAFE_FOR_FUZZING and RPC_COMMANDS_NOT_SAFE_FOR_FUZZING. Please update " << __FILE__ << ".\n";
361+            std::cerr << "Error: RPC command \"" << rpc_command << "\" found in *both* rpc_commands_safe_for_fuzzing and RPC_COMMANDS_NOT_SAFE_FOR_FUZZING. Please update " << __FILE__ << ".\n";

brunoerg commented at 10:47 am on October 25, 2024:

In 26591de46c93a0c73df8c9133af69dcc7f4db569: nit: If something got wrong with wallet/test/fuzz/rpc.cpp, this will ask to update the test/fuzz/rpc.cpp file.

in src/wallet/test/fuzz/rpc.cpp:48 in 26591de46c

43+
44+// RPC commands which are safe for fuzzing.
45+const std::vector<std::string> WALLET_RPC_COMMANDS_NOT_SAFE_FOR_FUZZING{
46+    "importwallet",
47+    "loadwallet",
48+};

dergoegge commented at 12:40 pm on October 25, 2024:

There are many more wallet rpcs that go to disk (e.g. createwallet). I would assume a lot of the rpcs that modify a wallet also flush the changes to disk. And I don’t think it is possible to even have a wallet that doesn’t exist on disk, so trying to avoid disk access can’t work.

To work around this I would suggest to create (and delete at the end) a new wallet (e.g. /tmp/fuzz-wallet-xyz) each iteration and call wallet rpcs on that specific wallet (could be more than one).

brunoerg commented at 1:58 pm on October 25, 2024: contributor

I think we will have here the same problems that we usually have for other wallet targets. The size of keypool, wallet encryption, etc… may cause timeout issues.

brunoerg commented at 12:16 pm on November 5, 2024: contributor

Are you still working on it?

kevkevinpal commented at 2:24 pm on November 7, 2024: contributor

Are you still working on it?

Sorry I’ve been quite busy, I can close this PR and leave it up for grabs since I havent had a chance to make much progress lately

kevkevinpal closed this on Nov 7, 2024

[tests] New fuzz target wallet_rpc #30570

Code Coverage & Benchmarks

Reviews

Conflicts