Fix Light Client validator set hash calculation

[thanethomson]

Closes #831

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

[thanethomson]

It's hard to know how to test this without doing (1) full integration testing or (2) mock testing with data captured from real nodes' RPC responses.

Both would be useful, but both would still require a significant amount of work. Depending on how urgent this solution is, we could possibly do these in follow-up PRs? (1) would be particularly useful as a job that regularly runs in CI (e.g. on a daily basis).

[codecov-io]

Codecov Report

Merging #834 (d9a03b7) into master (1cca3ac) will decrease coverage by 0.2%.
The diff coverage is 0.0%.

@@           Coverage Diff            @@
##           master    #834     +/-   ##
========================================
- Coverage    29.1%   28.8%   -0.3%     
========================================
  Files         194     196      +2     
  Lines       10766   10865     +99     
  Branches     4443    4474     +31     
========================================
- Hits         3139    3137      -2     
- Misses       4653    4752     +99     
- Partials     2974    2976      +2     

Impacted Files	Coverage Δ
light-client/src/components/io.rs	0.0% <0.0%> (ø)
proto/src/serializers/optional_from_str.rs	0.0% <0.0%> (ø)
rpc/src/client.rs	0.9% <0.0%> (-0.4%)	⬇️
rpc/src/client/bin/main.rs	0.3% <0.0%> (-0.1%)	⬇️
rpc/src/endpoint/consensus_state.rs	21.8% <ø> (ø)
rpc/src/endpoint/validators.rs	0.0% <0.0%> (ø)
rpc/src/error.rs	31.4% <ø> (ø)
rpc/src/lib.rs	100.0% <ø> (ø)
rpc/src/paging.rs	0.0% <0.0%> (ø)
proto/src/serializers/timestamp.rs	32.5% <0.0%> (-5.0%)	⬇️
... and 12 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 1cca3ac...d9a03b7. Read the comment docs.

[romac]

Looks great! Left some minor suggestions and a couple questions.

[romac]

Can you explain why we need/what does this attribute brings?

[thanethomson]

I'm in two minds about this right now.

On the one hand, I like the fact that it reduces the number of ways in which to construct a struct (this reduces the number of ways in which we can break downstream projects' code). If we restrict construction to the constructor, it also then, for certain structs, allows for parameter validation and additional guarantees that the resulting struct is most likely "correct".

On the other hand, we have some pretty massive structs where this doesn't work so well, where they have 9+ public fields. So the constructor-based approach becomes a little unwieldy. Usually I'd then use some builder pattern to construct such structs, but this will result in a significant amount more code (where I'm trying to avoid bloat, since if there's less code it's quicker to change).

I suppose, erring on the side of rapid development and less code (which is probably a good goal for the short- to medium-term), it's probably a better idea to go with the approach of direct struct construction?

[romac]

a)

On the one hand, I like the fact that it reduces the number of ways in which to construct a struct (this reduces the number of ways in which we can break downstream projects' code). If we restrict construction to the constructor, it also then, for certain structs, allows for parameter validation and additional guarantees that the resulting struct is most likely "correct".

Agreed, I think this is the right approach for structs that are exposed publicly and which we expect our users to instantiate themselves manually. But, even if possible, it's not really the case here as we expect users to use the higher-level methods rather than use perform and construct the requests directly.

b)

On the other hand, we have some pretty massive structs where this doesn't work so well, where they have 9+ public fields. So the constructor-based approach becomes a little unwieldy. Usually I'd then use some builder pattern to construct such structs, but this will result in a significant amount more code (where I'm trying to avoid bloat, since if there's less code it's quicker to change).

If we ever want to use the builder pattern everywhere, this may help: https://lib.rs/crates/derive_builder :)

c)

I suppose, erring on the side of rapid development and less code (which is probably a good goal for the short- to medium-term), it's probably a better idea to go with the approach of direct struct construction?

Seems fine to me for now, given the reasoning I point a) above.

[thanethomson]

derive_builder looks amazing. We should definitely investigate using that approach for all the internal Tendermint domain types!