Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the descriptor argument to createwallet #278

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sander2
Copy link
Contributor

@sander2 sander2 commented Feb 17, 2023

This adds the descriptor argument to the createwallet rpc.

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

opt_into_json(descriptors)?,
];
// from 23 on, the default value of the descriptors argument is true
let default_descriptors = self.version()? >= 230000;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -88,6 +90,9 @@ impl fmt::Display for Error {
Error::InvalidCookieFile => write!(f, "invalid cookie file"),
Error::UnexpectedStructure => write!(f, "the JSON result had an unexpected structure"),
Error::ReturnedError(ref s) => write!(f, "the daemon returned an error string: {}", s),
Error::Unsupported => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done

@sander2 sander2 force-pushed the feat/create-wallet-descriptor-arg branch 2 times, most recently from d314293 to 671a78f Compare July 19, 2023 14:53
@gregdhill
Copy link
Contributor

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

Copy link
Collaborator

@stevenroose stevenroose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stevenroose
Copy link
Collaborator

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
Copy link
Member

No objections from me.

@sander2 sander2 force-pushed the feat/create-wallet-descriptor-arg branch 3 times, most recently from 63ea85d to 80f9941 Compare October 23, 2023 13:43
@sander2 sander2 force-pushed the feat/create-wallet-descriptor-arg branch from 80f9941 to 6576be9 Compare October 23, 2023 13:50
@sander2 sander2 force-pushed the feat/create-wallet-descriptor-arg branch from 951b4b1 to ad106c2 Compare October 23, 2023 14:26
@sander2
Copy link
Contributor Author

sander2 commented Oct 23, 2023

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

lateminer added a commit to CoinBlack/rust-blackcoinmore-rpc that referenced this pull request Nov 20, 2023
@tcharding tcharding added this to the v0.20.0 milestone Apr 30, 2024
@tcharding
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants