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

BIPs 174 and 375: fix PSBT_OUT_SP_V0_LABEL value #1754

Conversation

nymius
Copy link
Contributor

@nymius nymius commented Jan 31, 2025

Assuming a by one increment in the keytype of the silent payments output fields, the following numeral to 0x09 in the hexadecimal system is 0x0a, not 0x10.

Assuming a by one increment in the keytype of the silent payments output
fields, the following numeral to 0x09 in the hexadecimal system is 0x0a,
not 0x10.
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.

Pinging @andrewtoth for feedback.

@@ -128,7 +128,7 @@ The new per-output types are defined as follows:
| 2
|-
| Silent Payment Label
| <tt>PSBT_OUT_SP_V0_LABEL = 0x10</tt>
| <tt>PSBT_OUT_SP_V0_LABEL = 0x0a</tt>
Copy link
Member

Choose a reason for hiding this comment

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

Per git blame, this was added in 4a7a7cf.

@@ -715,7 +715,7 @@ required for aggregation. If sorting was done, then the keys must be in the sort
| [[bip-0375.mediawiki|375]]
|-
| Silent Payment Label
| <tt>PSBT_OUT_SP_V0_LABEL = 0x10</tt>
| <tt>PSBT_OUT_SP_V0_LABEL = 0x0a</tt>
Copy link
Member

Choose a reason for hiding this comment

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

Per git blame, this was added in commit eb10cdb.

@jonatack jonatack changed the title Typo in PSBT_OUT_SP_V0_LABEL: "decimal increment" instead of hexadecimal increment BIPs 174 and 375: fix PSBT_OUT_SP_V0_LABEL value Jan 31, 2025
@jonatack jonatack added the Pending acceptance This BIP modification requires sign-off by the champion of the BIP being modified label Jan 31, 2025
Copy link
Contributor

@andrewtoth andrewtoth left a comment

Choose a reason for hiding this comment

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

LGMT ACK a9729b2

Thanks for catching this!

@jonatack jonatack removed the Pending acceptance This BIP modification requires sign-off by the champion of the BIP being modified label Feb 6, 2025
@jonatack jonatack merged commit ea7aae8 into bitcoin:master Feb 6, 2025
4 checks passed
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.

ACK a9729b2

@murchandamus
Copy link
Contributor

murchandamus commented Feb 6, 2025

This is not a typo, this is a change in the specification. While it probably is acceptable, this PR should have waited for sign-off from the author of BIP 174, @achow101.

Now that I look at the context, the same would also have been true for #1687, although that at least was just specifying new fields whereas this PR is changing the definition of specified fields.

@jonatack
Copy link
Member

jonatack commented Feb 6, 2025

Indeed, my mistake.

@achow101
Copy link
Member

achow101 commented Feb 7, 2025

lgtm

I think it is okay to merge prs without my ack that update 174 to list new fields defined in another bip or that sync the definitions with their defining bip.

Perhaps there should be a different document with the fields list instead of updating 174.

@murchandamus
Copy link
Contributor

murchandamus commented Feb 7, 2025

@achow101: Thanks for your review. That’s probably a good way to look at it: if another BIP specifies a new field, that BIP is the authoritative document and the field specification is merely mirrored into BIP 174 for the convenience of the audience. In so far, it would be sufficient to get the sign-off of the author of the new BIP, and I might have been a bit too nitpicky.—I still think that adding a new field may need slightly less attention than changing an existing value, but all good, let’s move on.

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.

5 participants