refactor: split `ge_parse` into explicit variants (compressed, uncompressed, uncompressed+hybrid) #1918

pull theStack wants to merge 1 commits into bitcoin-core:master from theStack:split-ge_parse changing 6 files +52 −31

theStack commented at 4:27 PM on August 19, 2026: contributor

This PR is a follow-up to #1915, see the suggestion #1915#pullrequestreview-4962713401.

Splits the ge_parse function into three explicit variants for parsing from: - compressed public keys (33-bytes, prefixes 0x02 or 0x03) - uncompressed public keys (65-bytes, prefix 0x04) - uncompressed and hybrid public keys (65-bytes, prefixes 0x04, 0x06 or 0x07)

The first two are the counter-parts for the existing serialization functions ge_serialize{33,65}. The latter one is only needed for legacy reasons and likely not needed in the future. Having it named explicitly helps to avoid accepting hybrid pubkeys unintentionally. ge_parse65 is currently unused outside of tests, but one potential use-case would be the serialization of the prevouts_summary in the silentpayments light-client API PR #1912 (see commit https://github.com/bitcoin-core/secp256k1/pull/1912/changes/82b95fde6fe2c75ec940ade339013939b8ecc5ea), unless we want to define a custom serialization there (where we e.g. remove the constant prefix byte).
theStack added the label tweak/refactor on Aug 19, 2026
theStack renamed this:
~~refactor: split `ge_parse` into explicit variants~~
refactor: split `ge_parse` into explicit variants (compressed, uncompressed, uncompressed+hybrid)
on Aug 19, 2026
in src/secp256k1.c:270 in 8cfd142f1f
```
 266 | @@ -267,12 +267,18 @@ static void secp256k1_pubkey_save(secp256k1_pubkey* pubkey, secp256k1_ge* ge) {
 267 |  
 268 |  int secp256k1_ec_pubkey_parse(const secp256k1_context* ctx, secp256k1_pubkey* pubkey, const unsigned char *input, size_t inputlen) {
 269 |      secp256k1_ge Q;
 270 | +    int res;
```
fjahr commented at 9:32 PM on August 19, 2026:

super nit: I think it's usually ret?

theStack commented at 12:19 AM on August 20, 2026:

Seems that in API functions we use ret almost exclusively for the to-be-returned value (i.e. paired with a return ret; at the bottom), which doesn't apply here. res is not a great name either, decided to be explicit and name it is_pubkey_valid (inspired by existing is_sec_valid instances in the codebase).
in src/secp256k1.c:277 in 8cfd142f1f
```
 273 |      ARG_CHECK(pubkey != NULL);
 274 |      memset(pubkey, 0, sizeof(*pubkey));
 275 |      ARG_CHECK(input != NULL);
 276 | -    if (!secp256k1_ge_parse(&Q, input, inputlen)) {
 277 | +    switch (inputlen) {
 278 | +        case 33: res = secp256k1_ge_parse33(&Q, input); break;
```
fjahr commented at 9:34 PM on August 19, 2026:

I like this concise style, just wondering why it's never been used anywhere outside of test code. I guess there is no particular reason?

theStack commented at 12:20 AM on August 20, 2026:

Fair question, maybe there was no good use-case for it so far, but not sure. Seems there is one switch/case statement in secp256k1_pippenger_bucket_window_inv, so there is at least no (unwritten) rule to completely avoid it in (potential) production code. No strong opinion, also happy to change to if/else if other reviewers feel strongly.

In the test I've changed to using a ternary expression, since the default branch is not needed anymore after your suggestion below to limit to 33/65 byte-sized pubkey serialization, so the switch/case became a bit less compelling.
in src/tests.c:6846 in 8cfd142f1f
```
6839 | @@ -6840,6 +6840,7 @@ static void test_random_pubkeys(void) {
6840 |      secp256k1_ge elem;
6841 |      secp256k1_ge elem2;
6842 |      unsigned char in[65];
6843 | +    int res;
6844 |      /* Generate some randomly sized pubkeys. */
6845 |      size_t len = testrand_bits(2) == 0 ? 65 : 33;
6846 |      if (testrand_bits(2) == 0) {
```
fjahr commented at 9:40 PM on August 19, 2026:

With the switch statement added further below I think this randomness here doesn't make sense anymore. If len isn't 33 or 65 we just end up hitting the default case there and we are not even calling any library function in the test then.

theStack commented at 12:20 AM on August 20, 2026:

Good point, pretty pointless now to generate differently-sized serializations and not do anything with them indeed. Adapted and simplified the test accordingly.
fjahr commented at 9:40 PM on August 19, 2026: contributor

Concept ACK

refactor: split `ge_parse` helper into explicit variants

Split into explicit variants for parsing from:
    - compressed public keys (33-bytes, prefix 0x02 or 0x03)
    - uncompressed public keys (65-bytes, prefix 0x04)
    - uncompressed and hybrid public keys (65-bytes, prefix 0x04, 0x06 or 0x07)

The latter one is only needed for legacy reasons and likely not needed
in the future. Having it named explicitly helps to avoid accepting
hybrid pubkeys unintentionally.

The `test_random_pubkeys` unit test is simplified; considering that
there is no way to call internal `_ge_parse` with variable pubkey
lengths anymore (serialization length is now implicit per function
name), there is also no point in generating different lengths than the
supported 33 or 65 anymore. Thanks to Fabian Jahr for the suggestion.

42dd54e46e

theStack force-pushed on Aug 20, 2026
theStack commented at 12:23 AM on August 20, 2026: contributor

Thanks for the detailed review @fjahr, addressed your suggestion of simplifying/adapting the test and renamed the res variable in the touched pubkey parsing API function.

Contributors

theStack

fjahr

Labels

tweak/refactor

Linked (view graph)

#1912 silentpayments: add light client API #1915 refactor: Move (de)ser helpers from musig and eckey to group