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

EPIC: gRPC requirements for IBC relayers #11012

Closed
4 tasks
adizere opened this issue Jan 24, 2022 · 11 comments · Fixed by #15597
Closed
4 tasks

EPIC: gRPC requirements for IBC relayers #11012

adizere opened this issue Jan 24, 2022 · 11 comments · Fixed by #15597
Assignees
Labels

Comments

@adizere
Copy link
Contributor

adizere commented Jan 24, 2022

Summary

IBC relayers such as the go-relayer and Hermes still use the RPC endpoint of full nodes. For easier interoperability, relayers should ideally switch completely to using the gRPC interface. That is a very long way to go, so in the meantime we'll focus on the most important endpoints that should be transitioned from RPC to gRPC.

Problem Definition

Highest priority

Complete list

This is the complete list of RPC endpoints that Hermes is currently (v0.10 ~January 2022) using:

  • /status:
    • To fetch node_info.id, for initializing the light client component
    • To fetch the chain’s latest height, used in many methods
  • /tx_search:
    • Query for NewBlock events, to assess node health wrt. transaction indexing
    • Query for packet events, to find pending packets
    • Query for transaction events, for confirming if packets are committed to the blockchain
    • Query for client update events, for the misbehavior detection task
  • /block_search and /block_results:
    • Query for block events, to find pending packets in the begin block or end block events
  • /consensus_params:
    • To fetch consensus_params.block.max_bytes parameter, for health check.
  • /genesis
    • Not used anymore! We’ll delete it. Was used for the client create method. ✅
  • /abci_query:
  • /broadcast_tx_sync
    • For broadcasting a transaction
  • /health:
    • Basic check to assess node health.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

Thanks for creating this issue. Agree that we should have relayers utilize gRPC as much as possible. Some of those endpoints you've listed are Tendermint endpoints and do not have gRPC options. For the application endpoints, we should definitely encourage gRPC usage.

Also, having this issue reside in the SDK probably won't get us much. Perhaps consider opening the same issue in both Hermes and go-relayer repos?

@adizere
Copy link
Contributor Author

adizere commented Jan 25, 2022

Thanks for the quick feedback!

having this issue reside in the SDK probably won't get us much. Perhaps consider opening the same issue in both Hermes and go-relayer repos?

I was considering this originally. But I decided against having the issue in Hermes/go-relayer repos because the work does not belong to either of those repos. To be concrete: These are requirements on any IBC-enabled chain (such as the SDK), they should inform the SDK (but perhaps more concretely IBC-go) roadmap.

Happy to move the issue around (maybe to the IBC-go repo) if I'm misunderstanding something, but given my current understanding the present repo seem most appropriate, let me know otherwise.

Some of those endpoints you've listed are Tendermint endpoints and do not have gRPC options.

Good point! The boundary Tendermint/SDK is unclear to me, it is necessary to triage them into SDK vs. Tendermint before we can make progress. Nevertheless, following the same logic as above: I'm curious if it's feasible a long-term SDK roadmap that ensures all these requirements being satisfied via a single interface such as gRPC. Or is IBC-go more appropriate?

@alexanderbez
Copy link
Contributor

Well what do you wish to be done in the SDK?

@tac0turtle
Copy link
Member

There is a balance to strike between tendermint and the sdk. The biggest issue from talking to folks is abci_query because it holds a mutex at the abci layer. Moving this to grpc and allowing parallel queries should alleviate this, but don't think we should try to wrap the entire rpc in the sdk.

@adizere
Copy link
Contributor Author

adizere commented Jan 31, 2022

Thanks Aleksandr and Marko for the input, appreciate it!

I changed the PR description to focus on the abci_query endpoint.

Well what do you wish to be done in the SDK?

I only have a naive answer at the moment (along the lines of: "let's work on wrapping everything in gRPC"). Hope it's cool to keep this issue open while gathering more understanding on which endpoints (in addition to abci_query) deserve moving to gRPC.

@tac0turtle tac0turtle added the T:Epic Epics label Nov 7, 2022
@tac0turtle tac0turtle changed the title gRPC requirements for IBC relayers EPIC: gRPC requirements for IBC relayers Nov 7, 2022
@tac0turtle
Copy link
Member

  • /abci_query:
  • /status:
  • /consensus_params: (add as part of the consensus module)
  • /genesis
    • This gives false genesis files if the node doesn't download the file. I highly recommend removing dependence on it.

from the above list I only see a need to support these. We are lady support tx_search and block search but it's non deterministic and may be deprecated in the future, we shouldn't rely on it.

seems like the only thing missing is status which we can do as a wrapper.

@adizere
Copy link
Contributor Author

adizere commented Apr 3, 2023

For the gRPC /status implementation, the requirements are that it should be able to respond with this information ideally:

  • everything exposed in the Comet-level /status
  • the height of the latest block that the application executed
  • the timestamp of the latest block that the application executed

Currently in Hermes relayer, we need 2 separate queries to obtain the latest info regarding the application status. It is very awkward. For completeness, this is what we currently do:

  1. call into Comet-level RPC /abci_info, obtain the height h which is the latest height the application executed
  2. call into Comet-level RPC /blockchain?minHeight={h}&maxHeight={h}, then extract the timestamp from the header of block height h

@jtieri
Copy link
Member

jtieri commented Apr 11, 2023

Late to the party here but wanted to provide a list of RPC endpoints that the go-relayer is using. We use almost all of the ones that @adizere has already mentioned but the go-relayer also utilizes:

/block - to retrieve the block time of some block at height h
/tx - to query a specific tx by hash and parse events from TxResult
/commit - to get the timestamp and apphash when composing a cometbft ConsensusState
/validators - to retrieve the hash of the next valset when composing a cometbft ConsensusState

it's not immediately clear to me which of these are relevant to the conversation here but I was linked to this issue and figured I'd chip in as one of the primary maintainers of the go-relayer

@tac0turtle
Copy link
Member

I can add the latter two to status, will amend my pr.

Never late 😄

@DavidNix
Copy link

I am also late to the party and like @jtieri, I work for Strangelove. My perspective is that of a node operator.

I am hoping (and it appears that) the move to gRPC is optional and the original JSON-REST endpoints will not go away. I'm advocating we keep REST in the unlikely event the endpoints are planning to be deprecated.

In short, supporting http2 over the public internet is more challenging for a node operator.

I'm a fan of gRPC and am aware its many advantages. However, there are some disadvantages:

  1. Protocol Buffers are more challenging to debug and inspect traffic compared to JSON-based REST APIs, which are human-readable.
  2. Some firewalls and proxies may not support HTTP/2, leading to potential issues with gRPC traffic. This can be problematic over the public internet, where intermediary devices may block or alter gRPC traffic.
  3. gRPC has a steeper learning curve compared to REST, requiring developers to learn new concepts such as protobufs, stub generation, and specific gRPC API patterns.
  4. Implementing gRPC can be more complex than REST, particularly in scenarios involving server-side streaming, client-side streaming, or bidirectional streaming.
  5. Load balancing gRPC can be more complicated than traditional REST services due to the use of long-lived connections with HTTP/2. Choices such as client-side load balancing, proxy-based load balancing, or look-aside load balancing are not required by a REST API.
  6. HTTP/2 with TLS can be more complex to setup.

@tac0turtle
Copy link
Member

tac0turtle commented Apr 15, 2023

nothing is being removed, this is only adding an endpoint to allow relayers to use grpc if they would like. The JSON-REST endpoints are a comet notion, cosmos-sdk provides the grpc on top

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants