refactor: split up internal pubkey serialization function into compressed/uncompressed variants #1774
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
w0xlt
approved these changes
Nov 18, 2025
furszy
reviewed
Nov 18, 2025
Member
furszy
left a comment
furszy
left a comment
There was a problem hiding this comment.
Looks good, just left a minor nit. No need to tackle it.
Comment on lines
282
to
292
| if (secp256k1_pubkey_load(ctx, &Q, pubkey)) { | ||
| ret = secp256k1_eckey_pubkey_serialize(&Q, output, &len, !!(flags & SECP256K1_FLAGS_BIT_COMPRESSION)); | ||
| if (ret) { | ||
| *outputlen = len; | ||
| if (flags & SECP256K1_FLAGS_BIT_COMPRESSION) { | ||
| secp256k1_eckey_pubkey_serialize33(&Q, output); | ||
| *outputlen = 33; | ||
| } else { | ||
| secp256k1_eckey_pubkey_serialize65(&Q, output); | ||
| *outputlen = 65; | ||
| } | ||
| ret = 1; | ||
| } | ||
| return ret; |
Member
There was a problem hiding this comment.
In adb76f8:
nit: could remove ret if you write it as:
if (!secp256k1_pubkey_load(ctx, &Q, pubkey)) return 0;
if (flags & SECP256K1_FLAGS_BIT_COMPRESSION) {
secp256k1_eckey_pubkey_serialize33(&Q, output);
*outputlen = 33;
} else {
secp256k1_eckey_pubkey_serialize65(&Q, output);
*outputlen = 65;
}
return 1;
Contributor
Author
There was a problem hiding this comment.
Makes sense yeah, will do if I have to retouch
Eunovo
added a commit
to Eunovo/bitcoin
that referenced
this pull request
Dec 1, 2025
f96f41f24f docs: update README c86ef7b21b ci: enable silentpayments module 8454a44a69 tests: add sha256 tag test a0559f55aa tests: add constant time tests 0d085b8616 tests: add BIP-352 test vectors d5f93574b8 silentpayments: add benchmarks for scanning 8edb04dd23 silentpayments: add examples/silentpayments.c 8caf19c3ac silentpayments: receiving 913fdee7e1 silentpayments: recipient label support ffffd7ff98 silentpayments: sending 8256fb3f41 build: add skeleton for new silentpayments (BIP352) module e7f7083b53 Merge bitcoin-core/secp256k1#1774: refactor: split up internal pubkey serialization function into compressed/uncompressed variants b6c2a3cd77 Merge bitcoin-core/secp256k1#1761: ecmult_multi: reduce strauss memory usage by 30% f5e815f430 remove secp256k1_eckey_pubkey_serialize function 0d3659c547 use new `_eckey_pubkey_serialize{33,65}` functions in modules (ellswift,musig) adb76f82ea use new `_eckey_pubkey_serialize{33,65}` functions in public API fc7458ca3e introduce `secp256k1_eckey_pubkey_serialize{33,65}` functions c8206b1ce6 Merge bitcoin-core/secp256k1#1771: ci: Use Python virtual environment in "x86_64-macos-native" job f252da7e6e ci: Use Python virtual environment in "x86_64-macos-native" job 115b135fe8 Merge bitcoin-core/secp256k1#1763: bench: Use `ALIGNMENT` macro instead of hardcoded value 153eea20c2 bench: Use `ALIGNMENT` macro instead of hardcoded value 26166c4f5f ecmult_multi: reduce strauss memory usage by 30% 7a2fff85e8 Merge bitcoin-core/secp256k1#1758: ci: Drop workaround for Valgrind older than 3.20.0 43e7b115f7 Merge bitcoin-core/secp256k1#1759: ci: Switch to macOS 15 Sequoia Intel-based image 8bc50b72ff ci: Switch to macOS 15 Sequoia Intel-based image c09519f0e3 ci: Drop workaround for Valgrind older than 3.20.0 git-subtree-dir: src/secp256k1 git-subtree-split: f96f41f24f8a43384e57a04d1cb73798c579b59a
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR splits up the pubkey serialization function
secp256k1_eckey_pubkey_serializeinto two variants for the compressed (33 bytes) and uncompressed (65 bytes) public key output format each, where only non-infinity group elements as input are allowed. The motivation is to simplify call-sites significantly, as they currently need to introduce two variables and a VERIFY_CHECKs on the return value and the in/out size parameter within a pre-processor block, typically leading to 8 lines of code. By using the new functions, the code is reduced to a single line of code that just calls the function (see #1773). This is helpful for already existing modules on master (ellswift, musig) and upcoming ones (silentpayments, see #1765).One drawback is that the public API function
secp256k1_ec_pubkey_serializeis now slightly more complex (we now call one of two functions instead of a single one, depending on whether the compressed flag is set or not), but that should hopefully not be a problem.The commits are intentionally kept small to ease review, happy to squash them if that is preferred.
(Kudos to w0xlt for the initial idea (#1765 (review)) and to real-or-random for the suggestion to split the already existing function (#1773 (comment)).)