Determine what in ClientState needs to be verified by counterparty

[AdityaSripal]

Currently there are a couple fields that are passed into NewClientState without being verified by the counterparty. This can pose potential dangers if a relayer creates an invalid client and the counterparty creates a connection with an invalid client.

Currently only the consensus state is verified by the counterparty

1. Unbonding Period is not verifed: Relayer sets the unbonding period in NewClientState and it is not verified, nor can it be changed.

2. ProofSpecs is not verified: Relayer sets the proofspecs when initializing the client state. However, counterparty doesn't check if these proofspecs match its own proof format.

3. ConsensusParams needs to be verified: To be implemented with IBC TM Clients should set their own Consensus Params #6516 , consensus params must also verified by counterparty.

Creating a seperate proof for each of these parameters would greatly increase the complexity of Connection handshake. Even batching the proofs together, doesn't solve complexity. It just hides the multiple proofs in a single proof.

Proposed solution:

• Add UnbondingPeriod, ProofSpecs, and ConsensusParams to ConsensusState.
• Counterparty continues to verify the consensus state proof during connection handshake as before
• Change GetSelfConsensusState to include these parameters.

When ConnectionHandshake occurs, counterparty can verify a single proof of the consensus state, and it can also trivially construct its own "self consensus state" to check if they match as it already does. This prevents changing any interface in 02-client and only makes the necessary changes to 07-tendermint/ConsensusState where the changes belong.

Pros:

• Allow counterparty to verify all relevant parameters of client-state while continuing to only use one proof
• No changes to 02-client, only change 07-tendermint

Cons:

• Increased size of 07-tendermint/ConsensusState

This con can be mitigated by only filling in UnbondingPeriod, ProofSpecs, and ConsensusParams when they change from their previous value (note: this should happen rarely). Thus they are only non-zero values on initial handshake and if they change. The tendermint ClientState will contain the latest non-zero values for these fields so they are always retrievable.

Thus, counterparty only has to verify these fields on initial handshake, and then if they are ever non-zero. This removes any unnecessary storage wastage and verification.

cc: @cwgoes @colin-axner @fedekunze

[cwgoes]

Right, this makes sense, nice catch, the specification is a bit vague on these points. All data which is utilised by the light client algorithm should be verified in the connection handshake, which includes the trusting period (not exactly the unbonding period, we should distinguish these), the proof specs, and the consensus parameters, as you mention.

The proposed solution seems like it would fix the high-level problem, but I wonder if it would be cleaner to put these fields in the Tendermint ClientState instead, since they aren't intended to be updated at each height? The ClientState is also verified during the handshake, so we should be able to achieve the same semantics. This would necessitate adding a field for "consensus verification parameters which are not updated at later heights" to the client state and adding another introspection function specifically for those parameters.

Thus, counterparty only has to verify these fields on initial handshake, and then if they are ever non-zero. This removes any unnecessary storage wastage and verification.

Do we currently have verification logic to deal with the case when one of these fields changes? I think not. We would need to add a way for the chain which the light client is validating to indicate that it wants to change the field prior to actually doing so. We do want this for chain ID & height, maybe there is a case for supporting changes to unbonding period & proof specs as well.

[AdityaSripal]

which includes the trusting period (not exactly the unbonding period, we should distinguish these)

Well from my understanding, the trusting period is something each lite client can choose for itself (a client-level parameter), so long as it is a fraction (<1) of the UnbondingPeriod which is a chain-level parameter that must be verified by counterparty.

In other words, different IBC clients for a chainA may choose different trustingPeriods (perhaps 1/2*X on one chain, 2/3*X for another). Yet they must both agree that the UnbondingPeriod is X for a given block range. Thus, trustingPeriod doesn't need to be verified but unbondingPeriod does. Please correct me if my understanding is wrong.

The ClientState is also verified during the handshake, so we should be able to achieve the same semantics

Don't think this is true as far as I know. The connection handshake verifies the ConnectionEnd and the ConsensusState. Since I don't want to add another proof to verify in the handshake, it made sense to add these client-paramaters into the ConsensusState for verification and adding the introspection to the current SelfConsensusState when necessary.

Do we currently have verification logic to deal with the case when one of these fields changes?

No, currently there is no way to update these fields at all after initialization. I do think we need to find a way to update these in addition to chain-id and height. At the moment, I'm thinking that when these fields do need to change the relayer fills in the fields of the ConsensusState with the updated values, as opposed to nil values when they remain the same.

As you said, there needs to be a way for chains to indicate that these paramaters are going to change at a particular height; along with a way for the counterparty to verify this in a proof so that the relayer's updated values can be verified as true. This would involve another required ICS-24 path that chains write to when they want to upgrade one of these parameters, and a function that allows counterparties to receive this proof and update their clients accordingly. (Currently believe this should be handled as a special case of the UpdateClient function, but we may choose a different approach so I don't intend to implement a solution to this now).

Implementation Path:

The ability to upgrade these parameters after initialization needs to be specified, so I don't plan to handle it here. Perhaps it is another concern to take into account along with #6378.

The PR addressing this will only concern itself with verifying these fields on client initialization by using ConsensusState (unless we decide to include another proof in connection handshake). It will ignore any changes to these fields after initialization, thus continuing to not support changes to these paramaters after intiialization.

Once we agree on a way to update these parameters, I can implement that part as well.

[cwgoes]

Well from my understanding, the trusting period is something each lite client can choose for itself (a client-level parameter), so long as it is a fraction (<1) of the UnbondingPeriod which is a chain-level parameter that must be verified by counterparty.

In other words, different IBC clients for a chainA may choose different trustingPeriods (perhaps 1/2X on one chain, 2/3X for another). Yet they must both agree that the UnbondingPeriod is X for a given block range. Thus, trustingPeriod doesn't need to be verified but unbondingPeriod does. Please correct me if my understanding is wrong.

Hmm - yes, we do want to allow custom trusting periods, but I wonder if there should be a minimum difference between the unbonding period and the trusting period enforced by the chain itself (since it has to do with expected evidence submission latency) - what you propose is fine for now though.

Don't think this is true as far as I know. The connection handshake verifies the ConnectionEnd and the ConsensusState. Since I don't want to add another proof to verify in the handshake, it made sense to add these client-paramaters into the ConsensusState for verification and adding the introspection to the current SelfConsensusState when necessary.

Ah no, you're right, just the connection state is verified, I was thinking of that, but we don't want to put this data there. Maybe we should verify the client state in the handshake in the future, but this proposed fix is probably easier for now.

As you said, there needs to be a way for chains to indicate that these paramaters are going to change at a particular height; along with a way for the counterparty to verify this in a proof so that the relayer's updated values can be verified as true. This would involve another required ICS-24 path that chains write to when they want to upgrade one of these parameters, and a function that allows counterparties to receive this proof and update their clients accordingly. (Currently believe this should be handled as a special case of the UpdateClient function, but we may choose a different approach so I don't intend to implement a solution to this now).

I also think that a special-case of updateClient would be cleanest.

The PR addressing this will only concern itself with verifying these fields on client initialization by using ConsensusState (unless we decide to include another proof in connection handshake). It will ignore any changes to these fields after initialization, thus continuing to not support changes to these paramaters after intiialization.

Sounds good.

[cwgoes]

Let's verify the ClientState, require that the client state expose a method which checks that it is a valid client state for your chain (this will require context access to look up prior self consensus states), and replace the current consensus state check with this.

[milosevic]

Agree with Chris regarding trusting period; we need to provide more clarity regarding invariants between trustingPeriod and unbondingPeriod. I just have a question regarding usage of ProofSpecs and ConsensusParams in lightClient operation (and connection handshake). Can someone please clarify this?

[AdityaSripal]

Note: dealing with this issue is no longer as simple since the ClientState no longer contains the LastHeader which was an effective commitment to a consensus state with the merging of #6873

Now I believe the connection handshake (INIT and TRY_OPEN) will most likely entail 2 proofs (both a proof of the latest consensus state), and a proof of the client state.

[cwgoes]

Now I believe the connection handshake (INIT and TRY_OPEN) will most likely entail 2 proofs (both a proof of the latest consensus state), and a proof of the client state.

Yes; both need to be proved, and both must verify in order for the handshake to safely continue.

[cwgoes]

Trusting period, trust level (maybe above a threshold), and max clock drift should be allowed to vary, all else fixed.