Use ed25519-consensus instead of ed25519-dalek #1046
Conversation
Closes informalsystems#355 (see that issue for more context; `ed25519-consensus` is a fork of `ed25519-zebra` that's Zcash-independent). This change ensures that `tendermint-rs` has the same signature verification as `tendermint`, which uses the validation criteria provided by the Go `ed25519consensus` library.
Codecov Report
@@ Coverage Diff @@
## master #1046 +/- ##
========================================
- Coverage 62.4% 62.4% -0.1%
========================================
Files 236 236
Lines 21321 21322 +1
========================================
- Hits 13325 13317 -8
- Misses 7996 8005 +9
Continue to review full report at Codecov.
|
| Signature | ||
| [ DisplayOnly<SignatureError> ] | ||
| | _ | { "signature error" }, |
There was a problem hiding this comment.
Would it not be worthwhile to bubble up the original error here? Unfortunately it seems like the ed25519_consensus::Error type doesn't implement std::fmt::Display unless you turn on the std feature.
There was a problem hiding this comment.
The motivation for removing the original error was that the code this is replacing used an opaque error type, so there's no additional information conveyed by the original error, and was easier to just remove it rather than patch the ed25519_consensus::Error type to have an extra Display impl. But I could add that Display impl in another release.
| @@ -184,8 +183,8 @@ impl Version { | |||
| #[cfg(not(feature = "amino"))] | |||
| fn encode_auth_signature_amino( | |||
There was a problem hiding this comment.
I wonder if this method's even necessary. Why would one want to convert a compile-time error to a runtime error like this?
There was a problem hiding this comment.
Not sure, I tried to leave the rest of the code untouched as a drop-in change.
| Signature | ||
| [ DisplayOnly<signature::Error> ] | ||
| |_| { format_args!("signature error") }, | ||
| |_| { "signature error" }, |
There was a problem hiding this comment.
Similar to my other comment: should we not find a way here to bubble up the original error?
There was a problem hiding this comment.
xref #1046 (comment)
I'd say target The changelog entry should probably be listed as a breaking change, right? |
Co-authored-by: Thane Thomson <[email protected]>
| Ed25519::try_from(bytes) | ||
| .map(PublicKey::Ed25519) | ||
| .ok() |
There was a problem hiding this comment.
| Ed25519::try_from(bytes) | |
| .map(PublicKey::Ed25519) | |
| .ok() | |
| Ed25519::try_from(bytes).map(PublicKey::Ed25519).ok() |
cargo fmt complaint
| @@ -3,7 +3,7 @@ use std::io::Write as _; | |||
| use std::net::{TcpListener, TcpStream}; | |||
| use std::thread; | |||
|
|
|||
| use ed25519_dalek::{self as ed25519}; | |||
| use ed25519_consensus; | |||
There was a problem hiding this comment.
| use ed25519_consensus; |
clippy lint
|
Just a changelog entry please and these (hopefully) last few lints, then we can merge. |
|
Updated and superseded by #1067 |
Closes #355 (see that issue for more context;
ed25519-consensusis a fork ofed25519-zebrathat's Zcash-independent).This change ensures that
tendermint-rshas the same signature verification astendermint, which uses the validation criteria provided by the Goed25519consensuslibrary.[ ] Added entry inI did not do this, because I'm not sure what release to target..changelog/