[tests] New fuzz target wallet_rpc #30570

kevkevinpal · 2024-08-01T23:42:27Z

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 · 2024-08-01T23:42:29Z

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 · 2024-08-01T23:45:37Z

src/test/fuzz/rpc.cpp

+    "loadwallet",
+};
+// RPC commands which are safe for fuzzing.
+const std::vector<std::string> WALLET_RPC_COMMANDS_SAFE_FOR_FUZZING{


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

kevkevinpal · 2024-08-01T23:48:01Z

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

DrahtBot · 2024-08-02T00:57:31Z

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

Hints

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

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

dergoegge · 2024-08-02T08:36:37Z

Concept ACK

brunoerg · 2024-08-02T12:23:10Z

Concept ACK

src/Makefile.test.include

DrahtBot · 2024-09-13T06:05:44Z

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

Hints

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.

brunoerg · 2024-09-24T17:23:40Z

What is the state of this?

kevkevinpal · 2024-09-25T03:19:47Z

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

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

kevkevinpal · 2024-09-25T13:16:37Z

@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

brunoerg · 2024-10-25T10:43:49Z

src/wallet/test/fuzz/rpc.cpp

+};
+
+RPCWalletFuzzTestingSetup* rpc_wallet_testing_setup = nullptr;
+std::string g_limit_to_rpc_command_wallet;


In 26591de: 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 · 2024-10-25T10:44:51Z

@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?

brunoerg · 2024-10-25T10:47:25Z

src/test/fuzz/rpc.cpp

            std::terminate();
        }
        if (safe_for_fuzzing && not_safe_for_fuzzing) {
-            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";
+            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";


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

dergoegge · 2024-10-25T12:40:29Z

src/wallet/test/fuzz/rpc.cpp

+const std::vector<std::string> WALLET_RPC_COMMANDS_NOT_SAFE_FOR_FUZZING{
+    "importwallet",
+    "loadwallet",
+};


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 · 2024-10-25T13:58:19Z

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 · 2024-11-05T12:16:14Z

Are you still working on it?

kevkevinpal · 2024-11-07T14:24:47Z

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 force-pushed the fuzzwalletrpc branch from 664dd71 to f94ae1c Compare August 1, 2024 23:43

kevkevinpal commented Aug 1, 2024

View reviewed changes

kevkevinpal force-pushed the fuzzwalletrpc branch from f94ae1c to 0bf4c56 Compare August 1, 2024 23:47

DrahtBot added the CI failed label Aug 2, 2024

maflcko reviewed Aug 2, 2024

View reviewed changes

kevkevinpal force-pushed the fuzzwalletrpc branch from 0bf4c56 to c994331 Compare August 31, 2024 01:49

hebasto reviewed Aug 31, 2024

View reviewed changes

src/Makefile.test.include Outdated Show resolved Hide resolved

DrahtBot removed the CI failed label Aug 31, 2024

DrahtBot mentioned this pull request Aug 31, 2024

build: Remove Autotools-based build system #30664

Merged

DrahtBot added the Needs rebase label Sep 2, 2024

kevkevinpal force-pushed the fuzzwalletrpc branch from c994331 to 352b68c Compare September 4, 2024 02:56

kevkevinpal changed the base branch from master to 24.x September 4, 2024 02:57

kevkevinpal changed the base branch from 24.x to master September 4, 2024 02:57

kevkevinpal force-pushed the fuzzwalletrpc branch 2 times, most recently from 985b8e6 to 2c8cb2b Compare September 4, 2024 03:00

DrahtBot added CI failed and removed Needs rebase CI failed labels Sep 4, 2024

DrahtBot mentioned this pull request Sep 11, 2024

Remove the legacy wallet and BDB dependency #28710

Merged

DrahtBot added the CI failed label Sep 13, 2024

kevkevinpal force-pushed the fuzzwalletrpc branch from 6ce9352 to 909aafe Compare September 13, 2024 12:01

kevkevinpal force-pushed the fuzzwalletrpc branch from 909aafe to 2b541b0 Compare September 25, 2024 13:01

test: new fuzz target wallet_rpc

26591de

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

kevkevinpal force-pushed the fuzzwalletrpc branch from 2b541b0 to 26591de Compare September 25, 2024 13:03

kevkevinpal marked this pull request as ready for review September 25, 2024 13:15

DrahtBot removed the CI failed label Sep 25, 2024

brunoerg mentioned this pull request Oct 17, 2024

Wallet fuzzing tracking issue #29901

Open

13 tasks

brunoerg reviewed Oct 25, 2024

View reviewed changes

dergoegge reviewed Oct 25, 2024

View reviewed changes

kevkevinpal closed this Nov 7, 2024

[tests] New fuzz target wallet_rpc #30570

[tests] New fuzz target wallet_rpc #30570

Uh oh!

Conversation

kevkevinpal commented Aug 1, 2024

Uh oh!

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

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

kevkevinpal Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

kevkevinpal commented Aug 1, 2024

Uh oh!

DrahtBot commented Aug 2, 2024

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

dergoegge commented Aug 2, 2024

Uh oh!

brunoerg commented Aug 2, 2024

Uh oh!

Uh oh!

DrahtBot commented Sep 13, 2024

Uh oh!

brunoerg commented Sep 24, 2024

Uh oh!

kevkevinpal commented Sep 25, 2024

Uh oh!

kevkevinpal commented Sep 25, 2024

Uh oh!

brunoerg Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

brunoerg commented Oct 25, 2024

Uh oh!

brunoerg Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

dergoegge Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

brunoerg commented Oct 25, 2024

Uh oh!

brunoerg commented Nov 5, 2024

Uh oh!

kevkevinpal commented Nov 7, 2024

Uh oh!

Uh oh!

DrahtBot commented Aug 1, 2024 •

edited

Loading