-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Conversation
There was a problem hiding this 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
|
||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
25da615
to
162c675
Compare
In dfa2645, I added the possibility of using unhardened derivation steps in the key origin information (while keeping the restriction for |
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).
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
|
||
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>. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
bip-wallet-policies.mediawiki
Outdated
|
||
=== 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. |
There was a problem hiding this comment.
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 :
- A change to bip32
KEY
expressions against miniscript/descriptors; they must be replaced with@
placeholders - Non-bip32 keys (e.g. raw and private keys) are not allowed (not sure if this is specified here)
/**
(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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
307112f
to
bb98f80
Compare
Thanks @jgriffiths and @bucko13 for the reviews! In bb98f80 I deleted the examples coming from HTLC miniscripts; they don't quite make sense as wallet policies. |
bip-wallet-policies.mediawiki
Outdated
|
||
* replacing each key placeholder with the corresponding key origin | ||
information; | ||
* replacing every <tt>/**</tt> with <tt>/<0;1>/*</tt>. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
bip-wallet-policies.mediawiki
Outdated
** 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]]). |
There was a problem hiding this comment.
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.
bip-wallet-policies.mediawiki
Outdated
== Test Vectors == | ||
|
||
[[bip-0044.mediawiki|BIP-44]], first account | ||
Descriptor template: pkh(@0) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 thanh
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.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
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
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
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
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
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.
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
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
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.
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
@luke-jr can this be assigned a BIP number? |
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.
07f0fcf
to
3bb2cfe
Compare
3bb2cfe
to
d4c650b
Compare
@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. |
@luke-jr could you merge this when you've got a minute? |
There was a problem hiding this 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
<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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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). |
There was a problem hiding this 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.
Yes, it's nice that the check is working now. Here's the entry it suggests:
|
Co-authored-by: Mark "Murch" Erhardt <[email protected]>
- Removed large example of taproot policy; replaced with the textual description - Added an example of a taproot wallet policy containing miniscript
ff61433
to
a0c8501
Compare
Rebased, and addressed the final comments (thank you!). |
There was a problem hiding this 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.
Co-authored-by: Mark "Murch" Erhardt <[email protected]>
Thanks @murchandamus for the thorough suggestions! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 7d0c08e
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: