From b76dcf4a434db421923a1529cf3cce34266cd180 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 4 Jun 2021 08:16:56 +0200 Subject: [PATCH 01/13] Make it possible to override maximum payload of RPC --- client/cli/src/config.rs | 6 ++++++ client/executor/src/native_executor.rs | 2 +- client/rpc-servers/src/lib.rs | 8 +++++--- client/service/src/config.rs | 2 ++ client/service/src/lib.rs | 2 ++ client/service/test/src/lib.rs | 1 + client/tracing/src/block/mod.rs | 4 ++-- test-utils/test-runner/src/utils.rs | 1 + utils/browser/src/lib.rs | 1 + 9 files changed, 21 insertions(+), 6 deletions(-) diff --git a/client/cli/src/config.rs b/client/cli/src/config.rs index a21a79afe9fdb..8d6bbfa52ea46 100644 --- a/client/cli/src/config.rs +++ b/client/cli/src/config.rs @@ -365,6 +365,11 @@ pub trait CliConfiguration: Sized { Ok(Some(Vec::new())) } + /// Get maximum RPC payload. + fn rpc_max_payload_override(&self) -> Result> { + Ok(None) + } + /// Get the prometheus configuration (`None` if disabled) /// /// By default this is `None`. @@ -527,6 +532,7 @@ pub trait CliConfiguration: Sized { rpc_methods: self.rpc_methods()?, rpc_ws_max_connections: self.rpc_ws_max_connections()?, rpc_cors: self.rpc_cors(is_dev)?, + rpc_max_payload_override: self.rpc_max_payload_override()?, prometheus_config: self.prometheus_config(DCV::prometheus_listen_port())?, telemetry_endpoints, telemetry_external_transport: self.telemetry_external_transport()?, diff --git a/client/executor/src/native_executor.rs b/client/executor/src/native_executor.rs index c94088a155260..6fc34b6f1a322 100644 --- a/client/executor/src/native_executor.rs +++ b/client/executor/src/native_executor.rs @@ -291,7 +291,7 @@ impl NativeExecutor { default_heap_pages: Option, max_runtime_instances: usize, ) -> Self { - let extended = D::ExtendHostFunctions::host_functions(); + let extended = D::ExtendHostFunctions::host_functions(); let mut host_functions = sp_io::SubstrateHostFunctions::host_functions() .into_iter() // filter out any host function overrides provided. diff --git a/client/rpc-servers/src/lib.rs b/client/rpc-servers/src/lib.rs index be6abea67b055..d62d35842fa5d 100644 --- a/client/rpc-servers/src/lib.rs +++ b/client/rpc-servers/src/lib.rs @@ -28,7 +28,7 @@ use log::error; use pubsub::PubSubMetadata; /// Maximal payload accepted by RPC servers. -pub const MAX_PAYLOAD: usize = 15 * 1024 * 1024; +pub const DEFAULT_MAX_PAYLOAD: usize = 15 * 1024 * 1024; /// Default maximum number of connections for WS RPC servers. const WS_MAX_CONNECTIONS: usize = 100; @@ -81,6 +81,7 @@ mod inner { addr: &std::net::SocketAddr, cors: Option<&Vec>, io: RpcHandler, + max_payload: Option, ) -> io::Result { http::ServerBuilder::new(io) .threads(4) @@ -92,7 +93,7 @@ mod inner { http::RestApi::Unsecure }) .cors(map_cors::(cors)) - .max_request_body_size(MAX_PAYLOAD) + .max_request_body_size(max_payload.unwrap_or(DEFAULT_MAX_PAYLOAD)) .start_http(addr) } @@ -121,9 +122,10 @@ mod inner { max_connections: Option, cors: Option<&Vec>, io: RpcHandler, + maybe_max_payload: Option, ) -> io::Result { ws::ServerBuilder::with_meta_extractor(io, |context: &ws::RequestContext| context.sender().into()) - .max_payload(MAX_PAYLOAD) + .max_payload(maybe_max_payload.unwrap_or(DEFAULT_MAX_PAYLOAD)) .max_connections(max_connections.unwrap_or(WS_MAX_CONNECTIONS)) .allowed_origins(map_cors(cors)) .allowed_hosts(hosts_filtering(cors.is_some())) diff --git a/client/service/src/config.rs b/client/service/src/config.rs index 5d8ee89225cb8..becf9c4ebca7d 100644 --- a/client/service/src/config.rs +++ b/client/service/src/config.rs @@ -93,6 +93,8 @@ pub struct Configuration { pub rpc_cors: Option>, /// RPC methods to expose (by default only a safe subset or all of them). pub rpc_methods: RpcMethods, + /// Maximum payload of rpc request/responses. + pub rpc_max_payload_override: Option, /// Prometheus endpoint configuration. `None` if disabled. pub prometheus_config: Option, /// Telemetry service URL. `None` if disabled. diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs index ae2cfbc8b8941..2c17a29756422 100644 --- a/client/service/src/lib.rs +++ b/client/service/src/lib.rs @@ -386,6 +386,7 @@ fn start_rpc_servers< deny_unsafe(&address, &config.rpc_methods), sc_rpc_server::RpcMiddleware::new(rpc_metrics.clone(), "http") ), + config.rpc_max_payload_override ), )?.map(|s| waiting::HttpServer(Some(s))), maybe_start_server( @@ -398,6 +399,7 @@ fn start_rpc_servers< deny_unsafe(&address, &config.rpc_methods), sc_rpc_server::RpcMiddleware::new(rpc_metrics.clone(), "ws") ), + config.rpc_max_payload_override ), )?.map(|s| waiting::WsServer(Some(s))), ))) diff --git a/client/service/test/src/lib.rs b/client/service/test/src/lib.rs index a80c53a8c21c5..128e2d5b95414 100644 --- a/client/service/test/src/lib.rs +++ b/client/service/test/src/lib.rs @@ -264,6 +264,7 @@ fn node_config BlockExecutor tracing::debug!(target: "state_tracing", "Captured {} spans and {} events", spans.len(), events.len()); let approx_payload_size = BASE_PAYLOAD + events.len() * AVG_EVENT + spans.len() * AVG_SPAN; - let response = if approx_payload_size > MAX_PAYLOAD { + let response = if approx_payload_size > DEFAULT_MAX_PAYLOAD { // TODO TraceBlockResponse::TraceError(TraceError { error: "Payload likely exceeds max payload size of RPC server.".to_string() diff --git a/test-utils/test-runner/src/utils.rs b/test-utils/test-runner/src/utils.rs index d8ab3860f28a8..d29ded49d1789 100644 --- a/test-utils/test-runner/src/utils.rs +++ b/test-utils/test-runner/src/utils.rs @@ -126,6 +126,7 @@ pub fn default_config(task_executor: TaskExecutor, mut chain_spec: Box Date: Fri, 4 Jun 2021 20:28:13 +0200 Subject: [PATCH 02/13] Finish it. --- client/cli/src/commands/run_cmd.rs | 31 +++++++++++++++++++----------- client/rpc-servers/src/lib.rs | 16 +++++++++------ 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/client/cli/src/commands/run_cmd.rs b/client/cli/src/commands/run_cmd.rs index 9ef14cfa02b82..60f14a4d39a7a 100644 --- a/client/cli/src/commands/run_cmd.rs +++ b/client/cli/src/commands/run_cmd.rs @@ -42,12 +42,11 @@ pub struct RunCmd { /// The node will be started with the authority role and actively /// participate in any consensus task that it can (e.g. depending on /// availability of local keys). - #[structopt( - long = "validator" - )] + #[structopt(long = "validator")] pub validator: bool, - /// Disable GRANDPA voter when running in validator mode, otherwise disable the GRANDPA observer. + /// Disable GRANDPA voter when running in validator mode, otherwise disable the GRANDPA + /// observer. #[structopt(long)] pub no_grandpa: bool, @@ -57,8 +56,8 @@ pub struct RunCmd { /// Listen to all RPC interfaces. /// - /// Default is local. Note: not all RPC methods are safe to be exposed publicly. Use an RPC proxy - /// server to filter out dangerous methods. More details: + /// Default is local. Note: not all RPC methods are safe to be exposed publicly. Use an RPC + /// proxy server to filter out dangerous methods. More details: /// . /// Use `--unsafe-rpc-external` to suppress the warning if you understand the risks. #[structopt(long = "rpc-external")] @@ -74,8 +73,8 @@ pub struct RunCmd { /// /// - `Unsafe`: Exposes every RPC method. /// - `Safe`: Exposes only a safe subset of RPC methods, denying unsafe RPC methods. - /// - `Auto`: Acts as `Safe` if RPC is served externally, e.g. when `--{rpc,ws}-external` is passed, - /// otherwise acts as `Unsafe`. + /// - `Auto`: Acts as `Safe` if RPC is served externally, e.g. when `--{rpc,ws}-external` is + /// passed, otherwise acts as `Unsafe`. #[structopt( long, value_name = "METHOD SET", @@ -88,8 +87,8 @@ pub struct RunCmd { /// Listen to all Websocket interfaces. /// - /// Default is local. Note: not all RPC methods are safe to be exposed publicly. Use an RPC proxy - /// server to filter out dangerous methods. More details: . + /// Default is local. Note: not all RPC methods are safe to be exposed publicly. Use an RPC + /// proxy server to filter out dangerous methods. More details: . /// Use `--unsafe-ws-external` to suppress the warning if you understand the risks. #[structopt(long = "ws-external")] pub ws_external: bool, @@ -100,6 +99,11 @@ pub struct RunCmd { #[structopt(long = "unsafe-ws-external")] pub unsafe_ws_external: bool, + /// Override the the maximum payload of RPC server (both http and ws) request and response, in + /// megabytes. Default is 15MiB if unset. + #[structopt(long = "rpc-max-payload")] + pub rpc_max_payload_override: Option, + /// Listen to all Prometheus data source interfaces. /// /// Default is local. @@ -190,7 +194,8 @@ pub struct RunCmd { #[structopt(long, conflicts_with_all = &["alice", "charlie", "dave", "eve", "ferdie", "one", "two"])] pub bob: bool, - /// Shortcut for `--name Charlie --validator` with session keys for `Charlie` added to keystore. + /// Shortcut for `--name Charlie --validator` with session keys for `Charlie` added to + /// keystore. #[structopt(long, conflicts_with_all = &["alice", "bob", "dave", "eve", "ferdie", "one", "two"])] pub charlie: bool, @@ -427,6 +432,10 @@ impl CliConfiguration for RunCmd { Ok(self.rpc_methods.into()) } + fn rpc_max_payload_override(&self) -> Result> { + Ok(self.rpc_max_payload_override) + } + fn transaction_pool(&self) -> Result { Ok(self.pool_config.transaction_pool()) } diff --git a/client/rpc-servers/src/lib.rs b/client/rpc-servers/src/lib.rs index d62d35842fa5d..82c2d7ca3fdb2 100644 --- a/client/rpc-servers/src/lib.rs +++ b/client/rpc-servers/src/lib.rs @@ -27,8 +27,10 @@ use jsonrpc_core::{IoHandlerExtension, MetaIoHandler}; use log::error; use pubsub::PubSubMetadata; +const MEGABYTE: usize = 1024 * 1024; + /// Maximal payload accepted by RPC servers. -pub const DEFAULT_MAX_PAYLOAD: usize = 15 * 1024 * 1024; +pub const DEFAULT_MAX_PAYLOAD: usize = 15 * MEGABYTE; /// Default maximum number of connections for WS RPC servers. const WS_MAX_CONNECTIONS: usize = 100; @@ -81,7 +83,7 @@ mod inner { addr: &std::net::SocketAddr, cors: Option<&Vec>, io: RpcHandler, - max_payload: Option, + maybe_max_payload_mb: Option, ) -> io::Result { http::ServerBuilder::new(io) .threads(4) @@ -93,7 +95,7 @@ mod inner { http::RestApi::Unsecure }) .cors(map_cors::(cors)) - .max_request_body_size(max_payload.unwrap_or(DEFAULT_MAX_PAYLOAD)) + .max_request_body_size(maybe_max_payload_mb.map(|mb| mb * MEGABYTE).unwrap_or(DEFAULT_MAX_PAYLOAD)) .start_http(addr) } @@ -117,15 +119,17 @@ mod inner { /// Start WS server listening on given address. /// /// **Note**: Only available if `not(target_os = "unknown")`. - pub fn start_ws>> ( + pub fn start_ws< + M: pubsub::PubSubMetadata + From>, + >( addr: &std::net::SocketAddr, max_connections: Option, cors: Option<&Vec>, io: RpcHandler, - maybe_max_payload: Option, + maybe_max_payload_mb: Option, ) -> io::Result { ws::ServerBuilder::with_meta_extractor(io, |context: &ws::RequestContext| context.sender().into()) - .max_payload(maybe_max_payload.unwrap_or(DEFAULT_MAX_PAYLOAD)) + .max_payload(maybe_max_payload_mb.map(|mb| mb * MEGABYTE).unwrap_or(DEFAULT_MAX_PAYLOAD)) .max_connections(max_connections.unwrap_or(WS_MAX_CONNECTIONS)) .allowed_origins(map_cors(cors)) .allowed_hosts(hosts_filtering(cors.is_some())) From 52e474ae667a179d713b623dddb6013a5402e548 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 4 Jun 2021 20:31:17 +0200 Subject: [PATCH 03/13] remove todo. --- client/tracing/src/block/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/tracing/src/block/mod.rs b/client/tracing/src/block/mod.rs index e57243f40118f..2d1b13445e14d 100644 --- a/client/tracing/src/block/mod.rs +++ b/client/tracing/src/block/mod.rs @@ -269,7 +269,7 @@ impl BlockExecutor tracing::debug!(target: "state_tracing", "Captured {} spans and {} events", spans.len(), events.len()); let approx_payload_size = BASE_PAYLOAD + events.len() * AVG_EVENT + spans.len() * AVG_SPAN; - let response = if approx_payload_size > DEFAULT_MAX_PAYLOAD { // TODO + let response = if approx_payload_size > DEFAULT_MAX_PAYLOAD { TraceBlockResponse::TraceError(TraceError { error: "Payload likely exceeds max payload size of RPC server.".to_string() From d4779966ac586d1f1959f3fd15bc1285cd64b085 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 4 Jun 2021 22:02:36 +0200 Subject: [PATCH 04/13] Update client/cli/src/commands/run_cmd.rs --- client/cli/src/commands/run_cmd.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/cli/src/commands/run_cmd.rs b/client/cli/src/commands/run_cmd.rs index 60f14a4d39a7a..c86c9b85fc90d 100644 --- a/client/cli/src/commands/run_cmd.rs +++ b/client/cli/src/commands/run_cmd.rs @@ -42,7 +42,7 @@ pub struct RunCmd { /// The node will be started with the authority role and actively /// participate in any consensus task that it can (e.g. depending on /// availability of local keys). - #[structopt(long = "validator")] + #[structopt(long)] pub validator: bool, /// Disable GRANDPA voter when running in validator mode, otherwise disable the GRANDPA From 2bde5a731f7ef06174725286df3cb8c239575a5b Mon Sep 17 00:00:00 2001 From: Zeke Mostov <32168567+emostov@users.noreply.github.com> Date: Wed, 16 Jun 2021 16:57:14 -0700 Subject: [PATCH 05/13] Apply suggestions from code review Co-authored-by: David --- client/cli/src/commands/run_cmd.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/client/cli/src/commands/run_cmd.rs b/client/cli/src/commands/run_cmd.rs index c86c9b85fc90d..8f2c3ffefef01 100644 --- a/client/cli/src/commands/run_cmd.rs +++ b/client/cli/src/commands/run_cmd.rs @@ -99,10 +99,10 @@ pub struct RunCmd { #[structopt(long = "unsafe-ws-external")] pub unsafe_ws_external: bool, - /// Override the the maximum payload of RPC server (both http and ws) request and response, in - /// megabytes. Default is 15MiB if unset. + /// Set the the maximum RPC payload size for both requests and responses (both http and ws), in + /// megabytes. Default is 15MiB. #[structopt(long = "rpc-max-payload")] - pub rpc_max_payload_override: Option, + pub rpc_max_payload: Option, /// Listen to all Prometheus data source interfaces. /// @@ -432,8 +432,8 @@ impl CliConfiguration for RunCmd { Ok(self.rpc_methods.into()) } - fn rpc_max_payload_override(&self) -> Result> { - Ok(self.rpc_max_payload_override) + fn rpc_max_payload(&self) -> Result> { + Ok(self.rpc_max_payload) } fn transaction_pool(&self) -> Result { From dda8b09f064bb745572529e5cc99792dc1e2aeac Mon Sep 17 00:00:00 2001 From: Zeke Mostov <32168567+emostov@users.noreply.github.com> Date: Wed, 16 Jun 2021 17:41:10 -0700 Subject: [PATCH 06/13] Apply suggestions from code review Co-authored-by: David --- client/rpc-servers/src/lib.rs | 2 +- client/tracing/src/block/mod.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/client/rpc-servers/src/lib.rs b/client/rpc-servers/src/lib.rs index 82c2d7ca3fdb2..668e6b89bc394 100644 --- a/client/rpc-servers/src/lib.rs +++ b/client/rpc-servers/src/lib.rs @@ -30,7 +30,7 @@ use pubsub::PubSubMetadata; const MEGABYTE: usize = 1024 * 1024; /// Maximal payload accepted by RPC servers. -pub const DEFAULT_MAX_PAYLOAD: usize = 15 * MEGABYTE; +pub const RPC_MAX_PAYLOAD_DEFAULT: usize = 15 * MEGABYTE; /// Default maximum number of connections for WS RPC servers. const WS_MAX_CONNECTIONS: usize = 100; diff --git a/client/tracing/src/block/mod.rs b/client/tracing/src/block/mod.rs index 2d1b13445e14d..6af4efaedf51f 100644 --- a/client/tracing/src/block/mod.rs +++ b/client/tracing/src/block/mod.rs @@ -23,7 +23,7 @@ use tracing::{Dispatch, dispatcher, Subscriber, Level, span::{Attributes, Record use tracing_subscriber::CurrentSpan; use sc_client_api::BlockBackend; -use sc_rpc_server::DEFAULT_MAX_PAYLOAD; +use sc_rpc_server::MAX_PAYLOAD_DEFAULT; use sp_api::{Core, Metadata, ProvideRuntimeApi, Encode}; use sp_blockchain::HeaderBackend; use sp_runtime::{ @@ -269,7 +269,7 @@ impl BlockExecutor tracing::debug!(target: "state_tracing", "Captured {} spans and {} events", spans.len(), events.len()); let approx_payload_size = BASE_PAYLOAD + events.len() * AVG_EVENT + spans.len() * AVG_SPAN; - let response = if approx_payload_size > DEFAULT_MAX_PAYLOAD { + let response = if approx_payload_size > MAX_PAYLOAD_DEFAULT { TraceBlockResponse::TraceError(TraceError { error: "Payload likely exceeds max payload size of RPC server.".to_string() From ce04e831ba23574919bd5b4a7f76172a7708dac8 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 16 Jun 2021 18:03:46 -0700 Subject: [PATCH 07/13] Incorporate suggestions --- client/cli/src/config.rs | 4 ++-- client/rpc-servers/src/lib.rs | 4 ++-- client/service/src/config.rs | 2 +- client/service/src/lib.rs | 4 ++-- client/service/test/src/lib.rs | 2 +- client/tracing/src/block/mod.rs | 4 ++-- test-utils/test-runner/src/utils.rs | 2 +- utils/browser/src/lib.rs | 2 +- 8 files changed, 12 insertions(+), 12 deletions(-) diff --git a/client/cli/src/config.rs b/client/cli/src/config.rs index 8d6bbfa52ea46..8ff9747150f81 100644 --- a/client/cli/src/config.rs +++ b/client/cli/src/config.rs @@ -366,7 +366,7 @@ pub trait CliConfiguration: Sized { } /// Get maximum RPC payload. - fn rpc_max_payload_override(&self) -> Result> { + fn rpc_max_payload(&self) -> Result> { Ok(None) } @@ -532,7 +532,7 @@ pub trait CliConfiguration: Sized { rpc_methods: self.rpc_methods()?, rpc_ws_max_connections: self.rpc_ws_max_connections()?, rpc_cors: self.rpc_cors(is_dev)?, - rpc_max_payload_override: self.rpc_max_payload_override()?, + rpc_max_payload: self.rpc_max_payload()?, prometheus_config: self.prometheus_config(DCV::prometheus_listen_port())?, telemetry_endpoints, telemetry_external_transport: self.telemetry_external_transport()?, diff --git a/client/rpc-servers/src/lib.rs b/client/rpc-servers/src/lib.rs index 668e6b89bc394..c0cabbb3dd128 100644 --- a/client/rpc-servers/src/lib.rs +++ b/client/rpc-servers/src/lib.rs @@ -95,7 +95,7 @@ mod inner { http::RestApi::Unsecure }) .cors(map_cors::(cors)) - .max_request_body_size(maybe_max_payload_mb.map(|mb| mb * MEGABYTE).unwrap_or(DEFAULT_MAX_PAYLOAD)) + .max_request_body_size(maybe_max_payload_mb.map(|mb| mb * MEGABYTE).unwrap_or(RPC_MAX_PAYLOAD_DEFAULT)) .start_http(addr) } @@ -129,7 +129,7 @@ mod inner { maybe_max_payload_mb: Option, ) -> io::Result { ws::ServerBuilder::with_meta_extractor(io, |context: &ws::RequestContext| context.sender().into()) - .max_payload(maybe_max_payload_mb.map(|mb| mb * MEGABYTE).unwrap_or(DEFAULT_MAX_PAYLOAD)) + .max_payload(maybe_max_payload_mb.map(|mb| mb.saturating_mul(MEGABYTE)).unwrap_or(RPC_MAX_PAYLOAD_DEFAULT)) .max_connections(max_connections.unwrap_or(WS_MAX_CONNECTIONS)) .allowed_origins(map_cors(cors)) .allowed_hosts(hosts_filtering(cors.is_some())) diff --git a/client/service/src/config.rs b/client/service/src/config.rs index becf9c4ebca7d..8cef9f8f3cf35 100644 --- a/client/service/src/config.rs +++ b/client/service/src/config.rs @@ -94,7 +94,7 @@ pub struct Configuration { /// RPC methods to expose (by default only a safe subset or all of them). pub rpc_methods: RpcMethods, /// Maximum payload of rpc request/responses. - pub rpc_max_payload_override: Option, + pub rpc_max_payload: Option, /// Prometheus endpoint configuration. `None` if disabled. pub prometheus_config: Option, /// Telemetry service URL. `None` if disabled. diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs index 2c17a29756422..c760922470b6d 100644 --- a/client/service/src/lib.rs +++ b/client/service/src/lib.rs @@ -386,7 +386,7 @@ fn start_rpc_servers< deny_unsafe(&address, &config.rpc_methods), sc_rpc_server::RpcMiddleware::new(rpc_metrics.clone(), "http") ), - config.rpc_max_payload_override + config.rpc_max_payload ), )?.map(|s| waiting::HttpServer(Some(s))), maybe_start_server( @@ -399,7 +399,7 @@ fn start_rpc_servers< deny_unsafe(&address, &config.rpc_methods), sc_rpc_server::RpcMiddleware::new(rpc_metrics.clone(), "ws") ), - config.rpc_max_payload_override + config.rpc_max_payload ), )?.map(|s| waiting::WsServer(Some(s))), ))) diff --git a/client/service/test/src/lib.rs b/client/service/test/src/lib.rs index 128e2d5b95414..6218bacc9dda3 100644 --- a/client/service/test/src/lib.rs +++ b/client/service/test/src/lib.rs @@ -264,7 +264,7 @@ fn node_config BlockExecutor tracing::debug!(target: "state_tracing", "Captured {} spans and {} events", spans.len(), events.len()); let approx_payload_size = BASE_PAYLOAD + events.len() * AVG_EVENT + spans.len() * AVG_SPAN; - let response = if approx_payload_size > MAX_PAYLOAD_DEFAULT { + let response = if approx_payload_size > RPC_MAX_PAYLOAD_DEFAULT { TraceBlockResponse::TraceError(TraceError { error: "Payload likely exceeds max payload size of RPC server.".to_string() diff --git a/test-utils/test-runner/src/utils.rs b/test-utils/test-runner/src/utils.rs index d29ded49d1789..1619d361ea3a3 100644 --- a/test-utils/test-runner/src/utils.rs +++ b/test-utils/test-runner/src/utils.rs @@ -126,7 +126,7 @@ pub fn default_config(task_executor: TaskExecutor, mut chain_spec: Box Date: Wed, 16 Jun 2021 18:59:07 -0700 Subject: [PATCH 08/13] Thread rpc_max_payload from configuration to trace_block --- client/rpc-servers/src/lib.rs | 4 +++- client/rpc/src/state/mod.rs | 7 +++++-- client/rpc/src/state/state_full.rs | 19 +++++++++++++------ client/service/src/builder.rs | 1 + client/tracing/src/block/mod.rs | 10 ++++++++-- 5 files changed, 30 insertions(+), 11 deletions(-) diff --git a/client/rpc-servers/src/lib.rs b/client/rpc-servers/src/lib.rs index 42749a3d6972d..9472ee758271e 100644 --- a/client/rpc-servers/src/lib.rs +++ b/client/rpc-servers/src/lib.rs @@ -89,6 +89,8 @@ mod inner { io: RpcHandler, maybe_max_payload_mb: Option, ) -> io::Result { + let max_request_body_size = maybe_max_payload_mb.map(|mb| mb.saturating_mul(MEGABYTE)) + .unwrap_or(RPC_MAX_PAYLOAD_DEFAULT); http::ServerBuilder::new(io) .threads(thread_pool_size.unwrap_or(HTTP_THREADS)) .health_api(("/health", "system_health")) @@ -99,7 +101,7 @@ mod inner { http::RestApi::Unsecure }) .cors(map_cors::(cors)) - .max_request_body_size(maybe_max_payload_mb.map(|mb| mb * MEGABYTE).unwrap_or(RPC_MAX_PAYLOAD_DEFAULT)) + .max_request_body_size(max_request_body_size) .start_http(addr) } diff --git a/client/rpc/src/state/mod.rs b/client/rpc/src/state/mod.rs index 803fc6797ee9a..ad9712a41db6b 100644 --- a/client/rpc/src/state/mod.rs +++ b/client/rpc/src/state/mod.rs @@ -182,6 +182,7 @@ pub fn new_full( client: Arc, subscriptions: SubscriptionManager, deny_unsafe: DenyUnsafe, + rpc_max_payload: Option, ) -> (State, ChildState) where Block: BlockT + 'static, @@ -193,9 +194,11 @@ pub fn new_full( Client::Api: Metadata, { let child_backend = Box::new( - self::state_full::FullState::new(client.clone(), subscriptions.clone()) + self::state_full::FullState::new( + client.clone(), subscriptions.clone(), rpc_max_payload + ) ); - let backend = Box::new(self::state_full::FullState::new(client, subscriptions)); + let backend = Box::new(self::state_full::FullState::new(client, subscriptions, rpc_max_payload)); (State { backend, deny_unsafe }, ChildState { backend: child_backend }) } diff --git a/client/rpc/src/state/state_full.rs b/client/rpc/src/state/state_full.rs index bea7ddfbb3b76..4572c2de9ea74 100644 --- a/client/rpc/src/state/state_full.rs +++ b/client/rpc/src/state/state_full.rs @@ -67,7 +67,8 @@ struct QueryStorageRange { pub struct FullState { client: Arc, subscriptions: SubscriptionManager, - _phantom: PhantomData<(BE, Block)> + _phantom: PhantomData<(BE, Block)>, + rpc_max_payload: Option } impl FullState @@ -78,8 +79,12 @@ impl FullState Block: BlockT + 'static, { /// Create new state API backend for full nodes. - pub fn new(client: Arc, subscriptions: SubscriptionManager) -> Self { - Self { client, subscriptions, _phantom: PhantomData } + pub fn new( + client: Arc, + subscriptions: SubscriptionManager, + rpc_max_payload: Option + ) -> Self { + Self { client, subscriptions, _phantom: PhantomData, rpc_max_payload } } /// Returns given block hash or best block hash if None is passed. @@ -541,9 +546,11 @@ impl StateBackend for FullState, ) -> FutureResult { Box::new(result( - sc_tracing::block::BlockExecutor::new(self.client.clone(), block, targets, storage_keys) - .trace_block() - .map_err(|e| invalid_block::(block, None, e.to_string())) + sc_tracing::block::BlockExecutor::new( + self.client.clone(), block, targets, storage_keys, self.rpc_max_payload + ) + .trace_block() + .map_err(|e| invalid_block::(block, None, e.to_string())) )) } } diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index ebf600b12f020..ca22322798463 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -804,6 +804,7 @@ fn gen_handler( client.clone(), subscriptions.clone(), deny_unsafe, + config.rpc_max_payload, ); (chain, state, child_state) }; diff --git a/client/tracing/src/block/mod.rs b/client/tracing/src/block/mod.rs index 04257b4ca00e2..ca176f5eb2076 100644 --- a/client/tracing/src/block/mod.rs +++ b/client/tracing/src/block/mod.rs @@ -54,6 +54,7 @@ const DEFAULT_TARGETS: &str = "pallet,frame,state"; const TRACE_TARGET: &str = "block_trace"; // The name of a field required for all events. const REQUIRED_EVENT_FIELD: &str = "method"; +const MEGABYTE: usize = 1024 * 1024; /// Tracing Block Result type alias pub type TraceBlockResult = Result; @@ -174,6 +175,7 @@ pub struct BlockExecutor { block: Block::Hash, targets: Option, storage_keys: Option, + rpc_max_payload: usize, } impl BlockExecutor @@ -189,8 +191,12 @@ impl BlockExecutor block: Block::Hash, targets: Option, storage_keys: Option, + rpc_max_payload: Option, ) -> Self { - Self { client, block, targets, storage_keys } + let rpc_max_payload = rpc_max_payload.map(|mb| mb.saturating_mul(MEGABYTE)) + .unwrap_or(RPC_MAX_PAYLOAD_DEFAULT); + + Self { client, block, targets, storage_keys, rpc_max_payload } } /// Execute block, record all spans and events belonging to `Self::targets` @@ -260,7 +266,7 @@ impl BlockExecutor tracing::debug!(target: "state_tracing", "Captured {} spans and {} events", spans.len(), events.len()); let approx_payload_size = BASE_PAYLOAD + events.len() * AVG_EVENT + spans.len() * AVG_SPAN; - let response = if approx_payload_size > RPC_MAX_PAYLOAD_DEFAULT { + let response = if approx_payload_size > self.rpc_max_payload { TraceBlockResponse::TraceError(TraceError { error: "Payload likely exceeds max payload size of RPC server.".to_string() From ba7ecdbbf0240f8c3631cfcf1053f9cef26b63bf Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 16 Jun 2021 19:10:04 -0700 Subject: [PATCH 09/13] Try obey line gitlab/check_line_width.sh --- client/cli/src/commands/run_cmd.rs | 3 ++- client/rpc-servers/src/lib.rs | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/client/cli/src/commands/run_cmd.rs b/client/cli/src/commands/run_cmd.rs index 2fd25258dca25..285ffc9fdca16 100644 --- a/client/cli/src/commands/run_cmd.rs +++ b/client/cli/src/commands/run_cmd.rs @@ -88,7 +88,8 @@ pub struct RunCmd { /// Listen to all Websocket interfaces. /// /// Default is local. Note: not all RPC methods are safe to be exposed publicly. Use an RPC - /// proxy server to filter out dangerous methods. More details: . + /// proxy server to filter out dangerous methods. More details: + /// . /// Use `--unsafe-ws-external` to suppress the warning if you understand the risks. #[structopt(long = "ws-external")] pub ws_external: bool, diff --git a/client/rpc-servers/src/lib.rs b/client/rpc-servers/src/lib.rs index 9472ee758271e..c93451e5cc678 100644 --- a/client/rpc-servers/src/lib.rs +++ b/client/rpc-servers/src/lib.rs @@ -134,8 +134,10 @@ mod inner { io: RpcHandler, maybe_max_payload_mb: Option, ) -> io::Result { + let rpc_max_payload = maybe_max_payload_mb.map(|mb| mb.saturating_mul(MEGABYTE)) + .unwrap_or(RPC_MAX_PAYLOAD_DEFAULT); ws::ServerBuilder::with_meta_extractor(io, |context: &ws::RequestContext| context.sender().into()) - .max_payload(maybe_max_payload_mb.map(|mb| mb.saturating_mul(MEGABYTE)).unwrap_or(RPC_MAX_PAYLOAD_DEFAULT)) + .max_payload(rpc_max_payload) .max_connections(max_connections.unwrap_or(WS_MAX_CONNECTIONS)) .allowed_origins(map_cors(cors)) .allowed_hosts(hosts_filtering(cors.is_some())) From 668832921e8b9e44eedf8f8ac6a65a94ed1e050f Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 16 Jun 2021 19:23:24 -0700 Subject: [PATCH 10/13] update state rpc tests --- client/rpc/src/state/tests.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/client/rpc/src/state/tests.rs b/client/rpc/src/state/tests.rs index cfc27c7bf525e..e413827552c9d 100644 --- a/client/rpc/src/state/tests.rs +++ b/client/rpc/src/state/tests.rs @@ -63,6 +63,7 @@ fn should_return_storage() { Arc::new(client), SubscriptionManager::new(Arc::new(TaskExecutor)), DenyUnsafe::No, + None, ); let key = StorageKey(KEY.to_vec()); @@ -105,6 +106,7 @@ fn should_return_child_storage() { client, SubscriptionManager::new(Arc::new(TaskExecutor)), DenyUnsafe::No, + None, ); let child_key = prefixed_storage_key(); let key = StorageKey(b"key".to_vec()); @@ -144,6 +146,7 @@ fn should_call_contract() { client, SubscriptionManager::new(Arc::new(TaskExecutor)), DenyUnsafe::No, + None, ); assert_matches!( @@ -162,6 +165,7 @@ fn should_notify_about_storage_changes() { client.clone(), SubscriptionManager::new(Arc::new(TaskExecutor)), DenyUnsafe::No, + None, ); api.subscribe_storage(Default::default(), subscriber, None.into()); @@ -200,6 +204,7 @@ fn should_send_initial_storage_changes_and_notifications() { client.clone(), SubscriptionManager::new(Arc::new(TaskExecutor)), DenyUnsafe::No, + None, ); let alice_balance_key = blake2_256(&runtime::system::balance_of_key(AccountKeyring::Alice.into())); @@ -242,6 +247,7 @@ fn should_query_storage() { client.clone(), SubscriptionManager::new(Arc::new(TaskExecutor)), DenyUnsafe::No, + None, ); let mut add_block = |nonce| { @@ -463,6 +469,7 @@ fn should_return_runtime_version() { client.clone(), SubscriptionManager::new(Arc::new(TaskExecutor)), DenyUnsafe::No, + None, ); let result = "{\"specName\":\"test\",\"implName\":\"parity-test\",\"authoringVersion\":1,\ @@ -490,6 +497,7 @@ fn should_notify_on_runtime_version_initially() { client.clone(), SubscriptionManager::new(Arc::new(TaskExecutor)), DenyUnsafe::No, + None, ); api.subscribe_runtime_version(Default::default(), subscriber); From a6d69de3bdcb5ceb2ee6d5bce05c8f9f1ca32f09 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 16 Jun 2021 20:04:52 -0700 Subject: [PATCH 11/13] Improve readbility --- client/rpc/src/state/state_full.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/client/rpc/src/state/state_full.rs b/client/rpc/src/state/state_full.rs index 4572c2de9ea74..f74c74defa79c 100644 --- a/client/rpc/src/state/state_full.rs +++ b/client/rpc/src/state/state_full.rs @@ -545,12 +545,16 @@ impl StateBackend for FullState, storage_keys: Option, ) -> FutureResult { + let block_executor = sc_tracing::block::BlockExecutor::new( + self.client.clone(), + block, + targets, + storage_keys, + self.rpc_max_payload, + ); Box::new(result( - sc_tracing::block::BlockExecutor::new( - self.client.clone(), block, targets, storage_keys, self.rpc_max_payload - ) - .trace_block() - .map_err(|e| invalid_block::(block, None, e.to_string())) + block_executor.trace_block() + .map_err(|e| invalid_block::(block, None, e.to_string())) )) } } From 13877298957e2a5520b3e363142f249f558f4d6e Mon Sep 17 00:00:00 2001 From: Zeke Mostov <32168567+emostov@users.noreply.github.com> Date: Wed, 16 Jun 2021 20:10:02 -0700 Subject: [PATCH 12/13] Apply suggestions from code review --- client/rpc/src/state/state_full.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/rpc/src/state/state_full.rs b/client/rpc/src/state/state_full.rs index f74c74defa79c..218cb35f0086e 100644 --- a/client/rpc/src/state/state_full.rs +++ b/client/rpc/src/state/state_full.rs @@ -68,7 +68,7 @@ pub struct FullState { client: Arc, subscriptions: SubscriptionManager, _phantom: PhantomData<(BE, Block)>, - rpc_max_payload: Option + rpc_max_payload: Option, } impl FullState @@ -82,7 +82,7 @@ impl FullState pub fn new( client: Arc, subscriptions: SubscriptionManager, - rpc_max_payload: Option + rpc_max_payload: Option, ) -> Self { Self { client, subscriptions, _phantom: PhantomData, rpc_max_payload } } From cd82ff846bc29c0398eb4d0ee3702c6597927995 Mon Sep 17 00:00:00 2001 From: Zeke Mostov <32168567+emostov@users.noreply.github.com> Date: Wed, 16 Jun 2021 20:11:46 -0700 Subject: [PATCH 13/13] Apply suggestions from code review --- client/tracing/src/block/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/client/tracing/src/block/mod.rs b/client/tracing/src/block/mod.rs index ca176f5eb2076..cd5cf1052004b 100644 --- a/client/tracing/src/block/mod.rs +++ b/client/tracing/src/block/mod.rs @@ -195,7 +195,6 @@ impl BlockExecutor ) -> Self { let rpc_max_payload = rpc_max_payload.map(|mb| mb.saturating_mul(MEGABYTE)) .unwrap_or(RPC_MAX_PAYLOAD_DEFAULT); - Self { client, block, targets, storage_keys, rpc_max_payload } }