Replace libsecp256k1 with secp265k1
#8100
Conversation
bkchr
added
A0-please_review
| let id = secp256k1::recovery::RecoveryId::from_i32(self.0[64] as i32).ok()?; | ||
| let sig = secp256k1::recovery::RecoverableSignature::from_compact(&self.0[..64], id).ok()?; | ||
|
|
||
| secp256k1::Secp256k1::verification_only().recover(&message, &sig) |
There was a problem hiding this comment.
Creating the Secp256k1 is super heavy and should be cached in a lazy_static or sth. AFAIR they are considering making the required data static, but it's not there yet in the C library.
Also see: tomusdrw/ethsign#37
| .map_err(|_| SecretStringError::InvalidSeedLength)?; | ||
| let public = PublicKey::from_secret_key(&secret); | ||
| let public = PublicKey::from_secret_key(&secp256k1::Secp256k1::signing_only(), &secret); |
There was a problem hiding this comment.
same here (context to be cached)
| let message = secp256k1::Message::parse(&blake2_256(message.as_ref())); | ||
| let sig: (_, _) = self.try_into().ok()?; | ||
| secp256k1::recover(&message, &sig.0, &sig.1) | ||
| let message = secp256k1::Message::from_slice(&blake2_256(message.as_ref())).ok()?; |
There was a problem hiding this comment.
could be an expect, since we know it's 32-bytes
| r.0[0..64].copy_from_slice(&x.0.serialize()[..]); | ||
| r.0[64] = x.1.serialize(); | ||
| r.0[0..64].copy_from_slice(&x.0.serialize_compact()[..]); | ||
| r.0[64] = x.1.to_i32() as u8; |
There was a problem hiding this comment.
This might cause issues in case of overflows - you can have multiple different RecoveryIds ending up with the same Signature type (malleability).
Seems that libsecp256k1 was simply storing u8, but the C library stores i32.
| secp256k1::sign(&message, &self.secret).into() | ||
| let message = secp256k1::Message::from_slice(&blake2_256(message)) | ||
| .expect("Message is 32 byte; qed"); | ||
| let sig = secp256k1::Secp256k1::signing_only().sign_recoverable(&message, &self.secret); |
| signature.copy_from_slice(sig); | ||
| let sig = Signature(signature); | ||
|
|
||
| match sig.recover(message) { |
There was a problem hiding this comment.
Message is not blake2_256 hashed here (and it used to be)
There was a problem hiding this comment.
The fact it wasn't caught by a test scares me.
|
|
||
| let msg = secp256k1::Message::from_slice(msg).map_err(|_| EcdsaVerifyError::BadSignature)?; | ||
|
|
||
| let pubkey = secp256k1::Secp256k1::verification_only().recover(&msg, &rs) |
|
|
||
| let msg = secp256k1::Message::from_slice(msg).map_err(|_| EcdsaVerifyError::BadSignature)?; | ||
|
|
||
| let pubkey = secp256k1::Secp256k1::verification_only().recover(&msg, &rs) |
| @@ -103,8 +103,18 @@ impl Public { | |||
| /// This will convert the full public key into the compressed format. | |||
| #[cfg(feature = "std")] | |||
| pub fn from_full(full: &[u8]) -> Result<Self, ()> { | |||
| secp256k1::PublicKey::parse_slice(full, None) | |||
| .map(|k| k.serialize_compressed()) | |||
| let pubkey = if full.len() == 64 { | |||
There was a problem hiding this comment.
The libsecp256k1 library is handling 3 kind of keys:
full (65 bytes), raw (64 bytes; no tag) and compressed.
Here we only handle the raw case, are you sure that secp256k1 is handling both full and compressed case?
There was a problem hiding this comment.
Yes. The C lib handles 33 and 65 byte signatures. But no 64 byte signatures:
https://github.com/bitcoin-core/secp256k1/blob/9a5a87e0f1276e0284446af1172056ea4693737f/src/eckey_impl.h#L17-L35
tomusdrw
added
the
D9-needsaudit 👮
|
This might be a silly question, but I thought the point of libsecp256k1 was to be used from substrate? This other library actually seems more awkward than that one, is there not a way to just patch libsecp instead? |
What you mean by this? |
Ah nevermind, I see the context from the linked issue. I thought this was trying to fix another issue. |
|
anyone working on this? |
|
closing due to lack of progress. |
shawntabrizi
removed
the
D9-needsaudit 👮
Fixes: #8089