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

Figure out RPC client and TLS #658

Open
Mirko-von-Leipzig opened this issue Jan 31, 2025 · 4 comments
Open

Figure out RPC client and TLS #658

Mirko-von-Leipzig opened this issue Jan 31, 2025 · 4 comments
Assignees
Milestone

Comments

@Mirko-von-Leipzig
Copy link
Contributor

Our public testnet and devnet RPC endpoints now use https and therefore require clients with TLS support. In the tonic crate this means enabling one the tls related features, and the client automatically gains TLS support e.g. cargo add tonic --features tls-native-roots.

Some users are using the gRPC client from our rpc crate -- however this crate does not enable TLS because internally our infrastructure is still http only. This means users of this client need to additionally override tonic with the TLS features. This is obscure and the client errors aren't very helpful in pointing out the issue since its just a connection rejection.

RPC client

We have a few options to improve this. Firstly though we should decide what the public interface of the rpc crate should be. The primary goal of the crate is to provide the RPC component of the node -- and despite the name, this is not meant as the canonical RPC client. Its intended to be the node's RPC server component.

Our options here:

  1. Remove the RPC client from the rpc crate. Users should generate their own client using rpc-proto.
  2. (1) but also add the client generation to the rpc-proto crate.
  3. Keep as is.

TLS features

If we go with (2) or (3) from above then we should also improve the TLS situation to make it easier to get right. Some options:

  1. Only document the cargo add tonic .. trick.
  2. Add proxy features and choose what the default should be to minimize surprises.

There are three tonic TLS features:

  • tls: Enables the rustls based TLS options for the transport feature. Not enabled by default.
  • tls-native-roots: Adds system trust roots to rustls-based gRPC clients using the rustls-native-certs crate. Not enabled by default.
  • tls-webpki-roots: Add the standard trust roots from the webpki-roots crate to rustls-based gRPC clients. Not enabled

The latter two automatically configure TLS using the system or the mozilla certificate stores respectively. I'm unsure how these features combine, if at all. The former allows users to configure their own certificate (and is enabled as part of the other two as well).

@igamigo
Copy link
Collaborator

igamigo commented Jan 31, 2025

  1. Remove the RPC client from the rpc crate. Users should generate their own client using rpc-proto.

A good alternative for replacing this may also be to use the client's RPC (enabled with the tonic feature) since it already is meant to be used against the network nodes. They can just use the RPC client and ignore other features.

@bobbinth
Copy link
Contributor

  1. Remove the RPC client from the rpc crate. Users should generate their own client using rpc-proto.

A good alternative for replacing this may also be to use the client's RPC (enabled with the tonic feature) since it already is meant to be used against the network nodes. They can just use the RPC client and ignore other features.

Agree that this could be a good solution especially since it already handles conversion of some of the auto-generated types into miden-base types. But we'd need to make sure we cover all of the endpoints available on the node's RPC component. For example, I don't believe we currently expose GetBlockByNumber endpoint from there.

Our options here:

  1. Remove the RPC client from the rpc crate. Users should generate their own client using rpc-proto.
  2. (1) but also add the client generation to the rpc-proto crate.
  3. Keep as is.

I think we should definitely do (1) - unless it causes some issues, and if (2) is negligible amount of work, we could do this too.

@bobbinth bobbinth added this to the v0.8 milestone Feb 10, 2025
@igamigo
Copy link
Collaborator

igamigo commented Feb 13, 2025

  1. Remove the RPC client from the rpc crate. Users should generate their own client using rpc-proto.

A good alternative for replacing this may also be to use the client's RPC (enabled with the tonic feature) since it already is meant to be used against the network nodes. They can just use the RPC client and ignore other features.

Agree that this could be a good solution especially since it already handles conversion of some of the auto-generated types into miden-base types. But we'd need to make sure we cover all of the endpoints available on the node's RPC component. For example, I don't believe we currently expose GetBlockByNumber endpoint from there.

Should we implement this (and any missing endpoint) on the client regardless of the solution for this issue?

@bobbinth
Copy link
Contributor

Should we implement this (and any missing endpoint) on the client regardless of the solution for this issue?

Yes! Let's do that!

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

No branches or pull requests

4 participants