-
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
388 - Add more motivation, and links to miniscript BIP #1644
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.
Approach ACK
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.
Looks good to me, have a couple suggestions.
Co-authored-by: Mark "Murch" Erhardt <murch@murch.one>
Thanks @murchandamus! I committed your suggestions, and I believe the changes also answer @jonatack's concern above. |
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.
LGTM
bip-0388.mediawiki
Outdated
@@ -14,7 +14,9 @@ | |||
|
|||
== Abstract == | |||
|
|||
Wallet policies build on top of output script descriptors to represent the types of descriptors that are typically used to represent "accounts" in a software wallet, or a hardware signing device, in a compact, reviewable way. A wallet policy always represents descriptors which produce all the receive and change addresses that are logically part of the same account. | |||
Software wallets and hardware signing devices sequester wallet uses into logically separate "accounts". |
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.
"sequester" can have the (generally negative) meaning to remove or withdraw into solitude or retirement, to banish, to exile
suggestion:
Software wallets and hardware signing devices typically enable separating wallet uses into different "accounts".
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 point on 'sequester'; I used 'partition the funds' in the rewrite, preferring to refer specifically to "funds" rather than the more generic "wallet uses".
I spent quite some time trying to reformulate the sentence, never feeling fully happy with it. I think the reason is that the while the goal of the accounts (= keeping funds partitioned) was stated, the practical effect (= designing a UX that abstracts away UTXOs, matching the user's expectation) was not stated at all in the entire abstract. So in c2655e0 I added a new sentence to this effect.
bip-0388.mediawiki
Outdated
@@ -65,7 +67,7 @@ Reusing keys across different UTXOs harms user privacy by allowing external part | |||
|
|||
By constraining the derivation path patterns to have a uniform structure, wallet policies prevent key reuse among the same or different UTXOs of the same account. | |||
|
|||
Using distinct public keys obtained from hardened derivation paths guarantees that no key reuse can happen also across accounts, and is strongly recommended. However, wallet policies do not mandate hardened derivation paths for the public keys, in order to maintain compatibility with existing deployments that do not adhere to this recommendation. | |||
It is strongly recommended to avoid key reuse across accounts. Distinct public keys per account can be guaranteed per hardened derivation paths. This specification does not mandate hardened derivation to maintain compatibility with existing deployments that do not adhere to this recommendation. |
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.
(In general, an active voice can be easier to read and more direct than a heavy passive voice.)
Perhaps s/per/by using/
and s/derivation to/derivation in order to
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.
Accepted the suggestion in c2655e0, and I added the repeated word 'distinct' to the derivation paths, as 'hardened' is of course not enough if one uses the exact same derivation path. Please let me know if you think I should reformulate in order to avoid repeating the word, but actually my impression is that it helps clarity - especially for readers who are not fully familiar with the details of 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.
I would say that the compartmentalization of different uses of the wallet’s funds, i.e. the not mixing of different purposes is the main purpose and could be highlighted more. Abstracting away the UTXOs feels like an orthogonal goal that would apply to a wallet with a single account the same way.
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.
You can only securely abstract away the UTXOs (getting the UX flows people are used to) if you properly segregate funds across accounts (that is, the signing device guarantees that you always know exactly how much you're spending from each account).
That wasn't much of a concern for single-sig accounts because malware tricking you into spending from another of your accounts is not an interesting attack.
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.
Looks good to me, and please feel free to ignore my suggestions if you feel that it’s ready as is, but it seems to me that the actual isolation of funds and transactions across accounts is obfuscated by highlighting the partition’s use in showing account changes.
Happy to merge either way.
bip-0388.mediawiki
Outdated
@@ -65,7 +67,7 @@ Reusing keys across different UTXOs harms user privacy by allowing external part | |||
|
|||
By constraining the derivation path patterns to have a uniform structure, wallet policies prevent key reuse among the same or different UTXOs of the same account. | |||
|
|||
Using distinct public keys obtained from hardened derivation paths guarantees that no key reuse can happen also across accounts, and is strongly recommended. However, wallet policies do not mandate hardened derivation paths for the public keys, in order to maintain compatibility with existing deployments that do not adhere to this recommendation. | |||
It is strongly recommended to avoid key reuse across accounts. Distinct public keys per account can be guaranteed per hardened derivation paths. This specification does not mandate hardened derivation to maintain compatibility with existing deployments that do not adhere to this recommendation. |
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 would say that the compartmentalization of different uses of the wallet’s funds, i.e. the not mixing of different purposes is the main purpose and could be highlighted more. Abstracting away the UTXOs feels like an orthogonal goal that would apply to a wallet with a single account the same way.
bip-0388.mediawiki
Outdated
@@ -14,7 +14,10 @@ | |||
|
|||
== Abstract == | |||
|
|||
Wallet policies build on top of output script descriptors to represent the types of descriptors that are typically used to represent "accounts" in a software wallet, or a hardware signing device, in a compact, reviewable way. A wallet policy always represents exactly two descriptors, which produce the receive and change addresses that are logically part of the same account. | |||
Software wallets and hardware signing devices typically partition funds into separate "accounts". When signing or visualizing transactions, this allows to show to the user aggregate in-flow or out-flow information for one or more involved accounts. |
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.
Software wallets and hardware signing devices typically partition funds into separate "accounts". When signing or visualizing transactions, this allows to show to the user aggregate in-flow or out-flow information for one or more involved accounts. | |
Software wallets and hardware signing devices typically feature "accounts" as a means to partition funds and transactions by purpose. This partitioning is also useful to show aggregate in-flow and out-flow information for the involved accounts when signing or visualizing transactions. |
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 don't quite agree with the rewrite:
- "accounts"/wallet policies logically partition funds based on the spending conditions; therefore, it's mostly the ownership rules that is semantically represented in the "account". While purpose has of course a lot of overlap with the ownership rules, I don't think it's the appropriate word to mention in this first paragraph.
- wallet policies / accounts partition funds, but don't quite partition transactions. That might seem the case today for most wallets, but just because they only allows creating transactions spending from (and receiving change to) a single account. More sophisticated wallets could (and hopefully will) allow creating transactions that involve multiple accounts, including with different spending conditions or with different owners.
Co-authored-by: Mark "Murch" Erhardt <murch@murch.one>
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.
Seems ready to merge. I will merge this tomorrow, unless more review comments appear that indicate otherwise.
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.
Had another look.
bip-0388.mediawiki
Outdated
@@ -14,7 +14,10 @@ | |||
|
|||
== Abstract == | |||
|
|||
Wallet policies build on top of output script descriptors to represent the types of descriptors that are typically used to represent "accounts" in a software wallet, or a hardware signing device, in a compact, reviewable way. A wallet policy always represents exactly two descriptors, which produce the receive and change addresses that are logically part of the same account. | |||
Software wallets and hardware signing devices typically partition funds into separate "accounts". When signing or visualizing transactions, this allows to show to the user aggregate in-flow or out-flow information for one or more involved accounts. |
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.
s/this allows to show to the user/this allows displaying to the user/
I'm not sure this section is an improvement over the current BIP (this may account for why @murchandamus and I keep trying to rewrite it :). Here's another attempt:
Software wallets and hardware signing devices typically partition funds into separate "accounts". When signing or visualizing transactions, this allows to show to the user aggregate in-flow or out-flow information for one or more involved accounts. | |
Software wallets and hardware signing devices typically allow the user to partition funds into separate "accounts". When signing or visualizing a transaction, aggregate flows of all accounts affected by the transaction may (and should) be displayed to the user. |
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.
Accepted most of it in 1811613; only left out the allow the user to
part as it's generally not the user's choice, but how (almost?) all software wallet structure funds.
bip-0388.mediawiki
Outdated
|
||
It is strongly recommended to avoid key reuse across accounts. Distinct public keys per account can be guaranteed by using distinct hardened derivation paths. This specification does not mandate hardened derivation in order to maintain compatibility with existing deployments that do not adhere to this recommendation. | ||
|
||
It is out of scope for this document to guarantee that users do not reuse extended public keys among different wallet accounts. This responsibility is left to the users and their software 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.
Unless the idea was to remove this mention, perhaps keep it.
It is out of scope for this document to guarantee that users do not reuse extended public keys among different wallet accounts. This responsibility is left to the users and their software wallet. | |
It is out of scope for this document to guarantee that users do not reuse extended public keys among different wallet accounts. This is still very important, but responsibility is left to the users and their software 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.
Added in 1811613, plus an additional the
.
Looks to me like @jonatack’s comments were addressed, so I went ahead and merged this. |
Seems fine. Would these changes merit a changelog entry? |
Since it's only cosmetic/explanatory changes (not affecting the specs), IMHO it's not worth tracking. |
One important aspect of wallet policies is how they help preventing pubkey reuse. I think it's worth explaining it in some detail in the motivation section.
Also adding links to BIP-379, and fixing a nit.
Strictly speaking, one might want to formally specify all the miniscript fragments in the grammar specification; however, that would require listing all the miniscript fragments, which doesn't seem useful. As the grammar for miniscript rules are somewhat obvious, I think a simple link to BIP-379 is more useful in practice.