Make crypto implementations replaceable (#1238)

* define CryptoProvider trait

* wip

* implement CryptoProvider for Substrate (WIP)

* comment issues

* ed25519_verify implementation for SubstrateHostFunctionsManager

* introduce EcdsaSecp256k1Signer

* doc a bit

* move CryptoProvider to tendermint crate instead

* rename host_functions module to crypto

* tendermint: Remove ED25519_SIGNATURE_SIZE

It has been deprecated for some time.

* tendermint: clean up mod crypto

* Disentangle CryptoProvider from k256 crate

The API needs to be abstract, so make p256k1 signature
an associated type of CryptoProvider.

* Add DefaultCryptoProvider

Guarded by the rust-crypto feature, this is an implementation of
CryptoProvider with pure Rust crates.

* tendermint: reorg mod crypto

Move CryptoProvider definition out of the crypto module into
the provider sub-module. Rename default_provider to default.

* tendermint: generalize hash methods

Add generic methods Header::hash_with and ValidatorSet::hash_with
enabling a custom CryptoProvider to be plugged in for calculating
hashes.
The .hash() methods, implemented with the DefaultCryptoProvider,
are feature-gated behind "rust-crypto".

* tendermint: eliminate get_split_point helper

2usize.next_power_of_two() / 1 == 1, and we have eliminated the other
two explicitly matched cases at the call site, so the non-catchall
match branches and the panics are dead code.

* light-client: eliminate Hasher

The Hasher trait is obviated by CryptoProvider.
Also, we could do with less dynamic dispatching.

* fix no_std

* Fix the tools build

* Fix wasm-light-client build

* kvstore-test: No need to copy vec-of-vecs

* Break down CryptoProvider into functional traits

Instead of a super-trait whose sole purpose is to bind down some
associated types that provide the actual functionality, provide:
- Sha256, a purpose-specific trait for SHA256 hashing that has
  a more human-friendly interface than rust-crypto.
- Nothing else for signing and verifying! These are covered by the
  signature framework, and it's easy to plug into that as the
  alt_crypto test demonstrates.

The crypto::default module, gated by the "rust-crypto" feature,
provides aliases for pure Rust implementations.

* alt_crypto test: Roll our own signature type

An alt implementation would not be able to reuse the signature
type from k256.

* Fix the wasm build

* Fix a clippy lint

* Fix a clippy lint in alt_crypto test

* Disentangle CommitValidator from the hasher

Remove the only purpose for using a hasher in the implementation
by returning the set of validators in
VerificationErrorDetail::FaultySigner.

CommitValidator can now revert to being a (weird) default-implemented
trait, with a ProdCommitValidator to anchor the implementation.

* light-client: Enable shrinking in backward test

* light-client: update edition to rust2021

* Recover LightClient::verify_backward

A critical code block was hastily commented out in this
"unstable" part of code.

* Fix light-client-js build, record technical debt

* tendermint: Make sha2 an optional dependency

Use the Sha2 alias from crypto::default instead of the direct
references to sha2 crate. All code that used Sha2 non-generically
is feature-gated behind "rust-crypto".

* Redesign crypto::Sha256, add MerkleHash trait

The host API for obtaining SHA256 digests on Substrate is a simple
function, it cannot work incrementally. Change the Sha256 trait to
match this, but provide a MerkleHash trait to retain incremental
Merkle hashing with Rust-Crypto conformant digest implementations.
The merkle::NonIncremental adapter type is provided to fit the lowest common
denominator Sha256 API to a Merkle hash implementation, at the
cost of some allocations and extra copying.

* Fix all-features build

* chore: fix clippy lints

* tendermint: "rust-crypto" does not imply "k256"

Also rename crypto::default::ecdsa_secp256 to ecdsa_secp256k1,
to harmonize the naming with the feature that gates this module.

* rpc: require tendermint/rust-crypto

Needed for NodeKey.

* Remove a bogus lint override

* Changelog entry for #1238

* tendermint: crypto::signature::Verifier trait

Define Verifier trait to abstract signature verification given a
PublicKey.

The ed25519-consensus dependency is made optional and gated by the
"rust-crypto" feature.
The Verifier implementation is provided for a dummy type
crate::crypto::default::signature::Verifier, using ed25519-consensus.
The Ed25519 key types in public_key and private_key module are redefined
to in-crate newtypes.

* tendermint: fix secp256k1 verification tests

* Guard public key extraction with rust-crypto

* tendermint: remove re-exports from signature crate

As we don't currently use the signature framework, it makes no
sense to provide its traits through the crate.

* tendermint: rework signature in alt_crypto test

Implement the new Verifier trait instead of the signature traits.

* Updated the changelog entry for #1238

---------

Co-authored-by: Blas Rodriguez Irizar <[email protected]>
Co-authored-by: Romain Ruetschi <[email protected]>

.changelog/unreleased/breaking-changes/1238-crypto-provider-traits.md

@@ -0,0 +1,46 @@
+- `[tendermint]` Make implementations of cryptographic primitives replaceable
+  ([#1238](https://github.com/informalsystems/tendermint-rs/pull/1238)).
+  * Provide a `Sha256` trait in module `crypto` and make digest hashing
+    implementations available through it.
+  * Provide a `Verifier` trait in module `crypto::signature` to enable
+    alternative implementations of signature verification available through it.
+    An `Error` enum is defined in the same module, representing the error cases
+    that can arise in the implementation in a deliberately opaque way.
+  * The module `crypto::default` provides pure Rust implementations of the
+    cryptographic traits. The module is made available by a
+    new `rust-crypto` feature, enabled by default.
+  * `merkle::simple_hash_from_byte_vectors` is made generic over an
+    implementation of the new `MerkleHash` trait. Implementations for
+    Rust-Crypto conformant digest objects and the non-incremental
+    `crypto::Sha256` API are provided in the crate.
+  * The `Header::hash` and `ValidatorSet::hash` methods are gated by the
+    `rust-crypto` feature. Generic hashing methods not dependent on
+    the default crypto implementations are added for both types,
+    named `hash_with`.
+  * Conversions to `account::Id` and `node::Id` from `PublicKey` and
+    curve-specific key types are gated by the `rust-crypto` feature.
+  * The `validator::Info::new` method is gated by the `rust-crypto` feature.
+  * Remove a deprecated constant `signature::ED25519_SIGNATURE_SIZE`.
+
+- `[tendermint-light-client-verifier]` Changes for the new Tendermint crypto API
+  ([#1238](https://github.com/informalsystems/tendermint-rs/pull/1238)).
+  * The `rust-crypto` feature, enabled by default, guards the
+    batteries-included implementation types: `ProdVerifier`, `ProdPredicates`,
+    `ProdVotingPowerCalculator`.
+  * Remove the `operations::hasher` API (`Hasher` and `ProdHasher`),
+    made unnecessary by the new crypto abstractions in the `tendermint` crate.
+  * The `VerificationPredicates` trait features a `Sha256` associated type
+    to represent the hasher implementation, replacing the `&dyn Hasher`
+    parameter passed to methods.
+  * Change the type of the `VerificationErrorDetail::FaultySigner` field
+    `validator_set` to `ValidatorSet`. This removes a hasher dependency from
+    `CommitValidator`, and `ProdCommitValidator` is now an empty dummy type.
+
+- `[tendermint-light-client]` Changes for the new Tendermint crypto API
+  ([#1238](https://github.com/informalsystems/tendermint-rs/pull/1238)).
+  * The `rust-crypto` feature enables the default crypto implementations,
+    and is required by the `rpc-client` and `unstable` features.
+    `ProdForkDetector` is guarded by this feature, and is made a specific
+    type alias to the hasher-generic `ProvidedForkDetector` type.
+  * `LightClientBuilder` gets another type parameter for the Merkle hasher.
+    Its generic constructors lose the `Hasher` parameter.

config/Cargo.toml

@@ -25,7 +25,7 @@ all-features = true
 rustdoc-args = ["--cfg", "docsrs"]
 
 [dependencies]
-tendermint = { version = "0.28.0", default-features = false, path = "../tendermint" }
+tendermint = { version = "0.28.0", default-features = false, features = ["rust-crypto"], path = "../tendermint" }
 flex-error = { version = "0.4.4", default-features = false }
 serde = { version = "1", features = ["derive"] }
 serde_json = "1"

config/tests/mod.rs

@@ -219,7 +219,7 @@ fn priv_validator_json_parser() {
     let raw_priv_validator_key = read_fixture("priv_validator_key.json");
     let priv_validator_key = PrivValidatorKey::parse_json(raw_priv_validator_key).unwrap();
     assert_eq!(
-        priv_validator_key.consensus_pubkey().to_hex(),
+        priv_validator_key.consensus_pubkey().public_key().to_hex(),
         "F26BF4B2A2E84CEB7A53C3F1AE77408779B20064782FBADBDF0E365959EE4534"
     );
 }

light-client-js/Cargo.toml

@@ -23,7 +23,7 @@ default = ["console_error_panic_hook"]
 serde = { version = "1.0", default-features = false, features = [ "derive" ] }
 serde_json = { version = "1.0", default-features = false }
 tendermint = { version = "0.28.0", default-features = false, path = "../tendermint" }
-tendermint-light-client-verifier = { version = "0.28.0", default-features = false, path = "../light-client-verifier" }
+tendermint-light-client-verifier = { version = "0.28.0", features = ["rust-crypto"], default-features = false, path = "../light-client-verifier" }
 wasm-bindgen = { version = "0.2.63", default-features = false, features = [ "serde-serialize" ] }
 serde-wasm-bindgen = { version = "0.4.5", default-features = false }
 

light-client-js/src/lib.rs

@@ -18,10 +18,14 @@ use tendermint::Time;
 use tendermint_light_client_verifier::{
     options::Options,
     types::{LightBlock, TrustThreshold},
-    ProdVerifier, Verifier,
+    Verifier,
 };
 use wasm_bindgen::{prelude::*, JsValue};
 
+// TODO: Use Web Crypto API for cryptographic routines.
+// https://github.com/informalsystems/tendermint-rs/issues/1241
+use tendermint_light_client_verifier::ProdVerifier;
+
 /// Check whether a given untrusted block can be trusted.
 #[wasm_bindgen]
 pub fn verify(untrusted: JsValue, trusted: JsValue, options: JsValue, now: JsValue) -> JsValue {

light-client-verifier/Cargo.toml

@@ -23,7 +23,8 @@ all-features = true
 rustdoc-args = ["--cfg", "docsrs"]
 
 [features]
-default = ["flex-error/std", "flex-error/eyre_tracer"]
+default = ["rust-crypto", "flex-error/std", "flex-error/eyre_tracer"]
+rust-crypto = ["tendermint/rust-crypto"]
 
 [dependencies]
 tendermint = { version = "0.28.0", path = "../tendermint", default-features = false }
@@ -35,3 +36,4 @@ flex-error = { version = "0.4.4", default-features = false }
 
 [dev-dependencies]
 tendermint-testgen = { path = "../testgen", default-features = false }
+sha2 = { version = "0.10", default-features = false }

light-client-verifier/src/errors.rs

@@ -9,7 +9,7 @@ use tendermint::{account::Id, Error as TendermintError};
 use crate::{
     operations::voting_power::VotingPowerTally,
     prelude::*,
-    types::{Hash, Height, Time, Validator, ValidatorAddress},
+    types::{Hash, Height, Time, Validator, ValidatorAddress, ValidatorSet},
 };
 
 define_error! {
@@ -161,13 +161,12 @@ define_error! {
         FaultySigner
             {
                 signer: Id,
-                validator_set: Hash
+                validator_set: ValidatorSet
             }
             | e | {
                 format_args!(
-                    "Found a faulty signer ({}) not present in the validator set ({})",
+                    "Found a faulty signer ({}) not present in the validator set",
                     e.signer,
-                    e.validator_set
                 )
             },
 

light-client-verifier/src/lib.rs

@@ -11,4 +11,7 @@ pub mod predicates;
 pub mod types;
 mod verifier;
 
-pub use verifier::{PredicateVerifier, ProdVerifier, Verdict, Verifier};
+pub use verifier::{PredicateVerifier, Verdict, Verifier};
+
+#[cfg(feature = "rust-crypto")]
+pub use verifier::ProdVerifier;

light-client-verifier/src/operations.rs

@@ -1,8 +1,5 @@
 //! Crypto function traits allowing mocking out during testing
 
-pub mod hasher;
-pub use self::hasher::*;
-
 pub mod voting_power;
 pub use self::voting_power::*;
 

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

@@ -4,48 +4,12 @@ use tendermint::block::CommitSig;
 
 use crate::{
     errors::VerificationError,
-    operations::{Hasher, ProdHasher},
     types::{SignedHeader, ValidatorSet},
 };
 
 /// Validates the commit associated with a header against a validator set
 pub trait CommitValidator: Send + Sync {
     /// Perform basic validation
-    fn validate(
-        &self,
-        signed_header: &SignedHeader,
-        validators: &ValidatorSet,
-    ) -> Result<(), VerificationError>;
-
-    /// Perform full validation, only necessary if we do full verification (2/3)
-    fn validate_full(
-        &self,
-        signed_header: &SignedHeader,
-        validator_set: &ValidatorSet,
-    ) -> Result<(), VerificationError>;
-}
-
-/// Production-ready implementation of a commit validator
-#[derive(Debug, Clone, PartialEq, Eq)]
-pub struct ProdCommitValidator {
-    hasher: ProdHasher,
-}
-
-impl ProdCommitValidator {
-    /// Create a new commit validator using the given [`Hasher`]
-    /// to compute the hash of headers and validator sets.
-    pub fn new(hasher: ProdHasher) -> Self {
-        Self { hasher }
-    }
-}
-
-impl Default for ProdCommitValidator {
-    fn default() -> Self {
-        Self::new(ProdHasher::default())
-    }
-}
-
-impl CommitValidator for ProdCommitValidator {
     fn validate(
         &self,
         signed_header: &SignedHeader,
@@ -71,12 +35,7 @@ impl CommitValidator for ProdCommitValidator {
         Ok(())
     }
 
-    // This check is only necessary if we do full verification (2/3)
-    //
-    // See https://github.com/informalsystems/tendermint-rs/issues/281
-    //
-    // It returns `ImplementationSpecific` error if it detects a signer
-    // that is not present in the validator set
+    /// Perform full validation, only necessary if we do full verification (2/3)
     fn validate_full(
         &self,
         signed_header: &SignedHeader,
@@ -96,11 +55,17 @@ impl CommitValidator for ProdCommitValidator {
             if validator_set.validator(*validator_address).is_none() {
                 return Err(VerificationError::faulty_signer(
                     *validator_address,
-                    self.hasher.hash_validator_set(validator_set),
+                    validator_set.clone(),
                 ));
             }
         }
 
         Ok(())
     }
 }
+
+/// Production-ready implementation of a commit validator.
+#[derive(Copy, Clone, Default, Debug)]
+pub struct ProdCommitValidator;
+
+impl CommitValidator for ProdCommitValidator {}

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

@@ -1,11 +1,12 @@
 //! Provides an interface and default implementation for the `VotingPower` operation
 
 use alloc::collections::BTreeSet as HashSet;
-use core::{convert::TryFrom, fmt};
+use core::{convert::TryFrom, fmt, marker::PhantomData};
 
 use serde::{Deserialize, Serialize};
 use tendermint::{
     block::CommitSig,
+    crypto::signature,
     trust_threshold::TrustThreshold as _,
     vote::{SignedVote, ValidatorIndex, Vote},
 };
@@ -98,11 +99,31 @@ pub trait VotingPowerCalculator: Send + Sync {
     ) -> Result<VotingPowerTally, VerificationError>;
 }
 
-/// Default implementation of a `VotingPowerCalculator`
-#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
-pub struct ProdVotingPowerCalculator;
+/// Default implementation of a `VotingPowerCalculator`, parameterized with
+/// the signature verification trait.
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+pub struct ProvidedVotingPowerCalculator<V> {
+    _verifier: PhantomData<V>,
+}
+
+// Safety: the only member is phantom data
+unsafe impl<V> Send for ProvidedVotingPowerCalculator<V> {}
+unsafe impl<V> Sync for ProvidedVotingPowerCalculator<V> {}
+
+impl<V> Default for ProvidedVotingPowerCalculator<V> {
+    fn default() -> Self {
+        Self {
+            _verifier: PhantomData,
+        }
+    }
+}
+
+/// Default implementation of a `VotingPowerCalculator`.
+#[cfg(feature = "rust-crypto")]
+pub type ProdVotingPowerCalculator =
+    ProvidedVotingPowerCalculator<tendermint::crypto::default::signature::Verifier>;
 
-impl VotingPowerCalculator for ProdVotingPowerCalculator {
+impl<V: signature::Verifier> VotingPowerCalculator for ProvidedVotingPowerCalculator<V> {
     fn voting_power_in(
         &self,
         signed_header: &SignedHeader,
@@ -146,7 +167,7 @@ impl VotingPowerCalculator for ProdVotingPowerCalculator {
             // Check vote is valid
             let sign_bytes = signed_vote.sign_bytes();
             if validator
-                .verify_signature(&sign_bytes, signed_vote.signature())
+                .verify_signature::<V>(&sign_bytes, signed_vote.signature())
                 .is_err()
             {
                 return Err(VerificationError::invalid_signature(