Skip to content

BIP39: update status from Proposed to Final#1677

Merged
murchandamus merged 1 commit into
bitcoin:masterfrom
scgbckbone:bip39_final
Nov 8, 2024
Merged

BIP39: update status from Proposed to Final#1677
murchandamus merged 1 commit into
bitcoin:masterfrom
scgbckbone:bip39_final

Conversation

@scgbckbone

Copy link
Copy Markdown
Contributor
  • also widely deployed

Comment thread bip-0039.mediawiki
Comment thread bip-0039.mediawiki Outdated
Aaron Voisine <voisine@gmail.com>
Sean Bowe <ewillbefull@gmail.com>
Comments-Summary: Unanimously Discourage for implementation
Comments-Summary: No comments yet.

@jonatack jonatack Oct 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For context, the "Unanimously Discourage for implementation" assessment was added as part of a wider change in #500. Seems fine to update if out of date / community agrees. Correcting myself, it depends on the content of the comments wiki page as mentioned in #1413 (comment).

More info per BIP 2:

Summary tones may be chosen from the following, but this BIP does not intend to cover all possible nuances and other summaries may be used as needed:

* No comments yet.
* Unanimously Recommended for implementation
* Unanimously Discourage for implementation
* Mostly Recommended for implementation, with some Discouragement
* Mostly Discouraged for implementation, with some Recommendation

.../...

To avoid doubt: comments and status are unrelated metrics to judge a BIP, and neither should be directly influencing the other.

@apoelstra apoelstra Oct 5, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for putting this clearly in the title of your PR, unlike the last attempt to remove this language #1413 (which I eventually had to find using git log --all).

But I don't think you'll get "community agreement" on deleting this language entirely.

@jonatack jonatack Oct 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@apoelstra thank you for the link to #1413 for more context.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But I don't think you'll get "community agreement" on deleting this language entirely.

even tho I agree to what is stated in #1413, I think the ship has already sailed and most of the wallets already implement it. "Unanimously Discourage for implementation" was therefore pointless back then and even more now.

If necessary, it would be better to add a Shortcomings(or similar) section to this BIP and "copy" the comment from #1413 which at least give reader some useful info, instead of just fuding with "Unanimously Discourage for implementation"

@jonatack jonatack Oct 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@scgbckbone I've edited my comments above. It may be a good idea to drop this here and discuss a change to the comment summary in a separate PR (assuming that updating the comment summary isn't incompatible with a Final status), and/or add relevant expert review to the wiki.

If necessary, it would be better to add a Shortcomings(or similar) section to this BIP

Agree, i.e. per @apoelstra's suggestions in #1413 (comment), perhaps best to do that before updating the BIP status to Final.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While I agree that the comments feature of the existing BIP Process has failed to be meaningfully adopted, I don’t think this change is in line with the currently active process. I would agree with BIP 39 being moved to Final, but I would suggest that you defer the change regarding the comment summary until a new BIP Process is adopted that supports the removal (as e.g. the one that I have been working on would).

@jonatack jonatack changed the title change BIP39 status to Final and remove discourage comment BIP39: update status to Final, remove discourage comment summary Oct 5, 2024
@scgbckbone scgbckbone force-pushed the bip39_final branch 2 times, most recently from ac2aa8e to f039233 Compare October 5, 2024 14:03
@jonatack jonatack added the Proposed BIP modification PR by non-owner to update BIP content label Oct 5, 2024
@jonatack

jonatack commented Oct 5, 2024

Copy link
Copy Markdown
Member

cc BIP author @prusnak

@katesalazar

Copy link
Copy Markdown
Contributor

NAK to changes out of process

@prusnak

prusnak commented Oct 6, 2024

Copy link
Copy Markdown
Contributor

Honestly, I could not care less what this repository says for this BIP. Reality is that literally all hardware wallets and vast majority of software wallets use BIP39.

The BIP process back in the days was a disaster.
We were forced to add certain features to the BIP (which we did not like) to satisfy the reviewers, then they said they did not like the changes anyway and they will not recommend using it. All this negative feedback happened more than a half a year ago after we started to use BIP39 in Trezor, so we could not do any further changes.

I recommend all wallets to consider using SLIP39 which has better wordlist, better checksum and can generate shamir secret shares. The only drawback is that the backup is now 20 words instead of 12 (for 128 bit of entropy).

Comment thread bip-0039.mediawiki Outdated
Aaron Voisine <voisine@gmail.com>
Sean Bowe <ewillbefull@gmail.com>
Comments-Summary: Unanimously Discourage for implementation
Comments-Summary: No comments yet.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While I agree that the comments feature of the existing BIP Process has failed to be meaningfully adopted, I don’t think this change is in line with the currently active process. I would agree with BIP 39 being moved to Final, but I would suggest that you defer the change regarding the comment summary until a new BIP Process is adopted that supports the removal (as e.g. the one that I have been working on would).

@murchandamus murchandamus added the PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author label Nov 7, 2024
@scgbckbone

Copy link
Copy Markdown
Contributor Author

I've reverted the comment back & rebased. Moving to final is "good enough" for now.

@jonatack jonatack changed the title BIP39: update status to Final, remove discourage comment summary BIP39: update status from Proposed to Final Nov 8, 2024

@jonatack jonatack left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK 829afcc

@jonatack jonatack removed the PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author label Nov 8, 2024
@jonatack jonatack requested a review from murchandamus November 8, 2024 15:10

@murchandamus murchandamus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK 829afcc

@murchandamus murchandamus merged commit c58a428 into bitcoin:master Nov 8, 2024
@katesalazar

Copy link
Copy Markdown
Contributor

ACK 829afcc

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

Labels

Proposed BIP modification PR by non-owner to update BIP content

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants