refactor: Consensus state key

catShaark commented 2 years ago

Summary

We have problem where a ConsensusStates gRPC query return in lexicographic ordering of Height, not numerical ordering. For example:

  Here is an example (trimmed from an actual query). If the chain has states for heights:
  830, 821, 766, 222, 104, 76, 30, 25, 20, 10
  The gRPC query (`QueryConsensusStatesRequest`) returns:
  10, 104, 20, 222, 25, 30, 76, 766, 8, 821, 830

_This problem is originally posted in this issue

Proposal

We index ConsensusState by converting Height.RevisionNumber along with Height.RevisionHeight directly to 16 bytes ( 2 uint64 -> 16 bytes, big-endian ) in instead of turning them into a string ( uint64 -> string + "-" + uint64 -> string ) and then to bytes.

Specifically :

Add Byte() method to exported.Height interface
Implement Byte() for 02-client.types.Height, we turn RevisionNumber and RevisionHeight to 16 bytes (like described above) and return that 16 bytes
Change ConsensusStateKey() so that they use Height.Byte() to create key for ConsensusState instead of using Height.String()

For Admin Use

[ ] Not duplicate issue
[ ] Appropriate labels applied
[ ] Appropriate contributors tagged/assigned

catShaark commented 2 years ago

This change is state breaking and require store migration, I think. Idk if that is okay or not ? If approved, I'll make a PR for this and close #1399 as well

crodriguezvega commented 2 years ago

Thanks for thinking about this problem, @catShaark. I think the solution that you propose is very clever!

Yes, so indeed we would need to run a migration to update the keys in the store... I think it's not a huge problem; we could release this as part of v4.0.0. We should make sure that we document the migration properly in the migration docs and that we thoroughly test it (both in integration and QA tests) to make sure it works well.

I guess the alternative would be otherwise to keep the store key as is but then try to do the order in code after retrieving the consensus states from the store...

crodriguezvega commented 2 years ago

On a second thought we might have a problem with this approach, since relayers will probably need to update their code to retrieve consensus state proofs from the updated path. So, for example in Hermes, they would need to update this path. The problem comes here that relayers would need to support both paths (since some chains would be running the old path and some chains the new one, until all of them upgrade to the new path) but there would be no way (unless we implement something) for relayers to know which path to use.

So even though I think the idea is good, the practical issues of deploying it might be too difficult to overcome...

catShaark commented 2 years ago

On a second thought we might have a problem with this approach, since relayers will probably need to update their code to retrieve consensus state proofs from the updated path. So, for example in Hermes, they would need to update this path. The problem comes here that relayers would need to support both paths (since some chains would be running the old path and some chains the new one, until all of them upgrade to the new path) but there would be no way (unless we implement something) for relayers to know which path to use.

Ohhh I see, that's one annoying drawback that we have. Is this something that we should encounter every times we have a store migration in core ? If so, ibc-go should not accept store migrations in core or do we have "special plans" for them ?

AdityaSripal commented 2 years ago

Hi @catShaark , unfortunately this is something that we realized after shipping stargate but is a technical debt that is very hard to fix. The main reason is that it requires a network wide upgrade since the consensus state path is part of ICS-24 and thus chains will expect their counterparties to store the consensus state in the ICS-24 path which stores heights in string representation.

Is this something that we should encounter every times we have a store migration in core

It is only an issue if it is a store path that is part of ICS24 or expected to be stored in the public store as defined in the specs. Private store information that isn't getting proved on counterparties can be migrated without an issue.

https://github.com/cosmos/ibc-go/issues/64

As @colin-axner mentions in this issue, we have metadata stored in tendermint client that does store the consensus state in the efficient way.

https://github.com/cosmos/ibc-go/blob/main/modules/light-clients/07-tendermint/types/store.go#L227

If you can use the tendermint client to return the heights then this can be done numerically. From 02-client it is not so easy unless we replicate the consensus states under the efficient keys in 02-client as well.

catShaark commented 2 years ago

thanks for the clarification, @AdityaSripal

cosmos / ibc-go