Make crypto implementations replaceable #1238

mzabaluev · 2022-11-21T14:28:31Z

Resolves: #1213

I took some of the changes from #1214 and made more extensive use of them in tendermint hashing.

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Added entry in .changelog/

It has been deprecated for some time.

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

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

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

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".

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.

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

mzabaluev · 2022-11-23T13:57:09Z

Minimized the failing corrupted_hash test to this input:

tc = TestCase { length: 2, chain: LightChain { info: Info { id: chain::Id(test-chain), height: block::Height(2), last_block_id: Some(Id { hash: Hash::Sha256(1843C619BB7E92A4D6420B0A2A2774D478613D5FBDAAB1D981720D4DA71B1270), part_set_header: Header { total: 0, hash: Hash::None } }), time: None }, light_blocks: [LightBlock { header: Some(Header { validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), next_validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), chain_id: Some("test-chain"), height: Some(1), time: Some(Time(2021-01-08 11:23:41.0)), proposer: None, last_block_id_hash: None }), commit: Some(Commit { header: Some(Header { validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), next_validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), chain_id: Some("test-chain"), height: Some(1), time: Some(Time(1970-01-01 0:00:01.0)), proposer: None, last_block_id_hash: None }), votes: Some([Vote { validator: Some(Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }), index: Some(0), header: Some(Header { validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), next_validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), chain_id: Some("test-chain"), height: Some(1), time: Some(Time(1970-01-01 0:00:01.0)), proposer: None, last_block_id_hash: None }), prevote: None, height: None, time: None, round: Some(1), nil: None }, Vote { validator: Some(Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }), index: Some(1), header: Some(Header { validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), next_validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), chain_id: Some("test-chain"), height: Some(1), time: Some(Time(1970-01-01 0:00:01.0)), proposer: None, last_block_id_hash: None }), prevote: None, height: None, time: None, round: Some(1), nil: None }]), round: Some(1) }), validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), next_validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), provider: Some(node::Id(badfadad0befeedc0c0adeadbeefc0ffeefacade)) }, LightBlock { header: Some(Header { validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), next_validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), chain_id: Some("test-chain"), height: Some(2), time: Some(Time(1970-01-01 0:00:02.0)), proposer: None, last_block_id_hash: Some(Hash::Sha256(BD301C7EC29BF24128086A09928F6582A69ED4DFA105D30B56E85F0661DC0C43)) }), commit: Some(Commit { header: Some(Header { validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), next_validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), chain_id: Some("test-chain"), height: Some(2), time: Some(Time(1970-01-01 0:00:02.0)), proposer: None, last_block_id_hash: Some(Hash::Sha256(BD301C7EC29BF24128086A09928F6582A69ED4DFA105D30B56E85F0661DC0C43)) }), votes: Some([Vote { validator: Some(Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }), index: Some(0), header: Some(Header { validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), next_validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), chain_id: Some("test-chain"), height: Some(2), time: Some(Time(1970-01-01 0:00:02.0)), proposer: None, last_block_id_hash: Some(Hash::Sha256(BD301C7EC29BF24128086A09928F6582A69ED4DFA105D30B56E85F0661DC0C43)) }), prevote: None, height: None, time: None, round: Some(1), nil: None }, Vote { validator: Some(Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }), index: Some(1), header: Some(Header { validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), next_validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), chain_id: Some("test-chain"), height: Some(2), time: Some(Time(1970-01-01 0:00:02.0)), proposer: None, last_block_id_hash: Some(Hash::Sha256(BD301C7EC29BF24128086A09928F6582A69ED4DFA105D30B56E85F0661DC0C43)) }), prevote: None, height: None, time: None, round: Some(1), nil: None }]), round: Some(1) }), validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), next_validators: Some([Validator { id: Some("1"), voting_power: Some(50), proposer_priority: None }, Validator { id: Some("2"), voting_power: Some(50), proposer_priority: None }]), provider: Some(node::Id(badfadad0befeedc0c0adeadbeefc0ffeefacade)) }] }, target_height: block::Height(1), trusted_height: block::Height(2) }

Farhad-Shabani · 2022-11-24T09:23:21Z

I have been reviewing the changes, some thoughts came to my mind, don’t know how much is applicable:

It seems that CryptoProvider should be implemented as a higher-level trait making hashing/Verification/Signing operations available all over the repo. while this way it already is fed upward from a struct, so it's making us introduce PhantomData & generic types to funcs
CryptoProvider already hosts incongruous types for different functionalities. These purposes can be decoupled, having them within multiple smaller traits. So it would get more manageable & modular for other possible future calls
Some types could be named more generic

Therefore, e.g.:

pub trait HashProvider: Default + Send + Sync {
	// Using `Hash` instead of `Sha256` as it might accept other hashing algorithms
	// Though, I noticed we also have `Hash` types in other places like:
	// `tendermint/hash.rs` and `tendermint/merkle.rs` don't know if these could be integrated
	type Hash: Digest<OutputSize = U32> + FixedOutputReset;
	type Hasher: Hasher<Self::Hash>

pub trait Hasher<H: HashProvider> {
	fn hasher(&self, hash: &H) -> Result<[u8; 32], Error>;

use signature::{Signature, Signer, Verifier};
pub trait SignatureProvider: Default + Send + Sync {
        // Using `Signature` instead of `EcdsaSecp256k1Signature` to not be curve specific
        type Signature: Signature;
        type Signer: Signer<Self::Signature>;
        type Verifier: Verifier<Self::Signature>;
}

Accordingly, we might be able to pass our current functions as default implementations(behaviors) of Hasher/Verifier/Signer traits rather than having them through feature="rust-crypto". Also, implementing the Hasher trait on the Header and Set structs will give us the hasher() method we need.
Another thing, I don’t know if we can refactor merkle.rs and have simple_hash_from_byte_vectors within a trait (name it e.g. HashTree), So we could accept various hashing tree logics and bound it byHashProvider

pub trait HashTree: HashProvider {
        fn simple_hash_from_byte_vectors(&self, byte_vecs: &[Vec<u8>]) -> Hash
}

mzabaluev · 2022-11-24T14:30:29Z

It seems that CryptoProvider should be implemented as a higher-level trait making hashing/Verification/Signing operations available all over the repo. while this way it already is fed upward from a struct, so it's making us introduce PhantomData & generic types to funcs

In this PR I actually managed to avoid introducing PhantomData, except where it was already present in the fake-Substrate verifier test created in #1214.

* `CryptoProvider` already hosts incongruous types for different functionalities. These purposes can be decoupled, having them within multiple smaller traits. So it would get more manageable & modular for other possible future calls

I like it, this continues breaking down of HostProvider into more sensible chunks, and I already see most of CryptoProvider is not needed in places that only care about hashing. Maybe even different signature algo providers could have each its own trait. Basically, if the need for a unified trait is not evident from usage, it can be split up.

pub trait Hasher<H: HashProvider> {
	fn hasher(&self, hash: &H) -> Result<[u8; 32], Error>;
}

From the name it's not very clear what does this trait do. In other crypto code, the term "hasher" is typically used for the digest state objects, but here it looks like it's meant for hashing self with the provided instance of a crypto provider.
Note that the current CryptoProvider trait is instanceless, because it's the associated types that it references that implement instanced digest APIs.

To think of it a bit more, maybe we just want to put more specialized and domain-friendly faces on top of Digest and Signature with blanket-implemented traits?

Accordingly, we might be able to pass our current functions as default implementations(behaviors) of Hasher/Verifier/Signer traits rather than having them through feature="rust-crypto". Also, implementing the Hasher trait on the Header and Set structs will give us the hasher() method we need.

So it's actually a "hash-me" trait.
I don't see a need for a trait that generalizes over Header and validator::Set, I think inherent methods would do the job.

Another thing, I don’t know if we can refactor merkle.rs and have simple_hash_from_byte_vectors within a trait (name it e.g. HashTree), So we could accept various hashing tree logics and bound it byHashProvider
pub trait HashTree: HashProvider {
        fn simple_hash_from_byte_vectors(&self, byte_vecs: &[Vec<u8>]) -> Hash
}

I would not couple hashing strategies in one trait hierarchy with the hashing provider, it seems to me like they could be implemented orthogonally. Alternative hashing strategies seem like YAGNI for the purposes of this change, so I'd put it off to a later redesign.

Farhad-Shabani · 2022-11-24T20:50:59Z

but here it looks like it's meant for hashing selfwith the provided instance of a crypto provider.
So it's actually a "hash-me" trait.

I meant so. And actually, having one of fn hash_me() or type Hash: Sha256 under HashProvider trait would suffice to traverse the merkle hashing tree with the desired hash function. Unless we want to have simultaneous access to different hashing funcs at once which I don't think to be the case

To think of it a bit more, maybe we just want to put more specialized and domain-friendly faces on top of Digest and Signature with blanket-implemented traits?

This could be a good idea

I would not couple hashing strategies in one trait hierarchy with the hashing provider, it seems to me like they could be implemented orthogonally. Alternative hashing strategies seem like YAGNI for the purposes of this change, so I'd put it off to a later redesign.

Agree, it could be the subject of future work if necessary

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.

Farhad-Shabani · 2022-12-16T00:08:18Z

I liked the generic interface over the Merkle tree 👍

While reviewing, I came up with some ideas you might want to consider.
I worked on it in this draft just to see how much would apply and as a reference to better convey my purposes.
The suggestions are updated as follows, considering comments made in the draft around incremental hashing:

Given the MerkleHash API, IMO the NonIncremental solution could be left to be implemented and kept within the codebases that need it (outside Tendermint's repo boundary), So we only stick with the best practice.
But, to do so, the digest method of Sha256 should be callable there, which requires MerkleHash be bounded by the Sha256
Since the digest interface can be given to any Hash function, I'd rename the Sha256 trait to something more general (like Hasher, e.g.), So it wouldn’t look like MerkleHash is bounded by the specific hash function.
Following the above, I think the copy_to_hash step could also be baked inside the digest method.
Also, having MerkleHash as a trait enables it to be invoked through hash_byte_vectors() method without needing simple_hash_from_byte_vectors() function
Respectively, considering MerkleHash implementation for sha::Sha256 (as the default hasher), all needed components would be there without defining feature="rust-crypto", (Unless this feature really worth for wasm runtime)
Providing an interface for signature verification seems applicable. This is examined by adding the signature/Verifier trait to the crypto.rs, and then refactoring the Pub_key.rs and validator.rs here

Wizdave97 · 2022-12-16T08:31:12Z

#1238 (comment) would work for us as long as the SignatureType and SignatureVerifier implementation can be modified when we pull in tendermint-rs

romac · 2023-01-17T09:53:27Z

FYI the signature crate just released a new major version, might be worth taking a look at the changes: https://github.com/RustCrypto/traits/blob/master/signature/CHANGELOG.md

mzabaluev · 2023-01-19T11:21:39Z

FYI the signature crate just released a new major version

We should update to it after ed25519-consensus and other crates release their updates using the same version.
As of now, we should provide our own trait because Henry's crate does not actually use signature due to some technical issues with the old framework version.

romac · 2023-01-19T14:46:42Z

We should update to it after ed25519-consensus and other crates release their updates using the same version.

Do you have a list of all the crates we need to wait on?

mzabaluev · 2023-01-20T13:13:52Z

Do you have a list of all the crates we need to wait on?

Not in this PR, but here's a brief survey of the dependency tree:

ed25519 (the framework lib, no implementation) - had a 2.0 release recently.
ecdsa - 0.15 release has been updated.
ed25519-consensus will probably need to be adapted to use the new framework crates, to spare us the need to define our own abstraction traits.

tony-iqlusion · 2023-01-20T15:13:39Z

ed25519-consensus doesn't impl the signature crate's traits, although I have a half-finished PR open:

penumbra-zone/ed25519-consensus#6

mzabaluev · 2023-01-20T15:17:16Z

ed25519-consensus doesn't impl the signature crate's traits, although I have a half-finished PR open:

@tony-iqlusion Now that the warts in signature preventing an efficient curve point backed implementation have been reportedly resolved in 2.0, can this be completed?

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.

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

Implement the new Verifier trait instead of the signature traits.

blasrodri · 2023-01-27T11:55:03Z

Really appreciate this!!

blasrodri · 2023-01-31T09:51:12Z

@Farhad-Shabani can you please review this PR? We really need it merged :)

romac · 2023-01-31T10:25:19Z

@Farhad-Shabani I approved the PR and merged main in. Feel free to merge this!

Farhad-Shabani · 2023-01-31T12:58:07Z

Thank you so much @mzabaluev, I truly appreciate your patience. And thank you @romac for reviewing it 🙏🏻
🎉 🎉 🎉

blasrodri and others added 17 commits November 17, 2022 15:36

define CryptoProvider trait

62dda70

wip

b7920c3

implement CryptoProvider for Substrate (WIP)

c4b52de

comment issues

a35c105

ed25519_verify implementation for SubstrateHostFunctionsManager

47b56bf

introduce EcdsaSecp256k1Signer

2d0fc38

doc a bit

6cbcbb3

move CryptoProvider to tendermint crate instead

e6edade

rename host_functions module to crypto

bcd6be0

tendermint: Remove ED25519_SIGNATURE_SIZE

7f5e812

It has been deprecated for some time.

tendermint: clean up mod crypto

3fecdef

Disentangle CryptoProvider from k256 crate

4a511c5

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

Add DefaultCryptoProvider

9575852

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

tendermint: reorg mod crypto

3573ef0

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

tendermint: generalize hash methods

2baed4f

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

a032364

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

f264742

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

mzabaluev requested a review from Farhad-Shabani November 21, 2022 14:28

mzabaluev mentioned this pull request Nov 21, 2022

Add crypto provider #1214

Closed

fix no_std

f0eee32

mzabaluev added 2 commits November 23, 2022 18:27

Fix the tools build

ce146c8

Fix wasm-light-client build

4a65669

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

d39148d

mzabaluev added 3 commits November 28, 2022 13:04

Merge branch 'main' into mikhail/add-crypto-provider

4503794

Merge branch 'main' into mikhail/add-crypto-provider

f1d9593

mzabaluev marked this pull request as ready for review December 15, 2022 13:59

mzabaluev requested a review from hu55a1n1 December 15, 2022 14:57

Farhad-Shabani mentioned this pull request Dec 16, 2022

Investigate some design tweaks over Crypto Implementation #1252

Closed

5 tasks

mzabaluev requested review from romac and removed request for hu55a1n1 December 19, 2022 13:51

Merge branch 'main' into mikhail/add-crypto-provider

24573b9

Merge branch 'main' into mikhail/add-crypto-provider

2a7f441

mzabaluev force-pushed the mikhail/add-crypto-provider branch from dd57553 to d525562 Compare January 24, 2023 11:29

mzabaluev added 5 commits January 24, 2023 13:52

tendermint: fix secp256k1 verification tests

672db2d

Guard public key extraction with rust-crypto

3d7ee0a

tendermint: remove re-exports from signature crate

3acfa23

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

2b49f63

Implement the new Verifier trait instead of the signature traits.

Updated the changelog entry for #1238

3741884

romac approved these changes Jan 31, 2023

View reviewed changes

Merge branch 'main' into mikhail/add-crypto-provider

5ef4502

Farhad-Shabani merged commit 47e28b5 into main Jan 31, 2023

Farhad-Shabani deleted the mikhail/add-crypto-provider branch January 31, 2023 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make crypto implementations replaceable #1238

Make crypto implementations replaceable #1238

mzabaluev commented Nov 21, 2022 •

edited

Loading

mzabaluev commented Nov 23, 2022

Farhad-Shabani commented Nov 24, 2022

mzabaluev commented Nov 24, 2022

Farhad-Shabani commented Nov 24, 2022

Farhad-Shabani commented Dec 16, 2022 •

edited

Loading

Wizdave97 commented Dec 16, 2022 •

edited

Loading

romac commented Jan 17, 2023

mzabaluev commented Jan 19, 2023

romac commented Jan 19, 2023

mzabaluev commented Jan 20, 2023

tony-iqlusion commented Jan 20, 2023

mzabaluev commented Jan 20, 2023

blasrodri commented Jan 27, 2023

blasrodri commented Jan 31, 2023

romac commented Jan 31, 2023 •

edited

Loading

Farhad-Shabani commented Jan 31, 2023

Make crypto implementations replaceable #1238

Make crypto implementations replaceable #1238

Conversation

mzabaluev commented Nov 21, 2022 • edited Loading

mzabaluev commented Nov 23, 2022

Farhad-Shabani commented Nov 24, 2022

mzabaluev commented Nov 24, 2022

Farhad-Shabani commented Nov 24, 2022

Farhad-Shabani commented Dec 16, 2022 • edited Loading

Wizdave97 commented Dec 16, 2022 • edited Loading

romac commented Jan 17, 2023

mzabaluev commented Jan 19, 2023

romac commented Jan 19, 2023

mzabaluev commented Jan 20, 2023

tony-iqlusion commented Jan 20, 2023

mzabaluev commented Jan 20, 2023

blasrodri commented Jan 27, 2023

blasrodri commented Jan 31, 2023

romac commented Jan 31, 2023 • edited Loading

Farhad-Shabani commented Jan 31, 2023

mzabaluev commented Nov 21, 2022 •

edited

Loading

Farhad-Shabani commented Dec 16, 2022 •

edited

Loading

Wizdave97 commented Dec 16, 2022 •

edited

Loading

romac commented Jan 31, 2023 •

edited

Loading