Add the descriptor argument to createwallet

[sander2]

This adds the descriptor argument to the createwallet rpc.

See the createwallet rpc info: 0.20, 0.21, 0.22, 0.23.

[apoelstra]

I'm a bit confused about the goal of this line. You are setting default_descriptors to whatever its default value would be ... so why provide it at all?

[sander2]

It was easier to determine the default value than to use a variable number of arguments. Also there are more createwallet arguments in the newest version that are not added yet (load_on_startup and external_signer), and if they're added in the future, some value will need to be passed here anyway, right?

[apoelstra]

FWIW in theory, no, it's possible to use named arguments rather than positional ones, so that we don't have to stick values in just to get to further values. But that's a bigger change and unrelated to this PR :).

It was easier to determine the default value than to use a variable number of arguments.

Okay, but simply dropping default_descriptors won't cause you to need a variable number of arguments. Right now you always pass it. I'm suggesting you never pass it.

[sander2]

@apoelstra if I don't include default_descriptors in the list of default arguments, the defaults would be applied incorrectly, right? At least if I understand the documentation of handle_defaults correctly:

/// Note, that `defaults` corresponds to the last elements of `args`.
///
/// ```norust
/// arg1 arg2 arg3 arg4
///           def1 def2

[stevenroose]

Handle defaults never touches the last argument anyway, so this will only become relevant if we add another argument here.

Also, on named arguments, my total refactor for async support migrates to using named arguments so we don't need the ugly handle_defaults stuff anymore.

[stevenroose]

To answer Andrew's concern:

I'm a bit confused about the goal of this line. You are setting default_descriptors to whatever its default value would be ... so why provide it at all?

He's only setting the default value that is filled if the actual value is not set.

[apoelstra]

Could you change this to UnsupportedArgument and add a couple &'static str param describing (a) the RPC being called, (b) the argument that isn't supported.

[sander2]

Sure, done

[gregdhill]

@apoelstra any objections to merging this PR? Right now we're blocked from upgrading the pinned version of Bitcoin Core beyond v22 because we cannot yet support descriptor wallets.

[stevenroose]

LGTM

[stevenroose]

Do you know what is wrong with the nightly test? The logs are expired, would you mind doing an empty force push to trigger re-run of the CI? Or manually check why nightly isn't working?

[apoelstra]

No objections from me.

[sander2]

Do you know what is wrong with the nightly test? The logs are expired, would you mind doing an empty force push to trigger re-run of the CI? Or manually check why nightly isn't working?

It took some trial and error, but the problem was that the selected version of serde_json requires rust 2021. I forced it now to 1.0.100 which is the last version that supported older compiler versions

[lateminer]

On L1154 is needed to add a check whether it is a descriptor wallet, as only legacy wallets have HD seeds.

For example:
let has_hd_seed = has_private_keys && !wallet_param.blank.unwrap_or(false) && !wallet_param.descriptor.unwrap_or(true);

[lateminer]

It makes sense to add an assertion in test_create_wallet():

        let has_descriptors = wallet_param.descriptor.unwrap_or(true);
        assert_eq!(wallet_info.descriptors.unwrap_or(true), has_descriptors);

(It will be also required to add descriptors field in GetWalletInfoResult struct).

[tcharding]

Added to v.020.0 milestone. FTR I'm attempting to push a quickish release out then start work on getting Core support up to date. Any and all help appreciated.