MVP Generic Sequential & Skipping Lite Client Verifiers #31
Conversation
|
Notes about the bisecting version:
The latter points are good justification for keeping the address in the actual vote structure included in the commit, as per tendermint/tendermint#3596 (comment). Otherwise, lite clients would have to also download the current validator set to know if the votes are from any validators they already trust. It might also mean we need to use some different traits for the skipping verifier. |
ebuchman
changed the title
|
Very nice and clean. |
|
The two main differences between
Can we maybe offload (1) to the caller and make (2) a parameter, so that we can merge |
This reverts commit f521f04.
This is just the simplest way to move forward implementing the traits of the lite package. There are alternatives: We do not want a create a circular dependency between lite and tendermint (which does not even compile). To avoid this we could: 1) make lite a module of tendermint, or, 2) replicate a lot of the types of the tendermint crate in the lite package (e.g. Time, Ids etc), or 3) have a dependency from tendermint to the lite package only (but then the trait impls do need to live in the lite crate instead with the types in the tendermint crate directly). copied changes over from d3ce237
Use concrete basic types for time, hash, bytes, validator id
|
After starting to work on tests I found myself essentially implementing a lot of types and functionality that already exists in the tendermint-rs and in #36, so starting to think that means we will actually test this using the real types and it's not worth it to duplicate all that in mocks. Not sure really how to interpret this yet, whether it means the the abstractions here aren't as useful as hoped or if they're maybe not abstract enough. The mocks would be a bit lighter than the real types, but it's not clear the effort of implementing them is really worth it ... would love more opinion. So I would vote to merge this as is without tests (unless there are still unaddressed review comments but I think I got everything) and to work on the tests along with the trait implementations in #36. Certainly this lends credence to |
There was a problem hiding this comment.
The mocks would be a bit lighter than the real types, but it's not clear the effort of implementing them is really worth it ... would love more opinion.
I do not think we should write mock impls if they aren't useful and it means about the same effort as writing actual implementations.
So I would vote to merge this as is without tests (unless there are still unaddressed review comments but I think I got everything) and to work on the tests along with the trait implementations in #36.
I agree! Besides the impl lite::Header for Header this only contain traits. I think we can merge this as is.
There are follow-up PR with impls of this traits and a few tests (currently against this branch) and more tests to come. It would be cleaner to merge this and base the remaining light client PRs to master instead of this PR-branch here.
|
@tarcieri / @tony-iqlusion: Do you usually merge or squash PRs here? The commit history on master looks like a mix of both. |
|
@liamsi I personally prefer merge commits, because they give you a cryptographically authenticated history of how code landed on |
|
Thanks @tarcieri. That makes sense. The team seems in favour of squashing though (like in the golang tendermint repo). And I think there is also merit in having a cleaner history on master. The original commit history is still around in the Pull request. Well ... as long as github is around I guess :-P In any case I'm in favour of consistency. Will stick to squash merging then. |
With SHA1. And trusting github. The git model is so broken it hurts my brain: https://ebuchman.github.io/posts/2019-01-26-software-collab-part1/ . So I air on the side of clean commit history - it's especially nice to have every commit correspond to just one PR so you can see how everything happened, and otherwise restrict merging to master so anyone can see that each merge got the approvals if they want to. Would love to see more tooling developed around authenticating that merges happened properly and that squash merges correspond to the same patches as the set of commits, but alas, we're not there yet. |
Yup 😜 |
This is an MVP of generic lite client verifiers for the sequential and skipping (ie. bisection) cases. It defines traits for the key data types (
Header,Validators,Validator,Commit,Vote) and exposes the minimal amount of info to express the core validation.