wallet, rpc: add BIP44 account in createwallet #29129

1. 
   brunoerg commented at 1:31 pm on December 21, 2023: contributor

   This PR adds an account parameter in createwallet RPC. It’s the BIP44 account that will be used in SetupDescriptorScriptPubKeyMans to fetch the descriptors from the external signer.
2. 
   DrahtBot commented at 1:31 pm on December 21, 2023: contributor

   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.

   Type	Reviewers
   Concept ACK	BrandonOdiwuor

   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.

3. DrahtBot renamed this:
   wallet, rpc: add BIP44 `account` in `createwallet`
   wallet, rpc: add BIP44 `account` in `createwallet`
   on Dec 21, 2023
4. brunoerg force-pushed on Dec 21, 2023
5. brunoerg referenced this in commit 523ad34656 on Dec 21, 2023
6. brunoerg marked this as ready for review on Dec 21, 2023
7. 
   BrandonOdiwuor commented at 5:08 am on December 22, 2023: contributor

   Concept ACK
8. 
   luke-jr commented at 11:08 pm on December 26, 2023: member

   Feels like this could be confused with real/beancounter accounts like we used to support years ago. Maybe hd_account or something?
9. wallet: add `hd_account` parameter in `SetupDescriptorScriptPubKeyMans`/`CreateWallet` dec25cc521
10. wallet, rpc: add `hd_account` in `createwallet`

    The `hd_account` parameter is the BIP44 account
    used to get the descriptors from the external
    signer.

    c11a6ab516
11. test: add coverage for `hd_account` parameter from `createwallet` RPC a0fe82801e
12. docs: add release note for #29129 911eba3f7a
13. brunoerg force-pushed on Dec 27, 2023
14. 
    achow101 commented at 1:22 am on December 27, 2023: member

    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.
15. 
    brunoerg commented at 1:22 am on December 27, 2023: contributor

    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.

16. 
    brunoerg commented at 1:25 am on December 27, 2023: contributor

    I think this might be a better candidate for a createwalletdescriptor (https://github.com/bitcoin/bitcoin/pull/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?

17. luke-jr changes_requested
18. 
    luke-jr commented at 8:19 pm on January 4, 2024: member

    Beyond wallet_name, none of the positional parameters make sense as positional, and that goes for this new one too. Migrating this to an options object should probably be done sooner rather than after making the problem worse
19. 
    achow101 commented at 8:43 pm on January 4, 2024: member

    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.

20. 
    ryanofsky commented at 0:01 am on January 10, 2024: contributor

    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 (review)

    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.

21. DrahtBot added the label CI failed on Jan 15, 2024
22. 
    brunoerg commented at 4:24 pm on January 22, 2024: contributor

    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.

23. brunoerg marked this as a draft on Feb 5, 2024
24. 
    brunoerg commented at 3:24 pm on July 16, 2024: contributor

    Closing for now.
25. brunoerg closed this on Jul 16, 2024

    ---

Contributors
brunoerg DrahtBot BrandonOdiwuor luke-jr achow101 ryanofsky

Labels
CI failed