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

Mention that BIP350 proposes to reduce the scope of BIP173 to Native Segwit v0 #1582

Merged

Conversation

murchandamus
Copy link
Contributor

@murchandamus murchandamus commented Apr 25, 2024

This is a follow-up to #945 to reduce the scope of BIP173 to native segwit v0.

I expect that you may have feedback about the phrasing, @sipa.

@murchandamus murchandamus marked this pull request as draft April 25, 2024 18:03
@sipa
Copy link
Member

sipa commented Apr 25, 2024

I think it suffices to add:

  • a comment about the discovered weakness in the checksum scheme
  • a reference to BIP-350 saying "Because of (that weakness), BIP-350 proposes using the scheme described in this BIP only for Witness v0 outputs."
  • A "Superseded-By: 350" header.

That avoids actually changing what BIP-173 proposed, even though we now know that wasn't ideal.

@murchandamus murchandamus force-pushed the 2024-04-reduce-bip173-to-v0 branch from 2be289c to 0f4c9e2 Compare April 25, 2024 18:10
bip-0173.mediawiki Outdated Show resolved Hide resolved
@murchandamus murchandamus force-pushed the 2024-04-reduce-bip173-to-v0 branch from 0f4c9e2 to 7a10449 Compare April 25, 2024 18:15
@murchandamus murchandamus marked this pull request as ready for review April 25, 2024 18:15
@sipa
Copy link
Member

sipa commented Apr 25, 2024

ACK 7a10449

@murchandamus murchandamus merged commit d402abb into bitcoin:master Apr 25, 2024
3 checks passed
@vostrnad
Copy link
Contributor

vostrnad commented Apr 25, 2024

Looks like you accidentally deleted most of the changes during a force push?

Edit: Never mind, looks like it wasn't accidental, however I feel like the PR title should have been updated.

@sipa
Copy link
Member

sipa commented Apr 25, 2024

@vostrnad I objected offline about the title change and other changes.

BIP173, for better or worse, still refers to the idea of using Bech32 for all Native Witness outputs. It has been largely supplanted by BIP350, and it now makes note of that, but we shouldn't be retroactively changing what the idea of BIP173 was.

Nevermind, you're referring to the PR title, not the BIP title?

@vostrnad
Copy link
Contributor

Yes, the PR title (and consequently the merge commit) still talks about changing the scope. Nothing serious, but I'd still prefer for future PRs to have just a little more time for outside review.

@murchandamus murchandamus changed the title Reduce scope of BIP173 to native segwit v0 Mention that BIP350 proposes to reduce the scope of BIP173 to Native Segwit v0 Apr 25, 2024
@murchandamus murchandamus deleted the 2024-04-reduce-bip173-to-v0 branch July 11, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants