Make Wallet require a change descriptor

[ValuedMammal]

All Wallet constructors are modified to require a change descriptor, where previously it was optional. Additionally we enforce uniqueness of the change descriptor to avoid ambiguity when deriving scripts and ensure the wallet will always have two distinct keychains.

Notable changes

• Add error DescriptorError::ExternalAndInternalAreTheSame
• Remove error CreateTxError::ChangePolicyDescriptor
• No longer rely on map_keychain

fixes #1383

Notes to the reviewers

Changelog notice

Changed:

Constructing a Wallet now requires two distinct descriptors.

Checklists

All Submissions:

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

[ValuedMammal]

Do we want to keep the change descriptor optional for example_cli? I'm guessing it won't hurt anything since we don't actually use the wallet.

[casey-bowman]

If two descriptors are the same except that each has a different extended public key that is replaced by the corresponding extended private key, do these qualify as distinct?

Also, to clarify, the receive descriptors for any two wallets must also be distinct with this change, too, correct? UPDATE - Oh, I see the issue #1383

[ValuedMammal]

Small changes

• Fixed key_* names in descriptor template docs
• Changed the new descriptor error to just ExternalAndInternalAreTheSame

[ValuedMammal]

1. How can we guarantee that `into_wallet_descriptor` returns the same public-descriptor string for all non-distinct descriptors?

2. How can we ensure that a non-wildcard descriptor and a wildcard descriptor has no overlaps (the non-wildcard descriptor's spk can be derived from the wildcard descriptor).

To point 1) I don't think you're objecting to checking equality for two values of Descriptor<DescriptorPublicKey>, which is only true for identical descriptors, i.e. ones that derive 100% of the same scripts. Maybe this was addressed in the discussion around point 2?

Note that the public descriptor will be common whether the wallet is given a public or private descriptor. So for example this should pass:

#[test]
fn same_descriptor_public_private() {
    // public + private of same descriptor should fail to create wallet
    let desc = "wpkh(tprv8ZgxMBicQKsPdcAqYBpzAFwU5yxBUo88ggoBqu1qPcHUfSbKK1sKMLmC7EAk438btHQrSdu3jGGQa6PA71nvH5nkDexhLteJqkM4dQmWF9g/84'/1'/0'/0/*)";
    let change = "wpkh([3c31d632/84'/1'/0']tpubDCYwFkks2cg78N7eoYbBatsFEGje8vW8arSKW4rLwD1AU1s9KJMDRHE32JkvYERuiFjArrsH7qpWSpJATed5ShZbG9KsskA5Rmi6NSYgYN2/0/*)";

    let err = Wallet::new_no_persist(desc, change, Network::Testnet);
    assert!(matches!(
        err,
        Err(DescriptorError::ExternalAndInternalAreTheSame),
    ));
}

[ValuedMammal]

Rebased on 358e842dcda0eb867d229881823c76dba1d7cce1

[ValuedMammal]

Rebased on 7607b492 and added two commits, but this may need more discussion and testing to decide if it's a reasonable solution

[notmandatory]

Note that the public descriptor will be common whether the wallet is given a public or private descriptor. So for example this should pass:

#[test]
fn same_descriptor_public_private() {
    // public + private of same descriptor should fail to create wallet
    let desc = "wpkh(tprv8ZgxMBicQKsPdcAqYBpzAFwU5yxBUo88ggoBqu1qPcHUfSbKK1sKMLmC7EAk438btHQrSdu3jGGQa6PA71nvH5nkDexhLteJqkM4dQmWF9g/84'/1'/0'/0/*)";
    let change = "wpkh([3c31d632/84'/1'/0']tpubDCYwFkks2cg78N7eoYbBatsFEGje8vW8arSKW4rLwD1AU1s9KJMDRHE32JkvYERuiFjArrsH7qpWSpJATed5ShZbG9KsskA5Rmi6NSYgYN2/0/*)";

    let err = Wallet::new_no_persist(desc, change, Network::Testnet);
    assert!(matches!(
        err,
        Err(DescriptorError::ExternalAndInternalAreTheSame),
    ));
}

This test does pass and I think is an adequate level of error checking. If we run into cases where users are adding duplicates descriptors that aren't caught we can add more sophisticated checking later and it won't break the API.

It's good documentation to add this test to tests/wallet.rs and I think you need a rebase and then this one looks ready to merge.

[ValuedMammal]

@storopoli I took your advice about more precise wording from this comment https://github.com/bitcoindevkit/bdk/pull/1390#discussion_r1541215579 and I even put the variable inside the format string in 0ffcd9a1cf376ef01b1c32f006aafa944c7fda2d :)