Slight rethinking of Wallet constructors (breaking change)

[thunderbiscuit]

Why This PR

Our current "constructors" on the Wallet type are as follow:

1. Wallet::new()
2. Wallet::new_with_genesis_hash()
3. Wallet::load_from_changeset()
4. Wallet::new_or_load()
5. Wallet::new_or_load_with_genesis_hash()

This PR proposes a revised set of constructors offering clearer boundaries between them and their intended use:

1. Wallet::new()
2. Wallet::new_with_genesis_hash()
3. Wallet::load()
4. Wallet::load_from_changeset() -> renamed load_no_priv()

A few observations on our current set of constructors and what led me to propose this PR:

• Wallet.new_or_load(). So... do you want to create a new wallet or load an existing one? It feels like you should know. If you don't know whether you're about to create a new wallet or load one from persistence, to me your issue lies upstream of this line in your code (but maybe there is a use case I'm not thinking of? let me know).
• The new_or_load_with_genesis_hash() is sort of named backwards (you don't load with a specific genesis hash, you can only do that on a new wallet). The clearer name for it would be new_with_genesis_hash_or_load().
• Loading a wallet is always done with a changeset; there is no other way to do it. In this sense, Wallet.load_from_changeset() is not optimally named. In fact, the name is also missing the core feature of the constructor: your wallet will not have private keys. An extra commit on this PR renames this method to load_no_priv(); feel free to bikeshed on the name I don't know that it's a particular good one yet and we can also just take that commit out if people like the load_from_changeset name.

Note that this PR doesn't touch the inner logic of the constructors. I simply renamed the methods, removed the Option on the changeset when calling the load methods, and fixed up the tests to break apart the behaviour tested. It did require removing the NewOrLoadError and adding the required variants to the LoadError.

Changelog notice

Breaking
    - The Wallet.new_or_load() method was removed [#1500]
    - The Wallet.load_from_changeset() method was renamed Wallet.load_no_priv() [#1500]
 
Added
    - A new method Wallet.load() was added to construct wallets [#1500]

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

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

[LLFourn]

I agree this API doesn't make sense anymore after the persistence backend was remove. IMO there should just be a single new function that takes a bdk_wallet::ChangeSet.

You can have methods on bdk_wallet::ChangeSet to create an initialization ChangeSet i.e. from two descriptors and a Network and another one for a specific genesis hash. Then you persist that and load the wallet with it.

I'd do this in a PR that moves bdk_chain::CombinedChangeSet to bdk_wallet::ChangeSet and then tries to leverage the control you have over it in this crate to create a seamless new function.

[thunderbiscuit]

A single new function that takes a bdk_wallet::ChangeSet.

Ok this is quite a departure from what we have. My initial reaction was to think that it might be odd to have to users dig into the idea of changesets before they've even made a first wallet ("darn, surely this Wallet::new method could create what I need internally?" says the newbie), but after sitting on it for 30 minutes I think you can't get away from changesets anyway, so might as well use them to their fullest.

It's more refactoring than I have here so before I get going on this it might be nice to get a Concept ACK from a few other devs on the team, but I'm up for it if that appears to be the cleanest and better way to move forward.

A few questions

1. The current CombinedChangeSet would need to somehow hold the private keys now, but it doesn't at the moment if I understand the the indexed_tx_graph field well

pub indexed_tx_graph: crate::indexed_tx_graph::ChangeSet<A, crate::keychain::ChangeSet<K>>,

with the keychain::ChangeSet

pub struct ChangeSet<K> {
    /// Contains the keychains that have been added and their respective descriptor
    pub keychains_added: BTreeMap<K, Descriptor<DescriptorPublicKey>>,
    /// Contains for each descriptor_id the last revealed index of derivation
    pub last_revealed: BTreeMap<DescriptorId, u32>,
}

Did I misunderstand your idea or maybe misunderstand the type and it does in fact have the descriptors somewhere in there that the wallet can then use to populate signers?

2. The move of bdk_chain::CombinedChangeSet -> bdk_wallet::CombinedChangeSet is being worked on by @evanlinjin as part of Move CombinedChangeSet back to bdk_wallet::ChangeSet #1496 right? Should I just build on that Evan or is it too unstable and better to wait for it to be merged?

[notmandatory]

1. The current CombinedChangeSet would need to somehow hold the private keys now, but it doesn't at the moment if I understand the the indexed_tx_graph field well

I'm warming up to this idea for simplifying the Wallet constructors too, but we should not add private descriptors to the change set. First off we can add new() and new_with_genesis_hash() functions to the WalletChangeSet that @evanlinjin is working on now. Those constructors can return the WalletChangeSet in a tuple with a SignerContainer. The SignerContainer would contain signers if the descriptors happen to have private keys and otherwise is empty.

[ValuedMammal]

it might be odd to have to users dig into the idea of changesets before they've even made a first wallet

I tend to agree it seems onerous to place handling the changeset structure before creating a wallet. But I also don't hate the idea of having one new constructor that takes a changeset parameter. Given the current state of things, I would go for something like this

  fn new(desc: E, change_desc: E, network: Network) {}
  fn new_with_genesis_hash(desc: E, change_desc: E, network: Network, gen: BlockHash) {}
  fn load(cs: ChangeSet) {}
  fn load_with_descriptors(cs: ChangeSet, desc: E, change_desc: E) {}

With load it's understood that no signing keys are present. load_with_descriptors would populate signers if the keys are available. Or we just have load and make the user create their own signers container and add them with add_signers

[notmandatory]

@ValuedMammal your approach is simpler for users and with docs and example should be easy to explain which Wallet constructor to use when. If you haven't already started one I'll work up some code for this API.

[evanlinjin]

I like @ValuedMammal's solution. However, I would have load_with_keymaps to avoid having to create error variants of "conflicting descriptors".

What if you want to create custom signers? You can still add signers later on with add_signer (but I think it's a bad idea). It makes more sense to just sign the psbt directly.

[notmandatory]

I like @ValuedMammal's solution. However, I would have load_with_keymaps to avoid having to create error variants of "conflicting descriptors".

I'd rather not make another big change to an API that bdk_wallet users are already familiar with. The load_with_descriptors constructor will fit easily for anyone who already builds their wallet with private key containing descriptors.

I'd be happy to see a complete Wallet make-over for 2.0 to enable [1..n] multi-descriptor wallets, decouple TxBuilder and coin selection, and maybe even your suggestion to only sign PSBTs outside of Wallet.

[evanlinjin]

In #1514, I've introduced a builder API for constructing a Wallet.

// Load a wallet that is persisted to SQLite database.
let mut conn = bdk_wallet::rusqlite::Connection::open(file_path)?;
let mut wallet = Wallet::load()
    // check loaded descriptors matches these values and extract private keys
    .descriptors(EXTERNAL_DESC, INTERNAL_DESC)
    // you can also manually add private keys
    .keymap(KeychainKind::External, external_keymap)
    .keymap(KeychainKind::Internal, internal_keymap)
    // ensure loaded wallet's genesis hash matches this value
    .genesis_hash(genesis_hash)
    // set a lookahead for our indexer
    .lookahead(101)
    .load_wallet(&mut conn)?
    .expect("must have data to load wallet");

// Create a non-persisted wallet.
let wallet = Wallet::create(EXTERNAL_DESC, INTERNAL_DESC)
    .network(Network::Testnet)
    .create_wallet_no_persist()?;

[notmandatory]

closing in favor of #1514