-
Notifications
You must be signed in to change notification settings - Fork 224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
simple_hash_from_byte_slices takes a Vec instead of slice #59
Conversation
- work in progress as we might want to let an inner function for the recursion - also we might simply want to use From to consume (or create a Vec) instead of changing the method signature (as suggested here: #57 (comment))
315c026
to
d0e5e27
Compare
I think this is good enough here. See |
direct vector initialization instead of going through &[&[u8]]
tendermint/src/merkle.rs
Outdated
/// Compute a simple Merkle root from the arbitrary sized byte slices | ||
pub fn simple_hash_from_byte_slices(byte_slices: &[&[u8]]) -> Hash { | ||
/// Compute a simple Merkle root from the arbitrary byte arrays | ||
pub fn simple_hash_from_byte_slices(byte_slices: Vec<Vec<u8>>) -> Hash { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those aren't slices 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to simple_hash_from_byte_vectors
in 937ba67.
I think a better name for the the method would rather be compute_merkle_root_from_leaves
or compute_merkle_root_from_leaf_values
instead of simple_hash_from_[some_type]
.
This would actually say what the method does instead of what type it expects as input (which the method signature clearly states). What do you think?
The current naming probably tries to as close as possible to the go implementation.
CC @ebuchman
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right about better naming happy to see this changed.
The method that computes the root from arbitrary given bytes doesn't take slices but Rust's array-type: vector. Also, update comment.
* build(deps): update tai64 requirement from 2 to 3 (informalsystems#22) Updates the requirements on [tai64](https://github.com/iqlusioninc/crates) to permit the latest version. - [Release notes](https://github.com/iqlusioninc/crates/releases) - [Commits](iqlusioninc/crates@canonical-path/v2.0.0...tai64/3.0.0) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> * simple_hash_from_byte_slices takes a Vec instead of slice * cargo fmt * WIP: pass in Vec<Vec<u8> instead of &Vec<Vec<u8> - work in progress as we might want to let an inner function for the recursion - also we might simply want to use From to consume (or create a Vec) instead of changing the method signature (as suggested here: informalsystems#57 (comment)) * use Vec<Vec<u8>> with inner recursion method * merge in changes of informalsystems#59 and simplify Header::hash * simplify tests: direct vector initialization instead of going through &[&[u8]] * remove obsolete comment * Further reduce boilerplate code by leveraging generics * max_gas should be i64, can be -1 * remove unneed clones * remove secret_connection * Create a trait for with a blanket impl instead of new type Thanks for the suggestion @tarcieri :-) * impl lite::Commit for commit::SignedHeader * Parse validators field in genesis api * Modify lite::Commit impl for SignedHeader to not include amino dep: - SignedVote can now be constructed without going through CanoncialVote (which is an amino-type) - modify impl to match new constructor and remove amino dep - make explicit where which types are amino_types (in vote.rs) * impl lite::ValidatorSetLookup * Add a few methods to tests and deduplicate code for fn hash of lite::ValidatorSet:: & Set - tests for lite::ValidatorSetLookup lite::ValidatorSet impl - delete `fn hash(self) -> merkle::Hash` from Set and only keep the impl of lite::ValidatorSet * implement utility traits for tendermint data types * Signing bytes need to encoded with encode_length_delimited * utilities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
For interest's sake, at what point does it warrant turning the merkle::Hash
type into a trait/struct? In that sort of case we could implement functions like simple_hash_from_byte_slices
or simple_hash_from_byte_vectors
as constructors for a merkle::Hash
object.
Thanks for the review @thanethomson! Yes, I think it would be worth turning simple merkle into a trait soon. It also would be cool if the hashing algo could be easily swappable. I also looked a bit into this implementation: https://github.com/filecoin-project/merkle_light/tree/master/src but the amount of generality seems a bit like overkill for simple merkle. I'll open an issue for this and a separate PR. Although it seems merging this is also blocked by the GPG check :-/ (although all commits are signed, not sure what is going on here ...) |
Further reduce boilerplate code by leveraging generics
* merge in changes of #59 and simplify Header::hash Further reduce boilerplate code by leveraging generics * Create a trait for with a blanket impl instead of new type Thanks for the suggestion @tarcieri :-) * impl lite::Commit for commit::SignedHeader * impl lite::ValidatorSetLookup * Modify lite::Commit impl for SignedHeader to not include amino dep: - SignedVote can now be constructed without going through CanoncialVote (which is an amino-type) - modify impl to match new constructor and remove amino dep - make explicit where which types are amino_types (in vote.rs) * Represent optional hash and block id as Option and correctly compute header hash when block id is empty. Represent empty Hash with Option * utilities * Add a few methods to tests and deduplicate code for fn hash of lite::ValidatorSet:: & Set - tests for lite::ValidatorSetLookup lite::ValidatorSet impl - delete `fn hash(self) -> merkle::Hash` from Set and only keep the impl of lite::ValidatorSet * Signing bytes need to encoded with encode_length_delimited * add mock testing for lite client * parse empty commit * Add TrustLevel as a trait; default behaviour is as before: accept iff +1/3 of the voting power signed * minor clarifications: updated comments and clarified test-name * Add a verify method which is closer to the spec / the current golang code (lite2) and tests: - rename former verify method to verify_header_and_commit (might be obsolete) - make the expired method public to be able to use and test it from the outside - add some helpers to be able to deal with deserialization hickups - add a bunch of tests using @Shivani912's generator code (with minor modifications) * remove unused unit return type * add `/validators` to rpc client * remove associated type (Vote) from SignedHeader * Fix a few minor issues from rebasing * make method for retrieving validators in rpc client async too and some minor changes due to rebasing on master * rename byteslices -> fields_bytes * Incorporate review comments, more tests, some renaming: - rename TrustLevel -> TrustThreshold - delete `verify_trusting` - use `verify_header_and_commit` in main `verify` method instead of `verify_commit_full` only ... (more tests pass) - add a few test-cases: in header_tests.json, commit_tests.json - keep track of test-cases that currently fail in separate files: commit_tests_failing.json, header_tests_failing.json - delete basic.json test * Refactor trait types to be slightly closer to the spec: - add a `voting_power_in` method to Commit trait (corresponds to votingpower_in(V1, V1) auxiliary function of spec - use above method to simplify `verify_commit_full` and `verify_commit_trusting` - move validator(id) lookup method to ValidatorSet trait - delete `ValidatorSetLookup` trait - take references everywhere (instead of cloning) - add a len() method the ValidatorSet trait - add in an is_empty method to ValidatorSet; otherwise: `trait `ValidatorSet` has a `len` method but no (possibly inherited) `is_empty` method` * Remove into_vec methods from Commit & ValidatorSet trait: - they are not used in the abstract verifier logic; hence they do not need to be part of the trait anymore * Add in a correctly formed (but invalid) hash instead of `[]byte("wrong PartsHeader hash!!")` -> still failing * use associated types for SignedHeader instead of generics improve the readability of the abstract traits / verification logic * Remove dependency on tendermint's time implementation (use SystemTime instead) and add method `CheckSupport` (see spec) * Be closer to the current spec (and less confusing hopefully): - delete `verify` add in a `verify` method that verifies a signed header - use check_support in the tests instead - add module documentation (see: cargo doc --open --no-deps) - make comment a todo to make it easier grepable - add in some feedback for shivani * Minor improvements on light client test: - remove obsolete comments and - introduce const for test files * From review: fix some comments (typo & stale comment) * Remove lite::Vote altogether * Optimize validator lookup: find() then clone() only clones the found elem instead of cloning the whole iter * moved SignedHeader struct from `rpc::endpoint` to `block::signed_header`
resolves #57