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

liamsi · 2019-12-10T23:09:30Z

Supersedes #63: clean-up the commit history and rebased the changes of #63 on master.

From #63's description:

ref #36

Update: Since this PR was opened (mostly to show the impact of #59), it grew to contain several further PRs added in by @yihuang (big thanks). These added 2 missing impls, resolved a few encoding edge cases and simple (json-based) tests.

Particularly:

impl lite::Commit for commit::SignedHeader #67
impl lite::ValidatorSetLookup #68 (can be ignored for reviewing this PR as the ValidatorSetLookup trait was deleted later)
Handle empty block id #66 (this as well as the following two mainly deal with decoding JSON )
Signing bytes need to encoded with encode_length_delimited #69
Implement several utility trait #70
add mock testing for lite client #72 (can be ignored for reviewing this PR because most of its changes got reverted in subsequent commits)
Represent last_commit as Option<Commit> #79 (deals with decoding JSON)

Additionally this PR introduces a TrustLevel trait via bd26e74 and adds a verify method that is closer to the (current) spec / go-code in 017c1ba (with a few tests from JSON).

tendermint/src/block/header.rs

+        byteslices.push(self.app_hash.as_ref().map_or(vec![], encode_hash));
+        byteslices.push(self.last_results_hash.as_ref().map_or(vec![], encode_hash));
+        byteslices.push(self.evidence_hash.as_ref().map_or(vec![], encode_hash));
+        byteslices.push(bytes_enc(self.proposer_address.as_bytes()));


liamsi · 2019-12-12T01:18:12Z

TODOs:

rebase on master
update added rpc client method for /validators to be async too
rename Light client: further trait impls, improvements, and tests #84 (comment) (and other occurrences)?
document main verify function (check with latest spec) and adopt names of functions in the spec
deduplicate by adding a verification method to the Commit trait (which takes a validator-set) instead of doing: https://github.com/interchainio/tendermint-rs/blob/7b264815946b540d2b49f0c47ed2a47c45c5387e/tendermint/src/lite/verifier.rs#L120-L133 and https://github.com/interchainio/tendermint-rs/blob/7b264815946b540d2b49f0c47ed2a47c45c5387e/tendermint/src/lite/verifier.rs#L163-L184
remove ValidatorSetLookup trait?
remove dependency on tendermint types

Further reduce boilerplate code by leveraging generics

@tarcieri

Thanks for the suggestion @tarcieri :-)

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

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

and correctly compute header hash when block id is empty. Represent empty Hash with Option

accept iff +1/3 of the voting power signed

@Shivani912

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

and some minor changes due to rebasing on master

tendermint/src/lite/verifier.rs

liamsi · 2019-12-12T18:45:35Z

tendermint/tests/lite.rs

+    run_test_cases(cases);
+}
+
+fn run_test_cases(cases: TestCases) {


Compare: https://github.com/Shivani912/tendermint/blob/1b0fcf645b1ca67a93a708674db806ce7c8f630e/types/lite-client/tests/lite_client_test.go#L13-L58

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

liamsi · 2019-12-12T21:31:16Z

tendermint/src/lite/verifier.rs

-        }
-        signed_power += val.power();
-    }
+    let signed_power = commit.voting_power_in(vals)?;


@ancazamfir @ebuchman Does this work for you? I think it's closer to what is in the spec and should also be less confusing. It's used here and below in verify_commit_trusting (we do lookups in both cases now).

referencing related comments: #36 (comment)

see votingpower_in(V1,V2) function in spec: https://github.com/tendermint/spec/blob/4f7c55507cb99c35c7774c583f182859283f3017/spec/consensus/light-client.md#functions-1

Yes, it looks better imo.

- they are not used in the abstract verifier logic; hence they do not need to be part of the trait anymore

liamsi · 2019-12-12T23:06:21Z

tendermint/src/lite/types.rs

-    /// and thus use an ternary enum here instead of the binary Option.
-    // TODO figure out if we want/can do an iter() method here that returns a
-    // VoteIterator instead of returning a vec
-    fn into_vec(&self) -> Vec<Option<Self::Vote>>;


The into_vec or any possibility to iterate over the underlying votes is not used in the abstract verifier code. The parts of the code that needed this is now moved into voting_power_in (see below). I think this makes the code more readable and closer to the spec.

liamsi · 2019-12-12T23:16:05Z

I'd appreciate another pass of reviews here. I started working on putting this all together but it would be nice to merge this soon and continue with the remaining parts of the light client (on-top of these changes):

initial config for light client
requesting from multiple peers (networking)
storing state (memory, DB)
detecting misbehaviour, gossiping misbehaviour
bisection
creating command line tool (light client daemon)

…g PartsHeader hash!!")` -> still failing

liamsi · 2019-12-13T06:51:51Z

tendermint/tests/support/lite/commit_tests_failing.json

+              "evidence_hash": "",
+              "proposer_address": "01F527D77D3FFCC4FCFF2DDC2952EEA5414F2A33"
+            },
+            "commit": null


Currently, this (null commit) won't even parse correctly. (In the SignedHeader struct a commit isn't optional).

liamsi · 2019-12-13T06:53:36Z

tendermint/tests/support/lite/header_tests_failing.json

+      "input": [
+        {
+          "signed_header": {
+            "header": null,


Currently, this (null header) won't even parse correctly. (In the SignedHeader struct a header isn't optional).

@Shivani912 why do we even have a null header? Is there ever a case where the rpc would return this?

improve the readability of the abstract traits / verification logic

… instead) and add method `CheckSupport` (see spec)

ancazamfir

Looks good! A couple of comments on the verifier.rs. Slowly catching up on the others.

tendermint/src/lite/verifier.rs

ancazamfir · 2019-12-13T19:59:01Z

tendermint/src/lite/verifier.rs

-        }
-        signed_power += val.power();
-    }
+    let signed_power = commit.voting_power_in(vals)?;


Yes, it looks better imo.

- 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

ebuchman

Awesome work, thanks! Few minor points but I think we can probably merge and follow up in more PRs

tendermint/src/lite/types.rs

tendermint/src/rpc/endpoint/commit.rs

tendermint/src/validator.rs

ebuchman · 2019-12-15T22:01:51Z

tendermint/tests/lite.rs

+    // Then for tests this structure could be used for the skipping case too:
+    // The light client get's two such LiteBlocks (one with height h and one with height h-x)
+    // This gives more flexibility on how to compose the tests and can be used
+    // to mock both rpc endpoints too?


This is a good idea cc @Shivani912.

Don't we need to include the NextValidatorSet as well ? Even in bisection, we're verifying the commit against the current Validators, but setting the NextValidators as the trusted ones when checking support for a subsequent commit.

tendermint/src/lite/verifier.rs

tendermint/src/lite/types.rs

tendermint/src/lite/verifier.rs

- remove obsolete comments and - introduce const for test files

…elem instead of cloning the whole iter

ebuchman

Awesome work thanks @liamsi and @yihuang !

liamsi mentioned this pull request Dec 10, 2019

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

Closed

liamsi requested review from ebuchman, tarcieri, ancazamfir and brapse December 10, 2019 23:10

ebuchman mentioned this pull request Dec 11, 2019

Release/v0.11.0 #92

Merged

liamsi commented Dec 12, 2019

View reviewed changes

liamsi and others added 19 commits December 12, 2019 02:25

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

8604b57

Further reduce boilerplate code by leveraging generics

Create a trait for with a blanket impl instead of new type

4070a59

Thanks for the suggestion @tarcieri :-)

impl lite::Commit for commit::SignedHeader

e128044

impl lite::ValidatorSetLookup

189f582

Add a few methods to tests and deduplicate code for fn hash of

f6c8775

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

3ea3dc4

Represent optional hash and block id as Option

0946487

and correctly compute header hash when block id is empty. Represent empty Hash with Option

add mock testing for lite client

d3b632b

parse empty commit

b3a790f

utilities

b410066

Add TrustLevel as a trait; default behaviour is as before:

6999382

accept iff +1/3 of the voting power signed

minor clarifications: updated comments and clarified test-name

013e94b

remove unused unit return type

55d91ff

add /validators to rpc client

b9c419a

remove associated type (Vote) from SignedHeader

bd08d62

Fix a few minor issues from rebasing

b6543c0

make method for retrieving validators in rpc client async too

904f9aa

and some minor changes due to rebasing on master

liamsi force-pushed the fix_lite_impl_merkle_merged branch from 42130c6 to 904f9aa Compare December 12, 2019 01:31

rename byteslices -> fields_bytes

035df2b

ancazamfir reviewed Dec 12, 2019

View reviewed changes

tendermint/src/lite/verifier.rs Outdated Show resolved Hide resolved

tendermint/src/lite/verifier.rs Outdated Show resolved Hide resolved

liamsi commented Dec 12, 2019

View reviewed changes

Remove into_vec methods from Commit & ValidatorSet trait:

e6c3b15

- they are not used in the abstract verifier logic; hence they do not need to be part of the trait anymore

liamsi commented Dec 12, 2019

View reviewed changes

liamsi requested a review from ancazamfir December 12, 2019 23:18

Add in a correctly formed (but invalid) hash instead of `[]byte("wron…

f3bdc3d

…g PartsHeader hash!!")` -> still failing

liamsi commented Dec 13, 2019

View reviewed changes

liamsi requested a review from milosevic December 13, 2019 07:29

liamsi added 2 commits December 13, 2019 09:00

use associated types for SignedHeader instead of generics

eeb5479

improve the readability of the abstract traits / verification logic

Remove dependency on tendermint's time implementation (use SystemTime…

a21a170

… instead) and add method `CheckSupport` (see spec)

ancazamfir reviewed Dec 13, 2019

View reviewed changes

ebuchman previously approved these changes Dec 16, 2019

View reviewed changes

liamsi added 4 commits December 16, 2019 10:29

Minor improvements on light client test:

2f90679

- remove obsolete comments and - introduce const for test files

From review: fix some comments (typo & stale comment)

a855701

Remove lite::Vote altogether

d907da6

Optimize validator lookup: find() then clone() only clones the found …

71d2808

…elem instead of cloning the whole iter

liamsi dismissed ebuchman’s stale review via 71d2808 December 16, 2019 10:21

moved SignedHeader struct from rpc::endpoint to block::signed_header

5891bae

liamsi mentioned this pull request Dec 16, 2019

Light client impl followup #96

Closed

17 tasks

liamsi requested a review from ebuchman December 16, 2019 15:26

ebuchman approved these changes Dec 16, 2019

View reviewed changes

ebuchman merged commit 0a621f6 into master Dec 16, 2019

ebuchman deleted the fix_lite_impl_merkle_merged branch December 16, 2019 17:55

tarcieri mentioned this pull request Dec 17, 2019

Handle empty block id #66

Merged

This was referenced Dec 28, 2019

Light Client Rust related follow up #110

Closed

Light Client Spec related follow up #111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

liamsi commented Dec 10, 2019 •

edited

Loading

This comment was marked as resolved.

liamsi commented Dec 12, 2019 •

edited

Loading

liamsi Dec 12, 2019

liamsi Dec 12, 2019 •

edited

Loading

ancazamfir Dec 13, 2019

liamsi Dec 12, 2019

liamsi commented Dec 12, 2019

liamsi Dec 13, 2019 •

edited

Loading

liamsi Dec 13, 2019

ebuchman Dec 15, 2019

ancazamfir left a comment

ancazamfir Dec 13, 2019

ebuchman left a comment

ebuchman Dec 15, 2019

ebuchman left a comment

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

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

Conversation

liamsi commented Dec 10, 2019 • edited Loading

This comment was marked as resolved.

liamsi commented Dec 12, 2019 • edited Loading

liamsi Dec 12, 2019

Choose a reason for hiding this comment

liamsi Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

ancazamfir Dec 13, 2019

Choose a reason for hiding this comment

liamsi Dec 12, 2019

Choose a reason for hiding this comment

liamsi commented Dec 12, 2019

liamsi Dec 13, 2019 • edited Loading

Choose a reason for hiding this comment

liamsi Dec 13, 2019

Choose a reason for hiding this comment

ebuchman Dec 15, 2019

Choose a reason for hiding this comment

ancazamfir left a comment

Choose a reason for hiding this comment

ancazamfir Dec 13, 2019

Choose a reason for hiding this comment

ebuchman left a comment

Choose a reason for hiding this comment

ebuchman Dec 15, 2019

Choose a reason for hiding this comment

ebuchman left a comment

Choose a reason for hiding this comment

liamsi commented Dec 10, 2019 •

edited

Loading

liamsi commented Dec 12, 2019 •

edited

Loading

liamsi Dec 12, 2019 •

edited

Loading

liamsi Dec 13, 2019 •

edited

Loading