Get the `maxExpectedTimePerBlock` parameter directly from the chain

hu55a1n1 commented 2 years ago

Summary

hermes currently expects users to provide the max_block_time parameter in the config and uses it to calculate the connection block delay as well as max clock drift. We can get this parameter directly from the chain (eg. using the /header Tendermint ABCI endpoint). Also, since this is a genesis parameter and isn't expected to change, we can cache it.

PS: Thanks to @ancazamfir for these ideas!

Acceptance Criteria

[ ] Provide inexpensive access to a chain's maxExpectedTimePerBlock param.
[ ] Remove the need for users to provide the max_block_time config parameter. Also, remove this config parameter entirely.
[ ] Do this in a chain-agnostic way (if possible).

For Admin Use

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

hu55a1n1 commented 2 years ago

Tentative Proposal

[ ] Extend the ChainHandle trait to provide a method that gives access to the maxExpectedTimePerBlock param.
[ ] Use the /header endpoint to get the parameter from Tendermint chains. (See https://github.com/informalsystems/tendermint-rs/issues/1106)
[ ] Cache the parameter (in the ChainHandle?) on startup.

romac commented 2 years ago

Cache the parameter (in the ChainHandle?) on startup.

You should be able to cache the value on first use in the CachingHandle, backed by a cache with no TTL.

adizere commented 2 years ago

I wonder if this feature is needed, or potentially blocking, anything. It seems like it would simplify relayer operator life, but more like a nice-to-have than a wanted feature/requirement.

I will optimistically move it to v2 milestone, but in case we see the need for it, we can re-prioritize it later.

adizere commented 2 years ago

It seems we need to query via genesis, not header, as suggested in an earlier comment. For production networks, it is usually impossible to get a response to the genesis query because it is too large (100MB, see eg cosmoshub-4); the genesis_chunked method is awkward because results are base64 encoded (apparently).

The best we can do would be then to rely on IBC-go to add a new gRPC endpoint for us to allow querying for for GenesisState of connection.

https://github.com/cosmos/ibc-go/blob/d0599131441f29c22f789672667fc527da295b37/proto/ibc/core/connection/v1/genesis.proto#L11

We can programatically obtain the max_expected_time_per_block from the Params in there.

Quoting Anca below:

There is a GenesisState defined in the ibc protos that includes this.
In genesis there is a param max_expected_time_per_block
...
    "ibc": {
       ..
      "connection_genesis": {
        "connections": [],
        "client_connection_paths": [],
        "next_connection_sequence": "0",
        "params": {
          "max_expected_time_per_block": "30000000000"
        }
      },

adizere commented 2 years ago

Depends on https://github.com/cosmos/ibc-go/issues/1628 -- priority note: The issue will likely be ready in Q4/2022.

informalsystems / hermes