Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get the maxExpectedTimePerBlock parameter directly from the chain #2037

Open
8 tasks
hu55a1n1 opened this issue Mar 30, 2022 · 5 comments
Open
8 tasks

Get the maxExpectedTimePerBlock parameter directly from the chain #2037

hu55a1n1 opened this issue Mar 30, 2022 · 5 comments
Labels
A: blocked Admin: blocked by another (internal/external) issue or PR I: configuration Internal: related to Hermes configuration I: logic Internal: related to the relaying logic I: rpc Internal: related to (g)RPC O: usability Objective: cause to improve the user experience (UX) and ease using the product
Milestone

Comments

@hu55a1n1
Copy link
Member

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
Copy link
Member Author

hu55a1n1 commented Mar 30, 2022

Tentative Proposal

@romac
Copy link
Member

romac commented Mar 30, 2022

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 adizere added this to the v0.15.0 milestone Apr 1, 2022
@adizere adizere added P-medium I: logic Internal: related to the relaying logic O: usability Objective: cause to improve the user experience (UX) and ease using the product I: rpc Internal: related to (g)RPC I: configuration Internal: related to Hermes configuration labels Apr 1, 2022
@adizere
Copy link
Member

adizere commented Apr 5, 2022

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 adizere modified the milestones: v0.15.0, v2 Apr 5, 2022
@adizere
Copy link
Member

adizere commented Jun 30, 2022

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 adizere added A: blocked Admin: blocked by another (internal/external) issue or PR and removed P-medium labels Jun 30, 2022
@adizere
Copy link
Member

adizere commented Jun 30, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: blocked Admin: blocked by another (internal/external) issue or PR I: configuration Internal: related to Hermes configuration I: logic Internal: related to the relaying logic I: rpc Internal: related to (g)RPC O: usability Objective: cause to improve the user experience (UX) and ease using the product
Projects
None yet
Development

No branches or pull requests

3 participants