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:

• I've signed all my commits
• I followed the contribution guidelines
• 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.

[storopoli]

Some comments

[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

[evanlinjin]

I'm concerned with the way we are checking whether the 2 keychains are "distinct". I have a few questions.

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).

For 2. also refer to my comment here: #1383 (comment)

[LLFourn]

ACK 04c8750

I'm going to be away so not going to get another chance to look at this. It LGTM. Feel free to keep shedding. Thanks for taking on this rather tedious PR.

[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 358e842

[storopoli]

ACK cddefbb

[ValuedMammal]

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

[storopoli]

Totally agree with the infallible.

ACK aec4dda

Note that you might want to update the original PR message details.