Refactor Light Client verification predicates interface for use from IBC

[thanethomson]

Closes #956

The main aim of this PR is to remove references to the LightBlock type from the verification predicates, as the LightBlock type isn't used in informalsystems/hermes#1252

Tomorrow I'll try working on refactoring that PR to make use of this code to see if it will work.

• Referenced an issue explaining the need for the change
• Updated all relevant documentation in docs
• Updated all code comments where relevant
• Wrote tests
• Added entry in .changelog/

[codecov-commenter]

Codecov Report

Merging #960 (d70a2c8) into master (bbad6fa) will increase coverage by 0.0%.
The diff coverage is 79.2%.

@@           Coverage Diff           @@
##           master    #960    +/-   ##
=======================================
  Coverage    72.0%   72.1%            
=======================================
  Files         203     203            
  Lines       16522   16632   +110     
=======================================
+ Hits        11912   12005    +93     
- Misses       4610    4627    +17     

Impacted Files	Coverage Δ
light-client-js/src/lib.rs	6.6% <ø> (ø)
light-client/src/builder/light_client.rs	0.0% <0.0%> (ø)
tendermint/src/block/signed_header.rs	91.6% <ø> (ø)
tendermint/src/error.rs	0.0% <0.0%> (ø)
light-client/src/types.rs	34.9% <42.8%> (+4.8%)	⬆️
light-client/src/operations/commit_validator.rs	98.0% <66.6%> (-2.0%)	⬇️
light-client/src/components/verifier.rs	87.9% <86.9%> (-12.1%)	⬇️
light-client/src/light_client.rs	83.6% <92.3%> (+0.3%)	⬆️
light-client/src/operations/hasher.rs	100.0% <100.0%> (ø)
light-client/src/operations/voting_power.rs	97.9% <100.0%> (-0.1%)	⬇️
... and 22 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 bbad6fa...d70a2c8. Read the comment docs.

[thanethomson]

The rustdoc error seems to only pop up with Rust nightly 1.57.0, whereas it compiles just fine with 1.56.0.

It's a problem in prost-derive's documentation:

error[E0034]: multiple applicable items in scope
Error:    --> /home/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/prost-derive-0.7.0/src/lib.rs:109:14
    |
109 |             .intersperse(quote!(|));
    |              ^^^^^^^^^^^ multiple `intersperse` found
    |
    = note: candidate #1 is defined in an impl of the trait `Iterator` for the type `Map<I, F>`
    = note: candidate #2 is defined in an impl of the trait `Itertools` for the type `T`
help: disambiguate the associated function for candidate #1
    |
105 ~         let tags = Iterator::intersperse(field
106 +             .tags()
107 +             .into_iter()
108 +             .map(|tag| quote!(#tag)), {
109 +         let mut _s = $crate::__private::TokenStream::new();
110 +         $crate::quote_each_token!(_s $($tt)*);
  ...
help: disambiguate the associated function for candidate #2
    |
105 ~         let tags = Itertools::intersperse(field
106 +             .tags()
107 +             .into_iter()
108 +             .map(|tag| quote!(#tag)), {
109 +         let mut _s = $crate::__private::TokenStream::new();
110 +         $crate::quote_each_token!(_s $($tt)*);
  ...

[adizere]

@josef-widder We may need your help here.

It seems the IBC handler (ICS 07) has no access to the set of next_validators for the untrusted header. For this reason, this field is an Option here. When the IBC handler calls into verify it does so with this field to None (cf. https://github.com/informalsystems/ibc-rs/pull/1321/files#diff-9a833365bf13b4ff1ce2cc757bf18ebabea582b2b97774040e0b7be03fcacbf4R93-R95). This check will effectively be disabled in the context of the IBC handler. This is probably a problem and we need to sort it out.

[adizere]

FYI this concern need not block the PR. (Josef will only return from holidays after Sept 27). Maybe @ancazamfir could help us sort out the problem.

[xla]

Structurally the refactoring makes sense to me, can't comment much on the internals of the verification.

[xla]

This implies that there is a common understanding what a "production" environment constitutes. More qualification would aid in contextualised this.

[thanethomson]

Yes this is something we've been talking/thinking about for a year now 😄 We're definitely open to suggestions on the naming front.

The original purpose, afaik, was to disambiguate mock versions from non-mock versions of the types, but I think @romac can elaborate on this better than I can?

[romac]

Yeah I am not super happy with the current terminology (that I came up with). I guess we need to differentiate between:
a) the trait (previously Predicates, now Verifier)
b) the "production" implementation (previously ProdPredicates, now ProdVerifier)
c) the potential mock implementations of the trait (no use for that yet)

I agree that the Prod prefix is subpar, maybe Default or Standard would be a better fit?

[xla]

LGTM

🛀 😷 🌽 💐

[romac]

Amazing job, @thanethomson! Very much an improvement over the previous API 💯

[romac]

Yeah I am not super happy with the current terminology (that I came up with). I guess we need to differentiate between:
a) the trait (previously Predicates, now Verifier)
b) the "production" implementation (previously ProdPredicates, now ProdVerifier)
c) the potential mock implementations of the trait (no use for that yet)

I agree that the Prod prefix is subpar, maybe Default or Standard would be a better fit?

[romac]

😍