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

388 - Add more motivation, and links to miniscript BIP #1644

Merged
merged 7 commits into from
Aug 21, 2024

Conversation

bigspider
Copy link
Contributor

@bigspider bigspider commented Jul 11, 2024

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.

bip-0388.mediawiki Outdated Show resolved Hide resolved
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Approach ACK

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.

Looks good to me, have a couple suggestions.

bip-0388.mediawiki Outdated Show resolved Hide resolved
bip-0388.mediawiki Outdated Show resolved Hide resolved
Co-authored-by: Mark "Murch" Erhardt <murch@murch.one>
@bigspider
Copy link
Contributor Author

Thanks @murchandamus! I committed your suggestions, and I believe the changes also answer @jonatack's concern above.

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.

LGTM

@@ -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".
Copy link
Member

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

Copy link
Contributor Author

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.

@@ -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.
Copy link
Member

@jonatack jonatack Jul 24, 2024

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

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.

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.

@@ -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.
Copy link
Contributor

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.

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

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

bip-0388.mediawiki Outdated Show resolved Hide resolved
Co-authored-by: Mark "Murch" Erhardt <murch@murch.one>
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.

Seems ready to merge. I will merge this tomorrow, unless more review comments appear that indicate otherwise.

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Had another look.

@@ -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.
Copy link
Member

@jonatack jonatack Aug 7, 2024

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:

Suggested change
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.

Copy link
Contributor Author

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.


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.
Copy link
Member

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.

Suggested change
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.

Copy link
Contributor Author

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.

@murchandamus murchandamus merged commit 97012a8 into bitcoin:master Aug 21, 2024
4 checks passed
@murchandamus
Copy link
Contributor

Looks to me like @jonatack’s comments were addressed, so I went ahead and merged this.

@bigspider bigspider deleted the bip388-updates branch August 22, 2024 12:22
@jonatack
Copy link
Member

Seems fine. Would these changes merit a changelog entry?

@bigspider
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants