Add crypto provider #1214

blasrodri · 2022-10-19T14:18:02Z

Draft definition of the CryptoProvider trait defined in #1213 + an actual instance of the trait
for a HostFunction* implementation

tony-iqlusion · 2022-10-19T19:19:29Z

It might make more sense to put this in the tendermint crate itself, since it already contains high-level functions for working with private and public keys as well as signing and verifying messages.

Then downstream crates like light-client-verifier can use those high-level APIs.

This keeps the core cryptographic functionality in one crate, instead of spread out.

blasrodri · 2022-10-20T00:23:23Z

tendermint/Cargo.toml

@@ -51,14 +52,14 @@ tendermint-proto = { version = "0.25.0", default-features = false, path = "../pr
 time = { version = "0.3", default-features = false, features = ["macros", "parsing"] }
 zeroize = { version = "1.1", default-features = false, features = ["zeroize_derive", "alloc"] }
 flex-error = { version = "0.4.4", default-features = false }
-k256 = { version = "0.11", optional = true, default-features = false, features = ["ecdsa", "sha256"] }
+k256 = { version = "0.11", default-features = false, features = ["ecdsa", "sha256"] }


Having this one optional depends on whether CryptoProvider will be under a feature flag.

As I understand it now, we lock the signature type in CryptoProvider to the one defined by k256. If we cannot abstract this associated type to less specific rust-crypto bounds ("must be fixed array of 256 bits"?) and still keep it usable in generic code, then k256 should become a hard dependency. But I thought dependency management was the whole purpose of this rework.

blasrodri · 2022-10-20T00:24:04Z

It might make more sense to put this in the tendermint crate itself, since it already contains high-level functions for working with private and public keys as well as signing and verifying messages.

Then downstream crates like light-client-verifier can use those high-level APIs.

This keeps the core cryptographic functionality in one crate, instead of spread out.

Moved it on 100fc53

codecov-commenter · 2022-10-24T13:44:11Z

Codecov Report

Merging #1214 (0cafbb9) into main (d102af4) will decrease coverage by 0.1%.
The diff coverage is 36.0%.

@@           Coverage Diff           @@
##            main   #1214     +/-   ##
=======================================
- Coverage   64.2%   64.1%   -0.2%     
=======================================
  Files        244     245      +1     
  Lines      21364   21401     +37     
=======================================
- Hits       13734   13731      -3     
- Misses      7630    7670     +40

Impacted Files	Coverage Δ
light-client/src/builder/light_client.rs	`0.0% <0.0%> (ø)`
tendermint/src/crypto.rs	`0.0% <0.0%> (ø)`
tendermint/src/lib.rs	`100.0% <ø> (ø)`
light-client-verifier/src/verifier.rs	`82.6% <17.6%> (-5.3%)`	⬇️
...client-verifier/src/operations/commit_validator.rs	`98.2% <100.0%> (+0.2%)`	⬆️
light-client-verifier/src/predicates.rs	`97.4% <100.0%> (ø)`
testgen/src/vote.rs	`84.0% <0.0%> (-1.7%)`	⬇️
abci/src/codec.rs	`88.8% <0.0%> (-1.3%)`	⬇️
abci/src/application/kvstore.rs	`73.3% <0.0%> (-1.1%)`	⬇️
testgen/src/commit.rs	`90.6% <0.0%> (-0.7%)`	⬇️
... and 5 more

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

mzabaluev · 2022-10-31T15:04:05Z

The new API looks good in general, will review once more when undrafted.
Would there be an example of how this can be used in a light client verifier?

blasrodri · 2022-10-31T16:06:59Z

The new API looks good in general, will review once more when undrafted.
Would there be an example of how this can be used in a light client verifier?

I'll add this, and un-draft it :)

tony-iqlusion · 2022-10-31T16:19:13Z

tendermint/src/host_functions.rs

@@ -0,0 +1,134 @@
+use crate::signature::Verifier;


As a general thought, "host functions" that contains only a CryptoProvider trait still feels like YAGNI to me.

I would personally suggest putting it under a crypto module (which should probably have been created a long time ago, with all of the existing cryptography code in the tendermint crate factored into it).

tony-iqlusion · 2022-10-31T16:23:44Z

The trait itself looks OK, aside from the commented out part.

However, it isn't terribly useful unless the existing cryptography in the crate(s) is changed to use it.

And that's where this sort of change might get a bit onerous: you either need to add a generic type parameter with a default, or a generic type parameter with feature-gated type aliases. I'd probably suggest the latter.

blasrodri · 2022-10-31T20:04:17Z

The new API looks good in general, will review once more when undrafted. Would there be an example of how this can be used in a light client verifier?

Please check it out here: ComposableFi/near-rs#16

mzabaluev · 2022-11-02T15:54:52Z

However, it isn't terribly useful unless the existing cryptography in the crate(s) is changed to use it.

Is this PR meant to start a piecemeal introduction of the larger set of changes first submitted in #1138?

add a generic type parameter with a default, or a generic type parameter with feature-gated type aliases.

I think a ready-to-use implementation of the crypto functions, using widely adopted pure Rust dependencies, should be provided in the tendermint crate for those users who don't need to bother with the alternatives. In the interests of dependency management, it needs to be feature-gated.

blasrodri · 2022-11-02T17:55:19Z

Can you please be more specific on how to make if feature-gated friendly?
Happy to implement it

mzabaluev · 2022-11-05T08:36:02Z

Can you please be more specific on how to make if feature-gated friendly? Happy to implement it

Cribbing from #1138 again, you could have a generic CommitValidator parameterized over the CryptoProvider trait, and a feature-gated production instance:

#[cfg(feature = "rust-crypto")]
pub type ProdCommitValidator = CommitValidator<RustCryptoProvider>;

But perhaps it's work for a follow-up PR. I will look into ComposableFi/near-rs#16 and the fork of tendermint-rs used in Hyperspace.

blasrodri · 2022-11-09T09:00:47Z

@mzabaluev gave my first shot on feature gating this. Would def appreciate some guidance :)

mzabaluev · 2022-11-09T11:33:18Z

light-client-verifier/src/operations/commit_validator.rs

+    #[cfg(feature = "rust-crypto")]
+    type RustCryptoProvider: CryptoProvider;
+


Feature-gating trait members is not a good idea, I think.

Perhaps I did not explain the idea more carefully. It's OK to work the abstract CryptoProvider generics wherever needed without feature-gating. So you could have:

// Kill the CommitValidator trait, its methods become inherent methods of the struct: pub struct CommitValidator<C> { // ... } impl<C: CryptoProvider> CommitValidator<C> { // The API that belonged to CommitValidator trait goes here } /// The batteries-included validator, for when you don't mind the dependencies on /// the full rust-crypto stack. #[cfg(feature = "rust-crypto")] pub type ProdCommitValidator = CommitValidator<RustCryptoProvider>;

@thanethomson I think all those traits having a single empty ProdFoo implementation don't carry much weight in the current design, since all the logic is provided in the default trait method bodies. So it's fair to replace them with generic types parameterized around the crypto provider, like in the example above.

…rovider

blasrodri · 2022-11-14T11:20:26Z

@mzabaluev any comments?

mzabaluev · 2022-11-15T11:15:56Z

tendermint/src/crypto.rs

+trait DefaultHostFunctions: CryptoProvider {
+    fn sha2_256(preimage: &[u8]) -> [u8; 32];
+    fn ed25519_verify(sig: &[u8], msg: &[u8], pub_key: &[u8]) -> Result<(), ()>;
+    fn secp256k1_verify(sig: &[u8], message: &[u8], public: &[u8]) -> Result<(), ()>;
+}


The purpose of this trait is not clear. Is it meant to be a "god trait" for all future functions that could be host-provided? Now, it only adds the crypto methods that I think could well be defined in CryptoProvider itself, because the implementation is tightly coupled with the associated type definitions there.

I agree with @tony-iqlusion that this umbrella trait is not necessarily needed until we realise a clear purpose for it.

light-client-verifier/Cargo.toml

mzabaluev · 2022-11-15T11:18:41Z

tendermint/src/crypto.rs

+    // type Ed25519Signer: Signer<ed25519::Signature>;
+    // type Ed25519Verifier: Verifier<ed25519::Signature>;


This should be removed, to be added later when/if needed.

mzabaluev · 2022-11-15T11:26:21Z

tendermint/Cargo.toml

@@ -51,14 +52,14 @@ tendermint-proto = { version = "0.25.0", default-features = false, path = "../pr
 time = { version = "0.3", default-features = false, features = ["macros", "parsing"] }
 zeroize = { version = "1.1", default-features = false, features = ["zeroize_derive", "alloc"] }
 flex-error = { version = "0.4.4", default-features = false }
-k256 = { version = "0.11", optional = true, default-features = false, features = ["ecdsa", "sha256"] }
+k256 = { version = "0.11", default-features = false, features = ["ecdsa", "sha256"] }


As I understand it now, we lock the signature type in CryptoProvider to the one defined by k256. If we cannot abstract this associated type to less specific rust-crypto bounds ("must be fixed array of 256 bits"?) and still keep it usable in generic code, then k256 should become a hard dependency. But I thought dependency management was the whole purpose of this rework.

mzabaluev · 2022-11-17T21:47:50Z

I think the functionality abstracted by CryptoProvider should cut deeper, if the goals of this change are to be realized. One instance where Sha256 is still used unconditionally is the hasher, which simply calls into intrinsic hash methods of tendermint domain types. An alternative hasher cannot replicate that without duplicating the implementation of those methods. I'm doing some experiments to see if we can come up with a more comprehensive redesign and maybe simplify the generics in light-client-verifier at the same time.

mzabaluev · 2022-11-21T14:29:18Z

Here's my crack at it: #1238

romac · 2023-01-19T14:45:32Z

Superseded by #1238

blasrodri added 7 commits October 17, 2022 14:47

define CryptoProvider trait

9b9261b

wip

8d4f615

implement CryptoProvider for Substrate (WIP)

d15583d

comment issues

0b44449

ed25519_verify implementation for SubstrateHostFunctionsManager

72133d3

introduce EcdsaSecp256k1Signer

88e9ff3

doc a bit

a325f63

blasrodri mentioned this pull request Oct 19, 2022

Trait-based cryptographic routines API to delegate implementation to downstream crates #1213

Closed

move CryptoProvider to tendermint crate instead

100fc53

blasrodri commented Oct 20, 2022

View reviewed changes

adizere requested a review from mzabaluev October 24, 2022 13:45

mzabaluev mentioned this pull request Oct 29, 2022

Adds HostFunctions to tendermint-light-client-verifier #1138

Closed

tony-iqlusion reviewed Oct 31, 2022

View reviewed changes

rename host_functions module to crypto

8048e4d

mzabaluev reviewed Nov 9, 2022

View reviewed changes

re implement ProdCommitValidator

63a2d0e

blasrodri force-pushed the add-crypto-provider branch from 9afc8bf to 63a2d0e Compare November 9, 2022 22:56

Merge remote-tracking branch 'informalsystems/main' into add-crypto-p…

bcec883

…rovider

mzabaluev suggested changes Nov 15, 2022

View reviewed changes

blasrodri added 2 commits November 15, 2022 17:32

re-define crypto provider

3cce56f

distinguish implementations based on feature flag

0cafbb9

mzabaluev mentioned this pull request Nov 21, 2022

Make crypto implementations replaceable #1238

Merged

5 tasks

romac closed this Jan 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add crypto provider #1214

Add crypto provider #1214

blasrodri commented Oct 19, 2022

tony-iqlusion commented Oct 19, 2022

blasrodri Oct 20, 2022

mzabaluev Nov 15, 2022

blasrodri commented Oct 20, 2022

codecov-commenter commented Oct 24, 2022 •

edited

Loading

mzabaluev commented Oct 31, 2022

blasrodri commented Oct 31, 2022

tony-iqlusion Oct 31, 2022

tony-iqlusion commented Oct 31, 2022

blasrodri commented Oct 31, 2022

mzabaluev commented Nov 2, 2022

blasrodri commented Nov 2, 2022

mzabaluev commented Nov 5, 2022

blasrodri commented Nov 9, 2022

mzabaluev Nov 9, 2022 •

edited

Loading

blasrodri commented Nov 14, 2022

mzabaluev Nov 15, 2022

mzabaluev Nov 15, 2022

mzabaluev Nov 15, 2022

mzabaluev commented Nov 17, 2022

mzabaluev commented Nov 21, 2022

romac commented Jan 19, 2023

		#[cfg(feature = "rust-crypto")]
		type RustCryptoProvider: CryptoProvider;

		// type Ed25519Signer: Signer<ed25519::Signature>;
		// type Ed25519Verifier: Verifier<ed25519::Signature>;

Add crypto provider #1214

Add crypto provider #1214

Conversation

blasrodri commented Oct 19, 2022

tony-iqlusion commented Oct 19, 2022

blasrodri Oct 20, 2022

Choose a reason for hiding this comment

mzabaluev Nov 15, 2022

Choose a reason for hiding this comment

blasrodri commented Oct 20, 2022

codecov-commenter commented Oct 24, 2022 • edited Loading

Codecov Report

mzabaluev commented Oct 31, 2022

blasrodri commented Oct 31, 2022

tony-iqlusion Oct 31, 2022

Choose a reason for hiding this comment

tony-iqlusion commented Oct 31, 2022

blasrodri commented Oct 31, 2022

mzabaluev commented Nov 2, 2022

blasrodri commented Nov 2, 2022

mzabaluev commented Nov 5, 2022

blasrodri commented Nov 9, 2022

mzabaluev Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

blasrodri commented Nov 14, 2022

mzabaluev Nov 15, 2022

Choose a reason for hiding this comment

mzabaluev Nov 15, 2022

Choose a reason for hiding this comment

mzabaluev Nov 15, 2022

Choose a reason for hiding this comment

mzabaluev commented Nov 17, 2022

mzabaluev commented Nov 21, 2022

romac commented Jan 19, 2023

codecov-commenter commented Oct 24, 2022 •

edited

Loading

mzabaluev Nov 9, 2022 •

edited

Loading