Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BIP 388: Wallet Policies for Descriptor Wallets #1389

Merged
merged 7 commits into from
May 8, 2024

Conversation

bigspider
Copy link
Contributor

@bigspider bigspider commented Nov 16, 2022

Initial version posted to bitcoin-dev in May.

Wallet policies are implemented in the Ledger bitcoin app (since version 2.1.0).

The following PR experimenting with an HWI integration might provide more context and an end-to-end demo:

Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to see progress on this! Just a small comment about only mentioning Bitcoin Core: i know of several projects using descriptors, and even more of them using descriptors without Miniscript.

bip-wallet-policies.mediawiki Outdated Show resolved Hide resolved

A more native, compact representation of the wallet receive/change might also benefit the UX of software wallets using descriptors to represent software wallets using descriptors (possibly with miniscript) for complex locking conditions.

We remark that wallet policies are not related to the ''policy'' language, a higher level language that can be compiled to miniscript.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still open to suggestions for a less-ambiguous name!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the suggestions below to reduce/remove malleability are taken, I'd suggest wallet patterns as these strings describe the pattern of derivation without including the keys.

Copy link

@joostjager joostjager May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if signing policy/pattern as a name can be useful too. Thinking of constraining outputs to spend to a predefined wallet, which isn't necessarily the wallet that is being spend from.

For example a policy that only allows spending to cold storage addresses derived from a pre-registered xpub.

@bigspider
Copy link
Contributor Author

In dfa2645, I added the possibility of using unhardened derivation steps in the key origin information (while keeping the restriction for change/address_index in the key placeholders).
While most wallets use hardened derivations for all but the last two steps, there are deployed use cases with unhardened derivations, and this only adds negligible implementation complexity to wallet policies.

jgriffiths added a commit to ElementsProject/libwally-core that referenced this pull request Feb 13, 2023
Policies are simply a restriction on the general variable substitution
mechanism that wally already supports; (a) the variables can only be of
the form @n where n is an integer, and (b) the substitution data must be
an xpub.

See bitcoin/bips#1389

WIP: Requires tests and documentation (and the BIP to be accepted).
jgriffiths added a commit to ElementsProject/libwally-core that referenced this pull request Feb 13, 2023
Policies are simply a restriction on the general variable substitution
mechanism that wally already supports; (a) the variables can only be of
the form @n where n is an integer, and (b) the substitution data must be
an xpub.

See bitcoin/bips#1389

WIP: Requires tests and documentation (and the BIP to be accepted).
bip-wallet-policies.mediawiki Outdated Show resolved Hide resolved
bip-wallet-policies.mediawiki Outdated Show resolved Hide resolved
bip-wallet-policies.mediawiki Outdated Show resolved Hide resolved

TODO: reference BIP-389 for multipath descriptors (not yet merged into the bips repository at time of writing).

The <tt>/**</tt> in the placeholder template represents commonly used paths for receive/change addresses, and is equivalent to <tt><0;1>/*</tt>.
Copy link

@jgriffiths jgriffiths Feb 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While /** is shorter, it also looks to be easy to mistype/confuse with /*. Compare with something like /+ or /++ for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer /** as it has a similar meaning in other languages (and was used in BIP-0129 for almost the same meaning).

As the /* alone is invalid for wallet policies, I don't see the risk of confusing it as very dangerous.

It's been pointed out in the past that /** is redundant since the /<M;N>/* expression is already more general, but in practice my expectation is that the vast majority of the use cases will just be happy with using /** for all the key expressions (which makes it a lot easier to inspect).
Complex taproot scripts with many leaves might find the /<M;N>/* notation more interesting, perhaps.

Copy link

@jgriffiths jgriffiths Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's fair enough, I wasn't aware of BIP-0129. I'm OK with /** in that case. Given that:

It's been pointed out in the past that /** is redundant since the /<M;N>/* expression is already more general

I think you should disallow <0;1>/* within wallet policies and require they be expressed as /**. Can't recall if that's already the case.

In fact I think you should go further and mandate the hardened indicator be only ' (or 'h, but only one or the other) in wallet policies. At the moment, the primary source of descriptor malleability (two textually different descriptors that describe the same thing, but have different checksums) is the key paths (plus no lexicographical sorting in the source sortedmulti key expression order). Given that you are replacing the keys with @n and enforcing monotonic numbering from left to right, doing the above will make it much simpler to identify standard policies/templates by not requiring all possible variants to be stored and compared. What do you think?

edit: Forgot about multi-path. You need additionally to state that multi-path extensions of length 3 and greater must be sorted in numerical order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mandating ' instead of h sounds good to me, but completely fixing malleability is probably a lost battle.
For example, impossible to make sure are not reused if descriptors are different because of multi/sortedmulti, as you pointed out. More crucially, you might or might not know the key origin info of some external xpub (although this is only within the key info vector, not in the descriptor template).

I'm not sure about disallowing the /<M;N>/* notation; for one thing, it's already adopted for descriptors and used in many multisig wallets. Moreover, I suspect using <M;N> other than 0;1 will have use cases: you might have the same root xpub in different spending conditions, but using different values for M;N. One use case might be delegation: or(and(Alice1,Bob),and(Alice2,Carl)): Alice might want to allow Bob to sign, but not Carl, so she only signs with Alice1. If only the /** notations is present, then the only option for Alice is to use two different xpubs; instead, one could have or(and(Alice/<0;1>/*,Bob),and(Alice/<2;3>/*, Carl)) and have a single xpub for Alice.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but completely fixing malleability is probably a lost battle.

Agreed, it wasn't baked in at the start and can't be reliably added now. My goal is just to make sure that the common cases are all represented with the same pattern as much as possible.

I'm not sure about disallowing the /<M;N>/* notation

To be clear, I'm only suggesting this when M=0 and N=1, not in the general case! its just a simple text substitution much as the hardening indicator would be, that would go a long way to making common patterns trivially identifiable. Agreed that there are use cases for other values here.


=== Implementation guidelines ===

Implementations must not necessarily implement all the possible wallet policies defined by this standard, but it is recommended to clearly document any limitation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not be better to avoid listing the miniscript/descriptor elements that are supported, and just define this bip in terms of :

  1. A change to bip32 KEY expressions against miniscript/descriptors; they must be replaced with @ placeholders
  2. Non-bip32 keys (e.g. raw and private keys) are not allowed (not sure if this is specified here)
  3. /** (or /+ or whatever is decided on) is used for <0;1>/*, and presumably multi-path is disallowed elsewhere.

Given that implementations may only support a subset of elements, you might as well open it up to future elements and avoid needing to update this bip.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an option, indeed.

I opted for a bottom-up definition (explicitly whitelisting the parts of the descriptor language that we adopt) because there are some inherent incompatibilities, plus it is hard to predict if more will come in the future. For example:

  • The combo descriptor seems inherently not fitting for wallet policies
  • Likewise, extensions like this would have to be "blacklisted" explicitly

On the other hand, the bottom-up approach leaves the exact spec for miniscript descriptors templates in the air, until there is no corresponding BIP...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we have to hope that at some point we stop shoe-horning in descriptor extensions for obsolete cases. Doing so makes a complete implementation prohibitively difficult, makes universal descriptor support less likely, and disincentivizes the eventual/priority movement of coins to modern UTXO/key derivation schemes.

combo, and the linked extension both have in common that they change the cardinality of the solved descriptor. I think you could get the benefit of forward compatibility for this BIP by just stating that final wildcards and multi-path are the only supported key expressions that change cardinality, and all others are forbidden. Then any future extensions to the path syntax, and any expression fragments that return multiple variants would automatically be excluded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that the plan is for extending descriptors liberally, but then implementations can choose which subset to implement. So the bottom-up approach is imho more likely to stay well-defined (but it's certainly more work and will need updates in the future).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave the approach here to your judgement, since there are arguments for both sides. My view is that descriptors become less useful the more incompatible implementations there are. In wally I'm likely just going to accept anything that looks like a correct pattern as long as it parses and doesn't change the cardinality when solved (assuming this BIP is accepted).

@bigspider
Copy link
Contributor Author

Thanks @jgriffiths and @bucko13 for the reviews!
I addressed most of the comments in da3e117.

In bb98f80 I deleted the examples coming from HTLC miniscripts; they don't quite make sense as wallet policies.


* replacing each key placeholder with the corresponding key origin
information;
* replacing every <tt>/**</tt> with <tt>/<0;1>/*</tt>.
Copy link

@joostjager joostjager May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could an alternative to using the /** suffix be to separate define the wallet policies for the inputs and outputs? The input policy can then specify /0/* and the output policy /1/*. Both would still reference the same entry in the key info vector. The added flexibility may also be useful to constrain outputs to a known-but-different wallet.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I am misunderstanding you, but: inputs can be /0/* or /1/* (changes can be spent too)

** Exactly 8 hex characters for the fingerprint of the master key from which this key is derived from (see [[bip-0032.mediawiki|BIP-32]] for details)
** Followed by zero or more <tt>/NUM'</tt> or <tt>/NUM</tt> path elements to indicate hardened or unhardened derivation steps between the fingerprint and the xpub that follows
** A closing bracket <tt>]</tt>
* Followed by the actual key, which is a serialized extended public key (as defined in [[bip-0032.mediawiki|BIP-32]]).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Echoing the discussion in LedgerHQ/app-bitcoin-new#153:

It would be great if a wallet policy can support 2-of-2 multisig transactions where one of the keys is ephemeral (completely random). This is useful when using presigned transactions to simulate covenants, for example to implement time-locked vaults.

== Test Vectors ==

[[bip-0044.mediawiki|BIP-44]], first account
Descriptor template: pkh(@0)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec above says that the @N must always be followed by /** or /<NUM:NUM>/*, but some examples and test vectors don't have that suffix.

 followed by a non-negative decimal number, with no leading zeros (except for <tt>@0</tt>)
* ''always'' followed by either:
** the string  <tt>/**</tt>, or
...

Is @0 the same as @0/**? Would be good to fix either the specification or the examples/test vectors.

Copy link

@jgriffiths jgriffiths Sep 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to also allow /* following @n (i.e. a single asterisk instead of two), since not all wallets use 0/1 paths to differentiate change addresses.

I'm OK with @n alone expanding to @0/** but this does add even more malleability to expressions. Regardless, the always section should be updated to allow single asterisks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naked @N was an oversight, I don't think it should be allowed as it would be confusing (Ledger's implementation doesn't permit it). I will fix it.

--

If we generalize the suffix to expressions other than <0;1>/*, then we have to make sure that no pubkey is repeated.
In practice, for /*, that would mean that all the placeholders must be followed by the exact suffix /*.

Out of curiosity, what software wallets use that scheme?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we generalize the suffix to expressions other than <0;1>/*

I'm not sure I follow. I'm not suggesting generalizing the suffix, at least in the sense of allowing general path expressions to be used as suffixes. I'm saying the only key expression suffixes that should be allowed are /**, /<m;n>/* and /*.

If we generalize the suffix to expressions other than <0;1>/*, then we have to make sure that no pubkey is repeated.

I assume you mean here that a key expression could be given as [/a/b/c]foo/** and then elsewhere as [/a/b/c/0]bar/* and/or [/a/b/c/1]bar/* giving rise to the same derived keys with different key identifiers.

In practice, for /, that would mean that all the placeholders must be followed by the exact suffix /

For a given key expression, yes. I'm fine with supporting /* being optional, and if present the implementation must ensure this.

Note also that at present this BIP does not state that implementations must ensure that KP expressions refer to distinct keys when substituting them into a policy, although that appears to be a requirement. I think it would also be clearer to state that all key expressions in a policy must be replaced by KP expressions (i.e. that no non-KP keys are allowed).

Also missing are that <m;n> expressions should be numerically sorted, and that explicit <0;1> should be disallowed.

I'm happy to make individual commits with suggested changes for the above for you to review/cherry-pick. Should I create a PR against your repo for this?.

Out of curiosity, what software wallets use that scheme?

Blockstream Green multisig wallets do not use bip44 style paths (the wallet predates the BIP). Also descriptors themselves do not enforce bip44 derivation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow. I'm not suggesting generalizing the suffix, at least in the sense of allowing general path expressions to be used as suffixes. I'm saying the only key expression suffixes that should be allowed are /**, /<m;n>/* and /*.

What I mean is that if we allow mixing things like @0/* with @1/<m;n>/*, then we for the xpub of @1 we would need to derive @1/m and @1/n and compare with the pubkey of the xpub in @0, which is quite expensive on a hardware wallet.

With no mixing, for any two placeholders like @0/<m;n>/* and @0/<p;q>/*, I can just check that the sets {m, n} and {p, q} are disjoint.

For a given key expression, yes. I'm fine with supporting /* being optional, and if present the implementation must ensure this.

Not just for a given key expression, I mean it across the entire wallet policy. That is, either the KPs all end with /*, or they all end with some /<NUM;NUM>/*.

Note also that at present this BIP does not state that implementations must ensure that KP expressions refer to distinct keys when substituting them into a policy, although that appears to be a requirement.

Good point, worth adding explicitly the "Additional rules", with the motivation that it's required in miniscript (and it's simpler to add this restriction globally, rather than on the individual miniscript parts of the policy).

So basically:

  • all the public keys (deserialized from the corresponding xpubs in the keys information vector) must be distinct
  • all the derivations in each KP expression referring to the same index in the keys information vector must be distinct

In order to allow optional extensions like /*, perhaps I could mention that possibility in a separate section?

I think it would also be clearer to state that all key expressions in a policy must be replaced by KP expressions (i.e. that no non-KP keys are allowed).

A wallet policy is the pair (descriptor_template, keys_information_vector), so not sure what you mean here. The grammar seems to specify exactly what are the allowed KP and KEY expressions, as far as I can tell.

Also missing are that <m;n> expressions should be numerically sorted, and that explicit <0;1> should be disallowed.

I like suggesting to sort <m;n> numerically, but not sure about disallowing <0;1>. I expect people to prefer not mixing @0/** with @0/<2;3>/*, as it's more explicit in that case to just write @0/<0;1>/*.

I'm happy to make individual commits with suggested changes for the above for you to review/cherry-pick. Should I create a PR against your repo for this?.

Sure, feel free to propose changes, but I'll try to incorporate the comments now. I'm also hoping to find the time for a general pass over the whole document to improve the Motivation section, as that was written before the BtcPrague meeting and it can surely be improved in hindsight.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, this ship along with other malleability concerns has unfortunately sailed. Although it would be nice to enforce ' rather than h for hardening in policies at least, @bigspider?

I agree; Ledger's implementation only allows ' (choice mostly because it looks better on a small screen).
The current specs indeed don't mention h.

I'll go over the other comments (and add the failure cases, thanks!) next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4b1f826 should address all the remaining comments from this thread, thanks @jgriffiths!

Suggested invalid test vectors:

  1. sh(multi(1,@0/<0;1>/*,@1/<1;2>/*)) Non-disjoint multipath expressions

This is actually valid as @0 and @1 are independent; added an example where both are @0 instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the miniscript reference, I suppose https://bitcoin.sipa.be/miniscript/ is the only one, although the mention of separate keys is quite buried in the page in the section on malleability.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bigspider Thanks!

The wally implementation (ElementsProject/libwally-core#369) is now complete and is being tested for addition to Jade. Once merged it would be good to add to the implementations section. At this stage it looks like this BIP has support in 3 HWW so getting a BIP number assigned should not be an issue.

It would be good IMO to squash all commits at this stage. I'll do a final review following wally code review and Jade testing. Many thanks for your responsiveness in addressing review comments.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI policy support has been merged into wally now and support in Jade is now being tested.

benma added a commit to benma/bitbox02-firmware that referenced this pull request Jun 20, 2023
We will support miniscript as part of output descriptors.

Descriptors are an expression language to specify Bitcoin outputs. For
example, `wsh(multi(2,<pubkey1>,<pubkey2>,<pubkey3>))` is a 2-of-3
multisig wrapped in a P2WSH (pay-to-witness-script-hash) output.
Descriptors specification: https://github.com/bitcoin/bitcoin/blob/8f402710371a40c5777dc3f9c4ba6ca8505a2f90/doc/descriptors.md

In fragments wrapping SCRIPT expressions, SCRIPT can also be a
miniscript expression, for example: `wsh(or_b(pk(<pubkey1>),s:pk(<pubkey2>)))`.

With hardware signers, we want to use xpubs with derivations instead
of raw pubkeys, and also have a shorter representation than inlining
the xpubs for readability. That is why we will follow the 'Wallet
Policies for Descriptor Wallets' proposal specied here:

https://github.com/bigspider/bips/blob/bip-wallet-policies/bip-wallet-policies.mediawiki (bitcoin/bips#1389).

The above example then become `wsh(or_b(pk(@0/**),s:pk(@1/**)))`. The
`@NUM` references a key provided in the keys list.

For the policy with the keys, one can then derive pkScripts for
receive and change addresses and use these to create addresses.
benma added a commit to benma/bitbox02-firmware that referenced this pull request Jun 20, 2023
We add a policies.rs file with functions to parse and validate a
policy containing miniscript.

Policy specification: bitcoin/bips#1389

We only support `wsh(<miniscript>)` policies for now. Taproot or other
policy fragments could be added in the future.

More validation checks are coming in later commits, such as:
- At least one key must be ours
- No duplicate keys possible in the policy
- No duplicate keys in the keys list
- All keys in the keys list are used, and all key references (@0, ...)
are valid.
- ...?

Also coming in later commits:
- Derive a pkScript at a keypath, generate receive address from that
- Policy registration (very similar to how multisig registration works
today)
- Signing transactions
benma added a commit to benma/bitbox02-firmware that referenced this pull request Jun 20, 2023
We add a policies.rs file with functions to parse and validate a
policy containing miniscript.

Policy specification: bitcoin/bips#1389

We only support `wsh(<miniscript>)` policies for now. Taproot or other
policy fragments could be added in the future.

More validation checks are coming in later commits, such as:
- At least one key must be ours
- No duplicate keys possible in the policy
- No duplicate keys in the keys list
- All keys in the keys list are used, and all key references (@0, ...)
are valid.
- ...?

Also coming in later commits:
- Derive a pkScript at a keypath, generate receive address from that
- Policy registration (very similar to how multisig registration works
today)
- Signing transactions
benma added a commit to benma/bitbox02-firmware that referenced this pull request Jun 20, 2023
We add a policies.rs file with functions to parse and validate a
policy containing miniscript.

Policy specification: bitcoin/bips#1389

We only support `wsh(<miniscript>)` policies for now. Taproot or other
policy fragments could be added in the future.

More validation checks are coming in later commits, such as:
- At least one key must be ours
- No duplicate keys possible in the policy
- No duplicate keys in the keys list
- All keys in the keys list are used, and all key references (@0, ...)
are valid.
- ...?

Also coming in later commits:
- Derive a pkScript at a keypath, generate receive address from that
- Policy registration (very similar to how multisig registration works
today)
- Signing transactions
benma added a commit to benma/bitbox02-firmware that referenced this pull request Jun 20, 2023
We add a policies.rs file with functions to parse and validate a
policy containing miniscript.

Policy specification: bitcoin/bips#1389

We only support `wsh(<miniscript>)` policies for now. Taproot or other
policy fragments could be added in the future.

More validation checks are coming in later commits, such as:
- At least one key must be ours
- No duplicate keys possible in the policy
- No duplicate keys in the keys list
- All keys in the keys list are used, and all key references (@0, ...)
are valid.
- ...?

Also coming in later commits:
- Derive a pkScript at a keypath, generate receive address from that
- Policy registration (very similar to how multisig registration works
today)
- Signing transactions
benma added a commit to benma/bitbox02-firmware that referenced this pull request Jun 20, 2023
We will support miniscript as part of output descriptors.

Descriptors are an expression language to specify Bitcoin outputs. For
example, `wsh(multi(2,<pubkey1>,<pubkey2>,<pubkey3>))` is a 2-of-3
multisig wrapped in a P2WSH (pay-to-witness-script-hash) output.
Descriptors specification: https://github.com/bitcoin/bitcoin/blob/8f402710371a40c5777dc3f9c4ba6ca8505a2f90/doc/descriptors.md

In fragments wrapping SCRIPT expressions, SCRIPT can also be a
miniscript expression, for example: `wsh(or_b(pk(<pubkey1>),s:pk(<pubkey2>)))`.

With hardware signers, we want to use xpubs with derivations instead
of raw pubkeys, and also have a shorter representation than inlining
the xpubs for readability. That is why we will follow the 'Wallet
Policies for Descriptor Wallets' proposal specied here:

https://github.com/bigspider/bips/blob/bip-wallet-policies/bip-wallet-policies.mediawiki (bitcoin/bips#1389).

The above example then become `wsh(or_b(pk(@0/**),s:pk(@1/**)))`. The
`@NUM` references a key provided in the keys list.

For the policy with the keys, one can then derive pkScripts for
receive and change addresses and use these to create addresses.
benma added a commit to benma/bitbox02-firmware that referenced this pull request Jun 20, 2023
We add a policies.rs file with functions to parse and validate a
policy containing miniscript.

Policy specification: bitcoin/bips#1389

We only support `wsh(<miniscript>)` policies for now. Taproot or other
policy fragments could be added in the future.

More validation checks are coming in later commits, such as:
- At least one key must be ours
- No duplicate keys possible in the policy
- No duplicate keys in the keys list
- All keys in the keys list are used, and all key references (@0, ...)
are valid.
- ...?

Also coming in later commits:
- Derive a pkScript at a keypath, generate receive address from that
- Policy registration (very similar to how multisig registration works
today)
- Signing transactions
benma added a commit to benma/bitbox02-firmware that referenced this pull request Jun 20, 2023
We add a policies.rs file with functions to parse and validate a
policy containing miniscript.

Policy specification: bitcoin/bips#1389

We only support `wsh(<miniscript>)` policies for now. Taproot or other
policy fragments could be added in the future.

More validation checks are coming in later commits, such as:
- At least one key must be ours
- No duplicate keys possible in the policy
- No duplicate keys in the keys list
- All keys in the keys list are used, and all key references (@0, ...)
are valid.
- ...?

Also coming in later commits:
- Derive a pkScript at a keypath, generate receive address from that
- Policy registration (very similar to how multisig registration works
today)
- Signing transactions
benma added a commit to benma/bitbox02-firmware that referenced this pull request Jun 29, 2023
We will support miniscript as part of output descriptors.

Descriptors are an expression language to specify Bitcoin outputs. For
example, `wsh(multi(2,<pubkey1>,<pubkey2>,<pubkey3>))` is a 2-of-3
multisig wrapped in a P2WSH (pay-to-witness-script-hash) output.
Descriptors specification: https://github.com/bitcoin/bitcoin/blob/8f402710371a40c5777dc3f9c4ba6ca8505a2f90/doc/descriptors.md

In fragments wrapping SCRIPT expressions, SCRIPT can also be a
miniscript expression, for example: `wsh(or_b(pk(<pubkey1>),s:pk(<pubkey2>)))`.

With hardware signers, we want to use xpubs with derivations instead
of raw pubkeys, and also have a shorter representation than inlining
the xpubs for readability. That is why we will follow the 'Wallet
Policies for Descriptor Wallets' proposal specied here:

https://github.com/bigspider/bips/blob/bip-wallet-policies/bip-wallet-policies.mediawiki (bitcoin/bips#1389).

The above example then become `wsh(or_b(pk(@0/**),s:pk(@1/**)))`. The
`@NUM` references a key provided in the keys list.

For the policy with the keys, one can then derive pkScripts for
receive and change addresses and use these to create addresses.
benma added a commit to benma/bitbox02-firmware that referenced this pull request Jun 29, 2023
We add a policies.rs file with functions to parse and validate a
policy containing miniscript.

Policy specification: bitcoin/bips#1389

We only support `wsh(<miniscript>)` policies for now. Taproot or other
policy fragments could be added in the future.

More validation checks are coming in later commits, such as:
- At least one key must be ours
- No duplicate keys possible in the policy
- No duplicate keys in the keys list
- All keys in the keys list are used, and all key references (@0, ...)
are valid.
- ...?

Also coming in later commits:
- Derive a pkScript at a keypath, generate receive address from that
- Policy registration (very similar to how multisig registration works
today)
- Signing transactions
@darosior
Copy link
Member

darosior commented Aug 9, 2023

@luke-jr can this be assigned a BIP number?

jgriffiths added a commit to ElementsProject/libwally-core that referenced this pull request Sep 6, 2023
See bitcoin/bips#1389

Policies are a restriction on the general variable substitution
mechanism that wally already supports, combined with a shorthand
notation for path derivation.

1  - Key variables can only be named @n where n is an integer
2  - Key variable names must increase sequentially from 0 to n
3  - Key variables names must be followed by '/*', '/**', or '/<m;n>/*'
4  - Key variables can only be serialized BIP32 public keys without paths
5  - All key expressions to substitute must be unique
6  - Al least one key expression must be present
7  - Key variables must appear in the policy in order from 0 to n
     (back-references are allowed for repeated keys)
8  - All key expressions in a policy must be in the form of Key variables
9  - All key expression must share the same solved cardinality (keys
     using '/*', cannot be mixed with those using '/**' or '/<m;n>/*')
10 - The solved cardinality of a policy must be 1 or 2 (e.g. no combo())`.
11 - All repeated references to the same key must use distinct
     derivations.

This initial change implements and tests points 1-4.

This implementation will ignore the whitelisted expression lists given
in the BIP, and instead accept any valid descriptor that doesn't have a
solved cardinality greater than 2. See the above linked github PR discussion
for the rationale behind this decision.
@luke-jr luke-jr changed the title [New BIP] Wallet Policies BIP 388: Wallet Policies for Descriptor Wallets Dec 26, 2023
@bigspider bigspider force-pushed the bip-wallet-policies branch from 07f0fcf to 3bb2cfe Compare January 8, 2024 10:50
@bigspider
Copy link
Contributor Author

bigspider commented Jan 8, 2024

I squashed the history, rebased and updated BIP number and type in 3bb2cfe d4c650b.

@bigspider bigspider force-pushed the bip-wallet-policies branch from 3bb2cfe to d4c650b Compare January 8, 2024 11:18
@bigspider
Copy link
Contributor Author

@luke-jr, is there any other further change that I missed?

Otherwise, it would be great to have this merged so I can start linking to it in docs.

@darosior
Copy link
Member

darosior commented Apr 2, 2024

@luke-jr could you merge this when you've got a minute?

Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ready for merge to me, although I have a few nits that you might want to address.

ACK d4c650b

bip-0388.mediawiki Outdated Show resolved Hide resolved
Comment on lines 103 to 140
<pre>
tr(musig(xpubA,xpubB,xpubC,xpubD,xpubE)/<0;1>/*), {
{
{
pk(musig(xpubA,xpubB,xpubC)/<2;3>/*),
{
pk(musig(xpubA,xpubB,xpubD)/<4;5>/*)
pk(musig(xpubA,xpubB,xpubE)/<6;7>/*),
}
},
{
pk(musig(xpubA,xpubC,xpubD)/<8;9>/*),
{
pk(musig(xpubA,xpubC,xpubE)/<10;11>/*),
pk(musig(xpubA,xpubD,xpubE)/<12;13>/*)
}
}
},
{
{
pk(musig(xpubB,xpubC,xpubD)/<14;15>/*),
pk(musig(xpubB,xpubC,xpubE)/<16;17>/*)
},
{
pk(musig(xpubB,xpubD,xpubE)/<18;19>/*),
{
pk(musig(xpubC,xpubD,xpubE)/<20;21>/*),
sortedmulti_a(3,
xpubA/<22;23>/*,
xpubB/<22;23>/*,
xpubC/<22;23>/*,
xpubD/<22;23>/*,
xpubE/<22;23>/*)
}
}
}
})
</pre>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example is hard to access since the format it uses is only introduced later in the Specification section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed; moreover, plus this is using musig which is not even part of the standard. A text description is probably sufficient: 95cf539.

bip-0388.mediawiki Outdated Show resolved Hide resolved
Descriptor:wsh(or_d(pk([6738736c/48'/0'/0'/100']xpub6FC1fXFP1GXQpyRFfSE1vzzySqs3Vg63bzimYLeqtNUYbzA87kMNTcuy9ubr7MmavGRjW2FRYHP4WGKjwutbf1ghgkUW9H7e3ceaPLRcVwa),and_v(v:multi(2,[b2b1f0cf/44'/0'/0'/100']xpub6EYajCJHe2CK53RLVXrN14uWoEttZgrRSaRztujsXg7yRhGtHmLBt9ot9Pd5ugfwWEu6eWyJYKSshyvZFKDXiNbBcoK42KRZbxwjRQpm5Js,[a666a867/44'/0'/0'/100']xpub6Dgsze3ujLi1EiHoCtHFMS9VLS1UheVqxrHGfP7sBJ2DBfChEUHV4MDwmxAXR2ayeytpwm3zJEU3H3pjCR6q6U5sP2p2qzAD71x9z5QShK2,[bb641298/44'/0'/0'/100']xpub6Dz8PHFmXkYkykQ83ySkruky567XtJb9N69uXScJZqweYiQn6FyieajdiyjCvWzRZ2GoLHMRE1cwDfuJZ6461YvNRGVBJNnLA35cZrQKSRJ),older(65535))))
<br>

TBD: add examples with taproot scripts and miniscript.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an open todo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed; added a taproot example in 95cf539.

bip-0388.mediawiki Show resolved Hide resolved
@murchandamus
Copy link
Contributor

murchandamus commented Apr 22, 2024

I also noticed that this BIP does not have a Rationale section, although it seems to me that the rationale is sufficiently covered in Motivation and Specification (and therefore no change is necessary in that regard).

Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the automated checks is failing, because the BIP is not added to the overview table. Please add your BIP to the table in README.mediawiki. You can find the expected table entry in the error message of the failed check.

@jonatack
Copy link
Member

jonatack commented May 6, 2024

Yes, it's nice that the check is working now. Here's the entry it suggests:

+> | [[bip-0388.mediawiki|388]]
+> | Applications
+> | Wallet Policies for Descriptor Wallets
+> | Salvatore Ingala
+> | Standard
+> | Draft
+> |-

bigspider and others added 5 commits May 7, 2024 10:58
Co-authored-by: Mark "Murch" Erhardt <murch@murch.one>
- Removed large example of taproot policy; replaced with the textual description
- Added an example of a taproot wallet policy containing miniscript
@bigspider bigspider force-pushed the bip-wallet-policies branch from ff61433 to a0c8501 Compare May 7, 2024 09:00
@bigspider
Copy link
Contributor Author

Rebased, and addressed the final comments (thank you!).
Please let me know if you prefer me to squash the new commits.

Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did another thorough read and noticed a few style and grammar nits. I did not review the examples or technical details of the proposal. I do not consider any of these comments to be blockers, please feel free to adopt or ignore them at your leisure.

It is not necessary to squash the commits, please follow your own preference.

bip-0388.mediawiki Outdated Show resolved Hide resolved
bip-0388.mediawiki Outdated Show resolved Hide resolved
bip-0388.mediawiki Outdated Show resolved Hide resolved
bip-0388.mediawiki Outdated Show resolved Hide resolved
bip-0388.mediawiki Outdated Show resolved Hide resolved
bip-0388.mediawiki Outdated Show resolved Hide resolved
bip-0388.mediawiki Outdated Show resolved Hide resolved
bip-0388.mediawiki Outdated Show resolved Hide resolved
bip-0388.mediawiki Outdated Show resolved Hide resolved
bip-0388.mediawiki Outdated Show resolved Hide resolved
bigspider and others added 2 commits May 7, 2024 22:10
Co-authored-by: Mark "Murch" Erhardt <murch@murch.one>
@bigspider
Copy link
Contributor Author

Thanks @murchandamus for the thorough suggestions!
I accepted almost all directly, and slightly modified the remaining ones in 7d0c08e.

Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 7d0c08e

@murchandamus murchandamus merged commit 56575ff into bitcoin:master May 8, 2024
3 checks passed
@bigspider bigspider deleted the bip-wallet-policies branch July 10, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.