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-373: denote different public key types/purposes consistently #1705

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

theStack
Copy link
Contributor

@theStack theStack commented Nov 29, 2024

While reviewing bitcoin/bitcoin#31247 a while ago I noticed that the mentioned public keys in the PSBT fields of this BIP are quite confusing. This is mostly due to a non-consistent mention of types (plain vs. x-only) and a purpose that is sometimes missing. This PR aims to improve this in the following ways:

  • be specific about the purpose of pubkey types in PSBT fields ("plain pubkey" alone doesn't say a lot, especially if it's used repeatedly within a field; explicitly call it "aggregate pubkey" or "participant pubkey" if applicable)
  • replace all uses of "plain pubkey" by "compressed pubkey" (the only category that should matter is whether the pubkey type is "x-only" or "plain")
  • use consistent word order, e.g. prefer "compressed aggregate public key" over "aggregate compressed public key"

Another possibility would be to even get rid of the "plain/compressed" terminology completely. From the fact that "x-only" is not explicitly mentioned and the size of 33 bytes it should be obvious that this is a plain public key.

@Sjors
Copy link
Member

Sjors commented Nov 29, 2024

replace all uses of "compressed pubkey" by "plain pubkey" (the only category that should matter is whether the pubkey type is "x-only" or "plain"; in the latter case, it's obvious from the size of 33 bytes that the key is compressed)

I find the term "compressed" more clear than "plain", so I don't think that part of the change is an improvement.

Similarly I find "compressed" vs "x-only" easier to understand than "plain 33 bytes" vs "x-only".

I would just change: "Why the plain aggregate public key instead of x-only?" to say "compressed".

@@ -46,53 +46,53 @@ The new per-input types are defined as follows:
| rowspan="2"|MuSig2 Participant Public Keys
| rowspan="2"|<tt>PSBT_IN_MUSIG2_PARTICIPANT_PUBKEYS = 0x1a</tt>
| <tt><33 byte plain aggregate pubkey></tt>
| <tt><33 byte compressed pubkey>*</tt>
| <tt><33 byte plain participant pubkey>*</tt>
Copy link
Member

Choose a reason for hiding this comment

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

Could also say "33 byte participant pubkey (compressed)"

Copy link
Member

@jonatack jonatack Dec 5, 2024

Choose a reason for hiding this comment

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

I like keeping "compressed" as well, maybe consistently throughout in the manner @Sjors suggests here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done as suggested. Not really fond of the rendering now:
image
But not sure how to improve that either.

| rowspan="2"|
| rowspan="2"|
| rowspan="2"| 0, 2
|-
| The MuSig2 aggregate plain public key<ref>'''Why the plain aggregate public key instead of x-only?'''
| The MuSig2 plain aggregate public key<ref>'''Why the plain aggregate public key instead of x-only?'''
Copy link
Member

Choose a reason for hiding this comment

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

Maybe "The MuSig2 aggregate public key (compressed)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

BIP 32 requires public keys to include their evenness byte. Aggregate public keys are expected to be
derived from, following [[bip-0328.mediawiki|BIP 328]], and therefore will
need to include the evenness. Furthermore, PSBT_IN_TAP_BIP32_DERIVATION fields include fingerprints
to identify master keys, and these fingerprints require full compressed public keys. By including
to identify master keys, and these fingerprints require full plain public keys. By including
Copy link
Member

Choose a reason for hiding this comment

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

require the y-coordinate of the public key, so x-only serialisation can't be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Included.

@theStack
Copy link
Contributor Author

theStack commented Nov 29, 2024

replace all uses of "compressed pubkey" by "plain pubkey" (the only category that should matter is whether the pubkey type is "x-only" or "plain"; in the latter case, it's obvious from the size of 33 bytes that the key is compressed)

I find the term "compressed" more clear than "plain", so I don't think that part of the change is an improvement.

I think more important than the concrete term is to stick to one and use it consistently, so I'm also fine with changing all occurences to "compressed" if there are no objections. My impression was that "compressed" is significantly less used now than it was in the past (when there was only 65 vs. 33 bytes), especially since x-only pubkeys could then be seen as "even more compressed" 😅

// EDIT: slightly off-topic for the BIP repo here, but if we change everything to "compressed", the RPC help for decodepsbt (bitcoin/bitcoin@abb8228) should probably updated to use that as well for consistency

@Sjors
Copy link
Member

Sjors commented Nov 29, 2024

My impression was that "compressed" is significantly less used now than it was in the past (when there was only 65 vs. 33 bytes), especially since x-only pubkeys could then be seen as "even more compressed" 😅

That's true, but there's no other word for x-inclusive key.

@jonatack jonatack assigned jonatack and unassigned jonatack Dec 5, 2024
@jonatack jonatack requested a review from achow101 December 5, 2024 02:02
@jonatack jonatack added the Pending acceptance This BIP modification requires sign-off by the champion of the BIP being modified label Dec 5, 2024
Improve the clarity of the BIP w.r.t. pubkeys in the following ways:
- be specific about the purpose of pubkey types in PSBT fields
  ("plain pubkey" alone doesn't say a lot, especially if it's used
   repeatedly within a field)
- replace all uses of "plain pubkey" by "compressed pubkey"
  (the only category that should matter is whether the pubkey type
   is "x-only" or "plain")
- use consistent word order, e.g. prefer
  "compressed aggregate public key" over "aggregate compressed public key"
@theStack theStack force-pushed the bip373_improve_pubkey_clarity branch from 953e807 to 50e8208 Compare December 5, 2024 23:47
@achow101
Copy link
Member

achow101 commented Dec 6, 2024

"Plain pubkey" is the terminology used by BIP 327, hence its usage here. However, if "compressed pubkey" is clearer to readers, then I'm find with that.

ACK 50e8208

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.

LGTM. @Sjors?

bip-0373.mediawiki Show resolved Hide resolved
@jonatack jonatack removed the Pending acceptance This BIP modification requires sign-off by the champion of the BIP being modified label Dec 7, 2024
@jonatack jonatack requested a review from Sjors December 7, 2024 16:16
@Sjors
Copy link
Member

Sjors commented Dec 9, 2024

ACK 50e8208

@jonatack jonatack merged commit 45f2934 into bitcoin:master Dec 9, 2024
4 checks passed
@theStack theStack deleted the bip373_improve_pubkey_clarity branch December 9, 2024 08:44
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