ibc-rs icon indicating copy to clipboard operation
ibc-rs copied to clipboard

`ConsensusState` trait should not have a `root()` method, and `ClientState.verify_membership()` should not take a root

Open plafer opened this issue 1 year ago • 3 comments

Not all client's ConsensusState have the concept of a root (e.g. solomachine). Instead of passing the root to verify_membership in the core handlers, we should instead fetch the root in the respective clients when needed (e.g. here in ibc-go's tendermint client).

plafer avatar Jun 22 '23 13:06 plafer

The question this brings... was wondering what the purpose of the ConsensusState would be if it could live and be defined without the root. Then, logically, why not include the timestamp() in the ClientState (Particularly given that it contains the latest height as well) and move away completely from the ConsensusState? Isn't the Solomachine a special case? Wouldn't this generalize the concept of ConsensusState too much?

Farhad-Shabani avatar Jun 27 '23 15:06 Farhad-Shabani

The big difference between ClientState and ConsensusState is how they're stored.

  • ClientState: clients/{identifier}/clientState
  • ConsensusState: clients/{identifier}/consensusStates/{height}

Specifically, we remember the history of ConsensusStates (indexed by height), whereas we only remember the latest ClientState. So ClientState.timestamp() would give us the timestamp of the latest ClientState, whereas ConsensusState.timestamp() is the timestamp associated with the ConsensusState at a given height.

The root() is not fundamental to IBC, whereas timestamp() is: it is needed to verify packet timeouts when timing out a packet. So we know for sure that every ConsensusState needs to have a timestamp.

plafer avatar Jun 28 '23 19:06 plafer

https://github.com/cosmos/ibc-rs/issues/784#issuecomment-1650572151 related to this issue as well. Worth considering :)

Farhad-Shabani avatar Jul 25 '23 21:07 Farhad-Shabani