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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

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:

• #28710 (Remove the legacy wallet and BDB dependency by achow101)
• #28574 (wallet: optimize migration process, batch db transactions by furszy)

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.

Feels like this could be confused with real/beancounter accounts like we used to support years ago. Maybe hd_account or something?

I think this might be a better candidate for a createwalletdescriptor (#29130) equivalent for external signers rather than continuing to jam more arguments into createwallet.

Feels like this could be confused with real/beancounter accounts like we used to support years ago. Maybe hd_account or something?

I agree, had same thought. Just changed in latest push.

I think this might be a better candidate for a createwalletdescriptor (#29130) equivalent for external signers rather than continuing to jam more arguments into createwallet.

You mean have a specific RPC for external signers or extending createwalletdescriptor?

You mean have a specific RPC for external signers or extending createwalletdescriptor?

Unsure of the final design, but something like createwalletdescriptor for external signers seems like it would be useful, and having an account argument in that would probably also make sense. Actually, that might make sense for createwalletdescriptor itself.

re: #29129 (comment)

I think this might be a better candidate for a createwalletdescriptor (#29130) equivalent for external signers rather than continuing to jam more arguments into createwallet.

This makes sense, and I added this idea to my list of potential createwalletdescriptor extensions: #29130 (comment)

But in general, as long as the createwallet blank option defaults to false and it creates descriptors by default, probably we should expect createwallet and createwalletdescriptor to accept some of the same options (and hopefully not duplicate the code implementing those options internally).

In this case, I'm not sure if it will be more common to have single bitcoin core wallets with multiple hd_accounts, or multiple bitcoin core wallets with single hd_accounts? In the latter case, it really does make sense to add the option to createwallet. And again as long as blank is not false, it seems like it would be useful to merge this PR to be able to control the descriptors that createwallet will add.

Also agree very much with luke-jr that it would be good not to allow this option to be passed by position, and instead to add an OBJ_NAMED_PARAMS options parameter instead. It probably makes sense to have a separate PR disallowing the other options from being passed by position as well, especially since 6 of the 7 options are bool values, so very easy to confuse if passed by position.

Also agree very much with luke-jr that it would be good not to allow this option to be passed by position, and instead to add an OBJ_NAMED_PARAMS options parameter instead. It probably makes sense to have a separate PR disallowing the other options from being passed by position as well, especially since 6 of the 7 options are bool values, so very easy to confuse if passed by position.

I strongly agree with that. I could turn this draft until we've that.

Closing for now.