ICS 7: Identify exact fields required for Tendermint light client state

[cwgoes]

Ref cosmos/cosmos-sdk#6736

At present, the specification and implementation do not agree on what fields a Tendermint light client state should store.

The specification (ICS 7) has the following fields:

interface ClientState {
  chainID: string
  validatorSet: List<Pair<Address, uint64>>
  trustLevel: Rational
  trustingPeriod: uint64
  unbondingPeriod: uint64
  latestHeight: Height
  latestTimestamp: uint64
  frozenHeight: Maybe<uint64>
  upgradeCommitmentPrefix: CommitmentPrefix
  upgradeKey: []byte
  maxClockDrift: uint64
  proofSpecs: []ProofSpec
}

The Cosmos SDK implementation, however, has the following fields:

type ClientState struct {
	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"`
}

Differences:

• upgradeCommitmentPrefix and upgradeKey have yet to be added to the implementation. This will be done soon (I believe @AdityaSripal is working on the upgrade-related features at the moment).
• The Cosmos SDK client state keeps a full Header, which includes chain ID, validator set, height, timestamp, and other metadata (defined here):

type Header struct {
	tmtypes.SignedHeader `json:"signed_header" yaml:"signed_header"` // contains the commitment root
	ValidatorSet         *tmtypes.ValidatorSet                       `json:"validator_set" yaml:"validator_set"`
}

tmtypes.SignedHeader (defined here) includes:

type SignedHeader struct {
	*Header `json:"header"`
	Commit *Commit `json:"commit"`
}

Commit (defined here) includes a lot of data we definitely don't need to keep in the client state, and some data which is just duplicated in the Header:

type Commit struct {
	// NOTE: The signatures are in order of address to preserve the bonded
	// ValidatorSet order.
	// Any peer with a block can gossip signatures by index with a peer without
	// recalculating the active ValidatorSet.
	Height     int64       `json:"height"`
	Round      int32       `json:"round"`
	BlockID    BlockID     `json:"block_id"`
	Signatures []CommitSig `json:"signatures"`

	// Memoized in first call to corresponding method.
	// NOTE: can't memoize in constructor because constructor isn't used for
	// unmarshaling.
	hash     tmbytes.HexBytes
	bitArray *bits.BitArray
}

As does Header (defined here):

type Header struct {
	// basic block info
	Version tmversion.Consensus `json:"version"`
	ChainID string              `json:"chain_id"`
	Height  int64               `json:"height"`
	Time    time.Time           `json:"time"`

	// prev block info
	LastBlockID BlockID `json:"last_block_id"`

	// hashes of block data
	LastCommitHash tmbytes.HexBytes `json:"last_commit_hash"` // commit from validators from the last block
	DataHash       tmbytes.HexBytes `json:"data_hash"`        // transactions

	// hashes from the app output from the prev block
	ValidatorsHash     tmbytes.HexBytes `json:"validators_hash"`      // validators for the current block
	NextValidatorsHash tmbytes.HexBytes `json:"next_validators_hash"` // validators for the next block
	ConsensusHash      tmbytes.HexBytes `json:"consensus_hash"`       // consensus params for current block
	AppHash            tmbytes.HexBytes `json:"app_hash"`             // state after txs from the previous block
	// root hash of all results from the txs from the previous block
	LastResultsHash tmbytes.HexBytes `json:"last_results_hash"`

	// consensus info
	EvidenceHash    tmbytes.HexBytes `json:"evidence_hash"`    // evidence included in the block
	ProposerAddress Address          `json:"proposer_address"` // original proposer of the block
}

ICS 7 is already set up to store the AppHash elsewhere (under a height-specific consensus state key). The client state should store only the absolute minimum information required for verification of subsequent headers, so I think the best option would be to figure out if any fields other than what the ICS 7 specification currently specifies are actually required for light client verification, add them if necessary, and simplify the light client verification API to take only the fields required. This is relevant not only for IBC but also for other light clients which may wish to avoid storing superfluous metadata.

[cwgoes]

cc @josef-widder @AdityaSripal @melekes

[josef-widder]

The client state should store only the absolute minimum information required for verification of subsequent headers

We will definitely need NextValidators (in addition to the Validators that are already at ICS07)

However, this is not only relevant for client state but also for ConsensusState. ConsensusState should have all the information from LightBlocks (we called them signed headers in the past) plus configuration parameters that are already there, i.e., trusting period, etc.

type LightBlock struct {
		Header          Header
		Commit          Commit
		Validators      ValidatorSet
		NextValidators  ValidatorSet
		Provider        PeerID
}

[cwgoes]

Per @AdityaSripal we can just add NextValidatorsHash to consensus states, so each consensus will have a height, a timestamp, a next validators hash, and an app hash.

I suspect this will also allow us to remove validatorSet from the client state, and rectify the SDK with the spec.

[josef-widder]

I am not sure about the consequences of removing validatorSet. Is the idea that once a header is verified we do not need to store the information anymore? Btw. the issue of missing NextValidators is also present in headers in ICS07. For verification we need both validatos and nextvalidators.

[cwgoes]

I am not sure about the consequences of removing validatorSet. Is the idea that once a header is verified we do not need to store the information anymore? Btw. the issue of missing NextValidators is also present in headers in ICS07. For verification we need both validatos and nextvalidators.

Yes; we can just store the hash of the validator set and/or next validator set, once verified.

[josef-widder]

Yes; we can just store the hash of the validator set and/or next validator set, once verified.

I guess we don't look into the validator set anymore; but I have to think about this again in the context of fork detection. However, the Nextvalidators will will use again when we verify the next header.

[cwgoes]

Yes; we can just store the hash of the validator set and/or next validator set, once verified.

I guess we don't look into the validator set anymore; but I have to think about this again in the context of fork detection. However, the Nextvalidators will will use again when we verify the next header.

If we need the full validator set, presumably it can be provided in the transaction & we can check against the hash.

[josef-widder]

Yeah, that's true. I was thinking about checking what evidence to submit. As this will rarely happen, if at all, we don't need store the validators explicitly, most likely, and provide additional information only when needed.

[AdityaSripal]

I guess we don't look into the validator set anymore; but I have to think about this again in the context of fork detection. However, the Nextvalidators will will use again when we verify the next header.

So when we verify the next header, we usually would use NextValidators to pass in for trustedVals in the lite.Verify function:

https://github.com/tendermint/tendermint/blob/v0.33.4/lite2/verifier.go#L140

This is because the NextValidators is the last validator set that light client has seen the chain commit to, so it is latest trusted validator set. In the ICS-7 implementation, we pass in the current ValidatorSet as the trustedVals. This is done because we already have the current ValidatorSet in the consensus state.

This will make the bisecting verification slightly less efficient from a typical light client, in the sense that a light client that uses NextValidatorSet as trustedVals will be able to skip ahead farther since the valset changes will be compared between NextValidatorSet and header.ValidatorSet as opposed to current ValidatorSet and header.ValidatorSet in cases where there is already a valset change between ValidatorSet and NextValidatorSet.

However, there is no correctness concern with just using ValidatorSet as trustedVals and it saves the need from saving two ValidatorSets for each consensus state

[AdityaSripal]

Note: We already need need the current ValidatorSet for Header verification

[josef-widder]

However, there is no correctness concern with just using ValidatorSet as trustedVals and it saves the need from saving two ValidatorSets for each consensus state

I am not sure why there is no correctness concern: If the validator set and the nextvalidator set of height h do not intersect, then the lightclient is not able to verify the correct header of height h+1 by using the validator set of height h.

[AdityaSripal]

We don't use the validator set of height h, we just check that h+1 validator set hashes to h NextValidatorsHash

https://github.com/tendermint/tendermint/blob/v0.33.4/lite2/verifier.go#L122

[josef-widder]

So you use h.NextValidatorsHash for sequential, and h.Validators for skipping. OK.

Regarding ICS07, this means that we have to add NextValidators to the specification, either as hash or as set (at several places). Right now, it is not there at all.

I suggest we add NextValidators also in the consensus state. Then you could use h.NextValidators also for skipping, which you do not use right now, as it is not there.

[cwgoes]

Ref cosmos/cosmos-sdk#7176, small discrepancy in consensus state, should be easy to fix.

[crodriguezvega]

Right now the fields in ClientState specified in ICS-07

interface ClientState {
  chainID: string
  trustLevel: Rational
  trustingPeriod: uint64
  unbondingPeriod: uint64
  latestHeight: Height
  frozenHeight: Maybe<uint64>
  upgradePath: []string
  maxClockDrift: uint64
  proofSpecs: []ProofSpec
}

are the same as the ones used in ibc-go:

type ClientState struct {
  ChainId    string
  TrustLevel Fraction
  // duration of the period since the LastestTimestamp during which the
  // submitted headers are valid for upgrade
  TrustingPeriod time.Duration
  // duration of the staking unbonding period
  UnbondingPeriod time.Duration 
  // 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 types.Height 
  // Latest height the client was updated to
  LatestHeight types.Height
  // Proof specifications used in verifying counterparty state
  ProofSpecs []*_go.ProofSpec
  // Path at which next upgraded client will be committed.
  // Each element corresponds to the key for a single CommitmentProof in the
  // chained proof. NOTE: ClientState must stored under
  // `{upgradePath}/{upgradeHeight}/clientState` ConsensusState must be stored
  // under `{upgradepath}/{upgradeHeight}/consensusState` For SDK chains using
  // the default upgrade module, upgrade_path should be []string{"upgrade",
  // "upgradedIBCState"}`
  UpgradePath []string 
  // allow_update_after_expiry is deprecated
  AllowUpdateAfterExpiry bool  // Deprecated: Do not use.
  // allow_update_after_misbehaviour is deprecated
  AllowUpdateAfterMisbehaviour bool  // Deprecated: Do not use.
}

except for AllowUpdateAfterExpiry and AllowUpdateAfterMisbehaviour, but these have now been deprecated.

So I think this issue can now be closed.