← Watch feed
Merge bitcoin-core/secp256k1#1918: refactor: split `ge_parse` into explicit variants (compressed, uncompressed, uncompressed+hybrid)
Public commit record
100/100 · Strong
What the developer wrote
Authored by merge-script
Merge bitcoin-core/secp256k1#1918: refactor: split `ge_parse` into explicit variants (compressed, uncompressed, uncompressed+hybrid)
de02108c51ca052245c7ba154b02a890e6815fdb tests: cover compressed pubkey parsing/serialization in `_ec_pubkey_parse` test (Sebastian Falbesoner)
31b1b300e812038e5b52f534370f7bd0cae01a79 refactor: split `ge_parse` helper into explicit variants (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #1915, see the suggestion https://github.com/bitcoin-core/secp256k1/pull/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).
ACKs for top commit:
fjahr:
Code review ACK de02108c51ca052245c7ba154b02a890e6815fdb
real-or-random:
utACK de02108c51ca052245c7ba154b02a890e6815fdb
Tree-SHA512: 8bc92f6f53eb5ce3546ecf0a1fa72a909ee6231a08e25a3a9f4070e7d630379fc3e5b2f9bc6dea25726b7d9c97c2cfd8ee39be413328c4159134ec55ec1a59c6
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Explains rationale or failure mode✓ Mentions testing or verification✓ Links an issue, advisory, or supporting reference✓ Names security-relevant behavior explicitly
What changed, and why it matters
This commit is a code cleanup (refactor) that splits one internal public-key parsing helper into three clearly named versions. It does not change what keys the public API accepts or rejects, and it adds more tests. There is no security vulnerability being fixed here.
Recommended action
No security action required. Treat as normal code-quality / maintainability improvement.
Security signals we found
01
Refactor only: no change to accepted public-key formats or validation rules
02
Public API behavior preserved: 33-byte compressed and 65-byte uncompressed/hybrid still accepted
03
Internal fixed-size callers now use size-specific parser, reducing risk of accidental hybrid acceptance in future code
04
New tests added for compressed key parsing round-trip and edge cases
Evidence from the diff
The change replaces the single secp256k1_ge_parse(elem, pub, size) helper with explicit secp256k1_ge_parse33, secp256k1_ge_parse65, and secp256k1_ge_parse_with_hybrid65 variants. The public secp256k1_ec_pubkey_parse still accepts 33-byte compressed and 65-byte uncompressed/hybrid keys exactly as before. Internal callers (MuSig pubnonce parsing, Silent Payments label parsing, extended-key parsing) now call the 33-byte-only variant, matching their fixed-size inputs. The commit also expands test coverage for compressed key parsing and updates random-pubkey tests to use the new helpers. No cryptographic behavior is altered.
Changed components
src/group.hsrc/group_impl.hsrc/modules/musig/session_impl.hsrc/modules/silentpayments/main_impl.hsrc/secp256k1.csrc/tests.cInspect captured patch +117 / −34
### src/group.h
@@ -205,8 +205,15 @@ static void secp256k1_ge_to_bytes_ext(unsigned char *data, const secp256k1_ge *g
* provided buffer is the output of secp256k1_ge_to_bytes_ext. */
static void secp256k1_ge_from_bytes_ext(secp256k1_ge *ge, const unsigned char *data);
-/** Parse a group element from a 33-byte compressed or 65-byte uncompressed public key. */
-static int secp256k1_ge_parse(secp256k1_ge *elem, const unsigned char *pub, size_t size);
+/** Parse a group element from a 33-byte compressed public key (prefix byte is 0x02 or 0x03). */
+static int secp256k1_ge_parse33(secp256k1_ge *elem, const unsigned char *pub);
+
+/** Parse a group element from a 65-byte uncompressed public key (prefix byte is 0x04). */
+static int secp256k1_ge_parse65(secp256k1_ge *elem, const unsigned char *pub);
+
+/** Parse a group element from a 65-byte uncompressed or hybrid public key (prefix byte is 0x04, 0x06 or 0x07).
+ * Only intended for legacy public APIs that must accept hybrid keys. */
+static int secp256k1_ge_parse_with_hybrid65(secp256k1_ge *elem, const unsigned char *pub);
/** Serialize a group element (that is not allowed to be infinity) to a compressed public key (33 bytes). */
static void secp256k1_ge_serialize33(secp256k1_ge *elem, unsigned char *pub33);
### src/group_impl.h
@@ -1081,11 +1081,24 @@ static void secp256k1_ge_from_bytes_ext(secp256k1_ge *ge, const unsigned char *d
SECP256K1_GE_VERIFY(ge);
}
-SECP256K1_INLINE static int secp256k1_ge_impl_parse(secp256k1_ge *elem, const unsigned char *pub, size_t size) {
- if (size == 33 && (pub[0] == SECP256K1_TAG_PUBKEY_EVEN || pub[0] == SECP256K1_TAG_PUBKEY_ODD)) {
+SECP256K1_INLINE static int secp256k1_ge_impl_parse33(secp256k1_ge *elem, const unsigned char *pub) {
+ if (pub[0] == SECP256K1_TAG_PUBKEY_EVEN || pub[0] == SECP256K1_TAG_PUBKEY_ODD) {
secp256k1_fe x;
return secp256k1_fe_set_b32_limit(&x, pub+1) && secp256k1_ge_set_xo_var(elem, &x, pub[0] == SECP256K1_TAG_PUBKEY_ODD);
- } else if (size == 65 && (pub[0] == SECP256K1_TAG_PUBKEY_UNCOMPRESSED || pub[0] == SECP256K1_TAG_PUBKEY_HYBRID_EVEN || pub[0] == SECP256K1_TAG_PUBKEY_HYBRID_ODD)) {
+ } else {
+ return 0;
+ }
+}
+static int secp256k1_ge_parse33(secp256k1_ge *elem, const unsigned char *pub) {
+ int ret = secp256k1_ge_impl_parse33(elem, pub);
+ if (ret) {
+ SECP256K1_GE_VERIFY(elem);
+ }
+ return ret;
+}
+
+SECP256K1_INLINE static int secp256k1_ge_impl_parse_with_hybrid65(secp256k1_ge *elem, const unsigned char *pub) {
+ if (pub[0] == SECP256K1_TAG_PUBKEY_UNCOMPRESSED || pub[0] == SECP256K1_TAG_PUBKEY_HYBRID_EVEN || pub[0] == SECP256K1_TAG_PUBKEY_HYBRID_ODD) {
secp256k1_fe x, y;
if (!secp256k1_fe_set_b32_limit(&x, pub+1) || !secp256k1_fe_set_b32_limit(&y, pub+33)) {
return 0;
@@ -1100,8 +1113,23 @@ SECP256K1_INLINE static int secp256k1_ge_impl_parse(secp256k1_ge *elem, const un
return 0;
}
}
-static int secp256k1_ge_parse(secp256k1_ge *elem, const unsigned char *pub, size_t size) {
- int ret = secp256k1_ge_impl_parse(elem, pub, size);
+static int secp256k1_ge_parse_with_hybrid65(secp256k1_ge *elem, const unsigned char *pub) {
+ int ret = secp256k1_ge_impl_parse_with_hybrid65(elem, pub);
+ if (ret) {
+ SECP256K1_GE_VERIFY(elem);
+ }
+ return ret;
+}
+
+SECP256K1_INLINE static int secp256k1_ge_impl_parse65(secp256k1_ge *elem, const unsigned char *pub) {
+ if (pub[0] == SECP256K1_TAG_PUBKEY_UNCOMPRESSED) {
+ return secp256k1_ge_impl_parse_with_hybrid65(elem, pub);
+ } else {
+ return 0;
+ }
+}
+static int secp256k1_ge_parse65(secp256k1_ge *elem, const unsigned char *pub) {
+ int ret = secp256k1_ge_impl_parse65(elem, pub);
if (ret) {
SECP256K1_GE_VERIFY(elem);
}
@@ -1158,7 +1186,7 @@ SECP256K1_INLINE static int secp256k1_ge_impl_parse_ext33(secp256k1_ge *ge, cons
secp256k1_ge_set_infinity(ge);
return 1;
}
- if (!secp256k1_ge_parse(ge, in33, 33)) {
+ if (!secp256k1_ge_parse33(ge, in33)) {
return 0;
}
return secp256k1_ge_is_in_correct_subgroup(ge);
### src/modules/musig/session_impl.h
@@ -168,7 +168,7 @@ int secp256k1_musig_pubnonce_parse(const secp256k1_context* ctx, secp256k1_musig
ARG_CHECK(in66 != NULL);
for (i = 0; i < 2; i++) {
- if (!secp256k1_ge_parse(&ges[i], &in66[33*i], 33)) {
+ if (!secp256k1_ge_parse33(&ges[i], &in66[33*i])) {
return 0;
}
if (!secp256k1_ge_is_in_correct_subgroup(&ges[i])) {
### src/modules/silentpayments/main_impl.h
@@ -363,7 +363,7 @@ int secp256k1_silentpayments_recipient_label_parse(const secp256k1_context* ctx,
memset(label, 0, sizeof(*label));
ARG_CHECK(in33 != NULL);
- if (!secp256k1_ge_parse(&ge, in33, 33)) {
+ if (!secp256k1_ge_parse33(&ge, in33)) {
return 0;
}
### src/secp256k1.c
@@ -263,12 +263,18 @@ static void secp256k1_pubkey_save(secp256k1_pubkey* pubkey, secp256k1_ge* ge) {
int secp256k1_ec_pubkey_parse(const secp256k1_context* ctx, secp256k1_pubkey* pubkey, const unsigned char *input, size_t inputlen) {
secp256k1_ge Q;
+ int is_pubkey_valid;
VERIFY_CHECK(ctx != NULL);
ARG_CHECK(pubkey != NULL);
memset(pubkey, 0, sizeof(*pubkey));
ARG_CHECK(input != NULL);
- if (!secp256k1_ge_parse(&Q, input, inputlen)) {
+ switch (inputlen) {
+ case 33: is_pubkey_valid = secp256k1_ge_parse33(&Q, input); break;
+ case 65: is_pubkey_valid = secp256k1_ge_parse_with_hybrid65(&Q, input); break;
+ default: is_pubkey_valid = 0;
+ }
+ if (!is_pubkey_valid) {
return 0;
}
if (!secp256k1_ge_is_in_correct_subgroup(&Q)) {
### src/tests.c
@@ -6264,15 +6264,22 @@ static void run_ec_pubkey_parse_test(void) {
0xA8, 0xFD, 0x17, 0xB4, 0x48, 0xA6, 0x85, 0x54, 0x19, 0x9C, 0x47, 0xD0, 0x8F, 0xFB, 0x10, 0xD4,
0xB8, 0x00
};
+ const unsigned char pubkeyc_comp[34] = {
+ /* Compressed serialization of G (y is even, so prefix is 0x02). */
+ 0x02, 0x79, 0xBE, 0x66, 0x7E, 0xF9, 0xDC, 0xBB, 0xAC, 0x55, 0xA0, 0x62, 0x95, 0xCE, 0x87, 0x0B,
+ 0x07, 0x02, 0x9B, 0xFC, 0xDB, 0x2D, 0xCE, 0x28, 0xD9, 0x59, 0xF2, 0x81, 0x5B, 0x16, 0xF8, 0x17,
+ 0x98, 0x00
+ };
unsigned char sout[65];
unsigned char shortkey[2] = { 0 };
secp256k1_ge ge;
secp256k1_pubkey pubkey;
size_t len;
int32_t i;
- /* Nothing should be reading this far into pubkeyc. */
+ /* Nothing should be reading this far into pubkeyc and pubkeyc_comp. */
SECP256K1_CHECKMEM_UNDEFINE(&pubkeyc[65], 1);
+ SECP256K1_CHECKMEM_UNDEFINE(&pubkeyc_comp[33], 1);
/* Zero length claimed, fail, zeroize, no illegal arg error. */
memset(&pubkey, 0xfe, sizeof(pubkey));
SECP256K1_CHECKMEM_UNDEFINE(shortkey, 2);
@@ -6300,6 +6307,8 @@ static void run_ec_pubkey_parse_test(void) {
SECP256K1_CHECKMEM_CHECK(&pubkey, sizeof(pubkey));
CHECK_ILLEGAL(CTX, secp256k1_pubkey_load(CTX, &ge, &pubkey));
}
+
+ /* Uncompressed public key parsing and serialization. */
memset(&pubkey, 0xfe, sizeof(pubkey));
SECP256K1_CHECKMEM_UNDEFINE(&pubkey, sizeof(pubkey));
/* 33 bytes claimed on otherwise valid input starting with 0x04, fail, zeroize output, no illegal arg error. */
@@ -6358,6 +6367,44 @@ static void run_ec_pubkey_parse_test(void) {
CHECK(len == 65);
/* Multiple illegal args. Should still set arg error only once. */
CHECK_ILLEGAL(CTX, secp256k1_ec_pubkey_parse(CTX, NULL, NULL, 65));
+
+ /* Compressed public key parsing and serialization. */
+ /* 32 bytes claimed on otherwise valid compressed input starting with 0x02, fail, zeroize output, no illegal arg error. */
+ memset(&pubkey, 0xfe, sizeof(pubkey));
+ SECP256K1_CHECKMEM_UNDEFINE(&pubkey, sizeof(pubkey));
+ CHECK(secp256k1_ec_pubkey_parse(CTX, &pubkey, pubkeyc_comp, 32) == 0);
+ SECP256K1_CHECKMEM_CHECK(&pubkey, sizeof(pubkey));
+ CHECK_ILLEGAL(CTX, secp256k1_pubkey_load(CTX, &ge, &pubkey));
+ /* 34 bytes claimed on otherwise valid compressed input starting with 0x02, fail, zeroize output, no illegal arg error. */
+ memset(&pubkey, 0xfe, sizeof(pubkey));
+ SECP256K1_CHECKMEM_UNDEFINE(&pubkey, sizeof(pubkey));
+ CHECK(secp256k1_ec_pubkey_parse(CTX, &pubkey, pubkeyc_comp, 34) == 0);
+ SECP256K1_CHECKMEM_CHECK(&pubkey, sizeof(pubkey));
+ CHECK_ILLEGAL(CTX, secp256k1_pubkey_load(CTX, &ge, &pubkey));
+ /* Valid compressed parse at length 33. */
+ memset(&pubkey, 0, sizeof(pubkey));
+ SECP256K1_CHECKMEM_UNDEFINE(&pubkey, sizeof(pubkey));
+ CHECK(secp256k1_ec_pubkey_parse(CTX, &pubkey, pubkeyc_comp, 33) == 1);
+ CHECK(secp256k1_ec_pubkey_parse(secp256k1_context_static, &pubkey, pubkeyc_comp, 33) == 1);
+ SECP256K1_CHECKMEM_CHECK(&pubkey, sizeof(pubkey));
+ SECP256K1_CHECKMEM_UNDEFINE(&ge, sizeof(ge));
+ CHECK(secp256k1_pubkey_load(CTX, &ge, &pubkey) == 1);
+ SECP256K1_CHECKMEM_CHECK(&ge.x, sizeof(ge.x));
+ SECP256K1_CHECKMEM_CHECK(&ge.y, sizeof(ge.y));
+ SECP256K1_CHECKMEM_CHECK(&ge.infinity, sizeof(ge.infinity));
+ CHECK(secp256k1_ge_eq_var(&ge, &secp256k1_ge_const_g));
+ /* secp256k1_ec_pubkey_serialize with too small output buffer, illegal arg error. Length is left untouched. */
+ len = 32;
+ CHECK_ILLEGAL(CTX, secp256k1_ec_pubkey_serialize(CTX, sout, &len, &pubkey, SECP256K1_EC_COMPRESSED));
+ CHECK(len == 32);
+ /* Valid compressed serialization, must round-trip to the input. */
+ len = 33;
+ SECP256K1_CHECKMEM_UNDEFINE(sout, 65);
+ CHECK(secp256k1_ec_pubkey_serialize(CTX, sout, &len, &pubkey, SECP256K1_EC_COMPRESSED) == 1);
+ SECP256K1_CHECKMEM_CHECK(sout, 33);
+ CHECK(len == 33);
+ CHECK(secp256k1_memcmp_var(sout, pubkeyc_comp, 33) == 0);
+
/* Try a bunch of prefabbed points with all possible encodings. */
for (i = 0; i < SECP256K1_EC_PARSE_TEST_NVALID; i++) {
ec_pubkey_parse_pointtest(valid[i], 1, 1);
@@ -6858,11 +6905,9 @@ static void test_random_pubkeys(void) {
secp256k1_ge elem;
secp256k1_ge elem2;
unsigned char in[65];
- /* Generate some randomly sized pubkeys. */
- size_t len = testrand_bits(2) == 0 ? 65 : 33;
- if (testrand_bits(2) == 0) {
- len = testrand_bits(6);
- }
+ int res;
+ /* Generate some random pubkeys with the two supported serialization sizes. */
+ size_t len = testrand_bits(1) == 0 ? 65 : 33;
if (len == 65) {
in[0] = testrand_bits(1) ? 4 : (testrand_bits(1) ? 6 : 7);
} else {
@@ -6871,17 +6916,14 @@ static void test_random_pubkeys(void) {
if (testrand_bits(3) == 0) {
in[0] = testrand_bits(8);
}
- if (len > 1) {
- testrand256(&in[1]);
- }
- if (len > 33) {
+ testrand256(&in[1]);
+ if (len == 65) {
testrand256(&in[33]);
}
- if (secp256k1_ge_parse(&elem, in, len)) {
+ res = (len == 33) ? secp256k1_ge_parse33(&elem, in) : secp256k1_ge_parse_with_hybrid65(&elem, in);
+ if (res) {
unsigned char out[65];
unsigned char firstb;
- int res;
- size_t size = len;
firstb = in[0];
/* If the pubkey can be parsed, it should round-trip... */
if (len == 33) {
@@ -6894,13 +6936,13 @@ static void test_random_pubkeys(void) {
if ((in[0] != 6) && (in[0] != 7)) {
CHECK(in[0] == out[0]);
}
- size = 65;
secp256k1_ge_serialize65(&elem, in);
- CHECK(secp256k1_ge_parse(&elem2, in, size));
+ CHECK(secp256k1_ge_parse65(&elem2, in));
CHECK(secp256k1_ge_eq_var(&elem2, &elem));
/* Check that the X9.62 hybrid type is checked. */
in[0] = testrand_bits(1) ? 6 : 7;
- res = secp256k1_ge_parse(&elem2, in, size);
+ CHECK(secp256k1_ge_parse65(&elem2, in) == 0);
+ res = secp256k1_ge_parse_with_hybrid65(&elem2, in);
if (firstb == 2 || firstb == 3) {
if (in[0] == firstb + 4) {
CHECK(res);
@@ -7556,7 +7598,7 @@ static void run_ecdsa_edge_cases(void) {
secp256k1_scalar_set_int(&ss, 1);
secp256k1_scalar_set_int(&msg, 0);
secp256k1_scalar_set_int(&sr, 0);
- CHECK(secp256k1_ge_parse(&key, pubkey_mods_zero, 33));
+ CHECK(secp256k1_ge_parse33(&key, pubkey_mods_zero));
CHECK(secp256k1_ecdsa_sig_verify( &sr, &ss, &key, &msg) == 0);
}
@@ -7575,7 +7617,7 @@ static void run_ecdsa_edge_cases(void) {
secp256k1_scalar_set_int(&ss, 0);
secp256k1_scalar_set_int(&msg, 0);
secp256k1_scalar_set_int(&sr, 1);
- CHECK(secp256k1_ge_parse(&key, pubkey, 33));
+ CHECK(secp256k1_ge_parse33(&key, pubkey));
CHECK(secp256k1_ecdsa_sig_verify(&sr, &ss, &key, &msg) == 0);
}
@@ -7602,8 +7644,8 @@ static void run_ecdsa_edge_cases(void) {
secp256k1_scalar_set_int(&ss, 2);
secp256k1_scalar_set_int(&msg, 0);
secp256k1_scalar_set_int(&sr, 2);
- CHECK(secp256k1_ge_parse(&key, pubkey, 33));
- CHECK(secp256k1_ge_parse(&key2, pubkey2, 33));
+ CHECK(secp256k1_ge_parse33(&key, pubkey));
+ CHECK(secp256k1_ge_parse33(&key2, pubkey2));
CHECK(secp256k1_ecdsa_sig_verify(&sr, &ss, &key, &msg) == 1);
CHECK(secp256k1_ecdsa_sig_verify(&sr, &ss, &key2, &msg) == 1);
secp256k1_scalar_negate(&ss, &ss);
@@ -7643,8 +7685,8 @@ static void run_ecdsa_edge_cases(void) {
secp256k1_scalar_set_int(&ss, 1);
secp256k1_scalar_set_int(&msg, 1);
secp256k1_scalar_set_b32(&sr, csr, NULL);
- CHECK(secp256k1_ge_parse(&key, pubkey, 33));
- CHECK(secp256k1_ge_parse(&key2, pubkey2, 33));
+ CHECK(secp256k1_ge_parse33(&key, pubkey));
+ CHECK(secp256k1_ge_parse33(&key2, pubkey2));
CHECK(secp256k1_ecdsa_sig_verify(&sr, &ss, &key, &msg) == 1);
CHECK(secp256k1_ecdsa_sig_verify(&sr, &ss, &key2, &msg) == 1);
secp256k1_scalar_negate(&ss, &ss);
@@ -7678,7 +7720,7 @@ static void run_ecdsa_edge_cases(void) {
secp256k1_scalar_set_int(&msg, 1);
secp256k1_scalar_negate(&msg, &msg);
secp256k1_scalar_set_b32(&sr, csr, NULL);
- CHECK(secp256k1_ge_parse(&key, pubkey, 33));
+ CHECK(secp256k1_ge_parse33(&key, pubkey));
CHECK(secp256k1_ecdsa_sig_verify(&sr, &ss, &key, &msg) == 1);
secp256k1_scalar_negate(&ss, &ss);
CHECK(secp256k1_ecdsa_sig_verify(&sr, &ss, &key, &msg) == 1);Risk score
Our methodology →Why this scored 15/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.