light-client: make generic over LightBlock type

[romac]

See #416 for background information.

This PR is a first rough draft meant to trigger discussion around the exact dependency graph and API we want to expose when we abstract over the concrete LightBlock type.

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

[brapse]

Definitely looks like a good first step. I think what we are looking for is to simplify the object graph to the point where it's easy to test and be general over changes to the underlying data structure. I think what might be good here is to look not just at the source of the dependency graph (TMLightBlock) but the uses. There are so many uses the LightBlock that are related to internal validation, for instance ensuring the validator set hashes actually match the validator set. In these cases it may be simpler have a separate type, ValidLightBlock which provides these guarantees at construction.

I raise this here as it isn't entirely clear to me how much of the underlying data structures we need to expose. Do we need the entire signed_header or just properties of it?

[romac]

I think what might be good here is to look not just at the source of the dependency graph (TMLightBlock) but the uses.

Yeah that's actually what I did, I only added methods to the LightBlock trait when they were required at use site. But that doesn't mean that this interface is as minimal as it could be, as the code at use site was written with the concrete TMLightBlock in mind.

There are so many uses the LightBlock that are related to internal validation, for instance ensuring the validator set hashes actually match the validator set. In these cases it may be simpler have a separate type, ValidLightBlock which provides these guarantees at construction.

Interesting idea, I definitely want to explore encoding more invariants in the types themselves.

I raise this here as it isn't entirely clear to me how much of the underlying data structures we need to expose. Do we need the entire signed_header or just properties of it?

Good question! For SignedHeader, I don't believe it's needed anymore, as it was only used to implement header hashing, voting power calculation and commit validation via the Commit trait, which has been split up in separate traits.

[romac]

Actually, SignedHeader is currently also used in the ConflictingHeadersEvidence struct, but could easily be removed by introducing a new private struct to the evidence module with the same definition.

[ebuchman]

what we are looking for is to simplify the object graph to the point where it's easy to test and be general over changes to the underlying data structure.

Right, this is how I've framed things as well, but if I understand correctly, here we have not actually simplified the object graph, and yet with the magic of associated types, managed to be quite general over changes to the underlying data structure?

It would be good to reason through what additional code would need to be written against this to support a change to the light block (eg. the changes coming in v0.34, which I think are minor, but still), because at first glance it seems like it would work, but maybe a bit more code than necessary.

Also note if we have SignedHeader (if we even do need it in here) we prob shouldn't need Commit (since the SignedHeader should contain it)?

ConflictingHeadersEvidence struct, but could easily be removed by introducing a new private struct to the evidence module with the same definition.

Yeh, we'll also need it eg. in the relayer to submit in a ClientUpdate tx. I guess we want a way to have all the light client logic and core interactions with the network and store to abstract over these types, but to still be able to pull out the concrete type from the store for use as eg. evidence or in ibc txs, or just as debug info over the rpc

[xla]

Pretty enticing in principle, but I think this could benefit from a PR to address the concerns @brapse raised.

[romac]

Yeah those concerns are valid, and exactly the type of discussion this PR is meant to raise. Once we settle on the right API, we should probably codify it in an ADR before altering the code.

[xla]

Should we drop this in favour of an ADR? It's been stale for a bit and we should exercise our discipline when it comes to design first. What you think @brapse @romac?

[ebuchman]

Cool to see how straight forward this is and what it can get us, even if it's not the ideal end state.

Do we want to leverage an intermediate state like this or wait to further simplify the object graph?

It might be valuable roughly as is in the current state, especially if we can leverage it for the upgrade to v0.34 tendermint (which does break the light block in a minor way, so we could test out trying to support both ...).

Ultimately we want to record the decisions in the ADR, perhaps with some comparison to the original design in ADR 03.

[ebuchman]

what we are looking for is to simplify the object graph to the point where it's easy to test and be general over changes to the underlying data structure.

Right, this is how I've framed things as well, but if I understand correctly, here we have not actually simplified the object graph, and yet with the magic of associated types, managed to be quite general over changes to the underlying data structure?

It would be good to reason through what additional code would need to be written against this to support a change to the light block (eg. the changes coming in v0.34, which I think are minor, but still), because at first glance it seems like it would work, but maybe a bit more code than necessary.

Also note if we have SignedHeader (if we even do need it in here) we prob shouldn't need Commit (since the SignedHeader should contain it)?

ConflictingHeadersEvidence struct, but could easily be removed by introducing a new private struct to the evidence module with the same definition.

Yeh, we'll also need it eg. in the relayer to submit in a ClientUpdate tx. I guess we want a way to have all the light client logic and core interactions with the network and store to abstract over these types, but to still be able to pull out the concrete type from the store for use as eg. evidence or in ibc txs, or just as debug info over the rpc

[xla]

Do we want to leverage an intermediate state like this or wait to further simplify the object graph?

The branch diverged quite a bit I wonder how much overhead it would be to get up to speed with mainline. Maybe a better strategy is to jump into the ADR, extract the code from the PR and use it as part of the proposed design. @romac If you pressed on time I can take this over, including speccing.

[romac]

@romac If you pressed on time I can take this over, including speccing.

Yes, I am going to be busy this week with the IBC handler ADR, so if you want to take a stab at it, that'd be great! :)

[brapse]

This is a great first step but will better serve as inspiration. Expect the next iteration as a new branch from @xla