Verify compressed argument in secp256k1_eckey_pubkey_serialize
#1642
Conversation
This file contains 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
Due to similarity to the public API function `secp256k1_ec_pubkey_serialize`, public API flags like `SECP256K1_EC_COMPRESSED` are sometimes mistakingly passed to newly proposed code (this is currently the case for several modules in secp256k1-zkp, see BlockstreamResearch/secp256k1-zkp#300). which is currently not detected. To avoid this in the future, a VERIFY_CHECK is added to check that the `compressed` argument is either 0 or 1.
real-or-random
approved these changes
Dec 9, 2024
| @@ -35,6 +36,8 @@ static int secp256k1_eckey_pubkey_parse(secp256k1_ge *elem, const unsigned char | |||
| } | |||
|
|
|||
| static int secp256k1_eckey_pubkey_serialize(secp256k1_ge *elem, unsigned char *pub, size_t *size, int compressed) { | |||
| VERIFY_CHECK(compressed == 0 || compressed == 1); | |||
There was a problem hiding this comment.
In other places, we simply rely on C's int-as-bool convention, so I suggest adding a comment that explains why we enforce 0/1 in this case.
stratospher
reviewed
Feb 24, 2025
There was a problem hiding this comment.
ACK 1823594. Got tests failures when passing public API flags to secp256k1_eckey_pubkey_serialize.
hebasto
added a commit
to hebasto/bitcoin
that referenced
this pull request
Mar 10, 2025
4ba1ba2af9 Merge bitcoin-core/secp256k1#1647: cmake: Adjust diagnostic flags for `clang-cl` abd25054a1 Merge bitcoin-core/secp256k1#1656: musig: Fix clearing of pubnonces 961ec25a83 musig: Fix clearing of pubnonces 3186082387 Merge bitcoin-core/secp256k1#1614: Add _ge_set_all_gej and use it in musig for own public nonces 6c2a39dafb Merge bitcoin-core/secp256k1#1639: Make static context const 432ac57705 Make static context const 1b1fc09341 Merge bitcoin-core/secp256k1#1642: Verify `compressed` argument in `secp256k1_eckey_pubkey_serialize` c0d9480fbb Merge bitcoin-core/secp256k1#1654: use `EXIT_` constants over magic numbers for indicating program execution status 13d389629a CONTRIBUTING: mention that `EXIT_` codes should be used c855581728 test, bench, precompute_ecmult: use `EXIT_...` constants for `main` return values 965393fcea examples: use `EXIT_...` constants for `main` return values 2e3bf13653 Merge bitcoin-core/secp256k1#1646: README: add instructions for verifying GPG signatures b682dbcf84 README: add instructions for verifying GPG signatures 00774d0723 Merge bitcoin-core/secp256k1#1650: schnorrsig: clear out masked secret key in BIP-340 nonce function a82287fb85 schnorrsig: clear out masked secret key in BIP-340 nonce function 4c50d73dd9 ci: Add new "Windows (clang-cl)" job 84c0bd1f72 cmake: Adjust diagnostic flags for clang-cl f79f46c703 Merge bitcoin-core/secp256k1#1641: doc: Improve cmake instructions in README 2ac9f558c4 doc: Improve cmake instructions in README 1823594761 Verify `compressed` argument in `secp256k1_eckey_pubkey_serialize` 8deef00b33 Merge bitcoin-core/secp256k1#1634: Fix some misspellings 39705450eb Fix some misspellings ec329c2501 Merge bitcoin-core/secp256k1#1633: release cleanup: bump version after 0.6.0 c97059f594 release cleanup: bump version after 0.6.0 64228a648f musig: Use _ge_set_all_gej for own public nonces 300aab1c05 tests: Improve _ge_set_all_gej(_var) tests 365f274ce3 group: Simplify secp256k1_ge_set_all_gej d3082ddead group: Add constant-time secp256k1_ge_set_all_gej git-subtree-dir: src/secp256k1 git-subtree-split: 4ba1ba2af953b7d124db9b80b34568e5c4a2d48a
hebasto
added a commit
to hebasto/bitcoin
that referenced
this pull request
Mar 17, 2025
70f149b9a1 Merge bitcoin-core/secp256k1#1662: bench: add ellswift to bench help output 6b3fe51fb6 bench: add ellswift to bench help output d84bb83e26 Merge bitcoin-core/secp256k1#1661: configure: Show exhaustive tests in summary 3f54ed8c1b Merge bitcoin-core/secp256k1#1659: include: remove WARN_UNUSED_RESULT for functions always returning 1 20b05c9d3f configure: Show exhaustive tests in summary e56716a3bc Merge bitcoin-core/secp256k1#1660: ci: Fix exiting from ci.sh on error d87c3bc58f ci: Fix exiting from ci.sh on error 1b6e081538 include: remove WARN_UNUSED_RESULT for functions always returning 1 2abb35b034 Merge bitcoin-core/secp256k1#1657: tests: remove unused uncounting_illegal_callback_fn 51907fa918 tests: remove unused uncounting_illegal_callback_fn a7a5117144 Merge bitcoin-core/secp256k1#1359: Fix symbol visibility issues, add test for it 13ed6f65dc Merge bitcoin-core/secp256k1#1593: Remove deprecated `_ec_privkey_{negate,tweak_add,tweak_mul}` aliases from API d1478763a5 build: Drop no longer needed `-fvisibility=hidden` compiler option 8ed1d83d92 ci: Run `tools/symbol-check.py` 41d32ab2de test: Add `tools/symbol-check.py` 88548058b3 Introduce `SECP256K1_LOCAL_VAR` macro 03bbe8c615 Merge bitcoin-core/secp256k1#1655: gha: Print all *.log files, in a separate action 59860bcc24 gha: Print all *.log files, in a separate action 4ba1ba2af9 Merge bitcoin-core/secp256k1#1647: cmake: Adjust diagnostic flags for `clang-cl` abd25054a1 Merge bitcoin-core/secp256k1#1656: musig: Fix clearing of pubnonces 961ec25a83 musig: Fix clearing of pubnonces 3186082387 Merge bitcoin-core/secp256k1#1614: Add _ge_set_all_gej and use it in musig for own public nonces 6c2a39dafb Merge bitcoin-core/secp256k1#1639: Make static context const 37d2c60bec Remove deprecated _ec_privkey_{negate,tweak_add,tweak_mul} aliases 432ac57705 Make static context const 1b1fc09341 Merge bitcoin-core/secp256k1#1642: Verify `compressed` argument in `secp256k1_eckey_pubkey_serialize` c0d9480fbb Merge bitcoin-core/secp256k1#1654: use `EXIT_` constants over magic numbers for indicating program execution status 13d389629a CONTRIBUTING: mention that `EXIT_` codes should be used c855581728 test, bench, precompute_ecmult: use `EXIT_...` constants for `main` return values 965393fcea examples: use `EXIT_...` constants for `main` return values 2e3bf13653 Merge bitcoin-core/secp256k1#1646: README: add instructions for verifying GPG signatures b682dbcf84 README: add instructions for verifying GPG signatures 00774d0723 Merge bitcoin-core/secp256k1#1650: schnorrsig: clear out masked secret key in BIP-340 nonce function a82287fb85 schnorrsig: clear out masked secret key in BIP-340 nonce function 4c50d73dd9 ci: Add new "Windows (clang-cl)" job 84c0bd1f72 cmake: Adjust diagnostic flags for clang-cl f79f46c703 Merge bitcoin-core/secp256k1#1641: doc: Improve cmake instructions in README 2ac9f558c4 doc: Improve cmake instructions in README 1823594761 Verify `compressed` argument in `secp256k1_eckey_pubkey_serialize` 8deef00b33 Merge bitcoin-core/secp256k1#1634: Fix some misspellings 39705450eb Fix some misspellings ec329c2501 Merge bitcoin-core/secp256k1#1633: release cleanup: bump version after 0.6.0 c97059f594 release cleanup: bump version after 0.6.0 64228a648f musig: Use _ge_set_all_gej for own public nonces 300aab1c05 tests: Improve _ge_set_all_gej(_var) tests 365f274ce3 group: Simplify secp256k1_ge_set_all_gej d3082ddead group: Add constant-time secp256k1_ge_set_all_gej git-subtree-dir: src/secp256k1 git-subtree-split: 70f149b9a1bf4ed3266f97774d0ae9577534bf40
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Due to similarity to the public API function
secp256k1_ec_pubkey_serialize, public API flags likeSECP256K1_EC_COMPRESSEDare sometimes mistakingly passed tosecp256k1_eckey_pubkey_serializein newly proposed code (this is currently the case for several modules in secp256k1-zkp, see BlockstreamResearch/secp256k1-zkp#300), which is currently not detected. To avoid this in the future, a VERIFY_CHECK is added to check that thecompressedargument is either 0 or 1.