simple_hash_from_byte_slices takes a Vec instead of slice #59

liamsi · 2019-11-08T12:35:51Z

resolves #57

- 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))

liamsi · 2019-11-09T13:27:59Z

I think this is good enough here.

See
https://github.com/interchainio/tendermint-rs/compare/lite_impl...lite_impl_simple_merkle_merged?expand=1
how this would affect the usage of simple_hash_from_byte_slices (including some further simplifications).

direct vector initialization instead of going through &[&[u8]]

tarcieri · 2019-11-15T12:35:32Z

tendermint/src/merkle.rs

-/// 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 {


Those aren't slices 😉

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

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.

@tarcieri

* 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

thanethomson

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.

liamsi · 2019-11-19T22:20:59Z

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

@tarcieri

* 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`

liamsi added 2 commits November 8, 2019 13:34

simple_hash_from_byte_slices takes a Vec instead of slice

6665e8e

cargo fmt

a6fcfae

liamsi mentioned this pull request Nov 8, 2019

simple_hash_from_byte_slices should take a Vec instead of slice #57

Closed

liamsi added 2 commits November 9, 2019 11:59

WIP: pass in Vec<Vec<u8> instead of &Vec<Vec<u8>

e34f1b6

- 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))

use Vec<Vec<u8>> with inner recursion method

d0e5e27

liamsi force-pushed the ismail/vector-simple_hash_from_byte_slices branch from 315c026 to d0e5e27 Compare November 9, 2019 12:33

liamsi added a commit that referenced this pull request Nov 9, 2019

merge in changes of #59 and simplify Header::hash

3758cf4

liamsi marked this pull request as ready for review November 9, 2019 13:24

liamsi requested a review from ebuchman November 9, 2019 13:24

liamsi added 2 commits November 9, 2019 15:15

simplify tests:

be377fa

direct vector initialization instead of going through &[&[u8]]

remove obsolete comment

b4478b8

liamsi mentioned this pull request Nov 11, 2019

Light client: further trait impls, improvements, and tests #63

Closed

liamsi requested a review from tarcieri November 15, 2019 11:45

tarcieri reviewed Nov 15, 2019

View reviewed changes

Rename simple_hash_from_byte_slices to simple_hash_from_byte_vectors

937ba67

The method that computes the root from arbitrary given bytes doesn't take slices but Rust's array-type: vector. Also, update comment.

liamsi requested review from tarcieri, brapse and thanethomson November 19, 2019 10:03

thanethomson approved these changes Nov 19, 2019

View reviewed changes

This was referenced Nov 19, 2019

Remove secret_connection and ring dependency #60

Merged

Generalize simple_merkle #73

Open

liamsi merged commit cf7e702 into master Nov 19, 2019

liamsi deleted the ismail/vector-simple_hash_from_byte_slices branch November 19, 2019 23:07

liamsi added a commit that referenced this pull request Dec 10, 2019

merge in changes of #59 and simplify Header::hash

98d0d2f

liamsi added a commit that referenced this pull request Dec 10, 2019

merge in changes of #59 and simplify Header::hash

030bd21

liamsi mentioned this pull request Dec 10, 2019

Light client: further trait impls, improvements, and tests #84

Merged

liamsi added a commit that referenced this pull request Dec 12, 2019

merge in changes of #59 and simplify Header::hash

8604b57

Further reduce boilerplate code by leveraging generics

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simple_hash_from_byte_slices takes a Vec instead of slice #59

simple_hash_from_byte_slices takes a Vec instead of slice #59

liamsi commented Nov 8, 2019 •

edited

Loading

liamsi commented Nov 9, 2019 •

edited

Loading

tarcieri Nov 15, 2019

liamsi Nov 16, 2019 •

edited

Loading

ebuchman Nov 25, 2019

thanethomson left a comment

liamsi commented Nov 19, 2019

simple_hash_from_byte_slices takes a Vec instead of slice #59

simple_hash_from_byte_slices takes a Vec instead of slice #59

Conversation

liamsi commented Nov 8, 2019 • edited Loading

liamsi commented Nov 9, 2019 • edited Loading

tarcieri Nov 15, 2019

Choose a reason for hiding this comment

liamsi Nov 16, 2019 • edited Loading

Choose a reason for hiding this comment

ebuchman Nov 25, 2019

Choose a reason for hiding this comment

thanethomson left a comment

Choose a reason for hiding this comment

liamsi commented Nov 19, 2019

liamsi commented Nov 8, 2019 •

edited

Loading

liamsi commented Nov 9, 2019 •

edited

Loading

liamsi Nov 16, 2019 •

edited

Loading