refactor: Move (de)ser helpers from musig and eckey to group #1915

pull fjahr wants to merge 3 commits into bitcoin-core:master from fjahr:2026-08-eckey-helper changing 10 files +114 −107

fjahr commented at 11:07 AM on August 15, 2026: contributor

These two helpers, secp256k1_musig_ge_serialize_ext and secp256k1_musig_ge_parse_ext, aren't really musig-specific. They (de)serialize a group element that can also be the point at infinity. This moves them to eckey as secp256k1_eckey_serialize_ext and secp256k1_eckey_parse_ext. They can't move further down to group.h because they depend on other eckey functions internally. There should be no functional change here.

I stumbled upon this while working on #1754 where I would want to reuse these helpers in that module, so it is split out of that. But the change seems to make sense on its own as well, so I wouldn't consider this prep work alone.
real-or-random added the label tweak/refactor on Aug 15, 2026
real-or-random commented at 1:05 PM on August 15, 2026: contributor

I agree that these helpers should not be specific to musig, so Concept ACK on moving them out. But the question is where to move them.

I had never considered the purpose of eckey before. What distinguishes an "EC (pub)key" from a "group element" in terms of implementation? If anything at all -- and I believe this was the intent of the author -- then precisely that a pubkey is not infinity. And indeed, all the functions in eckey follow this rule. It's somehow the internal backend for the secp256k1_pubkey type.

So in some sense, eckey is a very wrong place for these functions to end up. (Moreover, in musig, these ges are not really "keys".) But I totally see your reasoning, namely that the other (de)serialization functions are there.

But if not eckey, what should we do instead? Making a eckey_ext? This seems overkill.

I think one way out is to move all (de)serialization functions to group. In the end, the difference between a group element and a key is not that big. The only restriction is that the input of serialization may not be infinity, but we have other functions in group with this restriction. (We could additionally rename eckey to something which describes the remains more meaningfully, e.g., pubkey_tweak, but I'm not sure if it's worth the hassle.)

Alternatively, we could make separate group_ser module, but this feels also a bit unnatural to me.
refactor: Move pubkey parsing and serialization from eckey to group 2577fb1888
fjahr force-pushed on Aug 16, 2026
fjahr renamed this:
~~refactor: Move extended eckey helpers from musig to eckey~~
refactor: Move (de)ser helpers from musig and eckey to group
on Aug 16, 2026
theStack commented at 5:08 PM on August 16, 2026: contributor

Concept ACK

Agree that group seems a reasonable place to move the (de)serialization functions to.
fjahr commented at 6:43 PM on August 16, 2026: contributor

I think one way out is to move all (de)serialization functions to group.

Done in a new first commit, that also takes care of the dependency issue I mentioned in the description. I had thought about moving to group as well but having the (de)ser functions there already made me think it was the better choice, aside from being a smaller change. But I agree that moving both to group is the most consistent approach.

(We could additionally rename eckey to something which describes the remains more meaningfully, e.g., pubkey_tweak, but I'm not sure if it's worth the hassle.)

Hm, not sure if I understand you correctly because pubkey_tweak doesn't seem to fit so well with the privkey functions. My suggestion (pushed in the third commit) would be to simply rename the files eckey.h -> eckey_tweak.h and leave the function names unchanged. That is a very easy to review change and seems to make everything pretty consistent unless you insist that the file name always need to appear exactly in the funciton name as a namespace. At least historically that doesn't always seem to be the case.

in src/group.h:214 in 103289a1b4

 209 | + *  otherwise outputs the compressed serialization */
 210 | +static void secp256k1_ge_serialize_ext(unsigned char *out33, secp256k1_ge* ge);
 211 | +
 212 | +/** Outputs the point at infinity if the given byte array is all zero, otherwise
 213 | + *  attempts to parse compressed point serialization. */
 214 | +static int secp256k1_ge_parse_ext(secp256k1_ge* ge, const unsigned char *in33);

real-or-random commented at 9:13 AM on August 17, 2026:

While we're at it, can we rename these to secp256k1_ge_serialize_ext33 and secp256k1_ge_parse_ext33 for clarity and consistency?

fjahr commented at 1:36 PM on August 17, 2026:

Done

real-or-random commented at 9:19 AM on August 17, 2026: contributor
Hm, not sure if I understand you correctly because pubkey_tweak doesn't seem to fit so well with the privkey functions.

Oh, sure! I had missed this.

My suggestion (pushed in the third commit) would be to simply rename the files eckey.h -> eckey_tweak.h and leave the function names unchanged

That makes sense. Though now that I see it, I have a slight preference of dropping that commit entirely. (Sorry for the back and forth!) The renaming doesn't give us much except a better description of the module. But
- eckey is better if we want to add (non-serializiation) stuff in the future
- renaming is always annoying for git blame
Either way is fine with me in the end.

Also, if you want, you can add another commit that renames privkey in the eckey(_tweak) to seckey. We introduced this convention in the public API a while ago for consistency (#701, #1593), but it seems that we didn't entirely implement it internally. Fixing this seems to be a good fit for this reorganization PR, but don't feel urged to do it. It could certainly happen in another PR or never.
real-or-random commented at 9:19 AM on August 17, 2026: contributor

.

refactor: Move parsing helpers from musig to group

Also rename them to secp256k1_ge_serialize_ext33 and
secp256k1_ge_parse_ext33 to make the 33-byte format explicit.

d8ee6e6cf6

refactor: Rename privkey to seckey in eckey helpers bfd40bd0ce
fjahr force-pushed on Aug 17, 2026
fjahr commented at 1:43 PM on August 17, 2026: contributor

eckey is better if we want to add (non-serializiation) stuff in the future

Sure, that's a different perspective that I hadn't really considered. Dropped that part again.

add another commit that renames privkey in the eckey(_tweak) to seckey

Done
theStack approved
theStack commented at 3:17 PM on August 18, 2026: contributor

ACK bfd40bd0ce0e7cac4f6bb395ccfe90eaf625feb1

Unrelated to this refactoring PR, but noticing only now: what's a bit odd is that the ge serialize and parse functions (formerly _eckey_pubkey_{parse,serialize33,serialize65}) are not symmetric, as the latter also accepts hybrid pubkeys (i.e. 65 bytes with first byte being 0x06/0x07 rather than 0x04) as input, which we likely only ever need in a single API function (secp256k1_ec_pubkey_parse) for legacy reasons. Maybe a further split-up into e.g. "ge_parse33", "ge_parse65", and "ge_parse65_with_hybrid" would make sense for being explicit and symmetric, so the weird/exotic one stands out. As I'm writing this I notice that the silentpayments lightclient PR unintentionally accepts encodings in hybrid pubkey format for the prevouts summary.
real-or-random approved
real-or-random commented at 2:55 PM on August 19, 2026: contributor

utACK bfd40bd0ce0e7cac4f6bb395ccfe90eaf625feb1
theStack merged this on Aug 19, 2026
theStack closed this on Aug 19, 2026

Contributors

Labels

Linked (view graph)

#1754 BIP459: Add FullAgg (DahLIAS) module #1918 refactor: split `ge_parse` into explicit variants (compressed, uncompressed, uncompressed+hybrid)