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-85: Add language code & dice app, TPRV guidance, warn on BIP-32 divergence, grammar & clarity #1679

Merged
merged 16 commits into from
Oct 25, 2024

Conversation

akarve
Copy link
Contributor

@akarve akarve commented Oct 7, 2024

Reloading this PR with minimal, backward-compatible changes.

  • I believe there is consensus on the need to add a new champion here.
  • The reason for adding a new reference implementation is to support Python 3.x with a thoroughly unit-tested implementation. I was unable to get the original Python 2.x reference implementation to run.
  • If desired I can email bitcoin dev for more thoughts on these changes

@akarve
Copy link
Contributor Author

akarve commented Oct 7, 2024

@jonatack @scgbckbone Please have look.

Copy link
Contributor

@scgbckbone scgbckbone left a comment

Choose a reason for hiding this comment

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

wrong entropy is in base64 pwd application, currently:
d7ad61d4a76575c5bad773feeb40299490b224e8e5df6c8ad8fe3d0a6eed7b85ead9fef7bcca8160f0ee48dc6e92b311fc71f2146623cc6952c03ce82c7b63fe

correct:
74a2e87a9ba0cdd549bdd2f9ea880d554c6c355b08ed25088cfa88f3f1c4f74632b652fd4a8f5fda43074c6f6964a3753b08bb5210c8f5e75c07a4c2a20bf6e9

bip-0085.mediawiki Outdated Show resolved Hide resolved
@akarve akarve requested a review from scgbckbone October 7, 2024 02:31
@scgbckbone
Copy link
Contributor

could you possibly split this into "b64 entropy fix only PR" and "the rest" ?

@akarve
Copy link
Contributor Author

akarve commented Oct 7, 2024

could you possibly split this into "b64 entropy fix only PR" and "the rest" ?

Yeahbut I’d also include the BIP32 comment because it’s a fact.

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.

Thank you for opening this, was going to ask. If this is to be done, seems best to do it before updating the BIP status to Final in #1676.

  • Can you prefix each commit message with BIP85: or refer to it in the commit title?

  • Consider doing the test vector correction in a separate commit with a full explanation of what was incorrect and the fix.

  • Can you also provide an explanation of the Base64 entropy fix in the commit message.

  • Please provide a better commit message for the "add warning" commit.

  • Did you want to make the changes in the abstract and definitions sections that you did in BIP85: Clarify spec, correct test vectors, add Portuguese language code, add dice application #1600?

cc @nvk for review

bip-0085.mediawiki Outdated Show resolved Hide resolved
@scgbckbone
Copy link
Contributor

Can you also provide an explanation of the Base64 entropy fix in the commit message.

@jonatack this is my bug when I was adding base64 app to the BIP, seems it went unnoticed for long time...

@jonatack jonatack added the PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author label Oct 9, 2024
@akarve akarve changed the title BIP-85: Correct test vector, add language code, add dice app BIP-85: Add language code, add dice app, warn on BIP-32 divergence, grammar clarity Oct 13, 2024
@akarve akarve requested a review from jonatack October 13, 2024 23:56
@akarve
Copy link
Contributor Author

akarve commented Oct 14, 2024

Can you also provide an explanation of the Base64 entropy fix in the commit message.

@jonatack this is my bug when I was adding base64 app to the BIP, seems it went unnoticed for long time...

Here's the standalone fix: #1683

@akarve
Copy link
Contributor Author

akarve commented Oct 14, 2024

Did you want to make the changes in the abstract and definitions sections that you did in #1600?

I want to but not in this changeset. Let's get through the hard parts then we can optimize on top of that. I'm tryna keep the changesets as small as possible for now.

@akarve akarve changed the title BIP-85: Add language code, add dice app, warn on BIP-32 divergence, grammar clarity BIP-85: Add language code, add dice app, warn on BIP-32 divergence, grammar & clarity Oct 14, 2024
@jonatack
Copy link
Member

jonatack commented Oct 15, 2024

Might be good to add a changelog in this pull, as previously in #1600.

bip-0085.mediawiki Outdated Show resolved Hide resolved
bip-0085.mediawiki Outdated Show resolved Hide resolved
@akarve akarve requested a review from jonatack October 15, 2024 18:26
@akarve akarve changed the title BIP-85: Add language code, add dice app, warn on BIP-32 divergence, grammar & clarity BIP-85: Add language code & dice app, TPRV guidance, warn on BIP-32 divergence, grammar & clarity Oct 15, 2024
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.

Ok, I see that https://github.com/akarve/bipsea has been updated.

bip-0085.mediawiki Outdated Show resolved Hide resolved
bip-0085.mediawiki Outdated Show resolved Hide resolved
bip-0085.mediawiki Outdated Show resolved Hide resolved
@akarve akarve requested a review from jonatack October 20, 2024 22:44
@akarve
Copy link
Contributor Author

akarve commented Oct 20, 2024

In retrospect this basically nets out to a revert of the backwards incompatible part of #1600, reverted in #1674.
These lines.

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.

Review comments follow.

Suggested grammar touch-ups

--- a/bip-0085.mediawiki
+++ b/bip-0085.mediawiki
@@ -35,11 +35,11 @@ The terminology related to keychains used in the wild varies widely, for example
 
 ==Motivation==
 
-Most wallets implement BIP32 which defines how a BIP32 root key can be used to derive keychains. As a consequence, a backup of just the BIP32 root key is sufficient to include all keys derived from it. BIP32 does not have a human friendly serialization of the BIP32 root key (or BIP32 extended keys in general) which makes paper backups or manually restoring the key more error-prone. BIP39 was designed to solve this problem but rather than serialize the BIP32 root key, it takes some entropy, encoded to a "seed mnemonic", which is then hashed to derive the BIP39 seed which can be turned into the BIP32 root key. Saving the BIP39 mnemonic is enough to reconstruct the entire BIP32 keychain, but a BIP32 root key cannot be reversed back to the BIP39 mnemonic.
+Most wallets implement BIP32 which defines how a BIP32 root key can be used to derive keychains. As a consequence, a backup of just the BIP32 root key is sufficient to include all keys derived from it. BIP32 does not have a human-friendly serialization of the BIP32 root key (or BIP32 extended keys in general), which makes paper backups or manually restoring the key more error-prone. BIP39 was designed to solve this problem, but rather than serialize the BIP32 root key, it takes some entropy, encoded to a "seed mnemonic", which is then hashed to derive the BIP39 seed, which can be turned into the BIP32 root key. Saving the BIP39 mnemonic is enough to reconstruct the entire BIP32 keychain, but a BIP32 root key cannot be reversed back to the BIP39 mnemonic.
 
-Most wallets implement BIP39, so on initialization or restoration, the user must interact with a BIP39 mnemonic. Most wallets do not support BIP32 extended private keys, so each wallet must either share the same BIP39 mnemonic, or have a separate BIP39 mnemonic entirely. Neither scenarios are particularly satisfactory for security reasons. For example, some wallets may be inherently less secure like hot wallets on smartphones, Join Market servers, or Lightning Network nodes. Having multiple seeds is far from desirable, especially for those who rely on split key or redundancy backups in different geological locations. Adding is necessarily difficult and may result in users being more lazy with subsequent keys, resulting in compromised security or loss of keys.
+Most wallets implement BIP39, so on initialization or restoration, the user must interact with a BIP39 mnemonic. Most wallets do not support BIP32 extended private keys, so each wallet must either share the same BIP39 mnemonic, or have a separate BIP39 mnemonic entirely. Neither scenario is particularly satisfactory for security reasons. For example, some wallets may be inherently less secure, like hot wallets on smartphones, Join Market servers, or Lightning Network nodes. Having multiple seeds is far from desirable, especially for those who rely on split key or redundancy backups in different geological locations. Adding keys is necessarily difficult and may result in users being more lazy with subsequent keys, resulting in compromised security or loss of keys.
 
-There is added complication with wallets that implement other standards, or no standards at all. Bitcoin Core wallet uses a WIF as the ''hdseed'', and yet other wallets like Electrum use different mnemonic schemes to derive the BIP32 root key. Other cryptocurrencies like Monero also use an entirely different mnemonic scheme.
+There is an added complication with wallets that implement other standards, or no standards at all. The Bitcoin Core wallet uses a WIF as the ''hdseed'', and yet other wallets, like Electrum, use different mnemonic schemes to derive the BIP32 root key. Other cryptocurrencies, like Monero, use an entirely different mnemonic scheme.
 
 Ultimately, all of the mnemonic/seed schemes start with some "initial entropy" to derive a mnemonic/seed, and then process the mnemonic into a BIP32 key, or private key. We can use BIP32 itself to derive the "initial entropy" to then recreate the same mnemonic or seed according to the specific application standard of the target wallet. We can use a BIP44-like categorization to ensure uniform derivation according to the target application type.
 
@@ -47,9 +47,9 @@ Ultimately, all of the mnemonic/seed schemes start with some "initial entropy" t
 
 We assume a single BIP32 master root key. This specification is not concerned with how this was derived (e.g. directly or via a mnemonic scheme such as BIP39).
 
-For each application that requires its own wallet, a unique private key is derived from the BIP32 master root key using a fully hardened derivation path. The resulting private key (k) is then processed with HMAC-SHA512, where the key is "bip-entropy-from-k", and the message payload is the private key k: <code>HMAC-SHA512(key="bip-entropy-from-k", msg=k)</code>. The result produces 512 bits of entropy. Each application SHOULD use up to the required number of bits necessary for their operation truncating the rest.
+For each application that requires its own wallet, a unique private key is derived from the BIP32 master root key using a fully hardened derivation path. The resulting private key (k) is then processed with HMAC-SHA512, where the key is "bip-entropy-from-k", and the message payload is the private key k: <code>HMAC-SHA512(key="bip-entropy-from-k", msg=k)</code>. The result produces 512 bits of entropy. Each application SHOULD use up to the required number of bits necessary for their operation, and truncating the rest.
 
-The HMAC-SHA512 function is specified in [http://tools.ietf.org/html/rfc4231 RFC 4231].
+The HMAC-SHA512 function is specified in [https://tools.ietf.org/html/rfc4231 RFC 4231].

bip-0085.mediawiki Outdated Show resolved Hide resolved
bip-0085.mediawiki Outdated Show resolved Hide resolved
bip-0085.mediawiki Show resolved Hide resolved
bip-0085.mediawiki Outdated Show resolved Hide resolved
bip-0085.mediawiki Outdated Show resolved Hide resolved
bip-0085.mediawiki Outdated Show resolved Hide resolved
@akarve akarve requested a review from jonatack October 21, 2024 20:14
@akarve
Copy link
Contributor Author

akarve commented Oct 21, 2024

@jonatack thanks for the careful reviews. i think we're g2g. do committers squash and merge? if not should i rebase this? I was asked to preface all commits with BIP-85, and these commits are all orthogonal and semantic, but I personally would prefer a rebase because a lot of this is TMI for master, agree?

bip-0085.mediawiki Outdated Show resolved Hide resolved
bip-0085.mediawiki Outdated Show resolved Hide resolved
bip-0085.mediawiki Outdated Show resolved Hide resolved
bip-0085.mediawiki Outdated Show resolved Hide resolved
bip-0085.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.

do committers squash and merge? if not should i rebase this? I was asked to preface all commits with BIP-85, and these commits are all orthogonal and semantic, but I personally would prefer a rebase because a lot of this is TMI for master, agree?

People have different preferences. I prefer when the PR author organizes the commits in a logical, hygienic manner; and otherwise, I might squash in the merge to one commit.

bip-0085.mediawiki Outdated Show resolved Hide resolved
bip-0085.mediawiki Outdated Show resolved Hide resolved
bip-0085.mediawiki Outdated Show resolved Hide resolved
@@ -370,16 +429,42 @@ The reason for running the derived key through HMAC-SHA512 and truncating the re

Many thanks to Peter Gray and Christopher Allen for their input, and to Peter for suggesting extra application use cases.

==Change Log==
Copy link
Contributor Author

@akarve akarve Oct 22, 2024

Choose a reason for hiding this comment

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

@jonatack i've added a real changelog so that the semvers are more... semantic. i could go deeper in terms of detail (fixes, etc.) but this seems complete enough to be useful and importantly puts this commit at semver 1.3.0.

Copy link
Member

Choose a reason for hiding this comment

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

Suggest "Changelog" (no space), with entries ordered by most recent first (see https://keepachangelog.com/en/1.1.0/).

@akarve akarve requested a review from jonatack October 22, 2024 22:58
bip-0085.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.

Seems close, modulo a couple suggestions.

Pinging @scgbckbone @nvk @Rob1Ham @luisschwab for review/comments.

Reviewers: suggest looking at the whole change, not commit-by-commit.

* JavaScript library implementation: [https://github.com/hoganri/bip85-js]
* 1.3.0 Python 3.x library implementation: [https://github.com/akarve/bipsea]
* 1.1.0 Python 2.x library implementation: [https://github.com/ethankosakovsky/bip85]
* 1.0.0 JavaScript library implementation: [https://github.com/hoganri/bip85-js]
Copy link
Member

@jonatack jonatack Oct 24, 2024

Choose a reason for hiding this comment

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

Perhaps it would be easier for readers to reference these versions by moving this Reference Implementation section to just above the Changelog (and move the Acknowledgements section further down toward the end).

@@ -370,16 +429,42 @@ The reason for running the derived key through HMAC-SHA512 and truncating the re

Many thanks to Peter Gray and Christopher Allen for their input, and to Peter for suggesting extra application use cases.

==Change Log==
Copy link
Member

Choose a reason for hiding this comment

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

Suggest "Changelog" (no space), with entries ordered by most recent first (see https://keepachangelog.com/en/1.1.0/).

@jonatack jonatack added Proposed BIP modification and removed PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author labels Oct 25, 2024
Copy link
Contributor

@scgbckbone scgbckbone left a comment

Choose a reason for hiding this comment

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

ACK (but expect #1676 to follow)

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.

Thanks for looking @scgbckbone.

ACK 0e5f2da

@jonatack jonatack merged commit 8eac367 into bitcoin:master Oct 25, 2024
4 checks passed
@akarve akarve deleted the bip85-fixes branch October 25, 2024 15:48
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.

4 participants