ClientState in x/ibc/07 diverges from that in ICS 07

[ebuchman]

ClientState in x/ibc/07-tendermint looks like this:

// ClientState from Tendermint tracks the current validator set, latest height,
// and a possible frozen height.
type ClientState struct {
	// Client ID
	ID string `json:"id" yaml:"id"`

	TrustLevel tmmath.Fraction `json:"trust_level" yaml:"trust_level"`

	// Duration of the period since the LastestTimestamp during which the
	// submitted headers are valid for upgrade
	TrustingPeriod time.Duration `json:"trusting_period" yaml:"trusting_period"`

	// Duration of the staking unbonding period
	UnbondingPeriod time.Duration `json:"unbonding_period" yaml:"unbonding_period"`

	// MaxClockDrift defines how much new (untrusted) header's Time can drift into
	// the future.
	MaxClockDrift time.Duration

	// Block height when the client was frozen due to a misbehaviour
	FrozenHeight uint64 `json:"frozen_height" yaml:"frozen_height"`

	// Last Header that was stored by client
	LastHeader Header `json:"last_header" yaml:"last_header"`

	ProofSpecs []*ics23.ProofSpec `json:"proof_specs" yaml:"proof_specs"`
}

While in the ICS 07 it looks like:

interface ClientState {
  validatorSet: List<Pair<Address, uint64>>
  trustingPeriod: uint64
  unbondingPeriod: uint64
  latestHeight: uint64
  latestTimestamp: uint64
  frozenHeight: Maybe<uint64>
}

There also doesn't seem to be a protobuf file.

Should this divergence be reconciled? It seems things were added in the code that were necessary and that needs to be updated in the spec?

But also Header field in the code is a full on SignedHeader and ValidatorSet, so it includes the full Header, Commit, and ValidatorSet. Is all this really necessary? Having the Commit stored in here seems especially unecessary. From the spec, it looks like we just need a few fields from the header (height, timestamp) and the validator set.

[fedekunze]

@cwgoes

[cwgoes]

Thanks, we should fix this discrepancy.

But also Header field in the code is a full on SignedHeader and ValidatorSet, so it includes the full Header, Commit, and ValidatorSet. Is all this really necessary? Having the Commit stored in here seems especially unecessary. From the spec, it looks like we just need a few fields from the header (height, timestamp) and the validator set.

The client state should only need to store the latest validator set, I believe. cc @AdityaSripal is that wrong?

I've added TrustLevel, MaxClockDrift, and ProofSpecs to the spec in cosmos/ibc#447.

Do we have a canonical .proto file in the SDK yet? AFAIK this might still be blocked on Tendermint, but I'm not sure.

[colin-axner]

yea, there is no proto file because 02-client migration is blocked on Tendermint

[cwgoes]

Why does the client need to store its own identifier (ID string)? That should be in the key, no? cc @AdityaSripal

[fedekunze]

it's part of the interface. It technically doesn't need to tho

[cwgoes]

It would be nice to remove it in my opinion, since it's confusing to store the same data in two distinct places.

[AdityaSripal]

Looked into removing the Header, and realized it cannot be removed:

https://github.com/cosmos/cosmos-sdk/blob/master/x/ibc/07-tendermint/update.go#L92

So long as we use lite.Verify in our update function, we'll need the last SignedHeader submitted to the client to use as the "trusted header" in our verification function, and we'll need the ValidatorSet of the trusted header as well.

The ibctmtypes.Header contains just the SignedHeader and ValidatorSet, both of which are necessary for updating using the ClientState and Header. It makes sense to keep these fields in the ClientState, rather than having each consensus state store a header and valset

[cwgoes]

Hmm, this won't work e.g. with https://github.com/cosmos/ics/issues/420, unless we can reconstruct the SignedHeader from a ConsensusState (and possibly extra provided data, e.g. validator public keys, that we can validate against a hash). Is that possible?

[AdityaSripal]

We could... As far as I can tell the lite.Verify function uses the following fields from the trustedHeader:

Height, Time, NextValidatorsHash

Now Height and Time are already part of ConsensusState, and adding NextValidatorsHash to ConsensusState isn't a big deal. We can leave the other fields zeroed out, it's fine because Verify doesn't do any validation of the trustedHeader since it's already trusted.

If we do that, then yes; we can remove all of the Header stuff from ClientState; and in the update function of 07-tendermint we would also want to pass in the highest consensus state less than the given header and use it to construct a trusted header.

[cwgoes]

We could... As far as I can tell the lite.Verify function uses the following fields from the trustedHeader:

Height, Time, NextValidatorsHash

Now Height and Time are already part of ConsensusState, and adding NextValidatorsHash to ConsensusState isn't a big deal. We can leave the other fields zeroed out, it's fine because Verify doesn't do any validation of the trustedHeader since it's already trusted.

If we do that, then yes; we can remove all of the Header stuff from ClientState; and in the update function of 07-tendermint we would also want to pass in the highest consensus state less than the given header and use it to construct a trusted header.

I think that would be preferable, as we do want to support consensus state updates from past headers.

[milosevic]

We have discussed with Josef that we are missing some fields in Client and Consensus State so header verification (as part of update) can be done correctly. We will also need to make some changes to checkMisbehaviourAndUpdateState to be able to verify if submitted data is a valid fork. I would suggest creating an issue in IBC repo and do the work first there (as it will most probably leader to ICS07 changes) and then follow up here. We should probably avoid doing changes at the code level without fixing the spec first? Does this make sense? @cwgoes

[josef-widder]

@cwgoes: When you create an issue in the IBC repo, could you please tag me there so that I can participate in the discussion? I am working on light client specs both for light nodes and IBC. Thanks!

[ebuchman]

Might also be nice for the light.Verify to take a new LightHeader struct or Header interface which only exposed the relevant fields it needed cc @melekes

[cwgoes]

My understanding of the state of affairs - https://github.com/cosmos/ics/issues/456.