Use explicit names for wallet builder methods that validate rather than set

[thunderbiscuit]

This PR is a follow up to the dev call discussion where we decided it was better to PR these changes separate from #1533. This is a breaking change, but I think it's worth it because those will potentially cause runtime errors for users that expect one thing to happen and realize it's something else.

Left out of this PR but as surfaced in the call probably worth discussing is whether these methods make sense at all or whether they should be removed entirely. What does it mean to return an error when someone loads a wallet from persistence for which the genesis hash doesn't match the one persisted? Maybe worth a new issue; this PR simply attempts to name them correctly.

Description

See Q1 in comment here for more context into the initial question.

Two of the methods on the builder that loads a wallet were checkers/validators rather than setters:

• network()
• genesis_hash()

This is confusing for users because when loading a wallet from persistence those things are persisted and cannot be changed, and yet seemingly the option to do that is there with those methods (so now you're re-thinking what you think you know about the changeset and persistence). Moreover, the fields on the LoadParams type are correctly named check_network and check_genesis_hash. This PR simply brings those two methods in line with what they actually do and set on the struct they modify.

This modification was not done on the descriptors() method, because that one is both a validator and a setter if the descriptors passed contain private keys.

Note that I chose the names validate_network and validate_genesis_hash but I'm not married to them. If others prefer check_network and check_genesis_hash, I'm happy to fix them that way instead!

Changelog notice

Breaking:
  - The `LoadParams` type used in the wallet load builder renamed its  `network()` and `genesis_hash` methods to `check_network()` and `check_genesis_hash`. [#1537]

[#1537]: https://github.com/bitcoindevkit/bdk/pull/1537

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[ValuedMammal]

ACK d0b2bf0

[notmandatory]

⛏️ One more nit, can you change the commit message so it fits on one line, and this is a refactor (no new functionality) not a feature, so something like:

refactor(wallet)!: rename LoadParams to check_network and check_genesis_hash

If you want to add more descriptive / editorial on why these changes are made you can put that after a blank line in the commit message. Some good guidance here: https://www.conventionalcommits.org/en/v1.0.0/#specification

Going forward I want to be more strict for all PRs, especially about what is a breaking change refactor and what's a new breaking or non-breaking feature.

[evanlinjin]

I have an idea to make the API more understandable (hopefully). We can rename this method to check_descriptors and add a boolean input extract_keys: bool. This way the caller would know that this method is intended to check descriptors. However, if they also wish to extract keys here, they can.

[evanlinjin]

We will have to change the signature of LoadParams though.

pub struct LoadParams {
    pub(crate) check_descriptor: Option<(DescriptorToExtract, bool)>,
    pub(crate) check_change_descriptor: Option<(DescriptorToExtract, bool)>,
    // ...
}

where the bool is whether to extract keys for signers.

[ValuedMammal]

I'm actually fine with the current behavior to automatically extract keys. One thing that's still unclear to me is why we have both descriptors and keymap options

[evanlinjin]

@ValuedMammal

In terms of why keymap is there, I'm assuming that most people would store private keys as private keys (instead of as a private-key-descriptor). Or, are you saying, because we already have Wallet::set_keymap and associated methods, so we do not need LoadParams::keymap method?

In terms of why we want descriptors to also extract private keys, is because, only allowing public-key-descriptor input is deemed too "breaking" for our users (since the old behavior allowed inputting a secret-key-descriptor while loading).

[ValuedMammal]

yes referring to both methods on LoadParams specifically, considering they do nearly the same thing. I wondered if there was a deeper reason - like having the ability to sign txs with keys that don't actually belong to the wallet.

[evanlinjin]

That's an interesting thought. I don't think it is an intentional feature. P.S. but it might be useful.

Here are my current thoughts on the topic:

We need a keymap method on LoadParams, otherwise people who store secrets as raw keys need to re-craft secret-key descriptor(s) from their keys (which makes a bad API).

We need a descriptor method because we want the caller to be able to check the descriptor against what is loaded.

I also think the main intention of the descriptor method should be to check the loaded descriptor. Extracting the secret keys should be a "side quest". Therefore, I think my original proposal makes sense (to rename to check_descriptor and have a boolean to extract_secrets).

[ValuedMammal]

In this scenario would it be an error to pass in true for extract-keys if the descriptor only has an xpub? (oh, maybe just call it try_extract)

[evanlinjin]

@ValuedMammal that's a good point. I think it's okay just to ignore that fact? "There just aren't any keys to extract, so we do nothing" seems sound?

[thunderbiscuit]

Continuing the conversation here @evanlinjin @ValuedMammal.

I like the idea of the extract_secrets boolean! It's explicit, and that way the API forces users to grapple with exactly what is happening with persistence/load (we don't persist secret keys, so if you want a wallet that can sign on reload you'll need your private keys). Not only that, but we don't load your private keys by default, you'll need to ask for it. I think this approach is great, and will push changes to that effect.

• I like how Evan described extracting the secret keys as a side-quest. This is likely to be the main thing people use it for however, so the name check_descriptors() feels to me not quite aptly named: it's a check but you also load private keys? I find descriptors() to be a little more broad, if less precise because it covers both uses.
• I would return an error in case people feed xpubs to an API while also asking it to extract secret keys; it's a sign they misunderstand the API and will save them heartache later on (if they try to sign a tx but only had put xpubs in their load builder while asking to extract secrets and the API silently rerouted it to extracting nothing).

Last question: @evanlinjin wrote:

In terms of why keymap is there, I'm assuming that most people would store private keys as private keys (instead of as a private-key-descriptor).

What's the workflow for this? I have not even thought of that. What format are these keys in that people save them as keymaps? Would love to understand our broad expected uses/workflows. As I saw it, there were two so far (but let me know what this 3rd one is):

1. People have recovery phrases saved. When loading a wallet, the application layer must transform these into private (or public) descriptors, and hand them out to the wallet.
2. People have descriptor strings (private or public) saved. You simply give them out to the wallet when initializing.
3. ? you have keymaps stored somewhere? Or do you go directly from your recovery phrase to a keymap?

[thunderbiscuit]

Other corner case we need to be able to handle is when people give one public descriptor and one private. I assume that would cause all sorts of problems for the wallet (say you provided a public descriptor for your external keychain and a private one for your internal one)?

I suggest throwing an error for that as well. Also means you need to set the extract_keys boolean to the same value for both. How about a separate method on the builder to set that?

[notmandatory]

This PR is overridden by #1533 right?

[notmandatory]

Other corner case we need to be able to handle is when people give one public descriptor and one private. I assume that would cause all sorts of problems for the wallet (say you provided a public descriptor for your external keychain and a private one for your internal one)?

This shouldn't cause any big problems except you won't be able to sign a TX using the missing private descriptor's private keys. This has always been possible and no users have been complaining about it so I don't think we need to add special error handling at this point. But this is a good detail to explain in docs and examples.

[thunderbiscuit]

I kept out all changes that need more discussion and can most definitely wait for a next release. This PR really only does 2 things:

• Rename LoadPrams::network() -> LoadParams::check_network()
• Rename LoadPrams::genesis_hash() -> LoadParams::check_genesis_hash()

Rebased and ready to go!

[notmandatory]

ACK 2391b76