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

client.Context gRPC queries should use the gRPC port instead of abci_query #10996

Closed
4 tasks
amaury1093 opened this issue Jan 21, 2022 · 5 comments
Closed
4 tasks
Assignees
Labels
C: gRPC Issues and PRs related to the gRPC service and HTTP gateway. T: Client UX

Comments

@amaury1093
Copy link
Contributor

amaury1093 commented Jan 21, 2022

Summary

Making a gRPC query using client.Context should ping the gRPC server directly, instead of pinging Tendermint's abci_query RPC endpoint.

Problem Definition

In #10045, we removed baseapp's grpc router routing all queries through tendermint. This means that when pinging the gRPC endpoint directly, queries don't pass by Tendermint.

However, there's only 1 test file in the SDK that tests this:

s.conn, err = grpc.Dial(
val0.AppConfig.GRPC.Address,
grpc.WithInsecure(), // Or else we get "no transport security set"
)

All other tests, and probably other app developers, use the Cosmos SDK's client.Context abstraction. This client.Context pings the Tendermint RPC endpoint:

res, err := ctx.QueryABCI(abciReq)

It's slightly less performant to make queries via abci_query than via gRPC directly.

Proposal

Make the client.Context ping the gRPC by default. We should still provide an option to use Tendermint RPC if the user wishes.

An idea of the user-facing API could be:

// Option1: use Tendermint rpc
tmRpcClient, err := client.NewClientFromNode("tcp://0.0.0.0:26657")
tmClientConn := NewTmClientConn(tmRpcClient) // `NewTmClientConn` needs to be written as part of this issue
clientCtx = clientCtx.WithClientConn(tmClientConn)

// Option 2: use gRPC server endpoint
grpcClientConn := grpc.Dial(
	"1.2.3.4:9090",
	grpc.WithInsecure(),
)
clientCtx = clientCtx.WithClientConn(grpcClientConn)

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@amaury1093 amaury1093 added C: gRPC Issues and PRs related to the gRPC service and HTTP gateway. T: Client UX labels Jan 21, 2022
@tac0turtle
Copy link
Member

I think this overlaps with #10859?

@amaury1093
Copy link
Contributor Author

This issues sets the default to gRPC on the client side. I believe #10859 investigates if gRPC queries are still routed through tm on the server side.

They are separate issues IMO. but yeah, as you wrote in that other issue, we should make sure that OP there tested parallelism using the gRPC endpoint and not the TM rpc.

@tac0turtle
Copy link
Member

ah okay, I see what you mean

troian added a commit that referenced this issue Jan 21, 2022
fixes #10996

Signed-off-by: Artur Troian <troian.ap@gmail.com>
troian added a commit that referenced this issue Jan 22, 2022
fixes #10996

Signed-off-by: Artur Troian <troian.ap@gmail.com>
troian added a commit that referenced this issue Jan 23, 2022
fixes #10996

Signed-off-by: Artur Troian <troian.ap@gmail.com>
troian added a commit that referenced this issue Jan 26, 2022
fixes #10996

Signed-off-by: Artur Troian <troian.ap@gmail.com>
troian added a commit that referenced this issue Jan 26, 2022
fixes #10996

Signed-off-by: Artur Troian <troian.ap@gmail.com>
troian added a commit that referenced this issue Jan 27, 2022
fixes #10996

Signed-off-by: Artur Troian <troian.ap@gmail.com>
reuvenpo pushed a commit to scrtlabs/cosmos-sdk that referenced this issue Feb 21, 2022
fixes cosmos#10996

Signed-off-by: Artur Troian <troian.ap@gmail.com>
@alexanderbez alexanderbez self-assigned this May 2, 2022
@alexanderbez
Copy link
Contributor

I need additional clarification on what this issue is requesting. From what I gather, you want an interface defined and have Context#abciQuery use that interface to perform the ABCI query instead of directly using ctx.GetNode()? Is this correct?

@amaury1093
Copy link
Contributor Author

This issue was actually resolved by #11234.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: gRPC Issues and PRs related to the gRPC service and HTTP gateway. T: Client UX
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants