Light client impl followup

[liamsi]

Tracking TODOs for the Light client impl

Feedback / Review of #84

• Move additional checks to validate_vals_and_commit and make it function of Commit(?): Light client: further trait impls, improvements, and tests #84 (comment) - done in Bucky/some light follow up #109
• redundant check for sigs: Light client: further trait impls, improvements, and tests #84 (comment), Update specification of light client algorithm to align with the code tendermint/spec#61 (comment)
• Light client: further trait impls, improvements, and tests #84 (comment) (and general organizational issues: e.g. crate vs module etc)
  • remove dependencies on tendermint types (account::Id, Hash)
• add example to doc: Light client: further trait impls, improvements, and tests #84 (comment)
• From<SystemTimeEror> for lite::Error and Light client: further trait impls, improvements, and tests #84 (comment). Done in Bucky/some light follow up #109
• Light client: further trait impls, improvements, and tests #84 (comment) (return OK?) - Done in Bucky/some light follow up #109
• remove Validator trait ? Light client: further trait impls, improvements, and tests #84 (comment) - done in remove Validator trait; make ValidatorSet associated to Commit #105
• how to handle if/when votes don't contain validator id? probably worth discussing in separate issue: Light client: further trait impls, improvements, and tests #84 (comment) - done, added a comment in Bucky/some light follow up #109
• next steps: Light client: further trait impls, improvements, and tests #84 (comment)
• rename types.rs to interface.rs - Light client: bisection, store, requester #100 (comment)
• consider Client type and have can_trust be a method on the Client. Also put trust_threshold and trusting_period in the trusted state? Light client: bisection, store, requester #100 (comment)
• enforce off-by-one issues in the type system if possible, eg. TrustedState::new taking header h-1 and validators for h. Light client: bisection, store, requester #100 (comment). Also verify taking vals for the right height Light client: bisection, store, requester #100.
• Error::RequestFailed should wrap underlying requester errors. Light client: bisection, store, requester #100 (comment)
• Improve naming of variables like h1_xxx and h2. This should also be done simultaneously with the spec. eg. Light client: bisection, store, requester #100 (comment) and Light client: bisection, store, requester #100 (comment) and https://github.com/interchainio/tendermint-rs/pull/100/files#r360479080
• how to get latest height from a node? use height=0 ? Light client: bisection, store, requester #100 (comment). Could get rid of get_smaller_or_equal (Light client: bisection, store, requester #100). Need to do with spec.
• improve logic of check_support to better clarify the 3 conditions: ok (update state), real error (faulty node or system), not-enough-voting-power (not real error, just need to bisect)
• make trust_level satisfy Copy or Clone and don't pass by reference Light client: bisection, store, requester #100

[liamsi]

Something else I've noticed, and we can optimize for (but later): Currently all operations on the impls of traits traits that do not take an additional argument can be "cached" in a field instead of recomputed on each call.
In particular Header::hash: https://github.com/interchainio/tendermint-rs/blob/24955d18a99f3f3a846f519b770de5887159a0fa/tendermint/src/block/header.rs#L98
and ValidatorSet hash: https://github.com/interchainio/tendermint-rs/blob/24955d18a99f3f3a846f519b770de5887159a0fa/tendermint/src/validator.rs#L33
At least currently the API does only allow to create these structs without modifying them. These hashes could be computed once instead of on every call of hash(). Same for total_power.

[ebuchman]

With respect to removing the Validator trait. I think this is something we should try to do, since these methods aren't actually used within the light client itself, they only need to be exposed so that an implementation of Commit.voting_power_in, which takes a ValidatorSet, has access to these validator internals. Perhaps we can avoid this if the ValidatorSet is somehow an associated type on Commit so it can more readily access the internals without going through a trait.

This will also make it much easier to write tests for the core light client logic without having to mock out individual validators and the verify_signature method

[ebuchman]

Ok, some of these are addressed in #100 and #109, as noted in the edited opening post. The rest are split up into issues #110, #111, and #112 for things related to Rust, the spec, and the next steps, respectively :). Closing this for all those.