Delete Consensus States On RecoverClient

[AdityaSripal]

Upon client expiry and misbehaviour, we allow governance or an approved authority to substitute the invalid client with a subsitute client.

In the tendermint client, we copy over all the consensus states and update the client state according to the substitute client.

However, we do not delete the consensus states of the old client.

This is both wasteful in terms of space since all these consensus states will be expired.

It can also be an issue if the client was frozen due to misbehaviour, unfreezing the client and leaving the old consensus states may leave an unexpired malicious consensus state that can still be proven against.

Proposal:

Delete all old consensus states as part of the tendermint client proposal handler here:

https://github.com/cosmos/ibc-go/blob/main/modules/light-clients/07-tendermint/proposal_handle.go#L29

This can be a minor release for supported versions as it would help Dymension in their use of the tendermint client for their rollups. In their use case, they want to rollback the tendermint client upon misbehaviour. And deleting the old consensus states and replacing with a new client makes this trivial.

cc: @srdtrk @womensrights

[crodriguezvega]

This sounds like a good idea. Thank you @AdityaSripal.

In the tendermint client, we copy over all the consensus states and update the client state according to the substitute client.

Just a clarification: we copy over only the consensus state at the latest height of the substitute client, right?

Delete all old consensus states

I guess we can do something similar as we do in PruneAllExpiredConsensusStates, and delete all consensus states like this?

func PruneAllConsensusStates(
  ctx sdk.Context,
  clientStore storetypes.KVStore,
  cdc codec.BinaryCodec,
  clientState *ClientState,
) int {
  var heights []exported.Height

  pruneCb := func(height exported.Height) bool {
     _, found := GetConsensusState(clientStore, cdc, height)
    if !found { // consensus state should always be found
      return true
    }

    heights = append(heights, height)
    return false
  }

  IterateConsensusStateAscending(clientStore, pruneCb)

  for _, height := range heights {
    deleteConsensusState(clientStore, height)
    deleteConsensusMetadata(clientStore, height)
  }

  return len(heights)
}

Maybe as in a major release we could merge PruneAllExpiredConsensusStates and PruneAllConsensusStatesin a single function and pass in a boolean parameter to indicate if only the expired consensus states should be pruned.

Does this look correct then?

And a question: since there could be many consensus states to prune, do we need to worry about gas consumption? Or is this because this is executed as part of a governance proposal and gas is not changed? (I am not sure about this part)

[AdityaSripal]

Just a clarification: we copy over only the consensus state at the latest height of the substitute client, right?

Correct, yes thanks for the clarification. At the end, we should be left with just one consensus state which can be used to verify latest correct state. So prior consensus states are unnecessary..

Yes we can just use an iterator and delete. Since this is a gov proposal so it should be executed in the Endblock, so i don't believe gas will be an issue here. Though perhaps blockgasMeter will be?

[crodriguezvega]

I did a quick check on the number of consensus states stored for a couple of highly active channels:

• For the light client on Osmosis used in the channel to Cosmos Hub, there are 147326 consensus states.
• For the light client on Osmosis used in the channel to Noble, there are 67484 consensus states.

Even for the light client on DymensionHub used in the channel to Osmosis, there are 48946 consensus states

So if we remove the consensus states during the governance proposal, we would be doing quite a long iteration, and @tac0turtle has recommended not doing unbounded computation in governance proposals.

[crodriguezvega]

Pasting here some feedback from @tac0turtle on Slack:

"In feegrant we did two things. Iterate 200 grants for pruning. If there are more users can submit a tx to prune up to 100. The tx could help alleviate and charge fees for the iteration. An upgrade migration would also be a long down time"