Replace rust_secp256k1 with k256 #3407
Conversation
deuszx
force-pushed
the
secp256k1-use-k256
branch
4 times, most recently
from
f868144 to
4cf8f77
Compare
linera-base/src/crypto/secp256k1.rs
Outdated
| let message = Message::from_digest(CryptoHash::new(value).as_bytes().0); | ||
| let signature = secp.sign_ecdsa(&message, &secret.0); | ||
| let prehash = CryptoHash::new(value).as_bytes().0; | ||
| let (signature, _rid) = secret.0.sign_prehash_recoverable(&prehash).unwrap(); |
There was a problem hiding this comment.
TODO: @deuszx : fix unwrap
There was a problem hiding this comment.
Working on this right now. Simply changing this to return Result<Self, Error> affects a lot of the code - LiveVote/Vote creation etc.
There was a problem hiding this comment.
After talking to @afck we decided that it's "ok" to panic here. From reading the code it looks like the sources of the errors here are not controllable by us (like a wrong choice of k for the secp256k1 signing algorithm, which is an internal details of k256 anyway) and would most likely either lead to all signature creation fail or would be non-deterministically unsafe.
deuszx
force-pushed
the
secp256k1-use-k256
branch
from
26fcca2 to
72bc4a9
Compare
deuszx
force-pushed
the
secp256k1-use-k256
branch
from
72bc4a9 to
393f808
Compare
linera-base/src/crypto/mod.rs
Outdated
| #[error("Could not parse public key: {0}. Public key point at inifinity.")] | ||
| Secp256k1PointAtIfinity(String), |
There was a problem hiding this comment.
| #[error("Could not parse public key: {0}. Public key point at inifinity.")] | |
| Secp256k1PointAtIfinity(String), | |
| #[error("could not parse public key `{0}`: point at infinity")] | |
| Secp256k1PointAtInfinity(String), |
Fixing some typos, and error message following the documentation of the Error trait:
Error messages are typically concise lowercase sentences without trailing punctuation:
let err = "NaN".parse::<u32>().unwrap_err(); assert_eq!(err.to_string(), "invalid digit found in string");
There was a problem hiding this comment.
Will this work in non-std?
There was a problem hiding this comment.
Additionally, as a nit, I'd suggest that the context of ‘what we were trying to do when the error happened’ (parsing public key {0}) should be handled by thiserror-context rather than encoding every path to the error into the error enum.
There was a problem hiding this comment.
Will this work in non-std?
(assuming this is with regards to thiserror-context — sorry, I rearranged my comments for clarity)
thiserror already doesn't work in nostd. We don't use nostd anyway.
There was a problem hiding this comment.
.context("") works only on anyhow errors though, right?
There was a problem hiding this comment.
thiserror-context is a crate you can use to wrap your errors into a type that supports collecting error context in the same way as anyhow::Context.
There was a problem hiding this comment.
That requires adding new dependency to linera-base. What is this improving?
|
Modulo one non-nit comment I'm happy with this, but I'd appreciate a review from someone more familiar with elliptic curve cryptography. |
| Secp256k1Error(k256::ecdsa::Error), | ||
| #[error("could not parse public key: {0}: key point at inifinity.")] | ||
| Secp256k1PointAtIfinity(String), |
There was a problem hiding this comment.
| Secp256k1Error(k256::ecdsa::Error), | |
| #[error("could not parse public key: {0}: key point at inifinity.")] | |
| Secp256k1PointAtIfinity(String), | |
| #[error("could not parse public key `{0}`: point at infinity")] | |
| Secp256k1PointAtInfinity(String), |
(re-commenting suggestion since it was partially applied, which breaks the accept feature)
Motivation
rust_secp256k1did not work well with Wasm.Proposal
Replace with
k256.Test Plan
CI should catch any regressions and @Twey will check if this indeed works with web client.
Release Plan
Links