-
Notifications
You must be signed in to change notification settings - Fork 344
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.
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
Add secp256k1_verify/secp256k1_recover_pubkey tests from Project Wycheproof #2000
Conversation
68a6208
to
6197419
Compare
59ca547
to
4290df5
Compare
4290df5
to
818ec58
Compare
f3ffcd0
to
07e98dd
Compare
07e98dd
to
fa92691
Compare
1345f37
to
5ecf3d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff!
assert!(recovered0 == public_key || recovered1 == public_key); | ||
} | ||
|
||
fn from_der(data: &[u8]) -> Result<[u8; 64], String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering, wouldn't it have been easier to use a ready-made crate like der
for decoding the integers, with tag and everything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I think the issue is that you the test vectors make additional requirements because by nature, DER is non-deterministic, allowing signature malleability (i.e. different representations of the same signature) which allows creating multiple tx hashes from a single signature. As far as I can tell, all crypto use cases implement this themselves.
I already added the DER question here: #2001, so let's postpone it. For now DER only comes in through Project Wycheproof and is not something relevant for our APIs. In the Ethereum and Cosmos world DER is not used at all. Bitcoin is getting rid of it slowly.
5ecf3d3
to
0e9624a
Compare
The goal here is to increase our test coverage of the secp256k1 verifier in preparation for the upcoming secp256r1 verifier.
More than 2k additional test, all passing without touching the implementation 🎉
Update: the first 10 commits here are just more tests. The last one then uses the gained confidence and adapt the pubkey recovers as discussed in RustCrypto/elliptic-curves#991 (comment).Update 2: removed the fix and split it into #2014 because I don't like having it all together. This PR now purely adds tests, but for both
secp256k1_verify
andsecp256k1_recover_pubkey
.