BIP85: Clarify spec, correct test vectors, add Portuguese language code, add dice application #1600
Conversation
|
Glad to remove my implementation from reference implementations per #1580 but on review I think you will find that my implementation is more complete and unit-tested than the original. I was also unable to get the original to run at all after install. |
murchandamus
added
Proposed BIP modification
Pending acceptance
|
Paging @ethankosakovsky |
|
I will be happy to hear from @ethankosakovsky but he hasn't been active on GH since 2021. I have no idea if there's a procedure for this, nevertheless I volunteer to become a maintainer of this BIP if the author cannot be reached. |
akarve
changed the title
|
Hey @akarve, I’ve tried to send an email to Ethan, let’s see what comes back. If we do not hear from Ethan, it seems to me that BIP 2 provides an option for another champion to take over stewardship of an existing BIP when the original Champion falls off the face of the earth. |
|
@murchandamus Howdy. Any luck reaching Ethan? |
|
Hey @akarve, I was unable to reach @ethankosakovsky per the email address provided in this BIP. Sorry, there is very little precedent for this situation, so I’m still making this up as we go. We could have done this in parallel, beside trying to reach out directly, but I would like to propose the following next steps: Please post about your proposed changes on the Bitcoin Developer Mailing list, and point out that you volunteer to become the champion of BIP 85. You could also request in that email that if someone is in touch with Ethan, it would be appreciated if they could reach out to make him aware. Assuming that Ethan isn’t reactivated and nobody objects, we would then try to get some review for your changes, add you as a second BIP champion, and merge this PR. |
|
@murchandamus Done. The mailing list topic is "BIP-85 Champion Unreachable? Please weigh in." in case you'd like to comment. |
|
Yeah, I saw that. Could you please add yourself as a champion to the BIP in this PR? |
|
Will do. Can you advise as to whether champions for PRs in |
|
It may be useful to be consistent with bytes or bits in the document. Preferably use bits throughout? |
akarve
changed the title
Yeah, I've done that wherever possible though there are some cases where say BIP-39 uses bits or where one needs an odd number of bits so I've kept those cases as-is. I was more worried about changing the byte order for the XPRV application but went ahead with it in the name of consistency with BIP-32. |
|
@murchandamus hi, we about good to go here? lmk if i need to do anything for the failing "Diff checks" — it's objecting to changes to the header block, which are necessary in this rare case. |
There was a problem hiding this comment.
Thanks for your patience.
It would be best if you add yourself as a second author rather than replace Ethan.
Can you advise as to whether champions for PRs in
Status: Draftshould remedy inconsistencies with other BIPs, like this inconsistency with BIP-32 or leave them as is for backwards compatibility?
Given that this BIP was drafted more than four years ago, there seems to be a good chance that it is already in use. Insofar, it seems best to remain compatible with the original specification rather than fixing inconsistencies with other BIPs.
lmk if i need to do anything for the failing "Diff checks" — it's objecting to changes to the header block, which are necessary in this rare case.
Yeah, you will need to also update the table in the repository’s README to align it with your preamble’s fields.
The changes seem fine to me, but it would be great if some people that are more invested in the content of this BIP also reviewed it.
Compared to BIP-32, WIF is relatively unused. On the chance that there are security implications to the widely used BIP-32 order I stuck with BIP-32 ordering. I can change this if the reviewers don't agree. |
There was a problem hiding this comment.
Given that this BIP was drafted more than four years ago, there seems to be a good chance that it is already in use. Insofar, it seems best to remain compatible with the original specification rather than fixing inconsistencies with other BIPs.
Compared to BIP-32, WIF is relatively unused. On the chance that there are security implications to the widely used BIP-32 order I stuck with BIP-32 ordering. I can change this if the reviewers don't agree.
I had the impression that the scheme had not changed significantly (first 32 bytes for the chain code, last 32 bytes for the private key). Or is this about the endianness?
If it did change, especially in a backwards incompatible manner, could you please document the changes of the Specification in a Change Log as seen for example in BIP 352?
jonatack
added
the
PR Author action required
|
@murchandamus Change Log added. We should be good to go :) Thanks for your assistance in getting this over the line. |
jonatack
removed
the
Pending acceptance
akarve
force-pushed
the
master
branch
3 times, most recently
from
5c71ffb
to
e076afb
Compare
* Add new maintainer (author unreachable)
* Swap chain code and private key bytes in application 32' for consistentcy with BIP-32 (major change)
* Correct derived entropy for application 128169' test vector (major change)
* Clarify big endian serialization
* Add the Portuguese language (9') to application 39'
* Add dice application 89101'
* Clarify Testnet support for XPRV application 32'
* Minor grammar, format, clarity improvements
|
@jonatack Thank you. I turned around your feedback and rebased into a single commit for clarity. |
| * JavaScript library implementation: [https://github.com/hoganri/bip85-js] | ||
| * 2.0 Python library implementation: [https://github.com/akarve/bipsea] | ||
| * 1.0 Python library implementation: [https://github.com/ethankosakovsky/bip85] | ||
| * 1.0 JavaScript library implementation: [https://github.com/hoganri/bip85-js] |
There was a problem hiding this comment.
A few ideas for a follow-up:
- move this section to just above the change log, and/or more clearly mention that 2.0 and 1.0 refer to versions of this BIP
or
- consider whether it makes sense to drop the 1.0 versions from this list
| * Add the Portuguese language (9') to application 39' | ||
| * Add dice application 89101' | ||
| * Clarify Testnet support for XPRV application 32' | ||
| * Minor grammar, format, clarity improvements |
There was a problem hiding this comment.
For another pull, it may be good to standardize the various BIPs change logs on the same order (personally prefer to order by descending date, i.e. most recent first).
Edit: for what it's worth, descending date is also the recommended order in https://keepachangelog.com/en/1.1.0/
jonatack
removed
the
PR Author action required
|
what is this ? breaking changes after after 3 people talking for 4 months ?
BS #1344 (comment) (you'd just need to wait bit more, which you should if you're doing breaking changes, anyways) |
|
NACK |
|
Although the BIP is still in Draft status, I think it should have been marked as proposed or final a long time ago as it does appear to be deployed by a few projects. This should be well past the stage where breaking changes can be implemented. The breaking changes should have been discussed on the mailing list, and I think should have been in a new BIP which could supercede 85. Furthermore, the BIPs process currently has no way to distinguish BIP versions and the way that has been implemented here is confusing. I think this PR should be reverted and the breaking changes should be first discussed on the mailing list and with the projects that have already implemented BIP 85. I agree that the original author is unresponsive enough to justify a change of BIP champion, but I don't think that should mean the new champion can add any breaking changes that they wish. |
he's only unresponsive on GH, if BIP2 is followed and email sent, he'd resurface imo (as last time) |
|
NACK |
|
revert breaking changes PR #1673 |
#1600 (comment) and #1600 (comment) suggests that email was in fact tried, multiple times, and they were unreachable. |
Correct. Moreover an email was sent to the bitcoin dev list looking for him with no results. My understanding is that we followed BIP2 in this regard. |
Will propose that in a week if not proposed by someone before. Thank you everyone for the feedback. |
|
I've opened a full revert in #1674 to provide a clean slate. Any desirable changes from this pull can be proposed separately. |
Summary of changes:
Unit tests for all substantive changes can be found in the reference implementation here:
akarve/bipsea#63