Light Client Rust related follow up

[ebuchman]

Replacement for elements of #96 more specific to Rust implementation style and architecture.

We should update the ADR with the relevant changes from here as well.

Types:

• Make validate_vals_and_commit a method on Commit (Light client: further trait impls, improvements, and tests #84 (comment))
• Use validate_next_vals !
• Introduce a Client type to hold Store and Requester (?) (Light client: bisection, store, requester #100 (comment))
• Put trust_threshold and trusting_period in the Store (?) (Light client: bisection, store, requester #100 (comment))
• Enforce off-by-one issues in the type system, eg.:
  • TrustedState::new taking header h-1 and validators for h (Light client: bisection, store, requester #100 (comment))
  • verify taking vals for the right height (Light client: bisection, store, requester #100)
• make trust_level satisfy Copy or Clone and don't pass by reference (Light client: bisection, store, requester #100)
• memoize results of methods that don't take args, eg. .hash() (Light client impl followup  #96 (comment))
• use an enum for height to get the latest state instead of special-casing 0 (rpc: map height=0 to no parameters in the http call #118)
• introduce a simple event type and let bisection function (or a Client structure it's a method on) take a closure that will fire events. this can be used for logging so we don't need a print statement or logger inside the functions

Module Structure:

• Remove dependency on tendermint types, eg. Hash
• Make it a crate ?
• Rename types.rs to interface.rs (Light client: bisection, store, requester #100 (comment))
• Add example to doc
• Simplify the public API and make it harder/impossible to use incorrectly
• Unexpose verify_single once json tests are updated (525c89f#diff-f0c5bdd887eaf26a3fd9ec5bdc93e2d9R106 and 9fbb749#diff-afc724a3f81219fadc771326ccca3a84R8)

Errors:

• Error::RequestFailed should wrap underlying requester errors (Light client: bisection, store, requester #100 (comment))
• Deduplicate the Error::InvalidSignature with ErrorKind::SignatureInvalid
• Move to an error.rs file
• voting_power_in can't detect signatures from validators not in the current validator set (can't detect signatures from validators not in the current validator set)
• need two distinct errors for InsufficientVotingPower - one when it's insufficient to skip (ie. so do bisection) and one when it's insufficient to verify (ie. return error) Differentiate VotingPower errors #119

[ebuchman]

With respect to module structure, I would say there are a few goals:

• isolate concerns
• minimize dependencies
• keep trait implementations in a single place

The last point is to make it clear where to look for say all the implementations of the lite traits. Right now they're spread out across the tendermint crate.

So I think this suggests we try to eliminate the dependency of lite on tendermint data types (currently only Height and Hash), and to eliminate the dependency of tendermint on lite, and then move the implementations to their own small crate. Ie. we might have the following crates:

• tendermint
  • the current tendermint crate, but without the lite module or any of the implementations of traits in the lite module
• lite
  • the current lite module - just traits and functionality. uses no tendermint types
• tendermint-lite
  • implements the lite traits using tendermint types. To be used by the light node and by IBC
• tendermint-lite-node
  • abscissa application using the tendermint-lite implementation and the verify functions from lite to actually implement a node that performs syncing

The independence of tendermint-lite may seem excessive, but this is basically the set of common features (both tendermint and lite client logic) that both the light node and IBC will utilize.

Thoughts?

This also relates to #7 (comment) which proposes high level crate structure for this repo.

[ebuchman]

Re @xla review on #114 (#114 (review)) - consider not passing the store into the light client verify functions. See discussion in #114 (comment)

Also consider renamings:

• verify_and_update_bisection -> update_bisection
• _verify_and_update_bisection -> verify_and_update_bisection

[ebuchman]

Few more points from discussion after latest review #114 (review):

• ensure that time in headers is monotonic, ie. untrusted_header.bft_time() > trusted_header.bft_time() in verify_single
• check that the blockchain is not ahead of our local clock, ie. untrusted_header.bft_time() < now + X for every header we fetch

In theory, if we check that time is monotonic, we only need to check that the highest height we're trying to sync to is not ahead of our local clock (ie. we don't need to check this for every header, since all the other headers are less than the highest one). However, if we only check for monotonic time after we've trusted a pivot header, its possible we would trust a pivot header who's time is too far ahead of our local clock, and hence might be outside the trusting period.

[liamsi]

Also consider renamings:
verify_and_update_bisection -> update_bisection

This is only cool, if we also rename verify_and_update_single to update_single. I've renamed:
verify_and_update_bisection -> verify_and_update_bisection_inner in #132 / 31550c3 for now.

[liamsi]

Re @xla review on #114 (#114 (review)) - consider not passing the store into the light client verify functions. See discussion in #114 (comment)

If we don't pass in the store but a trusted_state instead, we won't store all the intermediate headers that we requested in between. The only things we can store is the final new trusted state. I think logic-wise this shouldn't be a problem but we need test and verify this too. The go-client stores all the headers it trusts in between (the pivot-headers and -vals). Again, I assume, we need to update the spec accordingly.

Removed the store from the bisection logic in 31550c3 and 8b2b964 of #132 (caller is now responsible to at least store the final resulting state, where prev. they would just pass in a store).

For consistency, we should do the same with verify_and_update_single, too...

[ebuchman]

Looks like that concern is resolved in #132 now by returning the list of all bisected headers. That PR addressed a bunch of these issues. I think it's time to close this issue and open a few new ones to capture the remaining points.

[ebuchman]

Some points turn out not to be relevant or necessary. Everything else is addressed or followed up in #136, #137, #138, #139, #140