wip: enable propagation of custom rpc builder stack layers #7976

smatthewenglish · 2024-04-29T22:06:09Z

here's all the info i've collected about this task...

if you want to continue, then we can continue with flattening it even more or support configuring additional middleware layers

the endgoal for this is that we can propgate custom Layers through the rpc-builder stack

if you look at how the node-builder api currently works, it would be great to plug in a layer.

reth/examples/cli-extension-event-hooks/src/main.rs

fn main() {
    Cli::parse_args()
        .run(|builder, _| async move {
            let handle = builder
                .node(EthereumNode::default())
                .on_node_started(|_ctx| {
                    println!("Node started");
                    Ok(())
                })
                .on_rpc_started(|_ctx, _handles| {
                    println!("RPC started");
                    Ok(())
                })
                .on_component_initialized(|_ctx| {
                    println!("All components initialized");
                    Ok(())
                })
                .launch()
                .await?;

            handle.wait_for_node_exit().await
        })
        .unwrap();
}

Ok yeah, that makes sense to me. We’d parse the arguments out into an option layer.

Is it that there would be a set of possible layers we could intake that would be specified with different flags, or more like they would be generic and we’d figure out once they’re injested how to handle them

what I'm looking for is something similar to the tower::ServiceBuilder<T> so we can install additional layers when we build the rpc server

for example here is where we launch the thing

reth/crates/node-builder/src/rpc.rs

let server_config = config.rpc.rpc_server_config();
let launch_rpc = modules.clone().start_server(server_config).map_ok(|handle| {
    if let Some(url) = handle.ipc_endpoint() {
        info!(target: "reth::cli", url=%url, "RPC IPC server started");
    }
    if let Some(addr) = handle.http_local_addr() {
        info!(target: "reth::cli", url=%addr, "RPC HTTP server started");
    }
    if let Some(addr) = handle.ws_local_addr() {
        info!(target: "reth::cli", url=%addr, "RPC WS server started");
    }
    handle
});

so perhaps we can extend RpcServerConfig with <M = Identity> so we can configure additional middlewares?

#[derive(Default)]
pub struct RpcServerConfig {
    /// Configs for JSON-RPC Http.
    http_server_config: Option<ServerBuilder<Identity, Identity>>,
    /// Allowed CORS Domains for http
    http_cors_domains: Option<String>,
    /// Address where to bind the http server to
    http_addr: Option<SocketAddr>,
    /// Configs for WS server
    ws_server_config: Option<ServerBuilder<Identity, Identity>>,
    /// Allowed CORS Domains for ws.
    ws_cors_domains: Option<String>,
    /// Address where to bind the ws server to
    ws_addr: Option<SocketAddr>,
    /// Configs for JSON-RPC IPC server
    ipc_server_config: Option<IpcServerBuilder>,
    /// The Endpoint where to launch the ipc server
    ipc_endpoint: Option<Endpoint>,
    /// JWT secret for authentication
    jwt_secret: Option<JwtSecret>,
}

goal should be that it's possible to plug in an additional Layer here:

reth/crates/node-builder/src/rpc.rs

let server_config = config.rpc.rpc_server_config();
let launch_rpc = modules.clone().start_server(server_config).map_ok(|handle| {
    if let Some(url) = handle.ipc_endpoint() {
        info!(target: "reth::cli", url=%url, "RPC IPC server started");
    }
    if let Some(addr) = handle.http_local_addr() {
        info!(target: "reth::cli", url=%addr, "RPC HTTP server started");
    }
    if let Some(addr) = handle.ws_local_addr() {
        info!(target: "reth::cli", url=%addr, "RPC WS server started");
    }
    handle
});

the way we currently use this, bypasses this intermediary step that uses the RpcServer type entirely

reth/crates/rpc/rpc-builder/src/lib.rs

/// Convenience function to do [RpcServerConfig::build] and [RpcServer::start] in one step
pub async fn start(self, modules: TransportRpcModules) -> Result<RpcServerHandle, RpcError> {
    self.build(&modules).await?.start(modules).await
}

the way the config is setup is basically with empty serverbuilders (Identity)

http_server_config: Option<ServerBuilder<Identity, Identity>>,
/// Allowed CORS Domains for http
http_cors_domains: Option<String>,
/// Address where to bind the http server to
http_addr: Option<SocketAddr>,
/// Configs for WS server
ws_server_config: Option<ServerBuilder<Identity, Identity>>,
/// Allowed CORS Domains for ws.
ws_cors_domains: Option<String>,
/// Address where to bind the ws server to
ws_addr: Option<SocketAddr>,
/// Configs for JSON-RPC IPC server
ipc_server_config: Option<IpcServerBuilder<Identity, Identity>>,

I think what we could do is something like

RpcServerConfig<Http =Identity, Rpc=Identity> {
   http: Http,
   rpc: Rpc
}`

Support the installation additional layers when building the RPC server, with the intention of enabling the propagation of custom layers thru the RPC builder stack.

Analogous to tower::ServiceBuilder<T>.

Ultimately will involve an update to rpc_server_config functions.

The resulting code should incorporate the intermediary RpcServer type for setup.

This code is an initial attempt at a simplification of RpcServerConfig with the intention of generalizing associated operations.

mattsse · 2024-05-02T09:28:49Z

crates/rpc/rpc-builder/src/lib.rs

-    http_server_config: Option<ServerBuilder<Identity, Identity>>,
-    /// Allowed CORS Domains for http
-    http_cors_domains: Option<String>,
-    /// Address where to bind the http server to
-    http_addr: Option<SocketAddr>,
-    /// Configs for WS server
-    ws_server_config: Option<ServerBuilder<Identity, Identity>>,
-    /// Allowed CORS Domains for ws.
-    ws_cors_domains: Option<String>,
-    /// Address where to bind the ws server to
-    ws_addr: Option<SocketAddr>,
-    /// Configs for JSON-RPC IPC server
-    ipc_server_config: Option<IpcServerBuilder<Identity, Identity>>,


we should keep all of those and instead use the additional HTTP and rpc fields for all ServerBuilders that were installed, this makes it to configure these layers for all servers

need to require Http: Clone, Rpc:Clone on start

would that be in launch_rpc_servers?

i added some kind of sketch of using it with an .option_layer(config.rpc.clone()) but that was a little bit of a stab in the dark, i'm trying to pick back up the thread of this pr rn

smatthewenglish · 2024-06-02T16:27:36Z

crates/rpc/rpc-builder/src/lib.rs

@@ -1497,50 +1497,60 @@ where
 /// Once the [RpcModule] is built via [RpcModuleBuilder] the servers can be started, See also
 /// [ServerBuilder::build] and [Server::start](jsonrpsee::server::Server::start).
 #[derive(Default, Debug)]
-pub struct RpcServerConfig {
+pub struct RpcServerConfig<Http = Identity, Rpc = Identity, Endpoint = String> 


@mattsse i think this is closer to what you had in mind, is that right?

yeah but we need a simpler way to configure this for server configs at the same time, so we don't support configuring different layers for different server types, because this would be too complex.

we can either store the layers as separate fields and keep identity for all servers or add helper functions that apply the layer for all the servers.

mattsse · 2024-06-03T12:19:45Z

crates/rpc/rpc-builder/src/lib.rs

@@ -1497,50 +1497,60 @@ where
 /// Once the [RpcModule] is built via [RpcModuleBuilder] the servers can be started, See also
 /// [ServerBuilder::build] and [Server::start](jsonrpsee::server::Server::start).
 #[derive(Default, Debug)]
-pub struct RpcServerConfig {
+pub struct RpcServerConfig<Http = Identity, Rpc = Identity, Endpoint = String> 


yeah but we need a simpler way to configure this for server configs at the same time, so we don't support configuring different layers for different server types, because this would be too complex.

we can either store the layers as separate fields and keep identity for all servers or add helper functions that apply the layer for all the servers.

mattsse · 2024-06-03T12:19:53Z

crates/rpc/rpc-builder/src/lib.rs

@@ -1497,50 +1497,60 @@ where
 /// Once the [RpcModule] is built via [RpcModuleBuilder] the servers can be started, See also
 /// [ServerBuilder::build] and [Server::start](jsonrpsee::server::Server::start).
 #[derive(Default, Debug)]
-pub struct RpcServerConfig {
+pub struct RpcServerConfig<Http = Identity, Rpc = Identity, Endpoint = String> 


endpoint is not needed here

smatthewenglish · 2024-06-03T15:13:15Z

@mattsse are you saying that you want to have a more simplified/streamlined config struct like this

pub struct RpcServerConfig {
    /// Configs for JSON-RPC Http.
    server_config: Option<ServerBuilder<Identity, Identity>>,
    /// Allowed CORS Domains for server
    cors_domain: Option<String>,
    /// Address where to bind the server to
    addr: Option<SocketAddr>,
    /// JWT secret for authentication
    jwt_secret: Option<JwtSecret>,
}

and that we'd change up the helper methods like with_http or w/e to have more sophisticated matching logic to construct the right server based on the generalized config?

mattsse · 2024-06-03T15:26:51Z

this is how the RpcServerConfig looks rn?

what I think we want is a way to
a) map all config's generics like fn http_middleware<HTTP>(self, ) that then applies it to all configured server configs
b) have the HTTP,rpc layers as separate fields

I think option a) is more intuitive

smatthewenglish · 2024-06-03T23:50:10Z

@mattsse right now it looks like this

pub struct RpcServerConfig {
    /// Configs for JSON-RPC Http.
    http_server_config: Option<ServerBuilder<Identity, Identity>>,
    /// Allowed CORS Domains for http
    http_cors_domains: Option<String>,
    /// Address where to bind the http server to
    http_addr: Option<SocketAddr>,
    /// Configs for WS server
    ws_server_config: Option<ServerBuilder<Identity, Identity>>,
    /// Allowed CORS Domains for ws.
    ws_cors_domains: Option<String>,
    /// Address where to bind the ws server to
    ws_addr: Option<SocketAddr>,
    /// Configs for JSON-RPC IPC server
    ipc_server_config: Option<IpcServerBuilder<Identity, Identity>>,
    /// The Endpoint where to launch the ipc server
    ipc_endpoint: Option<String>,
    /// JWT secret for authentication
    jwt_secret: Option<JwtSecret>,
}

i think i get what you're talking about, it's kinda building on the work i did in the last pr of making the rpc start up/building process more generalizable, thanks for sticking with me as i figure this out, i'm trying my best to understand what you're looking for 😅

so putting it all together, this is what i'm thinking about...

high level objective is working towards integrating additional layers in the rpc server. goal should be that it's possible to plug in an additional Layer here, in this code:

    let server_config = config.rpc.rpc_server_config();
    let launch_rpc = modules.clone().start_server(server_config).map_ok(|handle| {
        if let Some(url) = handle.ipc_endpoint() {
            info!(target: "reth::cli", url=%url, "RPC IPC server started");
        }
        if let Some(addr) = handle.http_local_addr() {
            info!(target: "reth::cli", url=%addr, "RPC HTTP server started");
        }
        if let Some(addr) = handle.ws_local_addr() {
            info!(target: "reth::cli", url=%addr, "RPC WS server started");
        }
        handle
    });

    let launch_auth = auth_module.clone().start_server(auth_config).map_ok(|handle| {

^which i guess means that we have a more generic method for getting the server build, that isn't tightly coupled to http, ws and ipc - is that right?

you mentioned that "the way we currently use this, bypasses this intermediary step that uses the RpcServer type entirely
"

are we trying to eliminate the RpcServer type?

you mentioned that the way the config is setup is basically with empty serverbuilders (Identity), and that we could something like

`RpcServerConfig<Http =Identity, Rpc=Identity> {
   http: Http,
   rpc: Rpc
}`

i think thats why my comment earlier was in the direction of making things maximally generic, in my interpretation, is that directionally correct but too far for right now?

one of the things thats confusing me a little i think is the options you outlined. could you maybe help me zero in on option a) 😅

mattsse · 2024-06-20T20:27:55Z

crates/node-core/src/args/rpc_server.rs

additional args are out of scope for this pr

mattsse · 2024-06-20T20:35:08Z

crates/rpc/rpc-builder/src/lib.rs

+    /// Additional middleware layer.
+    additional_middleware: Option<L>,


this is the right approach to configure the service for all servers

if we look at the ServerBuilder::set_http_middleware function then this accepts a http_middleware: tower::ServiceBuilder<T>,

so we can do the same here, starting with tower::ServiceBuilder<L>, then we also don't need to restrict the L generic above

mattsse · 2024-06-20T20:35:42Z

crates/rpc/rpc-builder/src/lib.rs

+    ///
+    /// - `M`: The type of the middleware layer, which must implement `tower::Layer<L> + Send + Sync
+    ///   + `static`.
+    pub fn with_additional_middleware<M>(self, layer: M) -> RpcServerConfig<M>


this should accept tower::ServiceBuilder<T>, and replace the current one

mattsse · 2024-06-20T20:45:12Z

crates/rpc/rpc-builder/src/lib.rs

+        builder: RpcServerConfig<L>,
+    ) -> Result<RpcServerHandle, RpcError>
+    where
+        L: tower::Layer<Identity> + Send + Sync + 'static,


this generic will be a bit wilder, because of the restriction jsonrpsee has, see for jsonrpsee::Server::start

I assume there will be some complications for ipc vs http/ws, so lets only focus on HTTP and ws first.

we could also support configuring the middleware for each transport individually but with 2 generics per transport this would be a bit of a mess I assume

perhaps it would be simpler to tackle the RpcMiddlerware type first which is the same for all

RpcMiddleware: tower::Layer<RpcService> + Clone + Send + 'static,

and installed via set_rpc_middleware<T>(self, rpc_middleware: RpcServiceBuilder<T>)

that sounds like a great plan! I'll take a crack at it 💪

so I got it to build as it is now - but i'm running into problems when i try to add additional layers like this

// Initialize the ServiceBuilder let mut service_builder = tower::ServiceBuilder::new(); // Conditionally add logging middleware if self.enable_logging { let trace_layer = tower_http::trace::TraceLayer::new_for_http(); service_builder = service_builder.layer(trace_layer); } // Conditionally add authentication middleware if self.enable_auth { let auth_layer = tower_http::auth::AddAuthorizationLayer::basic("username", "password"); service_builder = service_builder.layer(auth_layer); }

or even this

if self.enable_logging { let trace_layer = tower_http::trace::TraceLayer::new_for_http(); let service_builder = tower::ServiceBuilder::new().layer(trace_layer); config = config.with_additional_middleware(service_builder); } // Conditionally add authentication middleware if self.enable_auth { let auth_layer = tower_http::auth::AddAuthorizationLayer::basic("username", "password"); let service_builder = tower::ServiceBuilder::new().layer(auth_layer); config = config.with_additional_middleware(service_builder); }

'cause the compiler is expecting a certain ServiceBuilder/middleware configuration - and then it's getting changed

what do you think is the best way to resolve that issue?

maybe to create a more generalized abstraction for it? Like a middleware struct or something?

ok yeah, i'm finally looking at this jsonrpsee code

/// Ipc Server implementation /// /// This is an adapted `jsonrpsee` Server, but for `Ipc` connections. pub struct IpcServer<HttpMiddleware = Identity, RpcMiddleware = Identity> { /// The endpoint we listen for incoming transactions endpoint: String, id_provider: Arc<dyn IdProvider>, cfg: Settings, rpc_middleware: RpcServiceBuilder<RpcMiddleware>, http_middleware: tower::ServiceBuilder<HttpMiddleware>, } impl<HttpMiddleware, RpcMiddleware> IpcServer<HttpMiddleware, RpcMiddleware> { /// Returns the configured endpoint pub fn endpoint(&self) -> String { self.endpoint.clone() } } impl<HttpMiddleware, RpcMiddleware> IpcServer<HttpMiddleware, RpcMiddleware> where RpcMiddleware: for<'a> Layer<RpcService, Service: RpcServiceT<'a>> + Clone + Send + 'static, HttpMiddleware: Layer< TowerServiceNoHttp<RpcMiddleware>, Service: Service< String, Response = Option<String>, Error = Box<dyn std::error::Error + Send + Sync + 'static>, Future: Send + Unpin, > + Send, > + Send + 'static, {

i'll think this over. starting with RpcMiddlerware sounds reasonable to me, i might make a new branch for that so i can think about it freshly

smatthewenglish requested review from mattsse and Rjected as code owners April 29, 2024 22:06

smatthewenglish marked this pull request as draft April 29, 2024 22:06

mattsse requested changes May 2, 2024

View reviewed changes

emhane added the A-rpc Related to the RPC implementation label May 16, 2024

smatthewenglish mentioned this pull request May 30, 2024

fix: update broken link #8502

Merged

smatthewenglish commented Jun 2, 2024

View reviewed changes

smatthewenglish requested a review from mattsse June 2, 2024 16:58

mattsse requested changes Jun 3, 2024

View reviewed changes

smatthewenglish force-pushed the custom-rpc-layer-impl branch from 1abacb4 to 59b95d0 Compare June 4, 2024 00:29

smatthewenglish requested a review from mattsse June 4, 2024 01:04

smatthewenglish added 3 commits June 20, 2024 15:47

work

794c725

update

473be03

udpate

302f7c3

smatthewenglish force-pushed the custom-rpc-layer-impl branch from e250544 to 302f7c3 Compare June 20, 2024 20:01

mattsse requested changes Jun 20, 2024

View reviewed changes

smatthewenglish added 5 commits June 21, 2024 14:20

update

78bd370

fixing up pr

4b4fe6f

fixing up pr

dffc406

make pr

5512fc6

update

639210d

smatthewenglish requested a review from mattsse June 21, 2024 20:11

smatthewenglish added 4 commits June 24, 2024 11:07

build at enmd

45fd089

works

6cd45be

type

52a13c0

make pr

7bf7560

smatthewenglish closed this Jun 26, 2024

smatthewenglish mentioned this pull request Jun 28, 2024

refactor(rpc): remove intermediate types from rpc start up process #9180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wip: enable propagation of custom rpc builder stack layers #7976

wip: enable propagation of custom rpc builder stack layers #7976

smatthewenglish commented Apr 29, 2024 •

edited

Loading

mattsse May 2, 2024 •

edited

Loading

smatthewenglish May 30, 2024

smatthewenglish Jun 2, 2024

mattsse Jun 3, 2024

mattsse Jun 3, 2024

mattsse Jun 3, 2024

smatthewenglish commented Jun 3, 2024

mattsse commented Jun 3, 2024

smatthewenglish commented Jun 3, 2024 •

edited

Loading

mattsse Jun 20, 2024

mattsse Jun 20, 2024

mattsse Jun 20, 2024

mattsse Jun 20, 2024

smatthewenglish Jun 20, 2024

smatthewenglish Jun 21, 2024

smatthewenglish Jun 23, 2024

		/// Additional middleware layer.
		additional_middleware: Option<L>,

wip: enable propagation of custom rpc builder stack layers #7976

wip: enable propagation of custom rpc builder stack layers #7976

Conversation

smatthewenglish commented Apr 29, 2024 • edited Loading

mattsse May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smatthewenglish commented Jun 3, 2024

mattsse commented Jun 3, 2024

smatthewenglish commented Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smatthewenglish commented Apr 29, 2024 •

edited

Loading

mattsse May 2, 2024 •

edited

Loading

smatthewenglish commented Jun 3, 2024 •

edited

Loading