Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tests_exhaustive: check the result of secp256k1_ecdsa_sign #960

Merged
merged 1 commit into from
Jul 3, 2021
Merged

tests_exhaustive: check the result of secp256k1_ecdsa_sign #960

merged 1 commit into from
Jul 3, 2021

Conversation

niooss-ledger
Copy link
Contributor

Hello,

In test_exhaustive_sign, if secp256k1_ecdsa_sign fails, the signature which is then loaded by secp256k1_ecdsa_signature_load is garbage. Exit early with an error when this occurs.

By the way, I am wondering whether attribute SECP256K1_WARN_UNUSED_RESULT should be added to function secp256k1_ecdsa_sign: as (according to the documentation of this function) the nonce generation function may fail, it seems to be a good idea to force callers to check the value returned by this function. What do you think about this?

@niooss-ledger
tests_exhaustive: check the result of secp256k1_ecdsa_sign
a1ee83c 
If `secp256k1_ecdsa_sign` fails, the signature which is then loaded by
`secp256k1_ecdsa_signature_load` is garbage. Exit early with an error
when this occurs.
real-or-random
real-or-random approved these changes Jul 2, 2021
View reviewed changes
Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK a1ee83c

@real-or-random
Copy link
Contributor

Thanks for the PR!

By the way, I am wondering whether attribute SECP256K1_WARN_UNUSED_RESULT should be added to function secp256k1_ecdsa_sign: as (according to the documentation of this function) the nonce generation function may fail, it seems to be a good idea to force callers to check the value returned by this function. What do you think about this?

The default nonce generation function will fail only with astronomically low probability. So if you know that you have a valid secret key and you use the default nonce function (99% of the use cases), it's okay not to check the return value.

Having said that, I think we're not entirely consistent here... For example, the same argument would apply to secp256k1_ec_seckey_verify (https://github.com/bitcoin-core/secp256k1/blob/master/include/secp256k1.h#L632). Even secp256k1_ec_pubkey_negate
https://github.com/bitcoin-core/secp256k1/blob/master/include/secp256k1.h#L650 has SECP256K1_WARN_UNUSED_RESULT even if it's guaranteed to return 1 according to the docs...

Maybe we should have a look at this in #783 or in a follow up PR.

@sipa
Copy link
Contributor

sipa commented Jul 3, 2021

ACK a1ee83c

@real-or-random real-or-random merged commit ec3aaa5 into bitcoin-core:master Jul 3, 2021
@real-or-random real-or-random mentioned this pull request Jul 3, 2021
Open
@niooss-ledger niooss-ledger deleted the check-secp256k1_ecdsa_sign-in-tests branch July 3, 2021 18:48
@apoelstra apoelstra mentioned this pull request Jul 27, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@real-or-random real-or-random real-or-random approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@niooss-ledger @real-or-random @sipa