Hermes should check client expiration

[ancazamfir]

Crate

relayer

Summary of Bug

I did some tests with hermes, triggered by a recent discussion with @mircea-c about client expiration.
We don't check for client expiration in the following cases:

• create connection - we keep trying and in the meantime perform update client on the unexpired client many times.
• create channel - same as above (i think, still need to test)
• when workers are spawned - we spawn client, channel and connection workers that use expired clients
• when workers relay from events (here we try forever) or on clear packets (here we retry on clear packet block interval)
• CLIs don't have an initial check for expired clients but they fail quickly as there are no retries (note i haven't checked them all)
  The only place we do check is in the client refresh (part of client worker).

I think we need a good sweep of the code looking for early detection of expired clients.

Version

master

Steps to Reproduce

Acceptance Criteria

• do not spawn workers that would attempt to use expired clients
• exit workers if clients expire (trigger can be an IBC event)
• graceful handling in the channel and connection handshake CLIs (exit early before attempting and msg)

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate milestone (priority) applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[adizere]

I think we need a good sweep of the code looking for early detection of expired clients.

Could we have all the expiration-related checks localized in the ForeignClient::restore (and find)?

https://github.com/informalsystems/ibc-rs/blob/132090edc634f0544a028165d18a4961a28a4a94/relayer/src/foreign_client.rs#L319

[ancazamfir]

Could be, not entirely sure (don't see find being used, restore is only in the client worker). When starting workers I would go as early as spawn_workers_from_chain_to_chain or spawn_workers_for_client the latest, check the client state and if expired, don't spawn ANY worker and continue with the next client. For running workers, check the client state on every event. We can push the check deeper but we need a way to exit the worker on client expiration.

But in general I agree with you, ideally this check should be done in only a few places.