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

[fjahr]

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]

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.

[theStack]

Concept ACK

Agree that group seems a reasonable place to move the (de)serialization functions to.

[fjahr]

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.

[real-or-random]

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]

.

[fjahr]

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]

ACK bfd40bd

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]

utACK bfd40bd