wallet, rpc: add listdescriptors command

[S3RK]

Looking for concept ACKs

Rationale: allow users to inspect the contents of their newly created descriptor wallets.

Currently the command only returns xpubs which is not very useful in itself, but there are multiples ways to extend it:

• add an option to export xprv
• with wallet: add parent_desc to getaddressinfo #19136 it'll be possible to return normalised descriptors suitable for a watch-only purposes

The output is compatible with importdescriptors command so it could be easily used for backup/recover purposes.

Output example:

[
  {
    "desc": "wpkh(tpubD6NzVbkrYhZ4WW6E2ZETFyNfq2hfF23SKxqSGFvUpPAY58jmmuBybwqwFihAyQPk9KnwTt5516NDZRJ7k5QPeKjy7wuVd5WvXNxwwAs5tUD/*)#nhavpr5h",
    "timestamp": 1296688602,
    "active": false,
    "range": [
      0,
      999
    ],
    "next": 0
  }
]

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• tests: Run both descriptor and legacy tests within a single test invocation #20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)
• rpc: Remove duplicate name and argNames from CRPCCommand #20012 (rpc: Remove duplicate name and argNames from CRPCCommand by MarcoFalke)

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.

[promag]

Before going further with this we need to ensure access to ScriptPubKeyMan instances are thread safe and decide their life cycle.

At the moment CWallet::AddWalletDescriptor mutates m_spk_managers, m_internal_spk_managers and m_external_spk_managers while also destroying a DescriptorScriptPubKeyMan.

A simple solution to fix this in this new RPC is to lock cs_wallet but I'm not sure if this is the way to go. cc @achow101

[S3RK]

@promag For now I updated the code to acquire cs_wallet which seems reasonable and I don't see better alternatives anyway.

[achow101]

Locking cs_wallet should be sufficient.

Concept ACK.

Can you add some tests for this?

[Sjors]

Concept ACK

[jonatack]

Concept ACK

[S3RK]

Added functional test

[meshcollider]

utACK 425403c modulo linter issue

[meshcollider]

re-utACK 0e31abf

[achow101]

ACK 0e31abf

[luke-jr]

Needs:

diff --git a/test/functional/wallet_listdescriptors.py b/test/functional/wallet_listdescriptors.py
index c825c8680e8..d0604d199e5 100755
--- a/test/functional/wallet_listdescriptors.py
+++ b/test/functional/wallet_listdescriptors.py
@@ -7,7 +7,10 @@
 from test_framework.descriptors import (
     descsum_create
 )
-from test_framework.test_framework import BitcoinTestFramework
+from test_framework.test_framework import (
+    BitcoinTestFramework,
+    SkipTest,
+)
 from test_framework.util import (
     assert_equal,
     assert_raises_rpc_error,
@@ -19,6 +22,8 @@ class ReceivedByTest(BitcoinTestFramework):
         self.num_nodes = 1
 
     def skip_test_if_missing_module(self):
+        if not self.options.descriptors:
+            raise SkipTest("This test only supports descriptor wallets.")
         self.skip_if_no_wallet()
 
     # do not create any wallet by default

[S3RK]

@luke-jr interesting point, thanks. I took a different approach which is more consistent with other descriptor specific tests. I made the test work regardless of whether --descriptors is set.

@achow101 There are two more tests so far that run only with --descriptors flag:

wallet_descriptor
wallet_importdescriptors

I tested and both of them would work without the flag set. I suggest that we remove that flag from test_runner.py for those tests since it doesn't affect the behaviour. What do you think?

[luke-jr]

@luke-jr interesting point, thanks. I took a different approach which is more consistent with other descriptor specific tests. I made the test work regardless of whether --descriptors is set.

But as-is, the test FAILS without --descriptors set...?

[achow101]

I tested and both of them would work without the flag set. I suggest that we remove that flag from test_runner.py for those tests since it doesn't affect the behaviour. What do you think?

I have planned improvements for that, namely to not make tests dependent on --descriptors.

But as-is, the test FAILS without --descriptors set...?

It does not fail without --descriptors. It basically just ignores --descriptors and --legacy-wallet as a couple of tests already do.

In the interest of getting this merged, I think it is fine to leave the test as is and resolve the --descriptors problem with the similar tests together.

[achow101]

re-ACK bbb34e9

[jonatack]

Tested ACK bbb34e9 rebased to master

A few comments below. Happy to re-ACK quickly if you update.

[jonatack]

re-ACK 647b81b rebased to master, debug builds cleanly, reviewed diff since last review, tested with a descriptor wallet (and with a legacy wallet)

help output

listdescriptors

List descriptors imported into a descriptor-enabled wallet.
Result:
[                               (json array) Response is an array of descriptor objects
  {                             (json object)
    "desc" : "str",             (string) Descriptor string representation
    "timestamp" : n,            (numeric) The creation time of the descriptor
    "active" : true|false,      (boolean) Activeness flag
    "internal" : true|false,    (boolean, optional) Whether this is internal or external descriptor; defined only for active descriptors
    "range" : [                 (json array, optional) Defined only for ranged descriptors
      n,                        (numeric) Range start inclusive
      n                         (numeric) Range end inclusive
    ],
    "next" : n                  (numeric, optional) The next index to generate addresses from; defined only for ranged descriptors
  },
  ...
]

Examples:
> bitcoin-cli listdescriptors 
> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "listdescriptors", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/

[achow101]

re-ACK 647b81b

Changes since last just address the review comments.

[jonatack]

Needs a release note (can be done in a follow-up.)

[meshcollider]

re-utACK