From 08b176d54d42644850c995b2812e967837ab5349 Mon Sep 17 00:00:00 2001 From: Niklas Adolfsson Date: Tue, 21 Mar 2023 16:25:33 +0100 Subject: [PATCH] refactor: state RPC remove `max_rpc_payload_size` (#13649) This limit is not needed anymore as the JSON-RPC servers doesn't terminate the connection if the RPC max limit is exceeded anymore --- client/rpc/src/state/mod.rs | 10 ++---- client/rpc/src/state/state_full.rs | 10 ++---- client/rpc/src/state/tests.rs | 24 +++++++------- client/service/src/builder.rs | 8 ++--- client/tracing/src/block/mod.rs | 53 +++++++----------------------- 5 files changed, 30 insertions(+), 75 deletions(-) diff --git a/client/rpc/src/state/mod.rs b/client/rpc/src/state/mod.rs index 12d59ad3b382d..f81ec991db133 100644 --- a/client/rpc/src/state/mod.rs +++ b/client/rpc/src/state/mod.rs @@ -169,7 +169,6 @@ pub fn new_full( client: Arc, executor: SubscriptionTaskExecutor, deny_unsafe: DenyUnsafe, - rpc_max_payload: Option, ) -> (State, ChildState) where Block: BlockT + 'static, @@ -189,12 +188,9 @@ where + 'static, Client::Api: Metadata, { - let child_backend = Box::new(self::state_full::FullState::new( - client.clone(), - executor.clone(), - rpc_max_payload, - )); - let backend = Box::new(self::state_full::FullState::new(client, executor, rpc_max_payload)); + let child_backend = + Box::new(self::state_full::FullState::new(client.clone(), executor.clone())); + let backend = Box::new(self::state_full::FullState::new(client, executor)); (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 f26d42484f24f..20ca5f7131e71 100644 --- a/client/rpc/src/state/state_full.rs +++ b/client/rpc/src/state/state_full.rs @@ -66,7 +66,6 @@ pub struct FullState { client: Arc, executor: SubscriptionTaskExecutor, _phantom: PhantomData<(BE, Block)>, - rpc_max_payload: Option, } impl FullState @@ -79,12 +78,8 @@ where Block: BlockT + 'static, { /// Create new state API backend for full nodes. - pub fn new( - client: Arc, - executor: SubscriptionTaskExecutor, - rpc_max_payload: Option, - ) -> Self { - Self { client, executor, _phantom: PhantomData, rpc_max_payload } + pub fn new(client: Arc, executor: SubscriptionTaskExecutor) -> Self { + Self { client, executor, _phantom: PhantomData } } /// Returns given block hash or best block hash if None is passed. @@ -481,7 +476,6 @@ where targets, storage_keys, methods, - self.rpc_max_payload, ) .trace_block() .map_err(|e| invalid_block::(block, None, e.to_string())) diff --git a/client/rpc/src/state/tests.rs b/client/rpc/src/state/tests.rs index 1ccc609e4f0e4..ae193e662b0e7 100644 --- a/client/rpc/src/state/tests.rs +++ b/client/rpc/src/state/tests.rs @@ -55,7 +55,7 @@ async fn should_return_storage() { .add_extra_storage(b":map:acc2".to_vec(), vec![1, 2, 3]) .build(); let genesis_hash = client.genesis_hash(); - let (client, child) = new_full(Arc::new(client), test_executor(), DenyUnsafe::No, None); + let (client, child) = new_full(Arc::new(client), test_executor(), DenyUnsafe::No); let key = StorageKey(KEY.to_vec()); assert_eq!( @@ -103,7 +103,7 @@ async fn should_return_storage_entries() { .add_extra_child_storage(&child_info, KEY2.to_vec(), CHILD_VALUE2.to_vec()) .build(); let genesis_hash = client.genesis_hash(); - let (_client, child) = new_full(Arc::new(client), test_executor(), DenyUnsafe::No, None); + let (_client, child) = new_full(Arc::new(client), test_executor(), DenyUnsafe::No); let keys = &[StorageKey(KEY1.to_vec()), StorageKey(KEY2.to_vec())]; assert_eq!( @@ -134,7 +134,7 @@ async fn should_return_child_storage() { .build(), ); let genesis_hash = client.genesis_hash(); - let (_client, child) = new_full(client, test_executor(), DenyUnsafe::No, None); + let (_client, child) = new_full(client, test_executor(), DenyUnsafe::No); let child_key = prefixed_storage_key(); let key = StorageKey(b"key".to_vec()); @@ -165,7 +165,7 @@ async fn should_return_child_storage_entries() { .build(), ); let genesis_hash = client.genesis_hash(); - let (_client, child) = new_full(client, test_executor(), DenyUnsafe::No, None); + let (_client, child) = new_full(client, test_executor(), DenyUnsafe::No); let child_key = prefixed_storage_key(); let keys = vec![StorageKey(b"key1".to_vec()), StorageKey(b"key2".to_vec())]; @@ -196,7 +196,7 @@ async fn should_return_child_storage_entries() { async fn should_call_contract() { let client = Arc::new(substrate_test_runtime_client::new()); let genesis_hash = client.genesis_hash(); - let (client, _child) = new_full(client, test_executor(), DenyUnsafe::No, None); + let (client, _child) = new_full(client, test_executor(), DenyUnsafe::No); use jsonrpsee::{core::Error, types::error::CallError}; @@ -210,7 +210,7 @@ async fn should_call_contract() { async fn should_notify_about_storage_changes() { let mut sub = { let mut client = Arc::new(substrate_test_runtime_client::new()); - let (api, _child) = new_full(client.clone(), test_executor(), DenyUnsafe::No, None); + let (api, _child) = new_full(client.clone(), test_executor(), DenyUnsafe::No); let api_rpc = api.into_rpc(); let sub = api_rpc.subscribe("state_subscribeStorage", EmptyParams::new()).await.unwrap(); @@ -242,7 +242,7 @@ async fn should_notify_about_storage_changes() { async fn should_send_initial_storage_changes_and_notifications() { let mut sub = { let mut client = Arc::new(substrate_test_runtime_client::new()); - let (api, _child) = new_full(client.clone(), test_executor(), DenyUnsafe::No, None); + let (api, _child) = new_full(client.clone(), test_executor(), DenyUnsafe::No); let alice_balance_key = blake2_256(&runtime::system::balance_of_key(AccountKeyring::Alice.into())); @@ -278,7 +278,7 @@ async fn should_send_initial_storage_changes_and_notifications() { #[tokio::test] async fn should_query_storage() { async fn run_tests(mut client: Arc) { - let (api, _child) = new_full(client.clone(), test_executor(), DenyUnsafe::No, None); + let (api, _child) = new_full(client.clone(), test_executor(), DenyUnsafe::No); let mut add_block = |nonce| { let mut builder = client.new_block(Default::default()).unwrap(); @@ -480,7 +480,7 @@ async fn should_query_storage() { #[tokio::test] async fn should_return_runtime_version() { let client = Arc::new(substrate_test_runtime_client::new()); - let (api, _child) = new_full(client.clone(), test_executor(), DenyUnsafe::No, None); + let (api, _child) = new_full(client.clone(), test_executor(), DenyUnsafe::No); let result = "{\"specName\":\"test\",\"implName\":\"parity-test\",\"authoringVersion\":1,\ \"specVersion\":2,\"implVersion\":2,\"apis\":[[\"0xdf6acb689907609b\",4],\ @@ -501,7 +501,7 @@ async fn should_return_runtime_version() { async fn should_notify_on_runtime_version_initially() { let mut sub = { let client = Arc::new(substrate_test_runtime_client::new()); - let (api, _child) = new_full(client, test_executor(), DenyUnsafe::No, None); + let (api, _child) = new_full(client, test_executor(), DenyUnsafe::No); let api_rpc = api.into_rpc(); let sub = api_rpc @@ -530,7 +530,7 @@ fn should_deserialize_storage_key() { #[tokio::test] async fn wildcard_storage_subscriptions_are_rpc_unsafe() { let client = Arc::new(substrate_test_runtime_client::new()); - let (api, _child) = new_full(client, test_executor(), DenyUnsafe::Yes, None); + let (api, _child) = new_full(client, test_executor(), DenyUnsafe::Yes); let api_rpc = api.into_rpc(); let err = api_rpc.subscribe("state_subscribeStorage", EmptyParams::new()).await; @@ -540,7 +540,7 @@ async fn wildcard_storage_subscriptions_are_rpc_unsafe() { #[tokio::test] async fn concrete_storage_subscriptions_are_rpc_safe() { let client = Arc::new(substrate_test_runtime_client::new()); - let (api, _child) = new_full(client, test_executor(), DenyUnsafe::Yes, None); + let (api, _child) = new_full(client, test_executor(), DenyUnsafe::Yes); let api_rpc = api.into_rpc(); let key = StorageKey(STORAGE_KEY.to_vec()); diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index b69429b5a99dc..b04228e6bfc34 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -629,12 +629,8 @@ where let (chain, state, child_state) = { let chain = sc_rpc::chain::new_full(client.clone(), task_executor.clone()).into_rpc(); - let (state, child_state) = sc_rpc::state::new_full( - client.clone(), - task_executor.clone(), - deny_unsafe, - config.rpc_max_payload, - ); + let (state, child_state) = + sc_rpc::state::new_full(client.clone(), task_executor.clone(), deny_unsafe); let state = state.into_rpc(); let child_state = child_state.into_rpc(); diff --git a/client/tracing/src/block/mod.rs b/client/tracing/src/block/mod.rs index f5efadc34fb92..c0442abc125b9 100644 --- a/client/tracing/src/block/mod.rs +++ b/client/tracing/src/block/mod.rs @@ -34,38 +34,21 @@ use tracing::{ use crate::{SpanDatum, TraceEvent, Values}; use sc_client_api::BlockBackend; -use sc_rpc_server::RPC_MAX_PAYLOAD_DEFAULT; use sp_api::{Core, Encode, Metadata, ProvideRuntimeApi}; use sp_blockchain::HeaderBackend; use sp_core::hexdisplay::HexDisplay; -use sp_rpc::tracing::{BlockTrace, Span, TraceBlockResponse, TraceError}; +use sp_rpc::tracing::{BlockTrace, Span, TraceBlockResponse}; use sp_runtime::{ generic::BlockId, traits::{Block as BlockT, Header}, }; use sp_tracing::{WASM_NAME_KEY, WASM_TARGET_KEY, WASM_TRACE_IDENTIFIER}; -// Heuristic for average event size in bytes. -const AVG_EVENT: usize = 600 * 8; -// Heuristic for average span size in bytes. -const AVG_SPAN: usize = 100 * 8; -// Estimate of the max base RPC payload size when the Id is bound as a u64. If strings -// are used for the RPC Id this may need to be adjusted. Note: The base payload -// does not include the RPC result. -// -// The estimate is based on the JSON-RPC response message which has the following format: -// `{"jsonrpc":"2.0","result":[],"id":18446744073709551615}`. -// -// We care about the total size of the payload because jsonrpc-server will simply ignore -// messages larger than `sc_rpc_server::MAX_PAYLOAD` and the caller will not get any -// response. -const BASE_PAYLOAD: usize = 100; // Default to only pallet, frame support and state related traces 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; @@ -182,7 +165,6 @@ pub struct BlockExecutor { targets: Option, storage_keys: Option, methods: Option, - rpc_max_payload: usize, } impl BlockExecutor @@ -203,12 +185,8 @@ where targets: Option, storage_keys: Option, methods: Option, - rpc_max_payload: Option, ) -> 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, methods, rpc_max_payload } + Self { client, block, targets, storage_keys, methods } } /// Execute block, record all spans and events belonging to `Self::targets` @@ -289,24 +267,15 @@ where .collect(); 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 > self.rpc_max_payload { - TraceBlockResponse::TraceError(TraceError { - error: "Payload likely exceeds max payload size of RPC server.".to_string(), - }) - } else { - TraceBlockResponse::BlockTrace(BlockTrace { - block_hash: block_id_as_string(BlockId::::Hash(self.block)), - parent_hash: block_id_as_string(BlockId::::Hash(parent_hash)), - tracing_targets: targets.to_string(), - storage_keys: self.storage_keys.clone().unwrap_or_default(), - methods: self.methods.clone().unwrap_or_default(), - spans, - events, - }) - }; - - Ok(response) + Ok(TraceBlockResponse::BlockTrace(BlockTrace { + block_hash: block_id_as_string(BlockId::::Hash(self.block)), + parent_hash: block_id_as_string(BlockId::::Hash(parent_hash)), + tracing_targets: targets.to_string(), + storage_keys: self.storage_keys.clone().unwrap_or_default(), + methods: self.methods.clone().unwrap_or_default(), + spans, + events, + })) } }