Disallow creation of new Tendermint client state instance with a frozen height

[Farhad-Shabani]

Closes: #178

---

PR author checklist:

• Added changelog entry, using unclog.
• Added tests.
• Linked to GitHub issue.
• Updated code comments and documentation (e.g., docs/).
• Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

• Reviewed Files changed in the GitHub PR explorer.
• Manually tested (in case integration/unit/mock tests are absent).

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.05 🎉

Comparison is base (845cf48) 72.88% compared to head (3f569a8) 72.93%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #602      +/-   ##
==========================================
+ Coverage   72.88%   72.93%   +0.05%     
==========================================
  Files         126      126              
  Lines       15699    15721      +22     
==========================================
+ Hits        11442    11466      +24     
+ Misses       4257     4255       -2     

Impacted Files	Coverage Δ
crates/ibc/src/clients/ics07_tendermint/error.rs	19.04% <ø> (ø)
...ibc/src/core/ics02_client/handler/create_client.rs	82.35% <ø> (ø)
...es/ibc/src/core/ics02_client/msgs/create_client.rs	89.28% <ø> (ø)
...s/ibc/src/clients/ics07_tendermint/client_state.rs	65.57% <100.00%> (+1.29%)	⬆️
crates/ibc/src/mock/context.rs	75.30% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[plafer]

I don't think we want to disallow the creation of frozen clients (see my earlier comment on the issue). ibc-go and the spec allow for it; I don't see why we should reject it. There might be an obscure use case for it; but in the majority of cases, we would just allow a useless client to be created.

[Farhad-Shabani]

I don't think we want to disallow the creation of frozen clients (see my earlier comment on the issue).

Of course, I saw your comment. Note that just before this very recent PR in the IBC repo (that was triggered by our investigation around ClientTypePath), the spec was initializing a ClientState only by the frozenHeight = null, which is raised in #136 as another matter to investigate.

I don't think this is the right approach; the domain type should be allowed to be created as frozen.

Given the above reason, I disallowed creating a client with a frozen height. Though, from what I see in some other places of the spec (e.g, here for validating self client) similar condition - FrozenHeight must be null - is still enforcing. Besides, in IBC-go a new instance of Tendermint client state is always created with frozenHeight = clienttypes.ZeroHeight() not supporting client creation by taking in a frozen height.

Given the above references and considering the primary purpose of having a frozen height which relates to misbehavior detection, I was looking at the frozen_height as a state that should be only controlled by the IBC handler. But, nevertheless, If you still think? the allowance is not in conflict with the spec, potentially could be other functionalities for the frozen_height, and sometime later it will make sense to create such a client, that’s a different story.

[plafer]

Right, so things are a bit confusing on the ibc-go repo, and this is really a technicality.

Note that just before cosmos/ibc#956 very recent PR in the IBC repo (that was triggered by #592 around ClientTypePath), the spec was initializing a ClientState only by the frozenHeight = null, which is raised in #136 as another matter to investigate.

Unfortunately, I think the spec needs to be treated as a general reference and not a source of truth; ibc-go is really the source of truth.

Besides, in IBC-go a new instance of Tendermint client state is always created with frozenHeight = clienttypes.ZeroHeight() not supporting client creation by taking in a frozen height.

This NewClientState function is only called in the upgrade code here. The CreateClient handler gets the initial ClientState here, where the ClientState was built by the relayer, and could be frozen. I don't see any checks in the CreateClient code to ensure that the client is frozen.

Though, from what I see in some other places of the spec (e.g, here for validating self client) similar condition - FrozenHeight must be null - is still enforcing.

ValidateSelfClient is called in the connection handshake code, and indeed there it is necessary for the client to not be frozen. This is what I meant by "it will be a useless client"; no connections will be able to use it.

How I see it:

1. Whether or not we allow for a frozen client to be created is an arbitrary choice (either CreateClient fails, or the subsequent connection opening handshake fails); I prefer sticking to what ibc-go does, for consistency's sake. For example, if given the same frozen client, an ibc-go chain fails at the connection opening stage, while an ibc-rs chain fails at the CreateClient phase, it can cause unnecessary confusion to the developer debugging the situation.
2. We should ensure that we disallow the client to be frozen after an upgrade

[Farhad-Shabani]

Unfortunately, I think the spec needs to be treated as a general reference and not a source of truth; ibc-go is really the source of truth.

Whether or not we allow for a frozen client to be created is an arbitrary choice (either CreateClient fails, or the subsequent connection opening handshake fails); I prefer sticking to what ibc-go does, for consistency's sake. For example, if given the same frozen client, an ibc-go chain fails at the connection opening stage, while an ibc-rs chain fails at the CreateClient phase, it can cause unnecessary confusion to the developer debugging the situation.

Reached out to the IBC-go team to see what was the reason for allowing the creation of such a client. It is replied that a client is considered to be frozen if it has a non-zero frozen height (i.e.frozen height is used as a boolean) and they will open an issue to add a check for this in the 07-tendermint client state Validate to make sure that frozen height is zero during a new client instance creation.

Another thing caught my attention; IBC-go always sets FrozenHeight to 1 in misbehavior cases. Basically, FrozenHeight is always 0 or 1, acting like a boolean, and the height number doesn't matter. (They’ll fix this soon, see here) Again, makes no sense that users be able to set a predetermined frozen height.