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

adr: privval gRPC #5712

Merged
merged 15 commits into from
Dec 9, 2020
Merged

adr: privval gRPC #5712

merged 15 commits into from
Dec 9, 2020

Conversation

tac0turtle
Copy link
Contributor

@tac0turtle tac0turtle commented Nov 24, 2020

Description

There is already consensus on the migration of privval to gRPC. This is meant to document the motivation

Closes: #XXX

@tac0turtle tac0turtle self-assigned this Nov 30, 2020
@tac0turtle tac0turtle marked this pull request as ready for review December 2, 2020 12:24
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just have a few questions but on the whole it looks good

docs/architecture/adr-063-privval-grpc.md Outdated Show resolved Hide resolved
docs/architecture/adr-063-privval-grpc.md Show resolved Hide resolved
docs/architecture/adr-063-privval-grpc.md Outdated Show resolved Hide resolved
docs/architecture/adr-063-privval-grpc.md Outdated Show resolved Hide resolved
docs/architecture/adr-063-privval-grpc.md Show resolved Hide resolved
docs/architecture/adr-063-privval-grpc.md Outdated Show resolved Hide resolved
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM and within my limited knowledge, I agree with what is being proposed.

Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

docs/architecture/adr-063-privval-grpc.md Outdated Show resolved Hide resolved
docs/architecture/adr-063-privval-grpc.md Show resolved Hide resolved
docs/architecture/adr-063-privval-grpc.md Outdated Show resolved Hide resolved
@tarcieri
Copy link

tarcieri commented Dec 3, 2020

One thing I'm curious about is if it will still be feasible for remote signers to initiate the TCP connection to Tendermint, then expose a service.

I believe I've heard gRPC supports those sorts of patterns, but even if it does I'm not sure how well they'd be supported by e.g. Tonic.

@tac0turtle
Copy link
Contributor Author

One thing I'm curious about is if it will still be feasible for remote signers to initiate the TCP connection to Tendermint, then expose a service.

I believe I've heard gRPC supports those sorts of patterns, but even if it does I'm not sure how well they'd be supported by e.g. Tonic.

I haven't seen this with gRPC, do you have a link to something using it? I read about this in terms of webtransport but not gRPC (yet).

@tarcieri
Copy link

tarcieri commented Dec 3, 2020

This Stack Overflow answer suggests it isn't possible: https://stackoverflow.com/questions/30008476/can-both-ends-of-a-grpc-connection-accept-method-calls

So either the gRPC service would need to be provided by Tendermint, or the direction of all of the TCP connections would need to be reversed from what it is presently.

Comment on lines +30 to +34
service PrivValidatorAPI {
rpc GetPubKey(tendermint.proto.privval.PubKeyRequest) returns (tendermint.proto.privval.PubKeyResponse);
rpc SignVote(tendermint.proto.privval.SignVoteRequest) returns (tendermint.proto.privval.SignedVoteResponse);
rpc SignProposal(tendermint.proto.privval.SignProposalRequest) returns (tendermint.proto.privval.SignedProposalResponse);

Copy link
Contributor Author

@tac0turtle tac0turtle Dec 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This treats the remote signer as the server, fails the "Hollywood Principle", but we should make it the client. This way the remote signer isn't actually exposed to the network.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a debatable tradeoff. It might be good to loop in more stakeholders who are working on things that might integrate with remote signers, for example IBC relayers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jackzampolin do you have thoughts here? I know you recently did some work with signer servers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jleni do you have thoughts here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this in our team standup. We like the approach where Tendermint is the client. It simplifies implementation and follows client and server architecture, where the client makes requests. The attack vectors of having the remote signer exposed to the internet can be mitigated by using two-way TLS and setting up a firewall.

@tac0turtle
Copy link
Contributor Author

tac0turtle commented Dec 8, 2020

@joe-bowman do you have thoughts on the current approach and the one we are migrting to?

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

Successfully merging this pull request may close these issues.

5 participants