deps: Use ed25519-consensus instead of ed25519-dalek (#1067) #1245

hdevalence · 2022-12-06T03:34:41Z

Backports this change to the new main (was #1067).

Use ed25519-consensus instead of ed25519-dalek

Closes #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.

clippy fixes

Co-authored-by: Thane Thomson [email protected]

Remove redundant dependency

Signed-off-by: Thane Thomson [email protected]

cargo fmt

Signed-off-by: Thane Thomson [email protected]

Add changelog entries

Signed-off-by: Thane Thomson [email protected]

Co-authored-by: Henry de Valence [email protected]

codecov-commenter · 2022-12-06T03:47:39Z

Codecov Report

Merging #1245 (726c637) into main (560c852) will decrease coverage by 0.0%.
The diff coverage is 67.8%.

@@           Coverage Diff           @@
##            main   #1245     +/-   ##
=======================================
- Coverage   64.4%   64.4%   -0.1%     
=======================================
  Files        245     245             
  Lines      21581   21547     -34     
=======================================
- Hits       13917   13889     -28     
+ Misses      7664    7658      -6

Impacted Files	Coverage Δ
p2p/src/error.rs	`0.0% <ø> (ø)`
p2p/src/secret_connection/amino_types.rs	`0.0% <0.0%> (ø)`
tendermint/src/error.rs	`0.0% <0.0%> (ø)`
tendermint/src/private_key.rs	`28.7% <36.3%> (+4.9%)`	⬆️
p2p/src/secret_connection/protocol.rs	`59.8% <60.0%> (ø)`
p2p/src/secret_connection.rs	`86.6% <78.5%> (-0.2%)`	⬇️
p2p/src/secret_connection/public_key.rs	`80.6% <83.3%> (ø)`
tendermint/src/public_key.rs	`72.5% <85.7%> (-0.3%)`	⬇️
config/src/node_key.rs	`60.0% <100.0%> (ø)`
tendermint/src/account.rs	`91.0% <100.0%> (ø)`
... and 11 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mzabaluev

Generally looks good.

One thing which I find potentially problematic (though not a change I'm requesting now) is that ed25519-consensus does not integrate into the signature framework. Why is that?

mzabaluev · 2022-12-08T09:11:11Z

p2p/src/error.rs

@@ -40,7 +39,6 @@ define_error! {
            | _ | { "public key missing" },

        Signature
-            [ DisplayOnly<SignatureError> ]


This error is deliberately opaque, but we can't make the Display impl transparent in flex-error, so we have our own message. OK.

p2p/src/secret_connection.rs

mzabaluev · 2022-12-08T09:21:47Z

tendermint/src/private_key.rs

+    // Tendermint uses a serialization format inherited from Go that includes a
+    // cached copy of the public key as the second half.  This is somewhat
+    // dangerous, since there's no validation that the two parts are consistent
+    // with each other, so we ignore the second half and just check consistency
+    // with the re-derived data.
+    let signing_key = Ed25519::try_from(&keypair_bytes[0..32])
+        .map_err(|_| D::Error::custom("invalid signing key"))?;
+    let verification_key = VerificationKey::from(&signing_key);
+    if &keypair_bytes[32..64] != verification_key.as_bytes() {
+        return Err(D::Error::custom("keypair mismatch"));
+    }


tendermint/src/public_key.rs

mzabaluev · 2022-12-08T09:30:06Z

test/Cargo.toml

 flex-error = { version = "0.4.4", default-features = false }
 flume = { version = "0.10", default-features = false }
-rand_core = { version = "0.5", default-features = false, features = ["std"] }
+rand_core = { version = "0.6", default-features = false, features = ["std"] }


Huh, isis still has not updated their rand version? Another reason to switch.

hdevalence · 2022-12-09T04:54:20Z

Generally looks good.

One thing which I find potentially problematic (though not a change I'm requesting now) is that ed25519-consensus does not integrate into the signature framework. Why is that?

There's no particular reason; there wasn't a need to do it when the crate was written, and it wasn't possible to implement for trivial reasons -- AFAIR, it was because the Signature trait requires AsRef<[u8]>, but internally the ed25519_consensus::Signature type has two [u8; 32] arrays because that was slightly more convenient as an internal representation.

We could probably add it if it's important. To be honest, the main reason we're interested in this is that the ed25519-consensus types have nicer Debug output, and that propagates into our logs when using tendermint-rs types.

mzabaluev · 2022-12-09T10:35:43Z

@hdevalence I think it's OK to not conform to the signature traits, as we're not going to make the Ed25519 implementation replaceable in the near term as per discussion on Slack (recap: it's not supported by Web Crypto, corner cases addressed differently by other implementations so we need the precise behavior). The API is also close enough to not require significant rework if we decide we need it.

So, I think this just needs a change to do the exhaustive match for proto's key type enum, and we can merge it.

tony-iqlusion · 2022-12-09T14:36:26Z

AFAIR, it was because the Signature trait requires AsRef<[u8]>, but internally the ed25519_consensus::Signature type has two [u8; 32] arrays because that was slightly more convenient as an internal representation.

@hdevalence that bound is removed in a forthcoming signature v2.0 release, FWIW:

https://docs.rs/signature/2.0.0-pre.2/signature/index.html

hdevalence · 2022-12-13T20:05:02Z

@hdevalence I think it's OK to not conform to the signature traits, as we're not going to make the Ed25519 implementation replaceable in the near term as per discussion on Slack (recap: it's not supported by Web Crypto, corner cases addressed differently by other implementations so we need the precise behavior). The API is also close enough to not require significant rework if we decide we need it.

So, I think this just needs a change to do the exhaustive match for proto's key type enum, and we can merge it.

Cool, I updated the exhaustive match. I'd be happy to try to support the signature traits in a later release of the crate if it becomes relevant.

mzabaluev

Needs merging from main, but is otherwise good to go.

hdevalence · 2022-12-15T18:07:10Z

Rebased.

thanethomson · 2022-12-20T02:01:55Z

Was the failing check here due to clippy errors from Rust v1.66? If so, #1253's been merged and that should fix things.

thanethomson · 2023-01-04T13:28:28Z

@hdevalence please resolve the conflicts here and then we'll merge and cut a new release.

…#1067) * Use ed25519-consensus instead of ed25519-dalek 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. * clippy fixes Co-authored-by: Thane Thomson <[email protected]> * Remove redundant dependency Signed-off-by: Thane Thomson <[email protected]> * cargo fmt Signed-off-by: Thane Thomson <[email protected]> * Add changelog entries Signed-off-by: Thane Thomson <[email protected]> Co-authored-by: Henry de Valence <[email protected]>

hdevalence · 2023-01-10T01:26:48Z

Rebased and did another round of clippy fixes.

xla

(͠≖ ͜ʖ͠≖)👌

mzabaluev · 2023-01-10T14:13:33Z

Thank you @hdevalence and @xla!

hdevalence · 2023-01-13T19:31:35Z

Thanks for merging this! Are there other changes in flight at the moment, or would it be possible to get it into a release soon? (We'll have to wait for it to propagate through our dep tree before we can pick it up).

hdevalence mentioned this pull request Dec 6, 2022

Remove ed25519-dalek stub code penumbra-zone/penumbra#1693

Closed

3 tasks

hdevalence force-pushed the ed25519-consensus-rebase branch from e20f513 to b2e822a Compare December 6, 2022 03:36

thanethomson added the breaking label Dec 7, 2022

mzabaluev suggested changes Dec 8, 2022

View reviewed changes

mzabaluev added enhancement New feature or request dependencies Pull requests that update a dependency file labels Dec 9, 2022

mzabaluev approved these changes Dec 15, 2022

View reviewed changes

hdevalence force-pushed the ed25519-consensus-rebase branch from f5c8938 to 726c637 Compare December 15, 2022 18:07

thanethomson and others added 5 commits January 9, 2023 17:13

try clippy fix

174ec35

try clippy fix

0324f0c

try clippy fix

a2b6fb5

use exhaustive match

e39ee1c

hdevalence force-pushed the ed25519-consensus-rebase branch from 726c637 to e39ee1c Compare January 10, 2023 01:15

another round of clippy fixes

58d87b4

xla approved these changes Jan 10, 2023

View reviewed changes

mzabaluev merged commit 95e4505 into informalsystems:main Jan 10, 2023

hdevalence mentioned this pull request Feb 3, 2023

Release an 0.29 with #1245 included #1263

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: Use ed25519-consensus instead of ed25519-dalek (#1067) #1245

deps: Use ed25519-consensus instead of ed25519-dalek (#1067) #1245

hdevalence commented Dec 6, 2022

codecov-commenter commented Dec 6, 2022 •

edited

Loading

mzabaluev left a comment

mzabaluev Dec 8, 2022

mzabaluev Dec 8, 2022

mzabaluev Dec 8, 2022

hdevalence commented Dec 9, 2022 •

edited

Loading

mzabaluev commented Dec 9, 2022 •

edited

Loading

tony-iqlusion commented Dec 9, 2022

hdevalence commented Dec 13, 2022

mzabaluev left a comment

hdevalence commented Dec 15, 2022

thanethomson commented Dec 20, 2022

thanethomson commented Jan 4, 2023

hdevalence commented Jan 10, 2023

xla left a comment

mzabaluev commented Jan 10, 2023

hdevalence commented Jan 13, 2023 •

edited

Loading

deps: Use ed25519-consensus instead of ed25519-dalek (#1067) #1245

deps: Use ed25519-consensus instead of ed25519-dalek (#1067) #1245

Conversation

hdevalence commented Dec 6, 2022

codecov-commenter commented Dec 6, 2022 • edited Loading

Codecov Report

mzabaluev left a comment

Choose a reason for hiding this comment

mzabaluev Dec 8, 2022

Choose a reason for hiding this comment

mzabaluev Dec 8, 2022

Choose a reason for hiding this comment

mzabaluev Dec 8, 2022

Choose a reason for hiding this comment

hdevalence commented Dec 9, 2022 • edited Loading

mzabaluev commented Dec 9, 2022 • edited Loading

tony-iqlusion commented Dec 9, 2022

hdevalence commented Dec 13, 2022

mzabaluev left a comment

Choose a reason for hiding this comment

hdevalence commented Dec 15, 2022

thanethomson commented Dec 20, 2022

thanethomson commented Jan 4, 2023

hdevalence commented Jan 10, 2023

xla left a comment

Choose a reason for hiding this comment

mzabaluev commented Jan 10, 2023

hdevalence commented Jan 13, 2023 • edited Loading

codecov-commenter commented Dec 6, 2022 •

edited

Loading

hdevalence commented Dec 9, 2022 •

edited

Loading

mzabaluev commented Dec 9, 2022 •

edited

Loading

hdevalence commented Jan 13, 2023 •

edited

Loading