Make Wallet require a change descriptor #1390

ValuedMammal · 2024-03-27T02:50:04Z

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

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 · 2024-03-27T13:49:22Z

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

crates/bdk/examples/compiler.rs

crates/bdk/src/descriptor/error.rs

crates/bdk/src/descriptor/template.rs

crates/bdk/src/wallet/export.rs

casey-bowman · 2024-03-28T07:29:24Z

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.

How can we guarantee that into_wallet_descriptor returns the same public-descriptor string for all non-distinct descriptors?
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)

crates/bdk/src/wallet/mod.rs

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.

crates/bdk/src/descriptor/error.rs

ValuedMammal · 2024-04-05T13:48:05Z

Small changes

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

ValuedMammal · 2024-04-05T14:06:49Z

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 · 2024-04-14T19:25:38Z

Rebased on 358e842

storopoli

ACK cddefbb

ValuedMammal · 2024-05-13T20:32:09Z

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.

notmandatory · 2024-06-01T21:13:56Z

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.

notmandatory

ACK aec4dda

storopoli

ACK aec4dda

crates/wallet/src/wallet/mod.rs

ValuedMammal · 2024-06-04T22:12:36Z

@storopoli I took your advice about more precise wording from this comment #1390 (comment) and I even put the variable inside the format string in 0ffcd9a :)

storopoli

ACK 0ffcd9a

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 keystores. Notable changes * Add error DescriptorError::ExternalAndInternalAreTheSame * Remove error CreateTxError::ChangePolicyDescriptor * No longer rely on `map_keychain`

…hainKind

notmandatory

re-ACK 8bc3d35

The change descriptor is made optional, making this an effective reversion of bitcoindevkit#1390 and enables creating wallets with a single descriptor.

13e7008 doc(wallet): clarify docs for `Wallet::load` (valued mammal) 3951110 fix(wallet)!: Change method `LoadParams::descriptors` (valued mammal) b802714 example(wallet): simplify miniscript compiler example (valued mammal) 2ca8b6f test(wallet): Add tests for single descriptor wallet (valued mammal) 31f1c2d fix(wallet): Change FromSql type to `Option<_>` (valued mammal) 75155b7 feat(wallet): Add method `Wallet::create_single` (valued mammal) Pull request description: The change descriptor is made optional, making this an effective reversion of #1390 and enables creating wallets with a single descriptor. fixes #1511 ### Notes to the reviewers PR 1390 also removed an error case [`ChangePolicyDescriptor`](https://github.com/bitcoindevkit/bdk/blob/8eef350bd08057acc39b6fc50b1217db5e29b968/crates/wallet/src/wallet/mod.rs#L1529-L1533) and this can possibly be added back. In the case the wallet only has a single descriptor we allow any utxos to fund a tx that aren't specifically marked unspendable regardless of the change spend policy. ### Changelog notice * Added method `Wallet::create_single` that expects a single `D: IntoWalletDescriptor` as input and enables building a wallet with no internal keychain. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### New Features: * [x] This pull request breaks the existing API * [x] I've added tests for the new feature * [x] I've added docs for the new feature ACKs for top commit: evanlinjin: ACK 13e7008 Tree-SHA512: 3e6fe5d9165d62332ac1863ec769c4bc5c7cd3c7f3fbdb8f505906bcdc681fa73b3fef2571adb0e52e9a23d4257f66a6145838b90ec68596b5f4c64054a047fa

storopoli reviewed Mar 27, 2024

View reviewed changes

ValuedMammal force-pushed the refactor/wallet-descriptor branch 2 times, most recently from edd290d to 238c292 Compare March 27, 2024 23:30

ValuedMammal marked this pull request as ready for review March 27, 2024 23:30

casey-bowman mentioned this pull request Mar 28, 2024

Wallet must have unique Internal and External descriptors #1383

Closed

ValuedMammal force-pushed the refactor/wallet-descriptor branch from 238c292 to 6c6aae8 Compare March 31, 2024 21:52

evanlinjin reviewed Apr 1, 2024

View reviewed changes

crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved

ValuedMammal force-pushed the refactor/wallet-descriptor branch from 6c6aae8 to 04c8750 Compare April 1, 2024 21:34

LLFourn approved these changes Apr 5, 2024

View reviewed changes

crates/bdk/src/descriptor/error.rs Outdated Show resolved Hide resolved

ValuedMammal force-pushed the refactor/wallet-descriptor branch 2 times, most recently from e3ba751 to cba43de Compare April 5, 2024 13:28

ValuedMammal force-pushed the refactor/wallet-descriptor branch from cba43de to cd9afd5 Compare April 10, 2024 13:22

notmandatory assigned ValuedMammal Apr 11, 2024

notmandatory added module-wallet api A breaking API change labels Apr 11, 2024

notmandatory added this to the 1.0.0-alpha milestone Apr 11, 2024

ValuedMammal force-pushed the refactor/wallet-descriptor branch from cd9afd5 to 77d2bc9 Compare April 14, 2024 19:17

ValuedMammal force-pushed the refactor/wallet-descriptor branch from 77d2bc9 to cddefbb Compare April 23, 2024 18:55

storopoli approved these changes Apr 24, 2024

View reviewed changes

This was referenced May 6, 2024

Further work on #1203 #1428

Closed

Include the descriptor in keychain::Changeset #1203

Merged

ValuedMammal force-pushed the refactor/wallet-descriptor branch from cddefbb to aec4dda Compare May 13, 2024 20:30

storopoli approved these changes May 14, 2024

View reviewed changes

notmandatory approved these changes Jun 1, 2024

View reviewed changes

storopoli approved these changes Jun 3, 2024

View reviewed changes

ValuedMammal force-pushed the refactor/wallet-descriptor branch from aec4dda to cf98f62 Compare June 3, 2024 14:24

ValuedMammal commented Jun 3, 2024

View reviewed changes

crates/wallet/src/wallet/mod.rs Outdated Show resolved Hide resolved

ValuedMammal force-pushed the refactor/wallet-descriptor branch from cf98f62 to 40c7358 Compare June 4, 2024 16:13

storopoli approved these changes Jun 5, 2024

View reviewed changes

ValuedMammal added 4 commits June 5, 2024 06:29

test(wallet): Clarify docs for get_funded_wallet

c2513e1

ref(wallet)!: Make Wallet::public_descriptor infallible

412dee1

fix(wallet): LoadError::MissingDescriptor includes the missing Keyc…

8bc3d35

…hainKind

ValuedMammal force-pushed the refactor/wallet-descriptor branch from 0ffcd9a to 8bc3d35 Compare June 5, 2024 12:42

notmandatory approved these changes Jun 6, 2024

View reviewed changes

notmandatory merged commit c5a3b62 into bitcoindevkit:master Jun 6, 2024
12 checks passed

ValuedMammal deleted the refactor/wallet-descriptor branch June 10, 2024 21:38

notmandatory mentioned this pull request Jun 14, 2024

Release 1.0.0-alpha.13 #1471

Closed

31 tasks

notmandatory mentioned this pull request Jul 11, 2024

Please unbreak single-key wallets (and potentially remove internal/external distinction entirely) #1511

Closed

storopoli mentioned this pull request Jul 14, 2024

refactor(wallet)!: make internal descriptor optional in constructors #1513

Closed

8 tasks

ValuedMammal mentioned this pull request Aug 1, 2024

[wallet] Enable support for single descriptor wallets #1533

Merged

6 tasks

oleonardolima mentioned this pull request Aug 9, 2024

Proposal: Write Architectural Decision Records #1309

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Wallet require a change descriptor #1390

Make Wallet require a change descriptor #1390

ValuedMammal commented Mar 27, 2024 •

edited

Loading

ValuedMammal commented Mar 27, 2024 •

edited

Loading

storopoli left a comment

casey-bowman commented Mar 28, 2024 •

edited

Loading

evanlinjin left a comment

LLFourn left a comment

ValuedMammal commented Apr 5, 2024

ValuedMammal commented Apr 5, 2024

ValuedMammal commented Apr 14, 2024

storopoli left a comment

ValuedMammal commented May 13, 2024

storopoli left a comment

notmandatory commented Jun 1, 2024 •

edited

Loading

notmandatory left a comment

storopoli left a comment

ValuedMammal commented Jun 4, 2024

storopoli left a comment

notmandatory left a comment

Make Wallet require a change descriptor #1390

Make Wallet require a change descriptor #1390

Conversation

ValuedMammal commented Mar 27, 2024 • edited Loading

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

ValuedMammal commented Mar 27, 2024 • edited Loading

storopoli left a comment

Choose a reason for hiding this comment

casey-bowman commented Mar 28, 2024 • edited Loading

evanlinjin left a comment

Choose a reason for hiding this comment

LLFourn left a comment

Choose a reason for hiding this comment

ValuedMammal commented Apr 5, 2024

ValuedMammal commented Apr 5, 2024

ValuedMammal commented Apr 14, 2024

storopoli left a comment

Choose a reason for hiding this comment

ValuedMammal commented May 13, 2024

storopoli left a comment

Choose a reason for hiding this comment

notmandatory commented Jun 1, 2024 • edited Loading

notmandatory left a comment

Choose a reason for hiding this comment

storopoli left a comment

Choose a reason for hiding this comment

ValuedMammal commented Jun 4, 2024

storopoli left a comment

Choose a reason for hiding this comment

notmandatory left a comment

Choose a reason for hiding this comment

ValuedMammal commented Mar 27, 2024 •

edited

Loading

ValuedMammal commented Mar 27, 2024 •

edited

Loading

casey-bowman commented Mar 28, 2024 •

edited

Loading

notmandatory commented Jun 1, 2024 •

edited

Loading