Light client: bisection, store, requester #100

liamsi · 2019-12-19T21:46:37Z

Needs tests with concrete impls of store and requester. (e.g. simple in memory store in form of a map and a mocked rpc client).

ref: #96

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGES.md

- implement `can_trust` according to the latest spec CanTrustBisection (wip) - add a `Requester` and a `Store` trait - sue refs in parameters (as they are used in several places and we don't want to Copy)

tendermint/src/lite/verifier.rs

tendermint/src/lite/types.rs

- reduce params to clippy threshold (7); still a lot ... - rename vars for better readability

- rename expired to is_within_trust_period to be closer to the spec - use TrusteState trait in check_support params - use TrusteState in lite tests

tendermint/src/lite/verifier.rs

xla

Had a first pass with a bunch of style and doc improvements. We should probably rename types.rs given that it only holds traits.

@liamsi You mentioned that it could use more tests. Could you enumerate what cases/flows you'd like to cover?

tendermint/src/lite/types.rs

tendermint/src/lite/verifier.rs

xla · 2019-12-20T11:59:22Z

tendermint/src/lite/verifier.rs

+    )?;
+    store.add(&pivot_header)?;
+    let pivot_trusted = TS::new(pivot_header, pivot_next_vals);
+    can_trust(


As we see calls to the same function with slightly adjusted parameters this function body could benefit from inline comments expanding of what the order of events is. This will lessen the cognitive burden on the reader.

Added some inline comments. Is it more understandable now?

tendermint/src/lite/verifier.rs

liamsi · 2019-12-20T12:09:52Z

We should probably rename types.rs given that it only holds traits.

I like interfaces.rs. I would like to re-structure this in a separate PR if possible.

You mentioned that it could use more tests. Could you enumerate what cases/flows you'd like to cover?

Unit tests for verify_header and can_trust. We can probably re-use the JSON files in tests/lite to mock a simple requestor (and a store). Hope to write down my thoughts in more detail later.

ebuchman

Great start, thanks Ismail, but it seems there are a few bugs, some of which are reflected in the spec, and we need to be clearer about validator sets.

I'll leave some more comments on the spec.

tendermint/src/lite/types.rs

tendermint/src/lite/verifier.rs

* MockHeader and test_is_within_trust_period * remove Validator trait; make ValidatorSet associated to Commit (#105) * remove Validator trait; make ValidatorSet associated to Commit * remove stale comment

* Review comments: - update some comments / documentation - verify vals (not next vals) - store header and vals in trusted state * One offs & renaming related to trusted state

tendermint/src/lite/verifier.rs

- an_trust -> can_trust_bisection - remove a redundant check for trust period - remove redundant adding state to the store (added TODO)

liamsi · 2019-12-28T19:08:55Z

tendermint/src/lite/verifier.rs

+    // and stores the now trusted header again.
+    // Figure out if we want to store it here or in can_trust_bisection!
+    // My understanding is: with the current impl, we either bubbled up an
+    // error, or, successfully added sh2 (and its nex vals) to the trusted store here.


See: https://github.com/tendermint/spec/pull/61/files#r361808209
and https://github.com/tendermint/spec/pull/61/files#r361808447

* use ? instead of return Err(err) * remove unused errors * move a validation and add TODOs about them * check_support returns early for sequential case * drop let _ = * add comment to voting_power_in about signers

ebuchman

All the review comments have been addressed or tracked in issues #110, #111, #112, #113. Let's merge this and keep working on things in new PRs.

Comments addressed or tracked for follow up

Add bisection to light client module:

b1a12a1

- implement `can_trust` according to the latest spec CanTrustBisection (wip) - add a `Requester` and a `Store` trait - sue refs in parameters (as they are used in several places and we don't want to Copy)

liamsi commented Dec 19, 2019

View reviewed changes

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

liamsi commented Dec 19, 2019

View reviewed changes

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

liamsi added 2 commits December 20, 2019 01:41

Group params h1 & ha_next_vals into a type TrustedState

cb01db9

- reduce params to clippy threshold (7); still a lot ... - rename vars for better readability

Add VerifyHeader logic from spec:

27231a0

- rename expired to is_within_trust_period to be closer to the spec - use TrusteState trait in check_support params - use TrusteState in lite tests

liamsi requested a review from ebuchman December 20, 2019 10:34

liamsi marked this pull request as ready for review December 20, 2019 10:34

liamsi requested review from xla, ancazamfir and milosevic December 20, 2019 10:36

liamsi commented Dec 20, 2019

View reviewed changes

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

liamsi requested a review from brapse December 20, 2019 10:41

xla previously requested changes Dec 20, 2019

View reviewed changes

liamsi added 4 commits December 20, 2019 13:32

Review: doc improvements and minor improvements on naming

eb85243

Review: doc improvements

8703e3d

More doc improvements

ddd7731

Minor doc improvement

b4d9f53

ebuchman reviewed Dec 20, 2019

View reviewed changes

liamsi commented Dec 24, 2019

View reviewed changes

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

liamsi mentioned this pull request Dec 28, 2019

Review comments: #107

Merged

5 tasks

ebuchman and others added 3 commits December 28, 2019 15:12

MockHeader and test_is_within_trust_period (#104)

8c82f2d

* MockHeader and test_is_within_trust_period * remove Validator trait; make ValidatorSet associated to Commit (#105) * remove Validator trait; make ValidatorSet associated to Commit * remove stale comment

Fix clippy errors (#108)

20124e2

Review comments: (#107)

8a95d9f

* Review comments: - update some comments / documentation - verify vals (not next vals) - store header and vals in trusted state * One offs & renaming related to trusted state

liamsi commented Dec 28, 2019

View reviewed changes

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

Rename & remove redundant store of trusted state: c

b25b7e5

- an_trust -> can_trust_bisection - remove a redundant check for trust period - remove redundant adding state to the store (added TODO)

liamsi commented Dec 28, 2019

View reviewed changes

liamsi mentioned this pull request Dec 28, 2019

Light client impl followup #96

Closed

17 tasks

This was referenced Dec 28, 2019

Light Client Rust related follow up #110

Closed

Light Client Spec related follow up #111

Closed

Bucky/some light follow up (#109)

93b0fad

* use ? instead of return Err(err) * remove unused errors * move a validation and add TODOs about them * check_support returns early for sequential case * drop let _ = * add comment to voting_power_in about signers

liamsi requested a review from xla December 29, 2019 18:17

ebuchman approved these changes Dec 29, 2019

View reviewed changes

ebuchman merged commit 1de3770 into master Dec 29, 2019

ebuchman deleted the ismail/light_store_requests_bisect branch December 29, 2019 18:45

This was referenced Feb 2, 2020

Light client crate structure #136

Closed

More data in light client errors #139

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Light client: bisection, store, requester #100

Light client: bisection, store, requester #100

liamsi commented Dec 19, 2019 •

edited

Loading

xla left a comment

xla Dec 20, 2019

liamsi Dec 20, 2019

liamsi commented Dec 20, 2019 •

edited

Loading

ebuchman left a comment

liamsi Dec 28, 2019

ebuchman left a comment

Light client: bisection, store, requester #100

Light client: bisection, store, requester #100

Conversation

liamsi commented Dec 19, 2019 • edited Loading

xla left a comment

Choose a reason for hiding this comment

xla Dec 20, 2019

Choose a reason for hiding this comment

liamsi Dec 20, 2019

Choose a reason for hiding this comment

liamsi commented Dec 20, 2019 • edited Loading

ebuchman left a comment

Choose a reason for hiding this comment

liamsi Dec 28, 2019

Choose a reason for hiding this comment

ebuchman left a comment

Choose a reason for hiding this comment

liamsi commented Dec 19, 2019 •

edited

Loading

liamsi commented Dec 20, 2019 •

edited

Loading