Simplify some light client types and testing #132

liamsi · 2020-01-26T20:23:56Z

Main change: make TrustedState and SignedHeader structs (instead of traits with assoc. types):

as a result of parametrarizing TrustedState<C, H> and SignedHeader<C, H>
(instead of trait with assoc. types) a few things have to be (re)parametrarized and
others can be deleted as they are redundant and consumers can use the
param. structs instead of the trait impls (which duplicated code):
- parametrarize Store<C, H> too
- delete mocks: MockSignedHeader, MockState
- delete impl for block::signed_header::SignedHeader
- delete impl for lite::TrustedState in JSON tests
- remove State and state.rs module from implementation completely
- adapt params in verifier to match generic params of TrustedState & SignedHeader
More (minor) changes / improvements:
- remove dependency on block::Height by making lite::Height a u64 type alias
- differentiate error from verify_commit_(trusting vs full)
- increase test coverage (slightly only, compare codecovs total below with )
- simplify test further by removing votes_len from Commit
- use verify_update_single instead of verify_single in tests (unexport the latter) (f2fd6fe)
rename:
- verify_and_update_single -> verify_single
- verify_and_update_bisection -> verify_bisection
remove store from public facing functions

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

tendermint/Cargo.toml

tendermint-lite/src/lib.rs

liamsi · 2020-01-28T08:12:42Z

tendermint/src/lite/types.rs

+    }
+}
+
+// TODO: should this go in the central place all impls live instead? (currently lite_impl)


Do we want to keep types.rs purely abstract / generic only? If so this should go into a different file/module.

Not sure if it should go with the impls since it's a pretty generic thing, but maybe in its own module file in lite

tendermint/src/lite/types.rs

codecov-io · 2020-01-29T18:28:41Z

Codecov Report

Merging #132 into master will increase coverage by 3.93%.
The diff coverage is 67.67%.

@@            Coverage Diff            @@
##           master    #132      +/-   ##
=========================================
+ Coverage   32.96%   36.9%   +3.93%     
=========================================
  Files          92      90       -2     
  Lines        3009    3127     +118     
  Branches      430     463      +33     
=========================================
+ Hits          992    1154     +162     
+ Misses       1816    1718      -98     
- Partials      201     255      +54

Impacted Files	Coverage Δ
tendermint/src/lite_impl/validator_set.rs	`100% <ø> (+18.75%)`	⬆️
tendermint-lite/src/main.rs	`0% <0%> (ø)`	⬆️
tendermint-lite/src/requester.rs	`0% <0%> (ø)`	⬆️
tendermint/src/lite_impl/header.rs	`89.79% <100%> (+6.12%)`	⬆️
tendermint/src/lite/types.rs	`58.12% <57.96%> (-16.88%)`	⬇️
tendermint-lite/src/store.rs	`70% <75%> (+70%)`	⬆️
tendermint/src/lite_impl/signed_header.rs	`82.22% <80%> (-3.83%)`	⬇️
tendermint/src/lite/verifier.rs	`79.66% <85.1%> (+21.27%)`	⬆️
tendermint/src/serializers.rs	`0% <0%> (ø)`	⬆️
tendermint/src/vote.rs	`31.48% <0%> (ø)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a02ffa5...b7282b8. Read the comment docs.

- use same names as in current spec

…dHeader - as a result of parametrarizing `TrustedState<C, H>` and `SignedHeader<C, H>` (instead of trait with assoc. types) a few things have to be (re)parametrarized and other can be completely deleted as they are redundant and consumers can use the param structs instead of the trait impls: - parametrarize Store<C, H> too - delete mocks: MockSignedHeader, MockState - delete impl for block::signed_header::SignedHeader - delete impl for lite::TrustedState in JSON tests - remove `State` and state.rs module from implementation completely - adapt params in verifier to match generic params of TrustedState & SignedHeader - remove dep on block::Height by making lite::Height a u64 alias

- remove `DefaultTrustLevel` from tests - remove TrustThresholdOneThird and TrustThresholdTwoThirds from tendermint-lite (the latter was not used and the first duplicates the default impl) - use TrustThresholdFraction::default() instead

in constructor)

- rename `validate_untrusted` -> `validate` which calls the new method on Commit - simplify tests (`Option`s are not longer needed) - move length matches check to impl of `validate`

…instead - use MemStore of light crate in tests as a Mock

tendermint/src/lite/types.rs

…le in the super module) - mocks can be used in both types.rs and verifier.rs without decreasing codecov

- rename _verify_and_update_bisection -> verify_and_update_bisection_inner

…e spec - verify_and_update_single -> verify_single - verify_and_update_bisection -> verify_bisection - remove store from public facing functions

tendermint/src/lite/verifier.rs

…oization)

…xt_vals too

…rned states

@Shivani912

…t for @Shivani912 to take over :-)

ebuchman

Wonderful work! Left a bunch of questions and minor nits but they can be followed up on.

tendermint/src/lite/types.rs

ebuchman · 2020-02-02T01:36:41Z

tendermint/src/lite/types.rs

+    }
+}
+
+// TODO: should this go in the central place all impls live instead? (currently lite_impl)


Not sure if it should go with the impls since it's a pretty generic thing, but maybe in its own module file in lite

tendermint/src/lite/types.rs

tendermint/src/lite/verifier.rs

tendermint/Cargo.toml

tendermint/src/lite/types.rs

tendermint/src/lite_impl/signed_header.rs

tendermint/tests/lite.rs

liamsi force-pushed the ismail/light_cleanup branch from 31e0f00 to 046cf86 Compare January 27, 2020 19:21

liamsi commented Jan 28, 2020

View reviewed changes

tendermint/Cargo.toml Show resolved Hide resolved

liamsi force-pushed the ismail/light_cleanup branch from f2fd6fe to 19331b9 Compare January 28, 2020 08:04

liamsi commented Jan 28, 2020

View reviewed changes

tendermint-lite/src/lib.rs Show resolved Hide resolved

liamsi commented Jan 28, 2020

View reviewed changes

liamsi requested review from ebuchman and xla January 28, 2020 08:13

liamsi marked this pull request as ready for review January 28, 2020 08:13

liamsi commented Jan 28, 2020

View reviewed changes

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

liamsi force-pushed the ismail/light_cleanup branch from cf28563 to 585255c Compare January 28, 2020 08:55

liamsi requested review from ancazamfir and milosevic January 28, 2020 13:40

liamsi added 17 commits January 29, 2020 21:07

Differentiate error from verify_commit_(trusting vs full)

e05cc8f

- use same names as in current spec

Fix failing test :-)

5b2355f

Remove obsolete TODO: we don't take a ref anymore

7505e82

some checked arithmetic for height

368461f

type alias Mock types

07e289c

Add TrustThresholdFraction and a default for it

68e7bb1

- remove `DefaultTrustLevel` from tests - remove TrustThresholdOneThird and TrustThresholdTwoThirds from tendermint-lite (the latter was not used and the first duplicates the default impl) - use TrustThresholdFraction::default() instead

remove obsolete clone

a471444

remove unused lifetime (initially introduced to get rid of clones

eecef32

in constructor)

improve doc

9a18314

remove TestSuite struct from JSON tests

0da2f2c

Add tests for TrustThresholdFraction

45d6400

add test-case for is_within_trust_period to increase test-coverage

43af079

remove votes_len from commit; add a validate method to commit

215d702

- rename `validate_untrusted` -> `validate` which calls the new method on Commit - simplify tests (`Option`s are not longer needed) - move length matches check to impl of `validate`

unexport verify_single and use verify_and_update_single in tests …

847ee8f

…instead - use MemStore of light crate in tests as a Mock

type alias doesn't need to be pub here

2b8d5e9

update doc comment and revert to making State public in tendermint-lite

cf24fc0

liamsi force-pushed the ismail/light_cleanup branch from e85d832 to cf24fc0 Compare January 29, 2020 20:07

liamsi added 2 commits January 29, 2020 21:47

Remove len & is_empty() from ValidatSet trait

c00205d

add some very simple unit tests for basic types

418cd04

liamsi force-pushed the ismail/light_cleanup branch from db6c64a to 418cd04 Compare January 29, 2020 21:38

liamsi commented Jan 29, 2020

View reviewed changes

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

liamsi added 3 commits January 30, 2020 01:27

update comments and move mock types into a mock module (only accessib…

28ecf94

…le in the super module) - mocks can be used in both types.rs and verifier.rs without decreasing codecov

add missing #[cfg(test)] attribute (saves some compile time)

4831a41

remove store from inner recursion function

31550c3

- rename _verify_and_update_bisection -> verify_and_update_bisection_inner

liamsi force-pushed the ismail/light_cleanup branch from 91f1674 to 31550c3 Compare January 30, 2020 01:16

remove store from verify_and_update_bisection too

8b2b964

liamsi mentioned this pull request Jan 30, 2020

Light Client Rust related follow up #110

Closed

22 tasks

liamsi added 2 commits January 30, 2020 03:29

rename some public methods of the verifier to be more aligned with th…

d3d1d65

…e spec - verify_and_update_single -> verify_single - verify_and_update_bisection -> verify_bisection - remove store from public facing functions

make trust_level satisfy Copy + Clone and don't pass by reference

9f161ec

ancazamfir reviewed Jan 30, 2020

View reviewed changes

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

liamsi added 6 commits January 30, 2020 13:24

refactor bisection to return a list of intermediate states (using mem…

48a7a37

…oization)

use Vec instead of HashMap to memoize / return the interm. sates

ed8c06e

rename test helpers the assert_single_*

5f5b416

WIP: quick first test for bisection

712ead7

WIP: add debug output to mocks & modify init_trusted_state to take ne…

691acbc

…xt_vals too

WIP: minor refactoring: intro method to ensure uniqueness in the retu…

e1e9f3c

…rned states

liamsi changed the title ~~Simplify some light client related types~~ Simplify some light client types and testing Jan 31, 2020

liamsi added 2 commits January 31, 2020 22:20

Add some comments and clean-up the code to be slightly more consisten…

1879aba

…t for @Shivani912 to take over :-)

fix accidentally broken test

b7282b8

ebuchman approved these changes Feb 2, 2020

View reviewed changes

ebuchman merged commit d0e99f9 into master Feb 2, 2020

ebuchman deleted the ismail/light_cleanup branch February 2, 2020 03:32

This was referenced Feb 2, 2020

Differentiate VotingPower errors #119

Closed

Light Client minor improvements #138

Closed

This was referenced Feb 3, 2020

132 followup #141

Merged

Valid TrustThresholdFraction and further simplify tests code #142

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify some light client types and testing #132

Simplify some light client types and testing #132

liamsi commented Jan 26, 2020 •

edited

Loading

liamsi Jan 28, 2020

ebuchman Feb 2, 2020

codecov-io commented Jan 29, 2020 •

edited

Loading

ebuchman left a comment

ebuchman Feb 2, 2020

Simplify some light client types and testing #132

Simplify some light client types and testing #132

Conversation

liamsi commented Jan 26, 2020 • edited Loading

liamsi Jan 28, 2020

Choose a reason for hiding this comment

ebuchman Feb 2, 2020

Choose a reason for hiding this comment

codecov-io commented Jan 29, 2020 • edited Loading

Codecov Report

ebuchman left a comment

Choose a reason for hiding this comment

ebuchman Feb 2, 2020

Choose a reason for hiding this comment

liamsi commented Jan 26, 2020 •

edited

Loading

codecov-io commented Jan 29, 2020 •

edited

Loading