test: Remove legacy wallet RPC overloads

achow101 commented at 10:36 pm on May 8, 2025: member

RPCOverloadWrapper implemented overloads for legacy wallet only RPCs so that the same function call could be used within tests for both legacy wallets and descriptor wallets. With legacy wallets now removed, there is no need to continue to have these overloads.

For importaddress, addmultisigaddress, and importpubkey, the uses of these are converted to importdescriptors.

For importprivkey, a new helper function wallet_imporprivkey is introduced that does what the overload did. This is mainly to reduce verbosity as importprivkey was more widely used throughout the tests.

Some tests that used these RPCs are now also no longer relevant and have been removed.

DrahtBot commented at 10:36 pm on May 8, 2025: 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/32452.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, pablomartin4btc, rkrux, w0xlt

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:

#32290 (test: allow all functional tests to be run or skipped with –usecli by mzumsande)
#31668 (Added rescan option for import descriptors by saikiran57)
#29136 (wallet: addhdkey RPC to add just keys to wallets via new unused(KEY) descriptor by achow101)
#29032 (signet: omit commitment for some trivial challenges by Sjors)

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 May 8, 2025

DrahtBot added the label CI failed on May 9, 2025

DrahtBot commented at 0:13 am on May 9, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/41906136841 LLM reason (✨ experimental): The CI failure is due to linting errors identified by ruff and codespell.

Try to run the tests locally, according to the documentation. However, a CI failure may still 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.

Sjors commented at 10:56 am on May 9, 2025: member

Concept ACK. 2d8679c45107598689cc2af90e6324fa9172b6d3 looks reasonable, though linter is unhappy.

Maybe move b4f5b4e37d29c1a516186775736efda9d77c2fa9 “test: Remove unnecessary importprivkey from wallet_createwallet” to the top and then combine “Add wallet_importprivkey helper for replacing importprivkey” with the commit that replaces the usage. And then also drop importprivkey from the RPCOverloadWrapper.

Similarly for the later commits, it’s more clear if you drop one RPCOverloadWrapper function at a time while you replace its usage.

test: Remove unnecessary importprivkey from wallet_createwallet

This test was testing importprivkey behavior in a legacy wallet without
private keys. As legacy wallets no longer exist, this test case is no
longer relevant.

94c87bbbd0

test: Replace importprivkey with wallet_importprivkey

importprivkey was a legacy wallet only RPC which had a helper for
descriptor wallets in tests. Add wallet_importprivkey helper and use it
wherever importprivkey is used (other than backward compatibility tests)

fcc457573f

test: Replace usage of importaddress d314207779

achow101 force-pushed on May 9, 2025

achow101 commented at 7:06 pm on May 9, 2025: member

Fixed the linter and reorganized as suggested.

test: Replace usage of addmultisigaddress fe838dd391

test: Replace importpubkey 4d32c19516

test: Remove RPCOverloadWrapper

With the legacy wallet now removed, these overloads are no longer
needed.

b104d44227

achow101 force-pushed on May 9, 2025

DrahtBot removed the label CI failed on May 9, 2025

fanquake added this to the milestone 30.0 on May 12, 2025

Sjors approved

Sjors commented at 2:54 pm on May 12, 2025: member

Much better.

ACK b104d442277090337ce405d92f1398b7cc9bcdb7

pablomartin4btc commented at 9:10 pm on May 12, 2025: member

cr ACK b104d442277090337ce405d92f1398b7cc9bcdb7

Commit reorg proposed by @Sjors made sense.

On first commit, test: Remove unnecessary importprivkey from wallet_createwallet, the reasoning is more that importprivkey was only compatible with legacy wallets.

On the second commit, test: Replace importprivkey with wallet_importprivkey, I understand the practicality of having the helper due to verbosity but the name could be more related with “import descriptor” to avoid confusion with the deprecated command?

in test/functional/wallet_createwallet.py:48 in 94c87bbbd0 outdated

44@@ -45,7 +45,6 @@ def run_test(self):
45 
46         self.log.info('Test that private keys cannot be imported')
47         privkey, pubkey = generate_keypair(wif=True)
48-        assert_raises_rpc_error(-4, 'Cannot import private keys to a wallet with private keys disabled', w1.importprivkey, privkey)

rkrux commented at 11:28 am on May 16, 2025:

Mentioning for clarity: Fine to remove but as this test stood before this PR, w1 doesn’t seem to be a legacy wallet as I can see descriptors argument was not set to false in its creation. Maybe it was in some older diff.

achow101 commented at 6:05 pm on May 16, 2025:

With legacy wallets, this tested the importprivkey RPC. With descriptor wallets, it tests importdescriptors via the wrapper. But importdescriptors already has an explicit test on the next line, so this line is unnecessary.

in test/functional/test_framework/util.py:614 in fcc457573f outdated

609@@ -609,3 +610,13 @@ def sync_txindex(test_framework, node):
610     sync_start = int(time.time())
611     test_framework.wait_until(lambda: node.getindexinfo("txindex")["txindex"]["synced"])
612     test_framework.log.debug(f"Synced in {time.time() - sync_start} seconds")
613+
614+def wallet_importprivkey(wallet_rpc, privkey, timestamp, *, label=""):

rkrux commented at 1:38 pm on May 16, 2025:

Some function documentation here could prove to be helpful for others who are navigating the tests. Might even encourage to create more such helper/wrapper functions as needed.

rkrux commented at 1:44 pm on May 16, 2025: contributor

ACK b104d442277090337ce405d92f1398b7cc9bcdb7

RPCOverloadWrapper did catch my eye and made me wonder why it was added in the first place when I reviewed #32360 as it added an additional layer for calling the RPCs in the test framework - good that it can be removed after removal of legacy wallets, makes it slightly easier to track the flow in the framework.

On the second commit, test: Replace importprivkey with wallet_importprivkey, I understand the practicality of having the helper due to verbosity but the name could be more related with “import descriptor” to avoid confusion with the deprecated command?

I think the name is chosen so for contextualisation while abstracting away the fact it’s using importdescriptors underneath.

w0xlt commented at 10:20 pm on May 16, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/32452/commits/b104d442277090337ce405d92f1398b7cc9bcdb7

fanquake merged this on May 17, 2025

fanquake closed this on May 17, 2025

test: Remove legacy wallet RPC overloads #32452

Code Coverage & Benchmarks

Reviews

Conflicts