From e17a555850cb588cdee5cb5d52723eecd462618d Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 9 Oct 2024 10:54:51 +0300 Subject: [PATCH 01/70] archive/api: Add stroage diff method Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/api.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/api.rs b/substrate/client/rpc-spec-v2/src/archive/api.rs index b19738304000..033490bb88d4 100644 --- a/substrate/client/rpc-spec-v2/src/archive/api.rs +++ b/substrate/client/rpc-spec-v2/src/archive/api.rs @@ -19,7 +19,10 @@ //! API trait of the archive methods. use crate::{ - common::events::{ArchiveStorageResult, PaginatedStorageQuery}, + common::events::{ + ArchiveStorageDiffItem, ArchiveStorageDiffMethodResult, ArchiveStorageResult, + PaginatedStorageQuery, + }, MethodResult, }; use jsonrpsee::{core::RpcResult, proc_macros::rpc}; @@ -104,4 +107,17 @@ pub trait ArchiveApi { items: Vec>, child_trie: Option, ) -> RpcResult; + + /// Returns the storage difference between two blocks. + /// + /// # Unstable + /// + /// This method is unstable and subject to change in the future. + #[method(name = "archive_unstable_storageDiff", blocking)] + fn archive_unstable_storage_diff( + &self, + hash: Hash, + previous_hash: Option, + items: Vec>, + ) -> RpcResult; } From 4101ee69b3dbb519f6234a97ad310ee4583f319c Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 9 Oct 2024 10:55:08 +0300 Subject: [PATCH 02/70] storage/events: Add storage events and test serialization Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/common/events.rs | 169 ++++++++++++++++++ 1 file changed, 169 insertions(+) diff --git a/substrate/client/rpc-spec-v2/src/common/events.rs b/substrate/client/rpc-spec-v2/src/common/events.rs index b1627d74c844..216a2a4caf42 100644 --- a/substrate/client/rpc-spec-v2/src/common/events.rs +++ b/substrate/client/rpc-spec-v2/src/common/events.rs @@ -143,10 +143,179 @@ pub struct ArchiveStorageMethodErr { pub error: String, } +/// The type of the archive storage difference query. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub enum ArchiveStorageDiffType { + /// The result is provided as value of the key. + Value, + /// The result the hash of the value of the key. + Hash, +} + +/// The storage item to query. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct ArchiveStorageDiffItem { + /// The provided key. + pub key: Key, + /// The type of the storage query. + pub return_type: ArchiveStorageDiffType, + /// The child trie key if provided. + #[serde(skip_serializing_if = "Option::is_none")] + #[serde(default)] + pub child_trie_key: Option, +} + +/// The result of a storage difference call. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct ArchiveStorageDiffMethodResult { + /// Reported results. + pub result: Vec, +} + +/// The result of a storage difference call operation type. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub enum ArchiveStorageDiffOperationType { + /// The key is added. + Added, + /// The key is modified. + Modified, + /// The key is removed. + Deleted, +} + +/// The result of an individual storage difference key. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct ArchiveStorageDiffResult { + /// The hex-encoded key of the result. + pub key: String, + /// The result of the query. + #[serde(flatten)] + pub result: StorageResultType, + /// The operation type. + #[serde(rename = "type")] + pub operation_type: ArchiveStorageDiffOperationType, + /// The child trie key if provided. + #[serde(skip_serializing_if = "Option::is_none")] + #[serde(default)] + pub child_trie_key: Option, +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn archive_diff_input() { + // Item with Value. + let item = ArchiveStorageDiffItem { + key: "0x1", + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + }; + // Encode + let ser = serde_json::to_string(&item).unwrap(); + let exp = r#"{"key":"0x1","returnType":"value"}"#; + assert_eq!(ser, exp); + // Decode + let dec: ArchiveStorageDiffItem<&str> = serde_json::from_str(exp).unwrap(); + assert_eq!(dec, item); + + // Item with Hash. + let item = ArchiveStorageDiffItem { + key: "0x1", + return_type: ArchiveStorageDiffType::Hash, + child_trie_key: None, + }; + // Encode + let ser = serde_json::to_string(&item).unwrap(); + let exp = r#"{"key":"0x1","returnType":"hash"}"#; + assert_eq!(ser, exp); + // Decode + let dec: ArchiveStorageDiffItem<&str> = serde_json::from_str(exp).unwrap(); + assert_eq!(dec, item); + + // Item with Value and child trie key. + let item = ArchiveStorageDiffItem { + key: "0x1", + return_type: ArchiveStorageDiffType::Value, + child_trie_key: Some("0x2"), + }; + // Encode + let ser = serde_json::to_string(&item).unwrap(); + let exp = r#"{"key":"0x1","returnType":"value","childTrieKey":"0x2"}"#; + assert_eq!(ser, exp); + // Decode + let dec: ArchiveStorageDiffItem<&str> = serde_json::from_str(exp).unwrap(); + assert_eq!(dec, item); + + // Item with Hash and child trie key. + let item = ArchiveStorageDiffItem { + key: "0x1", + return_type: ArchiveStorageDiffType::Hash, + child_trie_key: Some("0x2"), + }; + // Encode + let ser = serde_json::to_string(&item).unwrap(); + let exp = r#"{"key":"0x1","returnType":"hash","childTrieKey":"0x2"}"#; + assert_eq!(ser, exp); + // Decode + let dec: ArchiveStorageDiffItem<&str> = serde_json::from_str(exp).unwrap(); + assert_eq!(dec, item); + } + + #[test] + fn archive_diff_output() { + // Item with Value. + let item = ArchiveStorageDiffResult { + key: "0x1".into(), + result: StorageResultType::Value("res".into()), + operation_type: ArchiveStorageDiffOperationType::Added, + child_trie_key: None, + }; + // Encode + let ser = serde_json::to_string(&item).unwrap(); + let exp = r#"{"key":"0x1","value":"res","type":"added"}"#; + assert_eq!(ser, exp); + // Decode + let dec: ArchiveStorageDiffResult = serde_json::from_str(exp).unwrap(); + assert_eq!(dec, item); + + // Item with Hash. + let item = ArchiveStorageDiffResult { + key: "0x1".into(), + result: StorageResultType::Hash("res".into()), + operation_type: ArchiveStorageDiffOperationType::Modified, + child_trie_key: None, + }; + // Encode + let ser = serde_json::to_string(&item).unwrap(); + let exp = r#"{"key":"0x1","hash":"res","type":"modified"}"#; + assert_eq!(ser, exp); + // Decode + let dec: ArchiveStorageDiffResult = serde_json::from_str(exp).unwrap(); + assert_eq!(dec, item); + + // Item with Hash, child trie key and removed. + let item = ArchiveStorageDiffResult { + key: "0x1".into(), + result: StorageResultType::Hash("res".into()), + operation_type: ArchiveStorageDiffOperationType::Deleted, + child_trie_key: Some("0x2".into()), + }; + // Encode + let ser = serde_json::to_string(&item).unwrap(); + let exp = r#"{"key":"0x1","hash":"res","type":"deleted","childTrieKey":"0x2"}"#; + assert_eq!(ser, exp); + // Decode + let dec: ArchiveStorageDiffResult = serde_json::from_str(exp).unwrap(); + assert_eq!(dec, item); + } + #[test] fn storage_result() { // Item with Value. From d781d67e2339a4e55ae80404aa4238d52b0b4428 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 9 Oct 2024 10:55:43 +0300 Subject: [PATCH 03/70] archive: Add initial implementation for storage diff Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/archive.rs | 102 +++++++++++++++++- 1 file changed, 101 insertions(+), 1 deletion(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index dd6c566a76ed..3be7277e440d 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -20,7 +20,14 @@ use crate::{ archive::{error::Error as ArchiveError, ArchiveApiServer}, - common::events::{ArchiveStorageResult, PaginatedStorageQuery}, + common::{ + events::{ + ArchiveStorageDiffItem, ArchiveStorageDiffMethodResult, + ArchiveStorageDiffOperationType, ArchiveStorageDiffResult, ArchiveStorageDiffType, + ArchiveStorageResult, PaginatedStorageQuery, + }, + storage::Storage, + }, hex_string, MethodResult, }; @@ -278,4 +285,97 @@ where Ok(storage_client.handle_query(hash, items, child_trie)) } + + fn archive_unstable_storage_diff( + &self, + hash: Block::Hash, + previous_hash: Option, + items: Vec>, + ) -> RpcResult { + let items = items + .into_iter() + .map(|item| { + let key = StorageKey(parse_hex_param(item.key)?); + + let child_trie_key_string = item.child_trie_key.clone(); + let child_trie_key = item + .child_trie_key + .map(|child_trie_key| parse_hex_param(child_trie_key)) + .transpose()? + .map(ChildInfo::new_default_from_vec); + Ok((key, item.return_type, child_trie_key, child_trie_key_string)) + }) + .collect::, ArchiveError>>()?; + + let previous_hash = if let Some(previous_hash) = previous_hash { + previous_hash + } else { + let Ok(Some(current_header)) = self.client.header(hash) else { + return Err(ArchiveError::InvalidParam(format!( + "Block header is not present: {}", + hash + )) + .into()); + }; + *current_header.parent_hash() + }; + + // Deduplicate the items. + let mut storage_results = Vec::new(); + + for item in items { + let (key, return_type, child_trie_key, child_trie_key_string) = item; + + let current_keys = { + if let Some(child_key) = child_trie_key.clone() { + self.client.child_storage_keys(hash, child_key, Some(&key), None) + } else { + self.client.storage_keys(hash, Some(&key), None) + } + } + .map_err(|error| ArchiveError::InvalidParam(error.to_string()))?; + + let previous_keys = { + if let Some(child_key) = child_trie_key.clone() { + self.client.child_storage_keys(previous_hash, child_key, Some(&key), None) + } else { + self.client.storage_keys(previous_hash, Some(&key), None) + } + } + .map_err(|error| ArchiveError::InvalidParam(error.to_string()))? + .collect::>(); + + let storage_client: Storage = Storage::new(self.client.clone()); + + for current_key in current_keys { + // The key is not present in the previous state. + let result = match return_type { + ArchiveStorageDiffType::Value => + storage_client.query_value(hash, ¤t_key, child_trie_key.as_ref()), + + ArchiveStorageDiffType::Hash => + storage_client.query_hash(hash, ¤t_key, child_trie_key.as_ref()), + }; + + let storage_result = match result { + Ok(Some(storage_result)) => storage_result, + Ok(None) => continue, + Err(error) => return Err(ArchiveError::InvalidParam(error.to_string()).into()), + }; + + if !previous_keys.contains(¤t_key) { + storage_results.push(ArchiveStorageDiffResult { + key: storage_result.key, + result: storage_result.result, + operation_type: ArchiveStorageDiffOperationType::Added, + child_trie_key: child_trie_key_string.clone(), + }); + + continue + } + } + } + + Ok(ArchiveStorageDiffMethodResult { result: vec![] }) + } } From 60e1a9322abf556c3d4a6eb5019a4f31d908d48c Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 9 Oct 2024 11:44:38 +0300 Subject: [PATCH 04/70] archive: Handle modified and delete cases Signed-off-by: Alexandru Vasile --- Cargo.lock | 1 + substrate/client/rpc-spec-v2/Cargo.toml | 1 + .../client/rpc-spec-v2/src/archive/archive.rs | 80 +++++++++++++++++-- 3 files changed, 76 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2747a3dece07..e8a495fb8a96 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -19552,6 +19552,7 @@ dependencies = [ "futures", "futures-util", "hex", + "itertools 0.11.0", "jsonrpsee 0.24.3", "log", "parity-scale-codec", diff --git a/substrate/client/rpc-spec-v2/Cargo.toml b/substrate/client/rpc-spec-v2/Cargo.toml index 58dd8b830beb..b9dc5fa141d4 100644 --- a/substrate/client/rpc-spec-v2/Cargo.toml +++ b/substrate/client/rpc-spec-v2/Cargo.toml @@ -42,6 +42,7 @@ log = { workspace = true, default-features = true } futures-util = { workspace = true } rand = { workspace = true, default-features = true } schnellru = { workspace = true } +itertools = { workspace = true } [dev-dependencies] jsonrpsee = { workspace = true, features = ["server", "ws-client"] } diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index 3be7277e440d..706245292cba 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -18,6 +18,7 @@ //! API implementation for `archive`. +use super::archive_storage::ArchiveStorage; use crate::{ archive::{error::Error as ArchiveError, ArchiveApiServer}, common::{ @@ -32,6 +33,7 @@ use crate::{ }; use codec::Encode; +use itertools::Itertools; use jsonrpsee::core::{async_trait, RpcResult}; use sc_client_api::{ Backend, BlockBackend, BlockchainEvents, CallExecutor, ChildInfo, ExecutorProvider, StorageKey, @@ -48,8 +50,6 @@ use sp_runtime::{ }; use std::{collections::HashSet, marker::PhantomData, sync::Arc}; -use super::archive_storage::ArchiveStorage; - /// The configuration of [`Archive`]. pub struct ArchiveConfig { /// The maximum number of items the `archive_storage` can return for a descendant query before @@ -322,6 +322,7 @@ where // Deduplicate the items. let mut storage_results = Vec::new(); + let mut storage_keys_set = HashSet::new(); for item in items { let (key, return_type, child_trie_key, child_trie_key_string) = item; @@ -335,20 +336,18 @@ where } .map_err(|error| ArchiveError::InvalidParam(error.to_string()))?; - let previous_keys = { + let mut previous_keys = { if let Some(child_key) = child_trie_key.clone() { self.client.child_storage_keys(previous_hash, child_key, Some(&key), None) } else { self.client.storage_keys(previous_hash, Some(&key), None) } } - .map_err(|error| ArchiveError::InvalidParam(error.to_string()))? - .collect::>(); + .map_err(|error| ArchiveError::InvalidParam(error.to_string()))?; let storage_client: Storage = Storage::new(self.client.clone()); for current_key in current_keys { - // The key is not present in the previous state. let result = match return_type { ArchiveStorageDiffType::Value => storage_client.query_value(hash, ¤t_key, child_trie_key.as_ref()), @@ -363,7 +362,10 @@ where Err(error) => return Err(ArchiveError::InvalidParam(error.to_string()).into()), }; + // The key is not present in the previous state. if !previous_keys.contains(¤t_key) { + storage_keys_set.insert(current_key.clone()); + storage_results.push(ArchiveStorageDiffResult { key: storage_result.key, result: storage_result.result, @@ -373,6 +375,72 @@ where continue } + + // Check for storage difference between the current and previous state. + let result = match return_type { + ArchiveStorageDiffType::Value => storage_client.query_value( + previous_hash, + ¤t_key, + child_trie_key.as_ref(), + ), + + ArchiveStorageDiffType::Hash => storage_client.query_hash( + previous_hash, + ¤t_key, + child_trie_key.as_ref(), + ), + }; + + let previous_storage_result = match result { + Ok(Some(storage_result)) => storage_result, + Ok(None) => continue, + Err(error) => return Err(ArchiveError::InvalidParam(error.to_string()).into()), + }; + + // Report the result only if the value has changed. + if storage_result.result != previous_storage_result.result { + storage_keys_set.insert(current_key.clone()); + + storage_results.push(ArchiveStorageDiffResult { + key: storage_result.key, + result: storage_result.result, + operation_type: ArchiveStorageDiffOperationType::Modified, + child_trie_key: child_trie_key_string.clone(), + }); + } + } + + // Check for removed keys. + for previous_key in previous_keys { + if !storage_keys_set.contains(&previous_key) { + let result = match return_type { + ArchiveStorageDiffType::Value => storage_client.query_value( + previous_hash, + &previous_key, + child_trie_key.as_ref(), + ), + + ArchiveStorageDiffType::Hash => storage_client.query_hash( + previous_hash, + &previous_key, + child_trie_key.as_ref(), + ), + }; + + let storage_result = match result { + Ok(Some(storage_result)) => storage_result, + Ok(None) => continue, + Err(error) => + return Err(ArchiveError::InvalidParam(error.to_string()).into()), + }; + + storage_results.push(ArchiveStorageDiffResult { + key: storage_result.key, + result: storage_result.result, + operation_type: ArchiveStorageDiffOperationType::Deleted, + child_trie_key: child_trie_key_string.clone(), + }); + } } } From 63da0319486cd114f2cb81b7fab530ac0eddb5f3 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 9 Oct 2024 14:40:35 +0300 Subject: [PATCH 05/70] archive/events: Derive more impl for diff items Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/common/events.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/rpc-spec-v2/src/common/events.rs b/substrate/client/rpc-spec-v2/src/common/events.rs index 216a2a4caf42..4d8cd91d0dd9 100644 --- a/substrate/client/rpc-spec-v2/src/common/events.rs +++ b/substrate/client/rpc-spec-v2/src/common/events.rs @@ -144,7 +144,7 @@ pub struct ArchiveStorageMethodErr { } /// The type of the archive storage difference query. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub enum ArchiveStorageDiffType { /// The result is provided as value of the key. From 25b9e5ac5a5176dbaf9e2dc0d38234129c5efe6b Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 9 Oct 2024 14:40:54 +0300 Subject: [PATCH 06/70] archive: Deduplicate input keys Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/archive.rs | 83 +++++++++++++++---- 1 file changed, 68 insertions(+), 15 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index 706245292cba..f3d8f6dc1aee 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -48,7 +48,11 @@ use sp_runtime::{ traits::{Block as BlockT, Header as HeaderT, NumberFor}, SaturatedConversion, }; -use std::{collections::HashSet, marker::PhantomData, sync::Arc}; +use std::{ + collections::{hash_map::Entry, HashMap, HashSet}, + marker::PhantomData, + sync::Arc, +}; /// The configuration of [`Archive`]. pub struct ArchiveConfig { @@ -292,20 +296,69 @@ where previous_hash: Option, items: Vec>, ) -> RpcResult { - let items = items - .into_iter() - .map(|item| { - let key = StorageKey(parse_hex_param(item.key)?); - - let child_trie_key_string = item.child_trie_key.clone(); - let child_trie_key = item - .child_trie_key - .map(|child_trie_key| parse_hex_param(child_trie_key)) - .transpose()? - .map(ChildInfo::new_default_from_vec); - Ok((key, item.return_type, child_trie_key, child_trie_key_string)) - }) - .collect::, ArchiveError>>()?; + let mut deduplicated: HashMap, Vec> = HashMap::new(); + + #[derive(Debug, PartialEq, Clone)] + struct DiffDetails { + key: StorageKey, + return_type: ArchiveStorageDiffType, + child_trie_key: Option, + child_trie_key_string: Option, + } + + for diff_item in items { + // Ensure the provided hex keys are valid before deduplication. + let key = StorageKey(parse_hex_param(diff_item.key)?); + let child_trie_key_string = diff_item.child_trie_key.clone(); + let child_trie_key = diff_item + .child_trie_key + .map(|child_trie_key| parse_hex_param(child_trie_key)) + .transpose()? + .map(ChildInfo::new_default_from_vec); + + let diff_item = DiffDetails { + key, + return_type: diff_item.return_type, + child_trie_key: child_trie_key.clone(), + child_trie_key_string, + }; + + match deduplicated.entry(child_trie_key.clone()) { + Entry::Occupied(mut entry) => { + let mut should_insert = true; + + for existing in entry.get() { + // This points to a different return type. + if existing.return_type != diff_item.return_type { + continue + } + // Keys and return types are identical. + if existing.key == diff_item.key { + should_insert = false; + break + } + // The current key is a longer prefix of the existing key. + if diff_item.key.as_ref().starts_with(&existing.key.as_ref()) { + should_insert = false; + break + } + + if diff_item.key.as_ref().starts_with(&existing.key.as_ref()) { + let to_remove = existing.clone(); + entry.get_mut().retain(|item| item != &to_remove); + break; + } + } + + if should_insert { + entry.get_mut().push(diff_item); + } + }, + Entry::Vacant(entry) => { + entry.insert(vec![diff_item]); + }, + } + } let previous_hash = if let Some(previous_hash) = previous_hash { previous_hash From 52cfaf79636beb46857aaedd159b8f1db3b4a4d3 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 9 Oct 2024 16:29:51 +0300 Subject: [PATCH 07/70] archive: Move code to different module Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/archive.rs | 220 ++-------------- .../src/archive/archive_storage.rs | 249 +++++++++++++++++- .../client/rpc-spec-v2/src/common/events.rs | 2 +- .../client/rpc-spec-v2/src/common/storage.rs | 15 ++ 4 files changed, 284 insertions(+), 202 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index f3d8f6dc1aee..4d8a29ac2917 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -18,22 +18,20 @@ //! API implementation for `archive`. -use super::archive_storage::ArchiveStorage; use crate::{ - archive::{error::Error as ArchiveError, ArchiveApiServer}, - common::{ - events::{ - ArchiveStorageDiffItem, ArchiveStorageDiffMethodResult, - ArchiveStorageDiffOperationType, ArchiveStorageDiffResult, ArchiveStorageDiffType, - ArchiveStorageResult, PaginatedStorageQuery, - }, - storage::Storage, + archive::{ + archive_storage::{ArchiveStorage, ArchiveStorageDiff}, + error::Error as ArchiveError, + ArchiveApiServer, + }, + common::events::{ + ArchiveStorageDiffItem, ArchiveStorageDiffMethodResult, ArchiveStorageResult, + PaginatedStorageQuery, }, hex_string, MethodResult, }; use codec::Encode; -use itertools::Itertools; use jsonrpsee::core::{async_trait, RpcResult}; use sc_client_api::{ Backend, BlockBackend, BlockchainEvents, CallExecutor, ChildInfo, ExecutorProvider, StorageKey, @@ -48,11 +46,7 @@ use sp_runtime::{ traits::{Block as BlockT, Header as HeaderT, NumberFor}, SaturatedConversion, }; -use std::{ - collections::{hash_map::Entry, HashMap, HashSet}, - marker::PhantomData, - sync::Arc, -}; +use std::{collections::HashSet, marker::PhantomData, sync::Arc}; /// The configuration of [`Archive`]. pub struct ArchiveConfig { @@ -296,69 +290,10 @@ where previous_hash: Option, items: Vec>, ) -> RpcResult { - let mut deduplicated: HashMap, Vec> = HashMap::new(); - - #[derive(Debug, PartialEq, Clone)] - struct DiffDetails { - key: StorageKey, - return_type: ArchiveStorageDiffType, - child_trie_key: Option, - child_trie_key_string: Option, - } - - for diff_item in items { - // Ensure the provided hex keys are valid before deduplication. - let key = StorageKey(parse_hex_param(diff_item.key)?); - let child_trie_key_string = diff_item.child_trie_key.clone(); - let child_trie_key = diff_item - .child_trie_key - .map(|child_trie_key| parse_hex_param(child_trie_key)) - .transpose()? - .map(ChildInfo::new_default_from_vec); - - let diff_item = DiffDetails { - key, - return_type: diff_item.return_type, - child_trie_key: child_trie_key.clone(), - child_trie_key_string, - }; + let storage_client = ArchiveStorageDiff::new(self.client.clone()); - match deduplicated.entry(child_trie_key.clone()) { - Entry::Occupied(mut entry) => { - let mut should_insert = true; - - for existing in entry.get() { - // This points to a different return type. - if existing.return_type != diff_item.return_type { - continue - } - // Keys and return types are identical. - if existing.key == diff_item.key { - should_insert = false; - break - } - // The current key is a longer prefix of the existing key. - if diff_item.key.as_ref().starts_with(&existing.key.as_ref()) { - should_insert = false; - break - } - - if diff_item.key.as_ref().starts_with(&existing.key.as_ref()) { - let to_remove = existing.clone(); - entry.get_mut().retain(|item| item != &to_remove); - break; - } - } - - if should_insert { - entry.get_mut().push(diff_item); - } - }, - Entry::Vacant(entry) => { - entry.insert(vec![diff_item]); - }, - } - } + // Deduplicate the items. + let trie_items = storage_client.deduplicate_items(items)?; let previous_hash = if let Some(previous_hash) = previous_hash { previous_hash @@ -373,130 +308,17 @@ where *current_header.parent_hash() }; - // Deduplicate the items. - let mut storage_results = Vec::new(); - let mut storage_keys_set = HashSet::new(); - - for item in items { - let (key, return_type, child_trie_key, child_trie_key_string) = item; - - let current_keys = { - if let Some(child_key) = child_trie_key.clone() { - self.client.child_storage_keys(hash, child_key, Some(&key), None) - } else { - self.client.storage_keys(hash, Some(&key), None) - } - } - .map_err(|error| ArchiveError::InvalidParam(error.to_string()))?; - - let mut previous_keys = { - if let Some(child_key) = child_trie_key.clone() { - self.client.child_storage_keys(previous_hash, child_key, Some(&key), None) - } else { - self.client.storage_keys(previous_hash, Some(&key), None) - } - } - .map_err(|error| ArchiveError::InvalidParam(error.to_string()))?; - - let storage_client: Storage = Storage::new(self.client.clone()); - - for current_key in current_keys { - let result = match return_type { - ArchiveStorageDiffType::Value => - storage_client.query_value(hash, ¤t_key, child_trie_key.as_ref()), - - ArchiveStorageDiffType::Hash => - storage_client.query_hash(hash, ¤t_key, child_trie_key.as_ref()), - }; - - let storage_result = match result { - Ok(Some(storage_result)) => storage_result, - Ok(None) => continue, - Err(error) => return Err(ArchiveError::InvalidParam(error.to_string()).into()), - }; - - // The key is not present in the previous state. - if !previous_keys.contains(¤t_key) { - storage_keys_set.insert(current_key.clone()); - - storage_results.push(ArchiveStorageDiffResult { - key: storage_result.key, - result: storage_result.result, - operation_type: ArchiveStorageDiffOperationType::Added, - child_trie_key: child_trie_key_string.clone(), - }); - - continue - } - - // Check for storage difference between the current and previous state. - let result = match return_type { - ArchiveStorageDiffType::Value => storage_client.query_value( - previous_hash, - ¤t_key, - child_trie_key.as_ref(), - ), - - ArchiveStorageDiffType::Hash => storage_client.query_hash( - previous_hash, - ¤t_key, - child_trie_key.as_ref(), - ), - }; - - let previous_storage_result = match result { - Ok(Some(storage_result)) => storage_result, - Ok(None) => continue, - Err(error) => return Err(ArchiveError::InvalidParam(error.to_string()).into()), - }; - - // Report the result only if the value has changed. - if storage_result.result != previous_storage_result.result { - storage_keys_set.insert(current_key.clone()); - - storage_results.push(ArchiveStorageDiffResult { - key: storage_result.key, - result: storage_result.result, - operation_type: ArchiveStorageDiffOperationType::Modified, - child_trie_key: child_trie_key_string.clone(), - }); - } - } + if trie_items.is_empty() { + let result = storage_client.handle_trie_queries(hash, previous_hash, Vec::new())?; + return Ok(ArchiveStorageDiffMethodResult { result }) + } - // Check for removed keys. - for previous_key in previous_keys { - if !storage_keys_set.contains(&previous_key) { - let result = match return_type { - ArchiveStorageDiffType::Value => storage_client.query_value( - previous_hash, - &previous_key, - child_trie_key.as_ref(), - ), - - ArchiveStorageDiffType::Hash => storage_client.query_hash( - previous_hash, - &previous_key, - child_trie_key.as_ref(), - ), - }; - - let storage_result = match result { - Ok(Some(storage_result)) => storage_result, - Ok(None) => continue, - Err(error) => - return Err(ArchiveError::InvalidParam(error.to_string()).into()), - }; - - storage_results.push(ArchiveStorageDiffResult { - key: storage_result.key, - result: storage_result.result, - operation_type: ArchiveStorageDiffOperationType::Deleted, - child_trie_key: child_trie_key_string.clone(), - }); - } - } + let mut storage_results = Vec::new(); + for trie_queries in trie_items { + let result = storage_client.handle_trie_queries(hash, previous_hash, trie_queries)?; + storage_results.extend(result); } - Ok(ArchiveStorageDiffMethodResult { result: vec![] }) + Ok(ArchiveStorageDiffMethodResult { result: storage_results }) } } diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 26e7c299de41..1a71908025c6 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -18,13 +18,23 @@ //! Implementation of the `archive_storage` method. -use std::sync::Arc; +use std::{ + collections::{hash_map::Entry, HashMap, HashSet}, + sync::Arc, +}; +use itertools::Itertools; +use jsonrpsee::core::RpcResult; use sc_client_api::{Backend, ChildInfo, StorageKey, StorageProvider}; use sp_runtime::traits::Block as BlockT; +use super::error::Error as ArchiveError; use crate::common::{ - events::{ArchiveStorageResult, PaginatedStorageQuery, StorageQueryType}, + events::{ + ArchiveStorageDiffItem, ArchiveStorageDiffOperationType, ArchiveStorageDiffResult, + ArchiveStorageDiffType, ArchiveStorageResult, PaginatedStorageQuery, StorageQueryType, + StorageResult, + }, storage::{IterQueryType, QueryIter, Storage}, }; @@ -127,3 +137,238 @@ where ArchiveStorageResult::ok(storage_results, discarded_items) } } + +/// Parse hex-encoded string parameter as raw bytes. +/// +/// If the parsing fails, returns an error propagated to the RPC method. +pub fn parse_hex_param(param: String) -> Result, ArchiveError> { + // Methods can accept empty parameters. + if param.is_empty() { + return Ok(Default::default()) + } + + array_bytes::hex2bytes(¶m).map_err(|_| ArchiveError::InvalidParam(param)) +} + +#[derive(Debug, PartialEq, Clone)] +pub struct DiffDetails { + key: StorageKey, + return_type: ArchiveStorageDiffType, + child_trie_key: Option, + child_trie_key_string: Option, +} + +pub struct ArchiveStorageDiff { + client: Storage, +} + +impl ArchiveStorageDiff { + pub fn new(client: Arc) -> Self { + Self { client: Storage::new(client) } + } +} + +impl ArchiveStorageDiff +where + Block: BlockT + 'static, + BE: Backend + 'static, + Client: StorageProvider + 'static, +{ + /// Deduplicate the provided iterms and return a list of `DiffDetails`. + /// + /// Each list corresponds to a single child trie or the main trie. + pub fn deduplicate_items( + &self, + items: Vec>, + ) -> RpcResult>> { + let mut deduplicated: HashMap, Vec> = HashMap::new(); + + for diff_item in items { + // Ensure the provided hex keys are valid before deduplication. + let key = StorageKey(parse_hex_param(diff_item.key)?); + let child_trie_key_string = diff_item.child_trie_key.clone(); + let child_trie_key = diff_item + .child_trie_key + .map(|child_trie_key| parse_hex_param(child_trie_key)) + .transpose()? + .map(ChildInfo::new_default_from_vec); + + let diff_item = DiffDetails { + key, + return_type: diff_item.return_type, + child_trie_key: child_trie_key.clone(), + child_trie_key_string, + }; + + match deduplicated.entry(child_trie_key.clone()) { + Entry::Occupied(mut entry) => { + let mut should_insert = true; + + for existing in entry.get() { + // This points to a different return type. + if existing.return_type != diff_item.return_type { + continue + } + // Keys and return types are identical. + if existing.key == diff_item.key { + should_insert = false; + break + } + // The current key is a longer prefix of the existing key. + if diff_item.key.as_ref().starts_with(&existing.key.as_ref()) { + should_insert = false; + break + } + + if diff_item.key.as_ref().starts_with(&existing.key.as_ref()) { + let to_remove = existing.clone(); + entry.get_mut().retain(|item| item != &to_remove); + break; + } + } + + if should_insert { + entry.get_mut().push(diff_item); + } + }, + Entry::Vacant(entry) => { + entry.insert(vec![diff_item]); + }, + } + } + + Ok(deduplicated.into_values().collect()) + } + + // This calls into the database. + fn fetch_storage( + &self, + hash: Block::Hash, + key: StorageKey, + maybe_child_trie: Option, + ty: ArchiveStorageDiffType, + ) -> RpcResult> { + match ty { + ArchiveStorageDiffType::Value => + self.client.query_value(hash, &key, maybe_child_trie.as_ref()), + ArchiveStorageDiffType::Hash => + self.client.query_hash(hash, &key, maybe_child_trie.as_ref()), + } + .map_err(|error| ArchiveError::InvalidParam(error).into()) + } + + // Check if the key starts with any of the provided items. + fn starts_with(key: &StorageKey, items: &[DiffDetails]) -> bool { + // User has requested all keys. + if items.is_empty() { + return true + } + + for item in items { + if key.as_ref().starts_with(&item.key.as_ref()) { + return true + } + } + + false + } + + /// It is guaranteed that all entries correspond to the same child trie or main trie. + pub fn handle_trie_queries( + &self, + hash: Block::Hash, + previous_hash: Block::Hash, + items: Vec, + ) -> RpcResult> { + let mut results = Vec::with_capacity(items.len()); + let mut keys_set = HashSet::new(); + + let maybe_child_trie = items.first().map(|item| item.child_trie_key.clone()).flatten(); + let maybe_child_trie_str = + items.first().map(|item| item.child_trie_key_string.clone()).flatten(); + let return_type = items + .first() + .map(|item| item.return_type) + .unwrap_or(ArchiveStorageDiffType::Value); + + let keys_iter = self + .client + .raw_keys_iter(hash, maybe_child_trie.clone()) + .map_err(|error| ArchiveError::InvalidParam(error))?; + + let mut previous_keys_iter = self + .client + .raw_keys_iter(previous_hash, maybe_child_trie.clone()) + .map_err(|error| ArchiveError::InvalidParam(error))?; + + for key in keys_iter { + if !Self::starts_with(&key, &items) { + continue + } + + let Some(storage_result) = + self.fetch_storage(hash, key.clone(), maybe_child_trie.clone(), return_type)? + else { + continue + }; + + // The key is not present in the previous state. + if !previous_keys_iter.contains(&key) { + keys_set.insert(key.clone()); + + results.push(ArchiveStorageDiffResult { + key: storage_result.key.clone(), + result: storage_result.result.clone(), + operation_type: ArchiveStorageDiffOperationType::Modified, + child_trie_key: maybe_child_trie_str.clone(), + }); + } + + // Report the result only if the value has changed. + let Some(previous_storage_result) = self.fetch_storage( + previous_hash, + key.clone(), + maybe_child_trie.clone(), + return_type, + )? + else { + continue + }; + if storage_result.result != previous_storage_result.result { + keys_set.insert(key.clone()); + + results.push(ArchiveStorageDiffResult { + key: storage_result.key, + result: storage_result.result, + operation_type: ArchiveStorageDiffOperationType::Modified, + child_trie_key: maybe_child_trie_str.clone(), + }); + } + } + + for previous_key in previous_keys_iter { + if !keys_set.contains(&previous_key) || !Self::starts_with(&previous_key, &items) { + continue; + } + + let Some(previous_storage_result) = self.fetch_storage( + previous_hash, + previous_key.clone(), + maybe_child_trie.clone(), + return_type, + )? + else { + continue + }; + + results.push(ArchiveStorageDiffResult { + key: previous_storage_result.key, + result: previous_storage_result.result, + operation_type: ArchiveStorageDiffOperationType::Deleted, + child_trie_key: maybe_child_trie_str.clone(), + }); + } + + Ok(results) + } +} diff --git a/substrate/client/rpc-spec-v2/src/common/events.rs b/substrate/client/rpc-spec-v2/src/common/events.rs index 4d8cd91d0dd9..78723b77f5b7 100644 --- a/substrate/client/rpc-spec-v2/src/common/events.rs +++ b/substrate/client/rpc-spec-v2/src/common/events.rs @@ -144,7 +144,7 @@ pub struct ArchiveStorageMethodErr { } /// The type of the archive storage difference query. -#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub enum ArchiveStorageDiffType { /// The result is provided as value of the key. diff --git a/substrate/client/rpc-spec-v2/src/common/storage.rs b/substrate/client/rpc-spec-v2/src/common/storage.rs index 2e24a8da8ca8..673e20b2bc78 100644 --- a/substrate/client/rpc-spec-v2/src/common/storage.rs +++ b/substrate/client/rpc-spec-v2/src/common/storage.rs @@ -248,4 +248,19 @@ where }); Ok((ret, maybe_next_query)) } + + /// Raw iterator over the keys. + pub fn raw_keys_iter( + &self, + hash: Block::Hash, + child_key: Option, + ) -> Result, String> { + let keys_iter = if let Some(child_key) = child_key { + self.client.child_storage_keys(hash, child_key, None, None) + } else { + self.client.storage_keys(hash, None, None) + }; + + keys_iter.map_err(|err| err.to_string()) + } } From ee6bed787ff93c24d8d3c68bee53fd60441415f7 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 9 Oct 2024 16:58:07 +0300 Subject: [PATCH 08/70] archive: Fetch value / hash or both Signed-off-by: Alexandru Vasile --- .../src/archive/archive_storage.rs | 156 ++++++++++++++---- 1 file changed, 121 insertions(+), 35 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 1a71908025c6..cdd5a3a3710c 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -158,6 +158,17 @@ pub struct DiffDetails { child_trie_key_string: Option, } +/// The type of storage query. +#[derive(Debug, PartialEq, Clone, Copy)] +enum FetchStorageType { + /// Only fetch the value. + Value, + /// Only fetch the hash. + Hash, + /// Fetch both the value and the hash. + Both, +} + pub struct ArchiveStorageDiff { client: Storage, } @@ -174,7 +185,7 @@ where BE: Backend + 'static, Client: StorageProvider + 'static, { - /// Deduplicate the provided iterms and return a list of `DiffDetails`. + /// Deduplicate the provided items and return a list of `DiffDetails`. /// /// Each list corresponds to a single child trie or the main trie. pub fn deduplicate_items( @@ -240,37 +251,86 @@ where Ok(deduplicated.into_values().collect()) } - // This calls into the database. + /// This calls into the database. fn fetch_storage( &self, hash: Block::Hash, key: StorageKey, maybe_child_trie: Option, - ty: ArchiveStorageDiffType, - ) -> RpcResult> { + ty: FetchStorageType, + ) -> RpcResult)>> { + let convert_err = |error| ArchiveError::InvalidParam(error); + match ty { - ArchiveStorageDiffType::Value => - self.client.query_value(hash, &key, maybe_child_trie.as_ref()), - ArchiveStorageDiffType::Hash => - self.client.query_hash(hash, &key, maybe_child_trie.as_ref()), + FetchStorageType::Value => { + let result = self + .client + .query_value(hash, &key, maybe_child_trie.as_ref()) + .map_err(convert_err)?; + + Ok(result.map(|res| (res, None))) + }, + + FetchStorageType::Hash => { + let result = self + .client + .query_hash(hash, &key, maybe_child_trie.as_ref()) + .map_err(convert_err)?; + + Ok(result.map(|res| (res, None))) + }, + + FetchStorageType::Both => { + let value = self + .client + .query_value(hash, &key, maybe_child_trie.as_ref()) + .map_err(convert_err)?; + + let Some(value) = value else { + return Ok(None); + }; + + let hash = self + .client + .query_hash(hash, &key, maybe_child_trie.as_ref()) + .map_err(convert_err)?; + + let Some(hash) = hash else { + return Ok(None); + }; + + Ok(Some((value, Some(hash)))) + }, } - .map_err(|error| ArchiveError::InvalidParam(error).into()) } - // Check if the key starts with any of the provided items. - fn starts_with(key: &StorageKey, items: &[DiffDetails]) -> bool { - // User has requested all keys. + /// Check if the key starts with any of the provided items. + /// + /// Returns a `FetchStorage` to indicate if the key should be fetched. + fn starts_with(key: &StorageKey, items: &[DiffDetails]) -> Option { + // User has requested all keys, by default this fallbacks to fetching the value. if items.is_empty() { - return true + return Some(FetchStorageType::Value) } + let mut value = false; + let mut hash = false; + for item in items { if key.as_ref().starts_with(&item.key.as_ref()) { - return true + match item.return_type { + ArchiveStorageDiffType::Value => value = true, + ArchiveStorageDiffType::Hash => hash = true, + } } } - false + match (value, hash) { + (true, true) => Some(FetchStorageType::Both), + (true, false) => Some(FetchStorageType::Value), + (false, true) => Some(FetchStorageType::Hash), + (false, false) => None, + } } /// It is guaranteed that all entries correspond to the same child trie or main trie. @@ -286,10 +346,6 @@ where let maybe_child_trie = items.first().map(|item| item.child_trie_key.clone()).flatten(); let maybe_child_trie_str = items.first().map(|item| item.child_trie_key_string.clone()).flatten(); - let return_type = items - .first() - .map(|item| item.return_type) - .unwrap_or(ArchiveStorageDiffType::Value); let keys_iter = self .client @@ -302,12 +358,12 @@ where .map_err(|error| ArchiveError::InvalidParam(error))?; for key in keys_iter { - if !Self::starts_with(&key, &items) { - continue - } + let Some(fetch_type) = Self::starts_with(&key, &items) else { + continue; + }; let Some(storage_result) = - self.fetch_storage(hash, key.clone(), maybe_child_trie.clone(), return_type)? + self.fetch_storage(hash, key.clone(), maybe_child_trie.clone(), fetch_type)? else { continue }; @@ -317,11 +373,22 @@ where keys_set.insert(key.clone()); results.push(ArchiveStorageDiffResult { - key: storage_result.key.clone(), - result: storage_result.result.clone(), - operation_type: ArchiveStorageDiffOperationType::Modified, + key: storage_result.0.key.clone(), + result: storage_result.0.result.clone(), + operation_type: ArchiveStorageDiffOperationType::Added, child_trie_key: maybe_child_trie_str.clone(), }); + + if let Some(second) = storage_result.1 { + results.push(ArchiveStorageDiffResult { + key: second.key.clone(), + result: second.result.clone(), + operation_type: ArchiveStorageDiffOperationType::Added, + child_trie_key: maybe_child_trie_str.clone(), + }); + } + + continue } // Report the result only if the value has changed. @@ -329,44 +396,63 @@ where previous_hash, key.clone(), maybe_child_trie.clone(), - return_type, + fetch_type, )? else { continue }; - if storage_result.result != previous_storage_result.result { + + if storage_result.0.result != previous_storage_result.0.result { keys_set.insert(key.clone()); results.push(ArchiveStorageDiffResult { - key: storage_result.key, - result: storage_result.result, + key: storage_result.0.key.clone(), + result: storage_result.0.result.clone(), operation_type: ArchiveStorageDiffOperationType::Modified, child_trie_key: maybe_child_trie_str.clone(), }); + + if let Some(second) = storage_result.1 { + results.push(ArchiveStorageDiffResult { + key: second.key.clone(), + result: second.result.clone(), + operation_type: ArchiveStorageDiffOperationType::Modified, + child_trie_key: maybe_child_trie_str.clone(), + }); + } } } for previous_key in previous_keys_iter { - if !keys_set.contains(&previous_key) || !Self::starts_with(&previous_key, &items) { + let Some(fetch_type) = Self::starts_with(&previous_key, &items) else { continue; - } + }; let Some(previous_storage_result) = self.fetch_storage( previous_hash, previous_key.clone(), maybe_child_trie.clone(), - return_type, + fetch_type, )? else { continue }; results.push(ArchiveStorageDiffResult { - key: previous_storage_result.key, - result: previous_storage_result.result, + key: previous_storage_result.0.key, + result: previous_storage_result.0.result, operation_type: ArchiveStorageDiffOperationType::Deleted, child_trie_key: maybe_child_trie_str.clone(), }); + + if let Some(second) = previous_storage_result.1 { + results.push(ArchiveStorageDiffResult { + key: second.key.clone(), + result: second.result.clone(), + operation_type: ArchiveStorageDiffOperationType::Deleted, + child_trie_key: maybe_child_trie_str.clone(), + }); + } } Ok(results) From 8948e749fc7636c8cdbe5bc5a00a5efa3ff3b364 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 4 Nov 2024 18:40:32 +0200 Subject: [PATCH 09/70] events: Add subscription events for ArchiveStorageDiffEvent Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/common/events.rs | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/substrate/client/rpc-spec-v2/src/common/events.rs b/substrate/client/rpc-spec-v2/src/common/events.rs index 78723b77f5b7..72b328aab0d2 100644 --- a/substrate/client/rpc-spec-v2/src/common/events.rs +++ b/substrate/client/rpc-spec-v2/src/common/events.rs @@ -176,7 +176,7 @@ pub struct ArchiveStorageDiffMethodResult { } /// The result of a storage difference call operation type. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub enum ArchiveStorageDiffOperationType { /// The key is added. @@ -205,6 +205,24 @@ pub struct ArchiveStorageDiffResult { pub child_trie_key: Option, } +/// The event generated by the `archive_storageDiff` method. +/// +/// The `archive_storageDiff` can generate the following events: +/// - `storageDiff` event - generated when a `ArchiveStorageDiffResult` is produced. +/// - `storageDiffError` event - generated when an error is produced. +/// - `storageDiffDone` event - generated when the `archive_storageDiff` method completed. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +#[serde(tag = "event")] +pub enum ArchiveStorageDiffEvent { + /// The `storageDiff` event. + StorageDiff(ArchiveStorageDiffResult), + /// The `storageDiffError` event. + StorageDiffError(ArchiveStorageMethodErr), + /// The `storageDiffDone` event. + StorageDiffDone, +} + #[cfg(test)] mod tests { use super::*; From fbfb0dc03b6da4693a509b085a260c846cd95df9 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 4 Nov 2024 18:40:48 +0200 Subject: [PATCH 10/70] archive: Make archive_storageDiff a subscription Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/archive/api.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/api.rs b/substrate/client/rpc-spec-v2/src/archive/api.rs index 033490bb88d4..1e5f6c8d10e0 100644 --- a/substrate/client/rpc-spec-v2/src/archive/api.rs +++ b/substrate/client/rpc-spec-v2/src/archive/api.rs @@ -20,7 +20,7 @@ use crate::{ common::events::{ - ArchiveStorageDiffItem, ArchiveStorageDiffMethodResult, ArchiveStorageResult, + ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageResult, PaginatedStorageQuery, }, MethodResult, @@ -113,11 +113,15 @@ pub trait ArchiveApi { /// # Unstable /// /// This method is unstable and subject to change in the future. - #[method(name = "archive_unstable_storageDiff", blocking)] + #[subscription( + name = "archive_unstable_storageDiff" => "archive_unstable_storageDiffEvent", + unsubscribe = "archive_unstable_storageDiff_stopStorageDiff", + item = ArchiveStorageDiffEvent, + )] fn archive_unstable_storage_diff( &self, hash: Hash, previous_hash: Option, items: Vec>, - ) -> RpcResult; + ); } From f7b33edd932edd70186348c5ab5555ac0fee6d09 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 4 Nov 2024 18:41:09 +0200 Subject: [PATCH 11/70] archive: Modify storageDiff to leverage subscription backpressure Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/archive.rs | 127 +++++++++--- .../src/archive/archive_storage.rs | 181 ++++++++++-------- 2 files changed, 199 insertions(+), 109 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index 4d8a29ac2917..563705427349 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -25,18 +25,23 @@ use crate::{ ArchiveApiServer, }, common::events::{ - ArchiveStorageDiffItem, ArchiveStorageDiffMethodResult, ArchiveStorageResult, + ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageResult, PaginatedStorageQuery, }, - hex_string, MethodResult, + hex_string, MethodResult, SubscriptionTaskExecutor, }; use codec::Encode; -use jsonrpsee::core::{async_trait, RpcResult}; +use futures::FutureExt; +use jsonrpsee::{ + core::{async_trait, RpcResult}, + PendingSubscriptionSink, +}; use sc_client_api::{ Backend, BlockBackend, BlockchainEvents, CallExecutor, ChildInfo, ExecutorProvider, StorageKey, StorageProvider, }; +use sc_rpc::utils::Subscription; use sp_api::{CallApiAt, CallContext}; use sp_blockchain::{ Backend as BlockChainBackend, Error as BlockChainError, HeaderBackend, HeaderMetadata, @@ -48,6 +53,8 @@ use sp_runtime::{ }; use std::{collections::HashSet, marker::PhantomData, sync::Arc}; +use tokio::sync::mpsc; + /// The configuration of [`Archive`]. pub struct ArchiveConfig { /// The maximum number of items the `archive_storage` can return for a descendant query before @@ -69,6 +76,12 @@ const MAX_DESCENDANT_RESPONSES: usize = 5; /// `MAX_DESCENDANT_RESPONSES`. const MAX_QUERIED_ITEMS: usize = 8; +/// The buffer capacity for each storage query. +/// +/// This is small because the underlying JSON-RPC server has +/// its down buffer capacity per connection as well. +const STORAGE_QUERY_BUF: usize = 16; + impl Default for ArchiveConfig { fn default() -> Self { Self { @@ -84,6 +97,8 @@ pub struct Archive, Block: BlockT, Client> { client: Arc, /// Backend of the chain. backend: Arc, + /// Executor to spawn subscriptions. + executor: SubscriptionTaskExecutor, /// The hexadecimal encoded hash of the genesis block. genesis_hash: String, /// The maximum number of items the `archive_storage` can return for a descendant query before @@ -101,12 +116,14 @@ impl, Block: BlockT, Client> Archive { client: Arc, backend: Arc, genesis_hash: GenesisHash, + executor: SubscriptionTaskExecutor, config: ArchiveConfig, ) -> Self { let genesis_hash = hex_string(&genesis_hash.as_ref()); Self { client, backend, + executor, genesis_hash, storage_max_descendant_responses: config.max_descendant_responses, storage_max_queried_items: config.max_queried_items, @@ -286,39 +303,97 @@ where fn archive_unstable_storage_diff( &self, + pending: PendingSubscriptionSink, hash: Block::Hash, previous_hash: Option, items: Vec>, - ) -> RpcResult { + ) { let storage_client = ArchiveStorageDiff::new(self.client.clone()); + let client = self.client.clone(); + + let fut = async move { + // Deduplicate the items. + let trie_items = match storage_client.deduplicate_items(items) { + Ok(items) => items, + Err(error) => { + pending.reject(error).await; + return + }, + }; - // Deduplicate the items. - let trie_items = storage_client.deduplicate_items(items)?; - - let previous_hash = if let Some(previous_hash) = previous_hash { - previous_hash - } else { - let Ok(Some(current_header)) = self.client.header(hash) else { - return Err(ArchiveError::InvalidParam(format!( - "Block header is not present: {}", - hash - )) - .into()); + let previous_hash = if let Some(previous_hash) = previous_hash { + previous_hash + } else { + let Ok(Some(current_header)) = client.header(hash) else { + pending + .reject(ArchiveError::InvalidParam(format!( + "Block header is not present: {}", + hash + ))) + .await; + + return + }; + *current_header.parent_hash() }; - *current_header.parent_hash() + + let Ok(mut sink) = pending.accept().await.map(Subscription::from) else { return }; + + let (tx, mut rx) = tokio::sync::mpsc::channel(STORAGE_QUERY_BUF); + if trie_items.is_empty() { + // May fail if the channel is closed or the connection is closed. + // which is okay to ignore. + let result = futures::future::join( + storage_client.handle_trie_queries(hash, previous_hash, Vec::new(), tx), + process_events(&mut rx, &mut sink), + ) + .await; + + // Send `StorageDiffDone` if the events were processed successfully. + if result.1 { + let _ = sink.send(&ArchiveStorageDiffEvent::StorageDiffDone).await; + } + } else { + for trie_queries in trie_items { + let storage_fut = storage_client.handle_trie_queries( + hash, + previous_hash, + trie_queries, + tx.clone(), + ); + let result = + futures::future::join(storage_fut, process_events(&mut rx, &mut sink)) + .await; + if !result.1 { + return; + } + } + + let _ = sink.send(&ArchiveStorageDiffEvent::StorageDiffDone).await; + } }; - if trie_items.is_empty() { - let result = storage_client.handle_trie_queries(hash, previous_hash, Vec::new())?; - return Ok(ArchiveStorageDiffMethodResult { result }) - } + self.executor.spawn("substrate-rpc-subscription", Some("rpc"), fut.boxed()); + } +} + +/// Returns true if the events where processed successfully, false otherwise. +async fn process_events( + rx: &mut mpsc::Receiver, + sink: &mut Subscription, +) -> bool { + while let Some(event) = rx.recv().await { + let is_error_event = std::matches!(event, ArchiveStorageDiffEvent::StorageDiffError(_)); - let mut storage_results = Vec::new(); - for trie_queries in trie_items { - let result = storage_client.handle_trie_queries(hash, previous_hash, trie_queries)?; - storage_results.extend(result); + if let Err(_) = sink.send(&event).await { + return false } - Ok(ArchiveStorageDiffMethodResult { result: storage_results }) + if is_error_event { + // Stop further processing if an error event is received. + return false + } } + + true } diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index cdd5a3a3710c..94e748428939 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -24,20 +24,19 @@ use std::{ }; use itertools::Itertools; -use jsonrpsee::core::RpcResult; use sc_client_api::{Backend, ChildInfo, StorageKey, StorageProvider}; use sp_runtime::traits::Block as BlockT; use super::error::Error as ArchiveError; use crate::common::{ events::{ - ArchiveStorageDiffItem, ArchiveStorageDiffOperationType, ArchiveStorageDiffResult, - ArchiveStorageDiffType, ArchiveStorageResult, PaginatedStorageQuery, StorageQueryType, - StorageResult, + ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageDiffOperationType, + ArchiveStorageDiffResult, ArchiveStorageDiffType, ArchiveStorageMethodErr, + ArchiveStorageResult, PaginatedStorageQuery, StorageQueryType, StorageResult, }, storage::{IterQueryType, QueryIter, Storage}, }; - +use tokio::sync::mpsc; /// Generates the events of the `archive_storage` method. pub struct ArchiveStorage { /// Storage client. @@ -183,7 +182,7 @@ impl ArchiveStorageDiff where Block: BlockT + 'static, BE: Backend + 'static, - Client: StorageProvider + 'static, + Client: StorageProvider + Send + Sync + 'static, { /// Deduplicate the provided items and return a list of `DiffDetails`. /// @@ -191,7 +190,7 @@ where pub fn deduplicate_items( &self, items: Vec>, - ) -> RpcResult>> { + ) -> Result>, ArchiveError> { let mut deduplicated: HashMap, Vec> = HashMap::new(); for diff_item in items { @@ -258,43 +257,27 @@ where key: StorageKey, maybe_child_trie: Option, ty: FetchStorageType, - ) -> RpcResult)>> { - let convert_err = |error| ArchiveError::InvalidParam(error); - + ) -> Result)>, String> { match ty { FetchStorageType::Value => { - let result = self - .client - .query_value(hash, &key, maybe_child_trie.as_ref()) - .map_err(convert_err)?; + let result = self.client.query_value(hash, &key, maybe_child_trie.as_ref())?; Ok(result.map(|res| (res, None))) }, FetchStorageType::Hash => { - let result = self - .client - .query_hash(hash, &key, maybe_child_trie.as_ref()) - .map_err(convert_err)?; + let result = self.client.query_hash(hash, &key, maybe_child_trie.as_ref())?; Ok(result.map(|res| (res, None))) }, FetchStorageType::Both => { - let value = self - .client - .query_value(hash, &key, maybe_child_trie.as_ref()) - .map_err(convert_err)?; - + let value = self.client.query_value(hash, &key, maybe_child_trie.as_ref())?; let Some(value) = value else { return Ok(None); }; - let hash = self - .client - .query_hash(hash, &key, maybe_child_trie.as_ref()) - .map_err(convert_err)?; - + let hash = self.client.query_hash(hash, &key, maybe_child_trie.as_ref())?; let Some(hash) = hash else { return Ok(None); }; @@ -333,29 +316,54 @@ where } } - /// It is guaranteed that all entries correspond to the same child trie or main trie. - pub fn handle_trie_queries( + fn send_result( + tx: &mpsc::Sender, + result: (StorageResult, Option), + operation_type: ArchiveStorageDiffOperationType, + child_trie_key: Option, + ) -> bool { + let res = ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { + key: result.0.key.clone(), + result: result.0.result.clone(), + operation_type, + child_trie_key: child_trie_key.clone(), + }); + if tx.blocking_send(res).is_err() { + return false + } + + if let Some(second) = result.1 { + let res = ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { + key: second.key.clone(), + result: second.result.clone(), + operation_type, + child_trie_key, + }); + if tx.blocking_send(res).is_err() { + return false + } + } + + true + } + + fn handle_trie_queries_inner( &self, hash: Block::Hash, previous_hash: Block::Hash, items: Vec, - ) -> RpcResult> { - let mut results = Vec::with_capacity(items.len()); + tx: &mpsc::Sender, + ) -> Result<(), String> { let mut keys_set = HashSet::new(); let maybe_child_trie = items.first().map(|item| item.child_trie_key.clone()).flatten(); let maybe_child_trie_str = items.first().map(|item| item.child_trie_key_string.clone()).flatten(); - let keys_iter = self - .client - .raw_keys_iter(hash, maybe_child_trie.clone()) - .map_err(|error| ArchiveError::InvalidParam(error))?; + let keys_iter = self.client.raw_keys_iter(hash, maybe_child_trie.clone())?; - let mut previous_keys_iter = self - .client - .raw_keys_iter(previous_hash, maybe_child_trie.clone()) - .map_err(|error| ArchiveError::InvalidParam(error))?; + let mut previous_keys_iter = + self.client.raw_keys_iter(previous_hash, maybe_child_trie.clone())?; for key in keys_iter { let Some(fetch_type) = Self::starts_with(&key, &items) else { @@ -372,20 +380,13 @@ where if !previous_keys_iter.contains(&key) { keys_set.insert(key.clone()); - results.push(ArchiveStorageDiffResult { - key: storage_result.0.key.clone(), - result: storage_result.0.result.clone(), - operation_type: ArchiveStorageDiffOperationType::Added, - child_trie_key: maybe_child_trie_str.clone(), - }); - - if let Some(second) = storage_result.1 { - results.push(ArchiveStorageDiffResult { - key: second.key.clone(), - result: second.result.clone(), - operation_type: ArchiveStorageDiffOperationType::Added, - child_trie_key: maybe_child_trie_str.clone(), - }); + if !Self::send_result( + &tx, + storage_result, + ArchiveStorageDiffOperationType::Added, + maybe_child_trie_str.clone(), + ) { + return Ok(()) } continue @@ -405,25 +406,22 @@ where if storage_result.0.result != previous_storage_result.0.result { keys_set.insert(key.clone()); - results.push(ArchiveStorageDiffResult { - key: storage_result.0.key.clone(), - result: storage_result.0.result.clone(), - operation_type: ArchiveStorageDiffOperationType::Modified, - child_trie_key: maybe_child_trie_str.clone(), - }); - - if let Some(second) = storage_result.1 { - results.push(ArchiveStorageDiffResult { - key: second.key.clone(), - result: second.result.clone(), - operation_type: ArchiveStorageDiffOperationType::Modified, - child_trie_key: maybe_child_trie_str.clone(), - }); + if !Self::send_result( + &tx, + storage_result, + ArchiveStorageDiffOperationType::Modified, + maybe_child_trie_str.clone(), + ) { + return Ok(()) } } } for previous_key in previous_keys_iter { + if keys_set.contains(&previous_key) { + continue + } + let Some(fetch_type) = Self::starts_with(&previous_key, &items) else { continue; }; @@ -438,23 +436,40 @@ where continue }; - results.push(ArchiveStorageDiffResult { - key: previous_storage_result.0.key, - result: previous_storage_result.0.result, - operation_type: ArchiveStorageDiffOperationType::Deleted, - child_trie_key: maybe_child_trie_str.clone(), - }); - - if let Some(second) = previous_storage_result.1 { - results.push(ArchiveStorageDiffResult { - key: second.key.clone(), - result: second.result.clone(), - operation_type: ArchiveStorageDiffOperationType::Deleted, - child_trie_key: maybe_child_trie_str.clone(), - }); + if !Self::send_result( + &tx, + previous_storage_result, + ArchiveStorageDiffOperationType::Deleted, + maybe_child_trie_str.clone(), + ) { + return Ok(()) } } - Ok(results) + Ok(()) + } + + /// It is guaranteed that all entries correspond to the same child trie or main trie. + pub async fn handle_trie_queries( + &self, + hash: Block::Hash, + previous_hash: Block::Hash, + items: Vec, + tx: mpsc::Sender, + ) -> Result<(), tokio::task::JoinError> { + let this = ArchiveStorageDiff { client: self.client.clone() }; + + tokio::task::spawn_blocking(move || { + let result = this.handle_trie_queries_inner(hash, previous_hash, items, &tx); + + if let Err(error) = result { + let error = + ArchiveStorageDiffEvent::StorageDiffError(ArchiveStorageMethodErr { error }); + let _ = tx.blocking_send(error); + } + }) + .await?; + + Ok(()) } } From f47db36606f45115f6a72060edfb97b5a2515680 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 11:38:32 +0200 Subject: [PATCH 12/70] archive: Deduplicate items to a separate function Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/archive.rs | 5 +- .../src/archive/archive_storage.rs | 131 +++++++++--------- 2 files changed, 68 insertions(+), 68 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index 563705427349..11e75c6850ab 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -20,7 +20,7 @@ use crate::{ archive::{ - archive_storage::{ArchiveStorage, ArchiveStorageDiff}, + archive_storage::{deduplicate_storage_diff_items, ArchiveStorage, ArchiveStorageDiff}, error::Error as ArchiveError, ArchiveApiServer, }, @@ -313,7 +313,7 @@ where let fut = async move { // Deduplicate the items. - let trie_items = match storage_client.deduplicate_items(items) { + let trie_items = match deduplicate_storage_diff_items(items) { Ok(items) => items, Err(error) => { pending.reject(error).await; @@ -340,6 +340,7 @@ where let Ok(mut sink) = pending.accept().await.map(Subscription::from) else { return }; let (tx, mut rx) = tokio::sync::mpsc::channel(STORAGE_QUERY_BUF); + if trie_items.is_empty() { // May fail if the channel is closed or the connection is closed. // which is okay to ignore. diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 94e748428939..8942b2f74689 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -184,72 +184,6 @@ where BE: Backend + 'static, Client: StorageProvider + Send + Sync + 'static, { - /// Deduplicate the provided items and return a list of `DiffDetails`. - /// - /// Each list corresponds to a single child trie or the main trie. - pub fn deduplicate_items( - &self, - items: Vec>, - ) -> Result>, ArchiveError> { - let mut deduplicated: HashMap, Vec> = HashMap::new(); - - for diff_item in items { - // Ensure the provided hex keys are valid before deduplication. - let key = StorageKey(parse_hex_param(diff_item.key)?); - let child_trie_key_string = diff_item.child_trie_key.clone(); - let child_trie_key = diff_item - .child_trie_key - .map(|child_trie_key| parse_hex_param(child_trie_key)) - .transpose()? - .map(ChildInfo::new_default_from_vec); - - let diff_item = DiffDetails { - key, - return_type: diff_item.return_type, - child_trie_key: child_trie_key.clone(), - child_trie_key_string, - }; - - match deduplicated.entry(child_trie_key.clone()) { - Entry::Occupied(mut entry) => { - let mut should_insert = true; - - for existing in entry.get() { - // This points to a different return type. - if existing.return_type != diff_item.return_type { - continue - } - // Keys and return types are identical. - if existing.key == diff_item.key { - should_insert = false; - break - } - // The current key is a longer prefix of the existing key. - if diff_item.key.as_ref().starts_with(&existing.key.as_ref()) { - should_insert = false; - break - } - - if diff_item.key.as_ref().starts_with(&existing.key.as_ref()) { - let to_remove = existing.clone(); - entry.get_mut().retain(|item| item != &to_remove); - break; - } - } - - if should_insert { - entry.get_mut().push(diff_item); - } - }, - Entry::Vacant(entry) => { - entry.insert(vec![diff_item]); - }, - } - } - - Ok(deduplicated.into_values().collect()) - } - /// This calls into the database. fn fetch_storage( &self, @@ -473,3 +407,68 @@ where Ok(()) } } + +/// Deduplicate the provided items and return a list of `DiffDetails`. +/// +/// Each list corresponds to a single child trie or the main trie. +pub fn deduplicate_storage_diff_items( + items: Vec>, +) -> Result>, ArchiveError> { + let mut deduplicated: HashMap, Vec> = HashMap::new(); + + for diff_item in items { + // Ensure the provided hex keys are valid before deduplication. + let key = StorageKey(parse_hex_param(diff_item.key)?); + let child_trie_key_string = diff_item.child_trie_key.clone(); + let child_trie_key = diff_item + .child_trie_key + .map(|child_trie_key| parse_hex_param(child_trie_key)) + .transpose()? + .map(ChildInfo::new_default_from_vec); + + let diff_item = DiffDetails { + key, + return_type: diff_item.return_type, + child_trie_key: child_trie_key.clone(), + child_trie_key_string, + }; + + match deduplicated.entry(child_trie_key.clone()) { + Entry::Occupied(mut entry) => { + let mut should_insert = true; + + for existing in entry.get() { + // This points to a different return type. + if existing.return_type != diff_item.return_type { + continue + } + // Keys and return types are identical. + if existing.key == diff_item.key { + should_insert = false; + break + } + // The current key is a longer prefix of the existing key. + if diff_item.key.as_ref().starts_with(&existing.key.as_ref()) { + should_insert = false; + break + } + + if diff_item.key.as_ref().starts_with(&existing.key.as_ref()) { + let to_remove = existing.clone(); + entry.get_mut().retain(|item| item != &to_remove); + break; + } + } + + if should_insert { + entry.get_mut().push(diff_item); + } + }, + Entry::Vacant(entry) => { + entry.insert(vec![diff_item]); + }, + } + } + + Ok(deduplicated.into_values().collect()) +} From 97546a49930a2d18ef7bdeacebb4ae60f48558a3 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 11:53:44 +0200 Subject: [PATCH 13/70] archive/tests: Check deduplication Signed-off-by: Alexandru Vasile --- .../src/archive/archive_storage.rs | 227 +++++++++++++++++- .../client/rpc-spec-v2/src/archive/tests.rs | 2 + substrate/client/service/src/builder.rs | 1 + 3 files changed, 229 insertions(+), 1 deletion(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 8942b2f74689..e2d69410ca6f 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -470,5 +470,230 @@ pub fn deduplicate_storage_diff_items( } } - Ok(deduplicated.into_values().collect()) + Ok(deduplicated + .into_iter() + .sorted_by_key(|(child_trie_key, _)| child_trie_key.clone()) + .map(|(_, values)| values) + .collect()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn dedup_empty() { + let items = vec![]; + let result = deduplicate_storage_diff_items(items).unwrap(); + assert!(result.is_empty()); + } + + #[test] + fn dedup_single() { + let items = vec![ArchiveStorageDiffItem { + key: "0x01".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + }]; + let result = deduplicate_storage_diff_items(items).unwrap(); + assert_eq!(result.len(), 1); + assert_eq!(result[0].len(), 1); + + let expected = DiffDetails { + key: StorageKey(vec![1]), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + child_trie_key_string: None, + }; + assert_eq!(result[0][0], expected); + } + + #[test] + fn dedup_with_different_keys() { + let items = vec![ + ArchiveStorageDiffItem { + key: "0x01".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + }, + ArchiveStorageDiffItem { + key: "0x02".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + }, + ]; + let result = deduplicate_storage_diff_items(items).unwrap(); + assert_eq!(result.len(), 1); + assert_eq!(result[0].len(), 2); + + let expected = vec![ + DiffDetails { + key: StorageKey(vec![1]), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + child_trie_key_string: None, + }, + DiffDetails { + key: StorageKey(vec![2]), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + child_trie_key_string: None, + }, + ]; + assert_eq!(result[0], expected); + } + + #[test] + fn dedup_with_same_keys() { + // Identical keys. + let items = vec![ + ArchiveStorageDiffItem { + key: "0x01".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + }, + ArchiveStorageDiffItem { + key: "0x01".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + }, + ]; + let result = deduplicate_storage_diff_items(items).unwrap(); + assert_eq!(result.len(), 1); + assert_eq!(result[0].len(), 1); + + let expected = vec![DiffDetails { + key: StorageKey(vec![1]), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + child_trie_key_string: None, + }]; + assert_eq!(result[0], expected); + } + + #[test] + fn dedup_with_same_prefix() { + // Identical keys. + let items = vec![ + ArchiveStorageDiffItem { + key: "0x01".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + }, + ArchiveStorageDiffItem { + key: "0x01ff".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + }, + ]; + let result = deduplicate_storage_diff_items(items).unwrap(); + assert_eq!(result.len(), 1); + assert_eq!(result[0].len(), 1); + + let expected = vec![DiffDetails { + key: StorageKey(vec![1]), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + child_trie_key_string: None, + }]; + assert_eq!(result[0], expected); + } + + #[test] + fn dedup_with_different_return_types() { + let items = vec![ + ArchiveStorageDiffItem { + key: "0x01".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + }, + ArchiveStorageDiffItem { + key: "0x01".into(), + return_type: ArchiveStorageDiffType::Hash, + child_trie_key: None, + }, + ]; + let result = deduplicate_storage_diff_items(items).unwrap(); + assert_eq!(result.len(), 1); + assert_eq!(result[0].len(), 2); + + let expected = vec![ + DiffDetails { + key: StorageKey(vec![1]), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + child_trie_key_string: None, + }, + DiffDetails { + key: StorageKey(vec![1]), + return_type: ArchiveStorageDiffType::Hash, + child_trie_key: None, + child_trie_key_string: None, + }, + ]; + assert_eq!(result[0], expected); + } + + #[test] + fn dedup_with_different_child_tries() { + let items = vec![ + ArchiveStorageDiffItem { + key: "0x01".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: Some("0x01".into()), + }, + ArchiveStorageDiffItem { + key: "0x01".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: Some("0x02".into()), + }, + ]; + let result = deduplicate_storage_diff_items(items).unwrap(); + assert_eq!(result.len(), 2); + assert_eq!(result[0].len(), 1); + assert_eq!(result[1].len(), 1); + + let expected = vec![ + vec![DiffDetails { + key: StorageKey(vec![1]), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: Some(ChildInfo::new_default_from_vec(vec![1])), + child_trie_key_string: Some("0x01".into()), + }], + vec![DiffDetails { + key: StorageKey(vec![1]), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: Some(ChildInfo::new_default_from_vec(vec![2])), + child_trie_key_string: Some("0x02".into()), + }], + ]; + assert_eq!(result, expected); + } + + #[test] + fn dedup_with_same_child_tries() { + let items = vec![ + ArchiveStorageDiffItem { + key: "0x01".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: Some("0x01".into()), + }, + ArchiveStorageDiffItem { + key: "0x01".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: Some("0x01".into()), + }, + ]; + let result = deduplicate_storage_diff_items(items).unwrap(); + assert_eq!(result.len(), 1); + assert_eq!(result[0].len(), 1); + + let expected = vec![DiffDetails { + key: StorageKey(vec![1]), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: Some(ChildInfo::new_default_from_vec(vec![1])), + child_trie_key_string: Some("0x01".into()), + }]; + assert_eq!(result[0], expected); + } } diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index 078016f5b3e2..b90c9ffb0077 100644 --- a/substrate/client/rpc-spec-v2/src/archive/tests.rs +++ b/substrate/client/rpc-spec-v2/src/archive/tests.rs @@ -36,6 +36,7 @@ use jsonrpsee::{ }; use sc_block_builder::BlockBuilderBuilder; use sc_client_api::ChildInfo; +use sc_rpc::testing::TokioTestExecutor; use sp_blockchain::HeaderBackend; use sp_consensus::BlockOrigin; use sp_core::{Blake2Hasher, Hasher}; @@ -78,6 +79,7 @@ fn setup_api( client.clone(), backend, CHAIN_GENESIS, + Arc::new(TokioTestExecutor::default()), ArchiveConfig { max_descendant_responses, max_queried_items }, ) .into_rpc(); diff --git a/substrate/client/service/src/builder.rs b/substrate/client/service/src/builder.rs index f27b7ec6fbad..57cd88f1276d 100644 --- a/substrate/client/service/src/builder.rs +++ b/substrate/client/service/src/builder.rs @@ -728,6 +728,7 @@ where client.clone(), backend.clone(), genesis_hash, + task_executor.clone(), // Defaults to sensible limits for the `Archive`. sc_rpc_spec_v2::archive::ArchiveConfig::default(), ) From baa3dd00e1d7cfc1f2b4ce3a3ee924611cc0adc1 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 12:59:15 +0200 Subject: [PATCH 14/70] archive: Fix deduplication shortest key Signed-off-by: Alexandru Vasile --- .../src/archive/archive_storage.rs | 34 ++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index e2d69410ca6f..84741a371ca0 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -447,13 +447,18 @@ pub fn deduplicate_storage_diff_items( should_insert = false; break } + + // The following two conditions ensure that we keep the shortest key. + // The current key is a longer prefix of the existing key. if diff_item.key.as_ref().starts_with(&existing.key.as_ref()) { should_insert = false; break } - if diff_item.key.as_ref().starts_with(&existing.key.as_ref()) { + // The existing key is a longer prefix of the current key. + // We need to keep the current key and remove the existing one. + if existing.key.as_ref().starts_with(&diff_item.key.as_ref()) { let to_remove = existing.clone(); entry.get_mut().retain(|item| item != &to_remove); break; @@ -696,4 +701,31 @@ mod tests { }]; assert_eq!(result[0], expected); } + + #[test] + fn dedup_with_shorter_key() { + let items = vec![ + ArchiveStorageDiffItem { + key: "0x01ff".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + }, + ArchiveStorageDiffItem { + key: "0x01".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + }, + ]; + let result = deduplicate_storage_diff_items(items).unwrap(); + assert_eq!(result.len(), 1); + assert_eq!(result[0].len(), 1); + + let expected = vec![DiffDetails { + key: StorageKey(vec![1]), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + child_trie_key_string: None, + }]; + assert_eq!(result[0], expected); + } } From 4683f6b4da3f2e0345f7ae9f1ff44e29d106e147 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 13:04:33 +0200 Subject: [PATCH 15/70] archive/tests: Check complex deduplication Signed-off-by: Alexandru Vasile --- .../src/archive/archive_storage.rs | 79 ++++++++++++++++++- 1 file changed, 78 insertions(+), 1 deletion(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 84741a371ca0..a36406582eec 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -703,7 +703,7 @@ mod tests { } #[test] - fn dedup_with_shorter_key() { + fn dedup_with_shorter_key_reverse_order() { let items = vec![ ArchiveStorageDiffItem { key: "0x01ff".into(), @@ -728,4 +728,81 @@ mod tests { }]; assert_eq!(result[0], expected); } + + #[test] + fn dedup_multiple_child_tries() { + let items = vec![ + ArchiveStorageDiffItem { + key: "0x02".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + }, + ArchiveStorageDiffItem { + key: "0x01".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: Some("0x01".into()), + }, + ArchiveStorageDiffItem { + key: "0x02".into(), + return_type: ArchiveStorageDiffType::Hash, + child_trie_key: Some("0x01".into()), + }, + ArchiveStorageDiffItem { + key: "0x01".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: Some("0x02".into()), + }, + ArchiveStorageDiffItem { + key: "0x01".into(), + return_type: ArchiveStorageDiffType::Hash, + child_trie_key: Some("0x02".into()), + }, + ArchiveStorageDiffItem { + key: "0x01ff".into(), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: Some("0x02".into()), + }, + ]; + + let result = deduplicate_storage_diff_items(items).unwrap(); + + let expected = vec![ + vec![DiffDetails { + key: StorageKey(vec![2]), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + child_trie_key_string: None, + }], + vec![ + DiffDetails { + key: StorageKey(vec![1]), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: Some(ChildInfo::new_default_from_vec(vec![1])), + child_trie_key_string: Some("0x01".into()), + }, + DiffDetails { + key: StorageKey(vec![2]), + return_type: ArchiveStorageDiffType::Hash, + child_trie_key: Some(ChildInfo::new_default_from_vec(vec![1])), + child_trie_key_string: Some("0x01".into()), + }, + ], + vec![ + DiffDetails { + key: StorageKey(vec![1]), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: Some(ChildInfo::new_default_from_vec(vec![2])), + child_trie_key_string: Some("0x02".into()), + }, + DiffDetails { + key: StorageKey(vec![1]), + return_type: ArchiveStorageDiffType::Hash, + child_trie_key: Some(ChildInfo::new_default_from_vec(vec![2])), + child_trie_key_string: Some("0x02".into()), + }, + ], + ]; + + assert_eq!(result, expected); + } } From 36f59d2cfaf0b80e5dcd660ca6e0169839008184 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 13:12:59 +0200 Subject: [PATCH 16/70] archive: Simplify logic of trie iteration Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/archive.rs | 50 +++++++------------ 1 file changed, 18 insertions(+), 32 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index 11e75c6850ab..071e04bc2a1e 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -313,13 +313,17 @@ where let fut = async move { // Deduplicate the items. - let trie_items = match deduplicate_storage_diff_items(items) { + let mut trie_items = match deduplicate_storage_diff_items(items) { Ok(items) => items, Err(error) => { pending.reject(error).await; return }, }; + // Default to using the main storage trie if no items are provided. + if trie_items.is_empty() { + trie_items.push(Vec::new()); + } let previous_hash = if let Some(previous_hash) = previous_hash { previous_hash @@ -338,40 +342,22 @@ where }; let Ok(mut sink) = pending.accept().await.map(Subscription::from) else { return }; - let (tx, mut rx) = tokio::sync::mpsc::channel(STORAGE_QUERY_BUF); - - if trie_items.is_empty() { - // May fail if the channel is closed or the connection is closed. - // which is okay to ignore. - let result = futures::future::join( - storage_client.handle_trie_queries(hash, previous_hash, Vec::new(), tx), - process_events(&mut rx, &mut sink), - ) - .await; - - // Send `StorageDiffDone` if the events were processed successfully. - if result.1 { - let _ = sink.send(&ArchiveStorageDiffEvent::StorageDiffDone).await; + for trie_queries in trie_items { + let storage_fut = storage_client.handle_trie_queries( + hash, + previous_hash, + trie_queries, + tx.clone(), + ); + let result = + futures::future::join(storage_fut, process_events(&mut rx, &mut sink)).await; + if !result.1 { + return; } - } else { - for trie_queries in trie_items { - let storage_fut = storage_client.handle_trie_queries( - hash, - previous_hash, - trie_queries, - tx.clone(), - ); - let result = - futures::future::join(storage_fut, process_events(&mut rx, &mut sink)) - .await; - if !result.1 { - return; - } - } - - let _ = sink.send(&ArchiveStorageDiffEvent::StorageDiffDone).await; } + + let _ = sink.send(&ArchiveStorageDiffEvent::StorageDiffDone).await; }; self.executor.spawn("substrate-rpc-subscription", Some("rpc"), fut.boxed()); From a084f47563e98977cf7cf373917d0ff8ffced00d Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 13:21:44 +0200 Subject: [PATCH 17/70] archive: Improve documentation Signed-off-by: Alexandru Vasile --- .../src/archive/archive_storage.rs | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index a36406582eec..ac081b3e8f4d 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -184,7 +184,13 @@ where BE: Backend + 'static, Client: StorageProvider + Send + Sync + 'static, { - /// This calls into the database. + /// Fetch the storage from the given key. + /// + /// This method returns: + /// - `None` if the storage is not present. + /// - `Some((StorageResult, None))` for `FetchStorageType::Value`. + /// - `Some((StorageResult, None))` for `FetchStorageType::Hash`. + /// - `Some((StorageResult, Some(StorageResult)))` for `FetchStorageType::Both`. fn fetch_storage( &self, hash: Block::Hash, @@ -250,6 +256,9 @@ where } } + /// Send the provided result to the `tx` sender. + /// + /// Returns `false` if the sender has been closed. fn send_result( tx: &mpsc::Sender, result: (StorageResult, Option), @@ -290,23 +299,28 @@ where ) -> Result<(), String> { let mut keys_set = HashSet::new(); + // Parse the child trie key as `ChildInfo` and `String`. let maybe_child_trie = items.first().map(|item| item.child_trie_key.clone()).flatten(); let maybe_child_trie_str = items.first().map(|item| item.child_trie_key_string.clone()).flatten(); + // Iterator over the current block. let keys_iter = self.client.raw_keys_iter(hash, maybe_child_trie.clone())?; + // Iterator over the previous block. let mut previous_keys_iter = self.client.raw_keys_iter(previous_hash, maybe_child_trie.clone())?; for key in keys_iter { let Some(fetch_type) = Self::starts_with(&key, &items) else { + // The key does not start with any of the provided items. continue; }; let Some(storage_result) = self.fetch_storage(hash, key.clone(), maybe_child_trie.clone(), fetch_type)? else { + // There is no storage result for the key. continue }; @@ -383,7 +397,13 @@ where Ok(()) } - /// It is guaranteed that all entries correspond to the same child trie or main trie. + /// The items provided to this method are obtained by calling `deduplicate_storage_diff_items`. + /// The deduplication method ensures that all items `Vec` correspond to the same + /// `child_trie_key`. + /// + /// This method will iterate over the keys of the main trie or a child trie and fetch the + /// given keys. The fetched keys will be sent to the provided `tx` sender to leverage + /// the backpressure mechanism. pub async fn handle_trie_queries( &self, hash: Block::Hash, From 77cac503fc339ef017ad78ceaf3c879e2a3e60e6 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 16:03:34 +0200 Subject: [PATCH 18/70] archive/events: Add derive(Eq) to archive storage diff events Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/common/events.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/common/events.rs b/substrate/client/rpc-spec-v2/src/common/events.rs index 72b328aab0d2..a1e677b5c337 100644 --- a/substrate/client/rpc-spec-v2/src/common/events.rs +++ b/substrate/client/rpc-spec-v2/src/common/events.rs @@ -81,7 +81,7 @@ pub struct StorageResult { } /// The type of the storage query. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub enum StorageResultType { /// Fetch the value of the provided key. @@ -136,7 +136,7 @@ pub struct ArchiveStorageMethodOk { } /// The error of a storage call. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct ArchiveStorageMethodErr { /// Reported error. @@ -176,7 +176,7 @@ pub struct ArchiveStorageDiffMethodResult { } /// The result of a storage difference call operation type. -#[derive(Debug, Clone, Copy, PartialEq, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub enum ArchiveStorageDiffOperationType { /// The key is added. @@ -188,7 +188,7 @@ pub enum ArchiveStorageDiffOperationType { } /// The result of an individual storage difference key. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct ArchiveStorageDiffResult { /// The hex-encoded key of the result. @@ -211,7 +211,7 @@ pub struct ArchiveStorageDiffResult { /// - `storageDiff` event - generated when a `ArchiveStorageDiffResult` is produced. /// - `storageDiffError` event - generated when an error is produced. /// - `storageDiffDone` event - generated when the `archive_storageDiff` method completed. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] #[serde(tag = "event")] pub enum ArchiveStorageDiffEvent { From e2324a4628c1f6f4d38d16e508ba464afcf0a604 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 16:03:53 +0200 Subject: [PATCH 19/70] archive/tests: Check query for main trie under prefix Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/tests.rs | 84 ++++++++++++++++++- 1 file changed, 81 insertions(+), 3 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index b90c9ffb0077..2e3a76030d0b 100644 --- a/substrate/client/rpc-spec-v2/src/archive/tests.rs +++ b/substrate/client/rpc-spec-v2/src/archive/tests.rs @@ -18,8 +18,9 @@ use crate::{ common::events::{ - ArchiveStorageMethodOk, ArchiveStorageResult, PaginatedStorageQuery, StorageQueryType, - StorageResultType, + ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageDiffOperationType, + ArchiveStorageDiffResult, ArchiveStorageDiffType, ArchiveStorageMethodOk, + ArchiveStorageResult, PaginatedStorageQuery, StorageQueryType, StorageResultType, }, hex_string, MethodResult, }; @@ -32,8 +33,10 @@ use super::{ use assert_matches::assert_matches; use codec::{Decode, Encode}; use jsonrpsee::{ - core::EmptyServerParams as EmptyParams, rpc_params, MethodsError as Error, RpcModule, + core::{server::Subscription as RpcSubscription, EmptyServerParams as EmptyParams}, + rpc_params, MethodsError as Error, RpcModule, }; + use sc_block_builder::BlockBuilderBuilder; use sc_client_api::ChildInfo; use sc_rpc::testing::TokioTestExecutor; @@ -87,6 +90,15 @@ fn setup_api( (client, api) } +async fn get_next_event(sub: &mut RpcSubscription) -> T { + let (event, _sub_id) = tokio::time::timeout(std::time::Duration::from_secs(60), sub.next()) + .await + .unwrap() + .unwrap() + .unwrap(); + event +} + #[tokio::test] async fn archive_genesis() { let (_client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); @@ -840,3 +852,69 @@ async fn archive_storage_discarded_items() { _ => panic!("Unexpected result"), }; } + +#[tokio::test] +async fn archive_storage_diff_main_trie() { + let (client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); + + let mut builder = BlockBuilderBuilder::new(&*client) + .on_parent_block(client.chain_info().genesis_hash) + .with_parent_block_number(0) + .build() + .unwrap(); + builder.push_storage_change(b":A".to_vec(), Some(b"B".to_vec())).unwrap(); + builder.push_storage_change(b":AA".to_vec(), Some(b"BB".to_vec())).unwrap(); + let prev_block = builder.build().unwrap().block; + let prev_hash = format!("{:?}", prev_block.header.hash()); + client.import(BlockOrigin::Own, prev_block.clone()).await.unwrap(); + + let mut builder = BlockBuilderBuilder::new(&*client) + .on_parent_block(prev_block.hash()) + .with_parent_block_number(1) + .build() + .unwrap(); + builder.push_storage_change(b":A".to_vec(), Some(b"11".to_vec())).unwrap(); + builder.push_storage_change(b":AA".to_vec(), Some(b"22".to_vec())).unwrap(); + let block = builder.build().unwrap().block; + let block_hash = format!("{:?}", block.header.hash()); + client.import(BlockOrigin::Own, block.clone()).await.unwrap(); + + // Search for items in the main trie with keys prefixed with ":A". + let items = vec![ArchiveStorageDiffItem:: { + key: hex_string(b":A"), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + }]; + let mut sub = api + .subscribe_unbounded( + "archive_unstable_storageDiff", + rpc_params![&block, &prev_hash, items.clone()], + ) + .await + .unwrap(); + + let event = get_next_event::(&mut sub).await; + assert_eq!( + ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { + key: hex_string(b":A"), + result: StorageResultType::Value(hex_string(b"11")), + operation_type: ArchiveStorageDiffOperationType::Modified, + child_trie_key: None, + }), + event, + ); + + let event = get_next_event::(&mut sub).await; + assert_eq!( + ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { + key: hex_string(b":AA"), + result: StorageResultType::Value(hex_string(b"22")), + operation_type: ArchiveStorageDiffOperationType::Modified, + child_trie_key: None, + }), + event, + ); + + let event = get_next_event::(&mut sub).await; + assert_eq!(ArchiveStorageDiffEvent::StorageDiffDone, event); +} From 4b059134a97a8d8dce49da110f881c83dc06b129 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 16:20:01 +0200 Subject: [PATCH 20/70] archive: Add trace and debug logs for storage diff Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/archive/archive.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index 071e04bc2a1e..5917cef6ae50 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -55,6 +55,8 @@ use std::{collections::HashSet, marker::PhantomData, sync::Arc}; use tokio::sync::mpsc; +pub(crate) const LOG_TARGET: &str = "rpc-spec-v2::archive"; + /// The configuration of [`Archive`]. pub struct ArchiveConfig { /// The maximum number of items the `archive_storage` can return for a descendant query before @@ -311,6 +313,8 @@ where let storage_client = ArchiveStorageDiff::new(self.client.clone()); let client = self.client.clone(); + log::trace!(target: LOG_TARGET, "Storage diff subscription started"); + let fut = async move { // Deduplicate the items. let mut trie_items = match deduplicate_storage_diff_items(items) { @@ -324,6 +328,7 @@ where if trie_items.is_empty() { trie_items.push(Vec::new()); } + log::trace!(target: LOG_TARGET, "Storage diff deduplicated items: {:?}", trie_items); let previous_hash = if let Some(previous_hash) = previous_hash { previous_hash @@ -353,6 +358,7 @@ where let result = futures::future::join(storage_fut, process_events(&mut rx, &mut sink)).await; if !result.1 { + log::debug!(target: LOG_TARGET, "Error processing trie queries"); return; } } @@ -370,6 +376,12 @@ async fn process_events( sink: &mut Subscription, ) -> bool { while let Some(event) = rx.recv().await { + let is_partial_trie_done = std::matches!(event, ArchiveStorageDiffEvent::StorageDiffDone); + if is_partial_trie_done { + log::debug!(target: LOG_TARGET, "Finished processing partial trie query"); + break + } + let is_error_event = std::matches!(event, ArchiveStorageDiffEvent::StorageDiffError(_)); if let Err(_) = sink.send(&event).await { @@ -377,6 +389,7 @@ async fn process_events( } if is_error_event { + log::debug!(target: LOG_TARGET, "Error event encountered while processing partial trie query"); // Stop further processing if an error event is received. return false } From 83cb9b436b4f09e651a15a9da2c36af88783e45b Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 16:22:44 +0200 Subject: [PATCH 21/70] archive: Send StorageDiffDone for partial trie queries Signed-off-by: Alexandru Vasile --- .../src/archive/archive_storage.rs | 35 +++++++++++++++---- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index ac081b3e8f4d..0df635b6f95c 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -28,13 +28,16 @@ use sc_client_api::{Backend, ChildInfo, StorageKey, StorageProvider}; use sp_runtime::traits::Block as BlockT; use super::error::Error as ArchiveError; -use crate::common::{ - events::{ - ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageDiffOperationType, - ArchiveStorageDiffResult, ArchiveStorageDiffType, ArchiveStorageMethodErr, - ArchiveStorageResult, PaginatedStorageQuery, StorageQueryType, StorageResult, +use crate::{ + archive::archive::LOG_TARGET, + common::{ + events::{ + ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageDiffOperationType, + ArchiveStorageDiffResult, ArchiveStorageDiffType, ArchiveStorageMethodErr, + ArchiveStorageResult, PaginatedStorageQuery, StorageQueryType, StorageResult, + }, + storage::{IterQueryType, QueryIter, Storage}, }, - storage::{IterQueryType, QueryIter, Storage}, }; use tokio::sync::mpsc; /// Generates the events of the `archive_storage` method. @@ -414,12 +417,32 @@ where let this = ArchiveStorageDiff { client: self.client.clone() }; tokio::task::spawn_blocking(move || { + log::trace!( + target: LOG_TARGET, + "handle_trie_queries: hash={:?}, previous_hash={:?}, items={:?}", + hash, + previous_hash, + items + ); + let result = this.handle_trie_queries_inner(hash, previous_hash, items, &tx); if let Err(error) = result { let error = ArchiveStorageDiffEvent::StorageDiffError(ArchiveStorageMethodErr { error }); + log::trace!( + target: LOG_TARGET, + "handle_trie_queries: sending error={:?}", + error, + ); + let _ = tx.blocking_send(error); + } else { + log::trace!( + target: LOG_TARGET, + "handle_trie_queries: sending storage diff done", + ); + let _ = tx.blocking_send(ArchiveStorageDiffEvent::StorageDiffDone); } }) .await?; From 85c810ee5b774e126f5eb9b545fa4434193de7ea Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 16:24:03 +0200 Subject: [PATCH 22/70] archive/tests: Check no changes between blocks Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/tests.rs | 47 ++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index 2e3a76030d0b..6ca4e892c87b 100644 --- a/substrate/client/rpc-spec-v2/src/archive/tests.rs +++ b/substrate/client/rpc-spec-v2/src/archive/tests.rs @@ -888,7 +888,7 @@ async fn archive_storage_diff_main_trie() { let mut sub = api .subscribe_unbounded( "archive_unstable_storageDiff", - rpc_params![&block, &prev_hash, items.clone()], + rpc_params![&block_hash, &prev_hash, items.clone()], ) .await .unwrap(); @@ -918,3 +918,48 @@ async fn archive_storage_diff_main_trie() { let event = get_next_event::(&mut sub).await; assert_eq!(ArchiveStorageDiffEvent::StorageDiffDone, event); } + +#[tokio::test] +async fn archive_storage_diff_no_changes() { + let (client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); + + // Build 2 identical blocks. + let mut builder = BlockBuilderBuilder::new(&*client) + .on_parent_block(client.chain_info().genesis_hash) + .with_parent_block_number(0) + .build() + .unwrap(); + builder.push_storage_change(b":A".to_vec(), Some(b"B".to_vec())).unwrap(); + builder.push_storage_change(b":AA".to_vec(), Some(b"BB".to_vec())).unwrap(); + let prev_block = builder.build().unwrap().block; + let prev_hash = format!("{:?}", prev_block.header.hash()); + client.import(BlockOrigin::Own, prev_block.clone()).await.unwrap(); + + let mut builder = BlockBuilderBuilder::new(&*client) + .on_parent_block(prev_block.hash()) + .with_parent_block_number(1) + .build() + .unwrap(); + builder.push_storage_change(b":A".to_vec(), Some(b"B".to_vec())).unwrap(); + builder.push_storage_change(b":AA".to_vec(), Some(b"BB".to_vec())).unwrap(); + let block = builder.build().unwrap().block; + let block_hash = format!("{:?}", block.header.hash()); + client.import(BlockOrigin::Own, block.clone()).await.unwrap(); + + // Search for items in the main trie with keys prefixed with ":A". + let items = vec![ArchiveStorageDiffItem:: { + key: hex_string(b":A"), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + }]; + let mut sub = api + .subscribe_unbounded( + "archive_unstable_storageDiff", + rpc_params![&block_hash, &prev_hash, items.clone()], + ) + .await + .unwrap(); + + let event = get_next_event::(&mut sub).await; + assert_eq!(ArchiveStorageDiffEvent::StorageDiffDone, event); +} From 152d13b0d705ddc8e5795a670c5ca9a3ff38a51e Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 16:29:25 +0200 Subject: [PATCH 23/70] archive/tests: Ensure added key Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/archive/tests.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index 6ca4e892c87b..57654ae1b1d4 100644 --- a/substrate/client/rpc-spec-v2/src/archive/tests.rs +++ b/substrate/client/rpc-spec-v2/src/archive/tests.rs @@ -875,6 +875,7 @@ async fn archive_storage_diff_main_trie() { .unwrap(); builder.push_storage_change(b":A".to_vec(), Some(b"11".to_vec())).unwrap(); builder.push_storage_change(b":AA".to_vec(), Some(b"22".to_vec())).unwrap(); + builder.push_storage_change(b":AAA".to_vec(), Some(b"222".to_vec())).unwrap(); let block = builder.build().unwrap().block; let block_hash = format!("{:?}", block.header.hash()); client.import(BlockOrigin::Own, block.clone()).await.unwrap(); @@ -915,6 +916,18 @@ async fn archive_storage_diff_main_trie() { event, ); + // Added key. + let event = get_next_event::(&mut sub).await; + assert_eq!( + ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { + key: hex_string(b":AAA"), + result: StorageResultType::Value(hex_string(b"222")), + operation_type: ArchiveStorageDiffOperationType::Added, + child_trie_key: None, + }), + event, + ); + let event = get_next_event::(&mut sub).await; assert_eq!(ArchiveStorageDiffEvent::StorageDiffDone, event); } From 4f549bf5a0d6c23c40fcf7284cbde077177d1bc7 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 16:33:19 +0200 Subject: [PATCH 24/70] archive/tests: Extend test with interleaved values and hashes Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/tests.rs | 43 ++++++++++++++++--- 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index 57654ae1b1d4..4b6f1634cb39 100644 --- a/substrate/client/rpc-spec-v2/src/archive/tests.rs +++ b/substrate/client/rpc-spec-v2/src/archive/tests.rs @@ -880,12 +880,21 @@ async fn archive_storage_diff_main_trie() { let block_hash = format!("{:?}", block.header.hash()); client.import(BlockOrigin::Own, block.clone()).await.unwrap(); - // Search for items in the main trie with keys prefixed with ":A". - let items = vec![ArchiveStorageDiffItem:: { - key: hex_string(b":A"), - return_type: ArchiveStorageDiffType::Value, - child_trie_key: None, - }]; + // Search for items in the main trie: + // - values of keys under ":A" + // - hashes of keys under ":AA" + let items = vec![ + ArchiveStorageDiffItem:: { + key: hex_string(b":A"), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + }, + ArchiveStorageDiffItem:: { + key: hex_string(b":AA"), + return_type: ArchiveStorageDiffType::Hash, + child_trie_key: None, + }, + ]; let mut sub = api .subscribe_unbounded( "archive_unstable_storageDiff", @@ -916,6 +925,17 @@ async fn archive_storage_diff_main_trie() { event, ); + let event = get_next_event::(&mut sub).await; + assert_eq!( + ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { + key: hex_string(b":AA"), + result: StorageResultType::Hash(format!("{:?}", Blake2Hasher::hash(b"22"))), + operation_type: ArchiveStorageDiffOperationType::Modified, + child_trie_key: None, + }), + event, + ); + // Added key. let event = get_next_event::(&mut sub).await; assert_eq!( @@ -928,6 +948,17 @@ async fn archive_storage_diff_main_trie() { event, ); + let event = get_next_event::(&mut sub).await; + assert_eq!( + ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { + key: hex_string(b":AAA"), + result: StorageResultType::Hash(format!("{:?}", Blake2Hasher::hash(b"222"))), + operation_type: ArchiveStorageDiffOperationType::Added, + child_trie_key: None, + }), + event, + ); + let event = get_next_event::(&mut sub).await; assert_eq!(ArchiveStorageDiffEvent::StorageDiffDone, event); } From d2080681fe899bea0c1c751e6b38812735802666 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 17:22:37 +0200 Subject: [PATCH 25/70] rpc-v2: Use indexmap crate Signed-off-by: Alexandru Vasile --- Cargo.lock | 1 + substrate/client/rpc-spec-v2/Cargo.toml | 1 + 2 files changed, 2 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index e8a495fb8a96..7fd2a7e685d2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -19552,6 +19552,7 @@ dependencies = [ "futures", "futures-util", "hex", + "indexmap 2.2.3", "itertools 0.11.0", "jsonrpsee 0.24.3", "log", diff --git a/substrate/client/rpc-spec-v2/Cargo.toml b/substrate/client/rpc-spec-v2/Cargo.toml index b9dc5fa141d4..5e5c8a6deca0 100644 --- a/substrate/client/rpc-spec-v2/Cargo.toml +++ b/substrate/client/rpc-spec-v2/Cargo.toml @@ -43,6 +43,7 @@ futures-util = { workspace = true } rand = { workspace = true, default-features = true } schnellru = { workspace = true } itertools = { workspace = true } +indexmap = { workspace = true } [dev-dependencies] jsonrpsee = { workspace = true, features = ["server", "ws-client"] } From 369a4d2a41902e36ebe8a25833517ece8de8e2b2 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 17:22:58 +0200 Subject: [PATCH 26/70] archive: Move prevHash as last parameter to archive diff Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/archive/api.rs | 2 +- substrate/client/rpc-spec-v2/src/archive/archive.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/api.rs b/substrate/client/rpc-spec-v2/src/archive/api.rs index 1e5f6c8d10e0..d3da879fc358 100644 --- a/substrate/client/rpc-spec-v2/src/archive/api.rs +++ b/substrate/client/rpc-spec-v2/src/archive/api.rs @@ -121,7 +121,7 @@ pub trait ArchiveApi { fn archive_unstable_storage_diff( &self, hash: Hash, - previous_hash: Option, items: Vec>, + previous_hash: Option, ); } diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index 5917cef6ae50..c3a2be0b8bfd 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -307,8 +307,8 @@ where &self, pending: PendingSubscriptionSink, hash: Block::Hash, - previous_hash: Option, items: Vec>, + previous_hash: Option, ) { let storage_client = ArchiveStorageDiff::new(self.client.clone()); let client = self.client.clone(); From a02cc84635da47b1a922cff916e9d6b9b64c96e6 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 17:23:27 +0200 Subject: [PATCH 27/70] archive: Preserve order of elements with indexMap Signed-off-by: Alexandru Vasile --- .../src/archive/archive_storage.rs | 35 ++++++++++++------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 0df635b6f95c..7762d614f2c8 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -269,8 +269,8 @@ where child_trie_key: Option, ) -> bool { let res = ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { - key: result.0.key.clone(), - result: result.0.result.clone(), + key: result.0.key, + result: result.0.result, operation_type, child_trie_key: child_trie_key.clone(), }); @@ -280,8 +280,8 @@ where if let Some(second) = result.1 { let res = ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { - key: second.key.clone(), - result: second.result.clone(), + key: second.key, + result: second.result, operation_type, child_trie_key, }); @@ -311,8 +311,19 @@ where let keys_iter = self.client.raw_keys_iter(hash, maybe_child_trie.clone())?; // Iterator over the previous block. - let mut previous_keys_iter = - self.client.raw_keys_iter(previous_hash, maybe_child_trie.clone())?; + // + // Note: `Itertools::contains` consumes the iterator until the given key is found, + // therefore we may lose keys and iterate over the previous block twice for deleted keys. + // + // Example: + // keys_iter: [0, 1, 2] + // previous_keys_iter: [1, 2, 3] + // -> the `previous_keys_iter` is entirely consumed while searching for `0`. + let previous_keys_iter: indexmap::IndexMap<_, _> = self + .client + .raw_keys_iter(previous_hash, maybe_child_trie.clone())? + .map(|key| (key, ())) + .collect(); for key in keys_iter { let Some(fetch_type) = Self::starts_with(&key, &items) else { @@ -327,10 +338,10 @@ where continue }; - // The key is not present in the previous state. - if !previous_keys_iter.contains(&key) { - keys_set.insert(key.clone()); + keys_set.insert(key.clone()); + // The key is not present in the previous state. + if !previous_keys_iter.contains_key(&key) { if !Self::send_result( &tx, storage_result, @@ -355,8 +366,6 @@ where }; if storage_result.0.result != previous_storage_result.0.result { - keys_set.insert(key.clone()); - if !Self::send_result( &tx, storage_result, @@ -368,7 +377,7 @@ where } } - for previous_key in previous_keys_iter { + for previous_key in previous_keys_iter.into_keys() { if keys_set.contains(&previous_key) { continue } @@ -379,7 +388,7 @@ where let Some(previous_storage_result) = self.fetch_storage( previous_hash, - previous_key.clone(), + previous_key, maybe_child_trie.clone(), fetch_type, )? From 637b446f7a65a79dc17db0eba0615621fb9ab440 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 17:23:49 +0200 Subject: [PATCH 28/70] archive/tests: Check deleted keys Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/tests.rs | 68 ++++++++++++++++++- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index 4b6f1634cb39..c354de1e7c4c 100644 --- a/substrate/client/rpc-spec-v2/src/archive/tests.rs +++ b/substrate/client/rpc-spec-v2/src/archive/tests.rs @@ -898,7 +898,7 @@ async fn archive_storage_diff_main_trie() { let mut sub = api .subscribe_unbounded( "archive_unstable_storageDiff", - rpc_params![&block_hash, &prev_hash, items.clone()], + rpc_params![&block_hash, items.clone(), &prev_hash], ) .await .unwrap(); @@ -999,7 +999,7 @@ async fn archive_storage_diff_no_changes() { let mut sub = api .subscribe_unbounded( "archive_unstable_storageDiff", - rpc_params![&block_hash, &prev_hash, items.clone()], + rpc_params![&block_hash, items.clone(), &prev_hash], ) .await .unwrap(); @@ -1007,3 +1007,67 @@ async fn archive_storage_diff_no_changes() { let event = get_next_event::(&mut sub).await; assert_eq!(ArchiveStorageDiffEvent::StorageDiffDone, event); } + +#[tokio::test] +async fn archive_storage_diff_deleted_changes() { + let (client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); + + // Blocks are imported as forks. + let mut builder = BlockBuilderBuilder::new(&*client) + .on_parent_block(client.chain_info().genesis_hash) + .with_parent_block_number(0) + .build() + .unwrap(); + builder.push_storage_change(b":A".to_vec(), Some(b"B".to_vec())).unwrap(); + builder.push_storage_change(b":AA".to_vec(), Some(b"BB".to_vec())).unwrap(); + let prev_block = builder.build().unwrap().block; + let prev_hash = format!("{:?}", prev_block.header.hash()); + client.import(BlockOrigin::Own, prev_block.clone()).await.unwrap(); + + let mut builder = BlockBuilderBuilder::new(&*client) + .on_parent_block(client.chain_info().genesis_hash) + .with_parent_block_number(0) + .build() + .unwrap(); + builder + .push_transfer(Transfer { + from: AccountKeyring::Alice.into(), + to: AccountKeyring::Ferdie.into(), + amount: 41, + nonce: 0, + }) + .unwrap(); + builder.push_storage_change(b":A".to_vec(), Some(b"B".to_vec())).unwrap(); + let block = builder.build().unwrap().block; + let block_hash = format!("{:?}", block.header.hash()); + client.import(BlockOrigin::Own, block.clone()).await.unwrap(); + + // Search for items in the main trie with keys prefixed with ":A". + let items = vec![ArchiveStorageDiffItem:: { + key: hex_string(b":A"), + return_type: ArchiveStorageDiffType::Value, + child_trie_key: None, + }]; + + let mut sub = api + .subscribe_unbounded( + "archive_unstable_storageDiff", + rpc_params![&block_hash, items.clone(), &prev_hash], + ) + .await + .unwrap(); + + let event = get_next_event::(&mut sub).await; + assert_eq!( + ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { + key: hex_string(b":AA"), + result: StorageResultType::Value(hex_string(b"BB")), + operation_type: ArchiveStorageDiffOperationType::Deleted, + child_trie_key: None, + }), + event, + ); + + let event = get_next_event::(&mut sub).await; + assert_eq!(ArchiveStorageDiffEvent::StorageDiffDone, event); +} From 76efb184629ed9df8b1178d5ba1158a91cc7295e Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 17:40:03 +0200 Subject: [PATCH 29/70] events: Add common wrappers for construction Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/common/events.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/substrate/client/rpc-spec-v2/src/common/events.rs b/substrate/client/rpc-spec-v2/src/common/events.rs index a1e677b5c337..198a60bf4cac 100644 --- a/substrate/client/rpc-spec-v2/src/common/events.rs +++ b/substrate/client/rpc-spec-v2/src/common/events.rs @@ -223,6 +223,23 @@ pub enum ArchiveStorageDiffEvent { StorageDiffDone, } +impl ArchiveStorageDiffEvent { + /// Create a new `ArchiveStorageDiffEvent::StorageDiffError` event. + pub fn err(error: String) -> Self { + Self::StorageDiffError(ArchiveStorageMethodErr { error }) + } + + /// Checks if the event is a `StorageDiffDone` event. + pub fn is_done(&self) -> bool { + matches!(self, Self::StorageDiffDone) + } + + /// Checks if the event is a `StorageDiffError` event. + pub fn is_err(&self) -> bool { + matches!(self, Self::StorageDiffError(_)) + } +} + #[cfg(test)] mod tests { use super::*; From ca1fa202adb4b16d6ce3b5c03f940ca21ba9335a Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 17:40:37 +0200 Subject: [PATCH 30/70] archive: Propagate errors via events Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/archive.rs | 20 +++++++------------ .../src/archive/archive_storage.rs | 8 +++----- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index c3a2be0b8bfd..51b953a61ace 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -316,11 +316,13 @@ where log::trace!(target: LOG_TARGET, "Storage diff subscription started"); let fut = async move { + let Ok(mut sink) = pending.accept().await.map(Subscription::from) else { return }; + // Deduplicate the items. let mut trie_items = match deduplicate_storage_diff_items(items) { Ok(items) => items, Err(error) => { - pending.reject(error).await; + let _ = sink.send(&ArchiveStorageDiffEvent::err(error.to_string())).await; return }, }; @@ -334,19 +336,13 @@ where previous_hash } else { let Ok(Some(current_header)) = client.header(hash) else { - pending - .reject(ArchiveError::InvalidParam(format!( - "Block header is not present: {}", - hash - ))) - .await; - + let message = format!("Block header is not present: {hash}"); + let _ = sink.send(&ArchiveStorageDiffEvent::err(message)).await; return }; *current_header.parent_hash() }; - let Ok(mut sink) = pending.accept().await.map(Subscription::from) else { return }; let (tx, mut rx) = tokio::sync::mpsc::channel(STORAGE_QUERY_BUF); for trie_queries in trie_items { let storage_fut = storage_client.handle_trie_queries( @@ -376,14 +372,12 @@ async fn process_events( sink: &mut Subscription, ) -> bool { while let Some(event) = rx.recv().await { - let is_partial_trie_done = std::matches!(event, ArchiveStorageDiffEvent::StorageDiffDone); - if is_partial_trie_done { + if event.is_done() { log::debug!(target: LOG_TARGET, "Finished processing partial trie query"); break } - let is_error_event = std::matches!(event, ArchiveStorageDiffEvent::StorageDiffError(_)); - + let is_error_event = event.is_err(); if let Err(_) = sink.send(&event).await { return false } diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 7762d614f2c8..25489be98dd4 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -33,8 +33,8 @@ use crate::{ common::{ events::{ ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageDiffOperationType, - ArchiveStorageDiffResult, ArchiveStorageDiffType, ArchiveStorageMethodErr, - ArchiveStorageResult, PaginatedStorageQuery, StorageQueryType, StorageResult, + ArchiveStorageDiffResult, ArchiveStorageDiffType, ArchiveStorageResult, + PaginatedStorageQuery, StorageQueryType, StorageResult, }, storage::{IterQueryType, QueryIter, Storage}, }, @@ -437,15 +437,13 @@ where let result = this.handle_trie_queries_inner(hash, previous_hash, items, &tx); if let Err(error) = result { - let error = - ArchiveStorageDiffEvent::StorageDiffError(ArchiveStorageMethodErr { error }); log::trace!( target: LOG_TARGET, "handle_trie_queries: sending error={:?}", error, ); - let _ = tx.blocking_send(error); + let _ = tx.blocking_send(ArchiveStorageDiffEvent::err(error)); } else { log::trace!( target: LOG_TARGET, From 03eefd77527abdc4a799ced26160751a9ef74049 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 17:47:53 +0200 Subject: [PATCH 31/70] archive: Check invalid parameters and events Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/tests.rs | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index c354de1e7c4c..06445d96d125 100644 --- a/substrate/client/rpc-spec-v2/src/archive/tests.rs +++ b/substrate/client/rpc-spec-v2/src/archive/tests.rs @@ -1071,3 +1071,37 @@ async fn archive_storage_diff_deleted_changes() { let event = get_next_event::(&mut sub).await; assert_eq!(ArchiveStorageDiffEvent::StorageDiffDone, event); } + +#[tokio::test] +async fn archive_storage_diff_invalid_params() { + let invalid_hash = hex_string(&INVALID_HASH); + let (_, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); + + // Invalid shape for parameters. + let items: Vec> = Vec::new(); + let err = api + .subscribe_unbounded( + "archive_unstable_storageDiff", + rpc_params!["123", items.clone(), &invalid_hash], + ) + .await + .unwrap_err(); + assert_matches!(err, + Error::JsonRpc(ref err) if err.code() == crate::chain_head::error::json_rpc_spec::INVALID_PARAM_ERROR && err.message() == "Invalid params" + ); + + // The shape is right, but the block hash is invalid. + let items: Vec> = Vec::new(); + let mut sub = api + .subscribe_unbounded( + "archive_unstable_storageDiff", + rpc_params![&invalid_hash, items.clone(), &invalid_hash], + ) + .await + .unwrap(); + + let event = get_next_event::(&mut sub).await; + assert_matches!(event, + ArchiveStorageDiffEvent::StorageDiffError(ref err) if err.error.contains("Header was not found") + ); +} From 9a1e792529dba6cf300275c1221a1a95c59601d9 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 17:49:01 +0200 Subject: [PATCH 32/70] archive: Fix clippy Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/archive/archive_storage.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 25489be98dd4..471474da3a6c 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -303,9 +303,9 @@ where let mut keys_set = HashSet::new(); // Parse the child trie key as `ChildInfo` and `String`. - let maybe_child_trie = items.first().map(|item| item.child_trie_key.clone()).flatten(); + let maybe_child_trie = items.first().and_then(|item| item.child_trie_key.clone()); let maybe_child_trie_str = - items.first().map(|item| item.child_trie_key_string.clone()).flatten(); + items.first().and_then(|item| item.child_trie_key_string.clone()); // Iterator over the current block. let keys_iter = self.client.raw_keys_iter(hash, maybe_child_trie.clone())?; From 971026de45ded571f222aac57d97e3aeb5973d03 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 5 Nov 2024 18:19:40 +0200 Subject: [PATCH 33/70] Add prdoc Signed-off-by: Alexandru Vasile --- prdoc/pr_5997.prdoc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 prdoc/pr_5997.prdoc diff --git a/prdoc/pr_5997.prdoc b/prdoc/pr_5997.prdoc new file mode 100644 index 000000000000..b6a7baaf66e8 --- /dev/null +++ b/prdoc/pr_5997.prdoc @@ -0,0 +1,16 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Implement archive_unstable_storageDiff method + +doc: + - audience: Node Dev + description: | + This PR implements the `archive_unstable_storageDiff` rpc-v2 method. + Developers can use this method to fetch the storage differences + between two blocks. This is useful for oracles and archive nodes. + For more details see: https://github.com/paritytech/json-rpc-interface-spec/blob/main/src/api/archive_unstable_storageDiff.md. + +crates: + - name: sc-rpc-spec-v2 + bump: minor From b774855c417d821e0152c67dcb555b1e32d86d0f Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 6 Nov 2024 12:55:34 +0200 Subject: [PATCH 34/70] archive/storage: Optimize space used for saved keys Signed-off-by: Alexandru Vasile --- .../src/archive/archive_storage.rs | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 471474da3a6c..7195254a628c 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -19,7 +19,7 @@ //! Implementation of the `archive_storage` method. use std::{ - collections::{hash_map::Entry, HashMap, HashSet}, + collections::{hash_map::Entry, HashMap}, sync::Arc, }; @@ -300,8 +300,6 @@ where items: Vec, tx: &mpsc::Sender, ) -> Result<(), String> { - let mut keys_set = HashSet::new(); - // Parse the child trie key as `ChildInfo` and `String`. let maybe_child_trie = items.first().and_then(|item| item.child_trie_key.clone()); let maybe_child_trie_str = @@ -312,6 +310,9 @@ where // Iterator over the previous block. // + // The hashmap contains the keys and a boolean to indicate if the key is present in the + // current hash. This information is later used to determine if the key has been deleted. + // // Note: `Itertools::contains` consumes the iterator until the given key is found, // therefore we may lose keys and iterate over the previous block twice for deleted keys. // @@ -319,13 +320,16 @@ where // keys_iter: [0, 1, 2] // previous_keys_iter: [1, 2, 3] // -> the `previous_keys_iter` is entirely consumed while searching for `0`. - let previous_keys_iter: indexmap::IndexMap<_, _> = self + let mut previous_keys_iter: indexmap::IndexMap<_, _> = self .client .raw_keys_iter(previous_hash, maybe_child_trie.clone())? - .map(|key| (key, ())) + .map(|key| (key, false)) .collect(); for key in keys_iter { + // Mark the key as present if it exists in the previous block. + previous_keys_iter.entry(key.clone()).and_modify(|e| *e = true); + let Some(fetch_type) = Self::starts_with(&key, &items) else { // The key does not start with any of the provided items. continue; @@ -338,8 +342,6 @@ where continue }; - keys_set.insert(key.clone()); - // The key is not present in the previous state. if !previous_keys_iter.contains_key(&key) { if !Self::send_result( @@ -377,11 +379,13 @@ where } } - for previous_key in previous_keys_iter.into_keys() { - if keys_set.contains(&previous_key) { - continue - } + // Iterate over the keys of the previous block that are not found in the current block. + let deleted_keys = + previous_keys_iter + .into_iter() + .filter_map(|(key, present)| if !present { Some(key) } else { None }); + for previous_key in deleted_keys { let Some(fetch_type) = Self::starts_with(&previous_key, &items) else { continue; }; From dc5f001ff51d08b9e57d419d4b2cd8ebe11095d8 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 11 Nov 2024 19:03:42 +0200 Subject: [PATCH 35/70] archive: Propagate all queries to handle_trie_queries Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/archive.rs | 21 ++++----- .../src/archive/archive_storage.rs | 45 ++++++++++--------- 2 files changed, 32 insertions(+), 34 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index 51b953a61ace..8a8a9598097f 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -344,19 +344,14 @@ where }; let (tx, mut rx) = tokio::sync::mpsc::channel(STORAGE_QUERY_BUF); - for trie_queries in trie_items { - let storage_fut = storage_client.handle_trie_queries( - hash, - previous_hash, - trie_queries, - tx.clone(), - ); - let result = - futures::future::join(storage_fut, process_events(&mut rx, &mut sink)).await; - if !result.1 { - log::debug!(target: LOG_TARGET, "Error processing trie queries"); - return; - } + let storage_fut = + storage_client.handle_trie_queries(hash, previous_hash, trie_items, tx.clone()); + + let result = + futures::future::join(storage_fut, process_events(&mut rx, &mut sink)).await; + if !result.1 { + log::debug!(target: LOG_TARGET, "Error processing trie queries"); + return; } let _ = sink.send(&ArchiveStorageDiffEvent::StorageDiffDone).await; diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 7195254a628c..64dbac5beb70 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -424,37 +424,40 @@ where &self, hash: Block::Hash, previous_hash: Block::Hash, - items: Vec, + trie_queries: Vec>, tx: mpsc::Sender, ) -> Result<(), tokio::task::JoinError> { let this = ArchiveStorageDiff { client: self.client.clone() }; tokio::task::spawn_blocking(move || { - log::trace!( - target: LOG_TARGET, - "handle_trie_queries: hash={:?}, previous_hash={:?}, items={:?}", - hash, - previous_hash, - items - ); - - let result = this.handle_trie_queries_inner(hash, previous_hash, items, &tx); - - if let Err(error) = result { + for items in trie_queries { log::trace!( target: LOG_TARGET, - "handle_trie_queries: sending error={:?}", - error, + "handle_trie_queries: hash={:?}, previous_hash={:?}, items={:?}", + hash, + previous_hash, + items ); - let _ = tx.blocking_send(ArchiveStorageDiffEvent::err(error)); - } else { - log::trace!( - target: LOG_TARGET, - "handle_trie_queries: sending storage diff done", - ); - let _ = tx.blocking_send(ArchiveStorageDiffEvent::StorageDiffDone); + let result = this.handle_trie_queries_inner(hash, previous_hash, items, &tx); + + if let Err(error) = result { + log::trace!( + target: LOG_TARGET, + "handle_trie_queries: sending error={:?}", + error, + ); + + let _ = tx.blocking_send(ArchiveStorageDiffEvent::err(error)); + } else { + log::trace!( + target: LOG_TARGET, + "handle_trie_queries: sending storage diff done", + ); + } } + + let _ = tx.blocking_send(ArchiveStorageDiffEvent::StorageDiffDone); }) .await?; From cdc4dc55caedd5463b712ea624c2a0d088ec80a0 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 11 Nov 2024 19:13:44 +0200 Subject: [PATCH 36/70] archive: Simplify process_events Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/archive.rs | 36 ++++++------------- .../src/archive/archive_storage.rs | 3 ++ 2 files changed, 14 insertions(+), 25 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index 8a8a9598097f..5dd0af8b8786 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -347,42 +347,28 @@ where let storage_fut = storage_client.handle_trie_queries(hash, previous_hash, trie_items, tx.clone()); - let result = - futures::future::join(storage_fut, process_events(&mut rx, &mut sink)).await; - if !result.1 { - log::debug!(target: LOG_TARGET, "Error processing trie queries"); - return; - } - - let _ = sink.send(&ArchiveStorageDiffEvent::StorageDiffDone).await; + // We don't care about the return value of this join: + // - process_events might encounter an error (if the client disconnected) + // - storage_fut might encounter an error while processing a trie queries and + // the error is propagated via the sink. + let _ = futures::future::join(storage_fut, process_events(&mut rx, &mut sink)).await; }; self.executor.spawn("substrate-rpc-subscription", Some("rpc"), fut.boxed()); } } -/// Returns true if the events where processed successfully, false otherwise. -async fn process_events( - rx: &mut mpsc::Receiver, - sink: &mut Subscription, -) -> bool { +/// Sends all the events to the sink. +async fn process_events(rx: &mut mpsc::Receiver, sink: &mut Subscription) { while let Some(event) = rx.recv().await { if event.is_done() { log::debug!(target: LOG_TARGET, "Finished processing partial trie query"); - break + } else if event.is_err() { + log::debug!(target: LOG_TARGET, "Error encountered while processing partial trie query"); } - let is_error_event = event.is_err(); - if let Err(_) = sink.send(&event).await { - return false - } - - if is_error_event { - log::debug!(target: LOG_TARGET, "Error event encountered while processing partial trie query"); - // Stop further processing if an error event is received. - return false + if sink.send(&event).await.is_err() { + return } } - - true } diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 64dbac5beb70..1bc05af5ce71 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -449,6 +449,9 @@ where ); let _ = tx.blocking_send(ArchiveStorageDiffEvent::err(error)); + + // It ok to return here on the first encountered error. + return } else { log::trace!( target: LOG_TARGET, From de7cf79d52808de25d3234d361d9dcc78d77413d Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 11 Nov 2024 19:15:46 +0200 Subject: [PATCH 37/70] archive: Add missing line Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/archive/archive_storage.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 1bc05af5ce71..38293ff345d7 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -40,6 +40,7 @@ use crate::{ }, }; use tokio::sync::mpsc; + /// Generates the events of the `archive_storage` method. pub struct ArchiveStorage { /// Storage client. From 199b46d54cf862e6e14e0991fb443685747698b1 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 11 Nov 2024 19:26:33 +0200 Subject: [PATCH 38/70] archive: Refactor with FetchedStorage enum Signed-off-by: Alexandru Vasile --- .../src/archive/archive_storage.rs | 51 ++++++++++--------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 38293ff345d7..35367fac7a15 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -172,6 +172,17 @@ enum FetchStorageType { Both, } +/// The return value of the `fetch_storage` method. +#[derive(Debug, PartialEq, Clone)] +enum FetchedStorage { + /// Storage value under a key. + Value(StorageResult), + /// Storage hash under a key. + Hash(StorageResult), + /// Both storage value and hash under a key. + Both { value: StorageResult, hash: StorageResult }, +} + pub struct ArchiveStorageDiff { client: Storage, } @@ -189,30 +200,24 @@ where Client: StorageProvider + Send + Sync + 'static, { /// Fetch the storage from the given key. - /// - /// This method returns: - /// - `None` if the storage is not present. - /// - `Some((StorageResult, None))` for `FetchStorageType::Value`. - /// - `Some((StorageResult, None))` for `FetchStorageType::Hash`. - /// - `Some((StorageResult, Some(StorageResult)))` for `FetchStorageType::Both`. fn fetch_storage( &self, hash: Block::Hash, key: StorageKey, maybe_child_trie: Option, ty: FetchStorageType, - ) -> Result)>, String> { + ) -> Result, String> { match ty { FetchStorageType::Value => { let result = self.client.query_value(hash, &key, maybe_child_trie.as_ref())?; - Ok(result.map(|res| (res, None))) + Ok(result.map(FetchedStorage::Value)) }, FetchStorageType::Hash => { let result = self.client.query_hash(hash, &key, maybe_child_trie.as_ref())?; - Ok(result.map(|res| (res, None))) + Ok(result.map(FetchedStorage::Hash)) }, FetchStorageType::Both => { @@ -226,7 +231,7 @@ where return Ok(None); }; - Ok(Some((value, Some(hash)))) + Ok(Some(FetchedStorage::Both { value, hash })) }, } } @@ -265,26 +270,22 @@ where /// Returns `false` if the sender has been closed. fn send_result( tx: &mpsc::Sender, - result: (StorageResult, Option), + result: FetchedStorage, operation_type: ArchiveStorageDiffOperationType, child_trie_key: Option, ) -> bool { - let res = ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { - key: result.0.key, - result: result.0.result, - operation_type, - child_trie_key: child_trie_key.clone(), - }); - if tx.blocking_send(res).is_err() { - return false - } + let items = match result { + FetchedStorage::Value(storage_result) | FetchedStorage::Hash(storage_result) => + vec![storage_result], + FetchedStorage::Both { value, hash } => vec![value, hash], + }; - if let Some(second) = result.1 { + for item in items { let res = ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { - key: second.key, - result: second.result, + key: item.key, + result: item.result, operation_type, - child_trie_key, + child_trie_key: child_trie_key.clone(), }); if tx.blocking_send(res).is_err() { return false @@ -368,7 +369,7 @@ where continue }; - if storage_result.0.result != previous_storage_result.0.result { + if storage_result != previous_storage_result { if !Self::send_result( &tx, storage_result, From f4ace064d1a8cdd868078783130ebc131bff4dd6 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 11 Nov 2024 20:28:08 +0200 Subject: [PATCH 39/70] archive: Refactor for optimal iteration cycles Signed-off-by: Alexandru Vasile --- Cargo.lock | 1 - substrate/client/rpc-spec-v2/Cargo.toml | 1 - .../src/archive/archive_storage.rs | 169 +++++++++--------- 3 files changed, 85 insertions(+), 86 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9c4a7536e0e0..8896037c6973 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -19777,7 +19777,6 @@ dependencies = [ "futures", "futures-util", "hex", - "indexmap 2.2.3", "itertools 0.11.0", "jsonrpsee 0.24.3", "log", diff --git a/substrate/client/rpc-spec-v2/Cargo.toml b/substrate/client/rpc-spec-v2/Cargo.toml index 5e5c8a6deca0..b9dc5fa141d4 100644 --- a/substrate/client/rpc-spec-v2/Cargo.toml +++ b/substrate/client/rpc-spec-v2/Cargo.toml @@ -43,7 +43,6 @@ futures-util = { workspace = true } rand = { workspace = true, default-features = true } schnellru = { workspace = true } itertools = { workspace = true } -indexmap = { workspace = true } [dev-dependencies] jsonrpsee = { workspace = true, features = ["server", "ws-client"] } diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 35367fac7a15..7244635ddb41 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -308,72 +308,104 @@ where items.first().and_then(|item| item.child_trie_key_string.clone()); // Iterator over the current block. - let keys_iter = self.client.raw_keys_iter(hash, maybe_child_trie.clone())?; - - // Iterator over the previous block. - // - // The hashmap contains the keys and a boolean to indicate if the key is present in the - // current hash. This information is later used to determine if the key has been deleted. - // - // Note: `Itertools::contains` consumes the iterator until the given key is found, - // therefore we may lose keys and iterate over the previous block twice for deleted keys. - // - // Example: - // keys_iter: [0, 1, 2] - // previous_keys_iter: [1, 2, 3] - // -> the `previous_keys_iter` is entirely consumed while searching for `0`. - let mut previous_keys_iter: indexmap::IndexMap<_, _> = self - .client - .raw_keys_iter(previous_hash, maybe_child_trie.clone())? - .map(|key| (key, false)) - .collect(); - - for key in keys_iter { - // Mark the key as present if it exists in the previous block. - previous_keys_iter.entry(key.clone()).and_modify(|e| *e = true); + let mut keys_iter = self.client.raw_keys_iter(hash, maybe_child_trie.clone())?; + let mut previous_keys_iter = + self.client.raw_keys_iter(previous_hash, maybe_child_trie.clone())?; + + let mut lhs = keys_iter.next(); + let mut rhs = previous_keys_iter.next(); + + loop { + // Check if the key was added or deleted or modified based on the + // lexicographical order of the keys. + let (operation_type, key) = match (&lhs, &rhs) { + (Some(lhs_key), Some(rhs_key)) => + if lhs_key < rhs_key { + let key = lhs_key.clone(); + + lhs = keys_iter.next(); + + (ArchiveStorageDiffOperationType::Added, key) + } else if lhs_key > rhs_key { + let key = rhs_key.clone(); + + rhs = previous_keys_iter.next(); + + (ArchiveStorageDiffOperationType::Deleted, key) + } else { + let key = lhs_key.clone(); + + lhs = keys_iter.next(); + rhs = previous_keys_iter.next(); + + (ArchiveStorageDiffOperationType::Modified, key) + }, + (Some(lhs_key), None) => { + let key = lhs_key.clone(); + + lhs = keys_iter.next(); + + (ArchiveStorageDiffOperationType::Added, key) + }, + (None, Some(rhs_key)) => { + let key = rhs_key.clone(); + + rhs = previous_keys_iter.next(); + + (ArchiveStorageDiffOperationType::Deleted, key) + }, + (None, None) => break, + }; let Some(fetch_type) = Self::starts_with(&key, &items) else { // The key does not start with any of the provided items. continue; }; - let Some(storage_result) = - self.fetch_storage(hash, key.clone(), maybe_child_trie.clone(), fetch_type)? - else { - // There is no storage result for the key. - continue - }; - - // The key is not present in the previous state. - if !previous_keys_iter.contains_key(&key) { - if !Self::send_result( - &tx, - storage_result, - ArchiveStorageDiffOperationType::Added, - maybe_child_trie_str.clone(), - ) { - return Ok(()) - } + let maybe_result = match operation_type { + ArchiveStorageDiffOperationType::Added => + self.fetch_storage(hash, key.clone(), maybe_child_trie.clone(), fetch_type)?, + ArchiveStorageDiffOperationType::Deleted => self.fetch_storage( + previous_hash, + key.clone(), + maybe_child_trie.clone(), + fetch_type, + )?, + ArchiveStorageDiffOperationType::Modified => { + let Some(storage_result) = self.fetch_storage( + hash, + key.clone(), + maybe_child_trie.clone(), + fetch_type, + )? + else { + continue + }; + + let Some(previous_storage_result) = self.fetch_storage( + previous_hash, + key.clone(), + maybe_child_trie.clone(), + fetch_type, + )? + else { + continue + }; - continue - } + // For modified records we need to check the actual storage values. + if storage_result == previous_storage_result { + continue + } - // Report the result only if the value has changed. - let Some(previous_storage_result) = self.fetch_storage( - previous_hash, - key.clone(), - maybe_child_trie.clone(), - fetch_type, - )? - else { - continue + Some(storage_result) + }, }; - if storage_result != previous_storage_result { + if let Some(storage_result) = maybe_result { if !Self::send_result( &tx, storage_result, - ArchiveStorageDiffOperationType::Modified, + operation_type, maybe_child_trie_str.clone(), ) { return Ok(()) @@ -381,37 +413,6 @@ where } } - // Iterate over the keys of the previous block that are not found in the current block. - let deleted_keys = - previous_keys_iter - .into_iter() - .filter_map(|(key, present)| if !present { Some(key) } else { None }); - - for previous_key in deleted_keys { - let Some(fetch_type) = Self::starts_with(&previous_key, &items) else { - continue; - }; - - let Some(previous_storage_result) = self.fetch_storage( - previous_hash, - previous_key, - maybe_child_trie.clone(), - fetch_type, - )? - else { - continue - }; - - if !Self::send_result( - &tx, - previous_storage_result, - ArchiveStorageDiffOperationType::Deleted, - maybe_child_trie_str.clone(), - ) { - return Ok(()) - } - } - Ok(()) } From 55489c7c0fc6fe1a486db8aa6dda4b57192c2014 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 12 Nov 2024 10:12:34 +0200 Subject: [PATCH 40/70] archive: Adjust code comments Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/archive/archive_storage.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 7244635ddb41..19cd89516dbe 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -307,7 +307,9 @@ where let maybe_child_trie_str = items.first().and_then(|item| item.child_trie_key_string.clone()); - // Iterator over the current block. + // Iterator over the current block and previous block + // at the same time to compare the keys. This approach effectively + // leverages backpressure to avoid memory consumption. let mut keys_iter = self.client.raw_keys_iter(hash, maybe_child_trie.clone())?; let mut previous_keys_iter = self.client.raw_keys_iter(previous_hash, maybe_child_trie.clone())?; @@ -453,7 +455,6 @@ where let _ = tx.blocking_send(ArchiveStorageDiffEvent::err(error)); - // It ok to return here on the first encountered error. return } else { log::trace!( From 4f948901a9dc03ea7ad1a85d22648a256f9e8e9e Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 12 Nov 2024 10:43:59 +0200 Subject: [PATCH 41/70] archive: Rename starts_with to belongs_to_query Signed-off-by: Alexandru Vasile --- .../rpc-spec-v2/src/archive/archive_storage.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 19cd89516dbe..2475435d65b0 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -236,10 +236,15 @@ where } } - /// Check if the key starts with any of the provided items. + /// Check if the key belongs to the provided query items. /// - /// Returns a `FetchStorage` to indicate if the key should be fetched. - fn starts_with(key: &StorageKey, items: &[DiffDetails]) -> Option { + /// A key belongs to the query items when: + /// - the provided key is a prefix of the key in the query items. + /// - the query items are empty. + /// + /// Returns an optional `FetchStorageType` based on the query items. + /// If the key does not belong to the query items, returns `None`. + fn belongs_to_query(key: &StorageKey, items: &[DiffDetails]) -> Option { // User has requested all keys, by default this fallbacks to fetching the value. if items.is_empty() { return Some(FetchStorageType::Value) @@ -359,8 +364,8 @@ where (None, None) => break, }; - let Some(fetch_type) = Self::starts_with(&key, &items) else { - // The key does not start with any of the provided items. + let Some(fetch_type) = Self::belongs_to_query(&key, &items) else { + // The key does not belong the the query items. continue; }; From a7e54a59bae79da81951ceda28739483324c5195 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 12 Nov 2024 12:41:37 +0200 Subject: [PATCH 42/70] Update prdoc Signed-off-by: Alexandru Vasile --- prdoc/pr_5997.prdoc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/prdoc/pr_5997.prdoc b/prdoc/pr_5997.prdoc index b6a7baaf66e8..6bac36a44586 100644 --- a/prdoc/pr_5997.prdoc +++ b/prdoc/pr_5997.prdoc @@ -13,4 +13,6 @@ doc: crates: - name: sc-rpc-spec-v2 - bump: minor + bump: major + - name: sc-service + bump: patch From 25728a1e9334cfe670ea3b525d1002f3d3405287 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 14 Nov 2024 08:29:00 +0200 Subject: [PATCH 43/70] common/events: Transform StorageResult into StorageEvent Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/common/events.rs | 23 ++++++------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/common/events.rs b/substrate/client/rpc-spec-v2/src/common/events.rs index 198a60bf4cac..59cfd79cdcd2 100644 --- a/substrate/client/rpc-spec-v2/src/common/events.rs +++ b/substrate/client/rpc-spec-v2/src/common/events.rs @@ -105,24 +105,15 @@ pub struct StorageResultErr { /// The result of a storage call. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -#[serde(untagged)] -pub enum ArchiveStorageResult { +#[serde(rename_all = "camelCase")] +#[serde(tag = "event")] +pub enum ArchiveStorageEvent { /// Query generated a result. - Ok(ArchiveStorageMethodOk), + StorageResult(StorageResult), /// Query encountered an error. - Err(ArchiveStorageMethodErr), -} - -impl ArchiveStorageResult { - /// Create a new `ArchiveStorageResult::Ok` result. - pub fn ok(result: Vec, discarded_items: usize) -> Self { - Self::Ok(ArchiveStorageMethodOk { result, discarded_items }) - } - - /// Create a new `ArchiveStorageResult::Err` result. - pub fn err(error: String) -> Self { - Self::Err(ArchiveStorageMethodErr { error }) - } + StorageErr(ArchiveStorageMethodErr), + /// Operation storage is done. + StorageDone, } /// The result of a storage call. From 8860e7e3864f6347fb213bff39ac8989ef6e1bbd Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 14 Nov 2024 08:29:22 +0200 Subject: [PATCH 44/70] archive/api: Make archive_storage a subscription Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/archive/api.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/api.rs b/substrate/client/rpc-spec-v2/src/archive/api.rs index d3da879fc358..a524212ec82e 100644 --- a/substrate/client/rpc-spec-v2/src/archive/api.rs +++ b/substrate/client/rpc-spec-v2/src/archive/api.rs @@ -20,8 +20,7 @@ use crate::{ common::events::{ - ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageResult, - PaginatedStorageQuery, + ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageEvent, PaginatedStorageQuery, }, MethodResult, }; @@ -101,12 +100,17 @@ pub trait ArchiveApi { /// /// This method is unstable and subject to change in the future. #[method(name = "archive_unstable_storage", blocking)] + #[subscription( + name = "archive_unstable_storage" => "archive_unstable_storageEvent", + unsubscribe = "archive_unstable_stopStorage", + item = ArchiveStorageEvent, + )] fn archive_unstable_storage( &self, hash: Hash, items: Vec>, child_trie: Option, - ) -> RpcResult; + ); /// Returns the storage difference between two blocks. /// From 533ecb9bf16d8d05288d624280e2c40d30dc741a Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 14 Nov 2024 08:29:37 +0200 Subject: [PATCH 45/70] rpc-v2: Rename ArchiveResult into ArchiveEvent Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/api.rs | 1 - .../client/rpc-spec-v2/src/archive/archive.rs | 5 +- .../src/archive/archive_storage.rs | 16 +++---- .../client/rpc-spec-v2/src/archive/tests.rs | 46 +++++++++---------- .../rpc-spec-v2/src/chain_head/event.rs | 2 +- 5 files changed, 34 insertions(+), 36 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/api.rs b/substrate/client/rpc-spec-v2/src/archive/api.rs index a524212ec82e..020810ff7f1b 100644 --- a/substrate/client/rpc-spec-v2/src/archive/api.rs +++ b/substrate/client/rpc-spec-v2/src/archive/api.rs @@ -99,7 +99,6 @@ pub trait ArchiveApi { /// # Unstable /// /// This method is unstable and subject to change in the future. - #[method(name = "archive_unstable_storage", blocking)] #[subscription( name = "archive_unstable_storage" => "archive_unstable_storageEvent", unsubscribe = "archive_unstable_stopStorage", diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index 5dd0af8b8786..0eb6a3ca64e2 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -25,8 +25,7 @@ use crate::{ ArchiveApiServer, }, common::events::{ - ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageResult, - PaginatedStorageQuery, + ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageEvent, PaginatedStorageQuery, }, hex_string, MethodResult, SubscriptionTaskExecutor, }; @@ -263,7 +262,7 @@ where hash: Block::Hash, items: Vec>, child_trie: Option, - ) -> RpcResult { + ) -> RpcResult { let items = items .into_iter() .map(|query| { diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 2475435d65b0..1d6da33450ce 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -33,7 +33,7 @@ use crate::{ common::{ events::{ ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageDiffOperationType, - ArchiveStorageDiffResult, ArchiveStorageDiffType, ArchiveStorageResult, + ArchiveStorageDiffResult, ArchiveStorageDiffType, ArchiveStorageEvent, PaginatedStorageQuery, StorageQueryType, StorageResult, }, storage::{IterQueryType, QueryIter, Storage}, @@ -78,7 +78,7 @@ where hash: Block::Hash, mut items: Vec>, child_key: Option, - ) -> ArchiveStorageResult { + ) -> ArchiveStorageEvent { let discarded_items = items.len().saturating_sub(self.storage_max_queried_items); items.truncate(self.storage_max_queried_items); @@ -89,20 +89,20 @@ where match self.client.query_value(hash, &item.key, child_key.as_ref()) { Ok(Some(value)) => storage_results.push(value), Ok(None) => continue, - Err(error) => return ArchiveStorageResult::err(error), + Err(error) => return ArchiveStorageEvent::err(error), } }, StorageQueryType::Hash => match self.client.query_hash(hash, &item.key, child_key.as_ref()) { Ok(Some(value)) => storage_results.push(value), Ok(None) => continue, - Err(error) => return ArchiveStorageResult::err(error), + Err(error) => return ArchiveStorageEvent::err(error), }, StorageQueryType::ClosestDescendantMerkleValue => match self.client.query_merkle_value(hash, &item.key, child_key.as_ref()) { Ok(Some(value)) => storage_results.push(value), Ok(None) => continue, - Err(error) => return ArchiveStorageResult::err(error), + Err(error) => return ArchiveStorageEvent::err(error), }, StorageQueryType::DescendantsValues => { match self.client.query_iter_pagination( @@ -116,7 +116,7 @@ where self.storage_max_descendant_responses, ) { Ok((results, _)) => storage_results.extend(results), - Err(error) => return ArchiveStorageResult::err(error), + Err(error) => return ArchiveStorageEvent::err(error), } }, StorageQueryType::DescendantsHashes => { @@ -131,13 +131,13 @@ where self.storage_max_descendant_responses, ) { Ok((results, _)) => storage_results.extend(results), - Err(error) => return ArchiveStorageResult::err(error), + Err(error) => return ArchiveStorageEvent::err(error), } }, }; } - ArchiveStorageResult::ok(storage_results, discarded_items) + ArchiveStorageEvent::ok(storage_results, discarded_items) } } diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index 06445d96d125..8a08ec84e2a3 100644 --- a/substrate/client/rpc-spec-v2/src/archive/tests.rs +++ b/substrate/client/rpc-spec-v2/src/archive/tests.rs @@ -20,7 +20,7 @@ use crate::{ common::events::{ ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageDiffOperationType, ArchiveStorageDiffResult, ArchiveStorageDiffType, ArchiveStorageMethodOk, - ArchiveStorageResult, PaginatedStorageQuery, StorageQueryType, StorageResultType, + ArchiveStorageEvent, PaginatedStorageQuery, StorageQueryType, StorageResultType, }, hex_string, MethodResult, }; @@ -392,13 +392,13 @@ async fn archive_storage_hashes_values() { }, ]; - let result: ArchiveStorageResult = api + let result: ArchiveStorageEvent = api .call("archive_unstable_storage", rpc_params![&block_hash, items.clone()]) .await .unwrap(); match result { - ArchiveStorageResult::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { + ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { // Key has not been imported yet. assert_eq!(result.len(), 0); assert_eq!(discarded_items, 0); @@ -420,13 +420,13 @@ async fn archive_storage_hashes_values() { let expected_hash = format!("{:?}", Blake2Hasher::hash(&VALUE)); let expected_value = hex_string(&VALUE); - let result: ArchiveStorageResult = api + let result: ArchiveStorageEvent = api .call("archive_unstable_storage", rpc_params![&block_hash, items]) .await .unwrap(); match result { - ArchiveStorageResult::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { + ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { assert_eq!(result.len(), 4); assert_eq!(discarded_items, 0); @@ -457,7 +457,7 @@ async fn archive_storage_closest_merkle_value() { api: &RpcModule>>, block_hash: String, ) -> HashMap { - let result: ArchiveStorageResult = api + let result: ArchiveStorageEvent = api .call( "archive_unstable_storage", rpc_params![ @@ -514,7 +514,7 @@ async fn archive_storage_closest_merkle_value() { .unwrap(); let merkle_values: HashMap<_, _> = match result { - ArchiveStorageResult::Ok(ArchiveStorageMethodOk { result, .. }) => result + ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, .. }) => result .into_iter() .map(|res| { let value = match res.result { @@ -625,7 +625,7 @@ async fn archive_storage_paginate_iterations() { // Calling with an invalid hash. let invalid_hash = hex_string(&INVALID_HASH); - let result: ArchiveStorageResult = api + let result: ArchiveStorageEvent = api .call( "archive_unstable_storage", rpc_params![ @@ -640,12 +640,12 @@ async fn archive_storage_paginate_iterations() { .await .unwrap(); match result { - ArchiveStorageResult::Err(_) => (), + ArchiveStorageEvent::Err(_) => (), _ => panic!("Unexpected result"), }; // Valid call with storage at the key. - let result: ArchiveStorageResult = api + let result: ArchiveStorageEvent = api .call( "archive_unstable_storage", rpc_params![ @@ -660,7 +660,7 @@ async fn archive_storage_paginate_iterations() { .await .unwrap(); match result { - ArchiveStorageResult::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { + ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { assert_eq!(result.len(), 1); assert_eq!(discarded_items, 0); @@ -671,7 +671,7 @@ async fn archive_storage_paginate_iterations() { }; // Continue with pagination. - let result: ArchiveStorageResult = api + let result: ArchiveStorageEvent = api .call( "archive_unstable_storage", rpc_params![ @@ -686,7 +686,7 @@ async fn archive_storage_paginate_iterations() { .await .unwrap(); match result { - ArchiveStorageResult::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { + ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { assert_eq!(result.len(), 1); assert_eq!(discarded_items, 0); @@ -697,7 +697,7 @@ async fn archive_storage_paginate_iterations() { }; // Continue with pagination. - let result: ArchiveStorageResult = api + let result: ArchiveStorageEvent = api .call( "archive_unstable_storage", rpc_params![ @@ -712,7 +712,7 @@ async fn archive_storage_paginate_iterations() { .await .unwrap(); match result { - ArchiveStorageResult::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { + ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { assert_eq!(result.len(), 1); assert_eq!(discarded_items, 0); @@ -723,7 +723,7 @@ async fn archive_storage_paginate_iterations() { }; // Continue with pagination. - let result: ArchiveStorageResult = api + let result: ArchiveStorageEvent = api .call( "archive_unstable_storage", rpc_params![ @@ -738,7 +738,7 @@ async fn archive_storage_paginate_iterations() { .await .unwrap(); match result { - ArchiveStorageResult::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { + ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { assert_eq!(result.len(), 1); assert_eq!(discarded_items, 0); @@ -749,7 +749,7 @@ async fn archive_storage_paginate_iterations() { }; // Continue with pagination. - let result: ArchiveStorageResult = api + let result: ArchiveStorageEvent = api .call( "archive_unstable_storage", rpc_params![ @@ -764,7 +764,7 @@ async fn archive_storage_paginate_iterations() { .await .unwrap(); match result { - ArchiveStorageResult::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { + ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { assert_eq!(result.len(), 1); assert_eq!(discarded_items, 0); @@ -775,7 +775,7 @@ async fn archive_storage_paginate_iterations() { }; // Continue with pagination until no keys are returned. - let result: ArchiveStorageResult = api + let result: ArchiveStorageEvent = api .call( "archive_unstable_storage", rpc_params![ @@ -790,7 +790,7 @@ async fn archive_storage_paginate_iterations() { .await .unwrap(); match result { - ArchiveStorageResult::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { + ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { assert_eq!(result.len(), 0); assert_eq!(discarded_items, 0); }, @@ -815,7 +815,7 @@ async fn archive_storage_discarded_items() { client.import(BlockOrigin::Own, block.clone()).await.unwrap(); // Valid call with storage at the key. - let result: ArchiveStorageResult = api + let result: ArchiveStorageEvent = api .call( "archive_unstable_storage", rpc_params![ @@ -842,7 +842,7 @@ async fn archive_storage_discarded_items() { .await .unwrap(); match result { - ArchiveStorageResult::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { + ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { assert_eq!(result.len(), 1); assert_eq!(discarded_items, 2); diff --git a/substrate/client/rpc-spec-v2/src/chain_head/event.rs b/substrate/client/rpc-spec-v2/src/chain_head/event.rs index bd9863060910..b5571e5fbea7 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/event.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/event.rs @@ -235,7 +235,7 @@ pub struct OperationCallDone { pub output: String, } -/// The response of the `chainHead_call` method. +/// The response of the `chainHead_storage` method. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct OperationStorageItems { From 0849e8ee9f4d476271f546fcb53009139c25a430 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 14 Nov 2024 15:00:22 +0200 Subject: [PATCH 46/70] events: Add wrappers for ArchiveStorageEvent Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/common/events.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/substrate/client/rpc-spec-v2/src/common/events.rs b/substrate/client/rpc-spec-v2/src/common/events.rs index 59cfd79cdcd2..2578cd988a8c 100644 --- a/substrate/client/rpc-spec-v2/src/common/events.rs +++ b/substrate/client/rpc-spec-v2/src/common/events.rs @@ -116,6 +116,23 @@ pub enum ArchiveStorageEvent { StorageDone, } +impl ArchiveStorageEvent { + /// Create a new `ArchiveStorageEvent::StorageErr` event. + pub fn err(error: String) -> Self { + Self::StorageErr(ArchiveStorageMethodErr { error }) + } + + /// Checks if the event is a `StorageDone` event. + pub fn is_done(&self) -> bool { + matches!(self, Self::StorageDone) + } + + /// Checks if the event is a `StorageErr` event. + pub fn is_err(&self) -> bool { + matches!(self, Self::StorageErr(_)) + } +} + /// The result of a storage call. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] From a2f92b6d9ff1fd79aa449435f0ab322b368a4b00 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 14 Nov 2024 15:44:47 +0200 Subject: [PATCH 47/70] common/storage: Introduce a common storage subcription handler Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/common/storage.rs | 148 +++++++++++------- 1 file changed, 94 insertions(+), 54 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/common/storage.rs b/substrate/client/rpc-spec-v2/src/common/storage.rs index 673e20b2bc78..4affec265097 100644 --- a/substrate/client/rpc-spec-v2/src/common/storage.rs +++ b/substrate/client/rpc-spec-v2/src/common/storage.rs @@ -24,7 +24,7 @@ use sc_client_api::{Backend, ChildInfo, StorageKey, StorageProvider}; use sp_runtime::traits::Block as BlockT; use tokio::sync::mpsc; -use super::events::{StorageResult, StorageResultType}; +use super::events::{StorageQuery, StorageQueryType, StorageResult, StorageResultType}; use crate::hex_string; /// Call into the storage of blocks. @@ -70,9 +70,6 @@ pub enum IterQueryType { /// The result of making a query call. pub type QueryResult = Result, String>; -/// The result of iterating over keys. -pub type QueryIterResult = Result<(Vec, Option), String>; - impl Storage where Block: BlockT + 'static, @@ -199,56 +196,6 @@ where } } - /// Iterate over at most the provided number of keys. - /// - /// Returns the storage result with a potential next key to resume iteration. - pub fn query_iter_pagination( - &self, - query: QueryIter, - hash: Block::Hash, - child_key: Option<&ChildInfo>, - count: usize, - ) -> QueryIterResult { - let QueryIter { ty, query_key, pagination_start_key } = query; - - let mut keys_iter = if let Some(child_key) = child_key { - self.client.child_storage_keys( - hash, - child_key.to_owned(), - Some(&query_key), - pagination_start_key.as_ref(), - ) - } else { - self.client.storage_keys(hash, Some(&query_key), pagination_start_key.as_ref()) - } - .map_err(|err| err.to_string())?; - - let mut ret = Vec::with_capacity(count); - let mut next_pagination_key = None; - for _ in 0..count { - let Some(key) = keys_iter.next() else { break }; - - next_pagination_key = Some(key.clone()); - - let result = match ty { - IterQueryType::Value => self.query_value(hash, &key, child_key), - IterQueryType::Hash => self.query_hash(hash, &key, child_key), - }?; - - if let Some(value) = result { - ret.push(value); - } - } - - // Save the next key if any to continue the iteration. - let maybe_next_query = keys_iter.next().map(|_| QueryIter { - ty, - query_key, - pagination_start_key: next_pagination_key, - }); - Ok((ret, maybe_next_query)) - } - /// Raw iterator over the keys. pub fn raw_keys_iter( &self, @@ -264,3 +211,96 @@ where keys_iter.map_err(|err| err.to_string()) } } + +/// Generates storage events for `chainHead_storage` and `archive_storage` subscriptions. +pub struct StorageSubscriptionClient { + /// Storage client. + client: Storage, + _phandom: PhantomData<(BE, Block)>, +} + +impl Clone for StorageSubscriptionClient { + fn clone(&self) -> Self { + Self { client: self.client.clone(), _phandom: PhantomData } + } +} + +impl StorageSubscriptionClient { + /// Constructs a new [`StorageSubscriptionClient`]. + pub fn new(client: Arc) -> Self { + Self { client: Storage::new(client), _phandom: PhantomData } + } +} + +impl StorageSubscriptionClient +where + Block: BlockT + 'static, + BE: Backend + 'static, + Client: StorageProvider + Send + Sync + 'static, +{ + /// Generate storage events to the provided sender. + pub async fn generate_events( + &mut self, + hash: Block::Hash, + items: Vec>, + child_key: Option, + tx: mpsc::Sender, + ) -> Result<(), tokio::task::JoinError> { + let this = self.clone(); + + tokio::task::spawn_blocking(move || { + for item in items { + match item.query_type { + StorageQueryType::Value => { + let rp = this.client.query_value(hash, &item.key, child_key.as_ref()); + if tx.blocking_send(rp).is_err() { + break; + } + }, + StorageQueryType::Hash => { + let rp = this.client.query_hash(hash, &item.key, child_key.as_ref()); + if tx.blocking_send(rp).is_err() { + break; + } + }, + StorageQueryType::ClosestDescendantMerkleValue => { + let rp = + this.client.query_merkle_value(hash, &item.key, child_key.as_ref()); + if tx.blocking_send(rp).is_err() { + break; + } + }, + StorageQueryType::DescendantsValues => { + let query = QueryIter { + query_key: item.key, + ty: IterQueryType::Value, + pagination_start_key: None, + }; + this.client.query_iter_pagination_with_producer( + query, + hash, + child_key.as_ref(), + &tx, + ) + }, + StorageQueryType::DescendantsHashes => { + let query = QueryIter { + query_key: item.key, + ty: IterQueryType::Hash, + pagination_start_key: None, + }; + this.client.query_iter_pagination_with_producer( + query, + hash, + child_key.as_ref(), + &tx, + ) + }, + } + } + }) + .await?; + + Ok(()) + } +} From 7fec81a395ca05a019c17aa68fbb4d0d572410c1 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 14 Nov 2024 15:45:07 +0200 Subject: [PATCH 48/70] archive/api: Remove pagination items from archive storage Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/archive/api.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/api.rs b/substrate/client/rpc-spec-v2/src/archive/api.rs index 020810ff7f1b..d1b0b7bbdefb 100644 --- a/substrate/client/rpc-spec-v2/src/archive/api.rs +++ b/substrate/client/rpc-spec-v2/src/archive/api.rs @@ -20,7 +20,7 @@ use crate::{ common::events::{ - ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageEvent, PaginatedStorageQuery, + ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageEvent, StorageQuery, }, MethodResult, }; @@ -107,7 +107,7 @@ pub trait ArchiveApi { fn archive_unstable_storage( &self, hash: Hash, - items: Vec>, + items: Vec>, child_trie: Option, ); From b6523d2cf6aa4deb63fb4d2821f0a3e9d8c22ba5 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 14 Nov 2024 15:45:26 +0200 Subject: [PATCH 49/70] common/events: Add constructor methods for clarity Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/common/events.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/substrate/client/rpc-spec-v2/src/common/events.rs b/substrate/client/rpc-spec-v2/src/common/events.rs index 2578cd988a8c..d01588031a5c 100644 --- a/substrate/client/rpc-spec-v2/src/common/events.rs +++ b/substrate/client/rpc-spec-v2/src/common/events.rs @@ -122,6 +122,11 @@ impl ArchiveStorageEvent { Self::StorageErr(ArchiveStorageMethodErr { error }) } + /// Create a new `ArchiveStorageEvent::StorageResult` event. + pub fn result(result: StorageResult) -> Self { + Self::StorageResult(result) + } + /// Checks if the event is a `StorageDone` event. pub fn is_done(&self) -> bool { matches!(self, Self::StorageDone) From bb655839ca1090271f212e9571fd4131af8ccb21 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 14 Nov 2024 15:45:51 +0200 Subject: [PATCH 50/70] archive: Remove ArchiveStorage in favour of common archive object Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/archive.rs | 104 +++++++++++------ .../src/archive/archive_storage.rs | 105 +----------------- 2 files changed, 69 insertions(+), 140 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index 0eb6a3ca64e2..f17479e15839 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -20,12 +20,15 @@ use crate::{ archive::{ - archive_storage::{deduplicate_storage_diff_items, ArchiveStorage, ArchiveStorageDiff}, + archive_storage::{deduplicate_storage_diff_items, ArchiveStorageDiff}, error::Error as ArchiveError, ArchiveApiServer, }, - common::events::{ - ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageEvent, PaginatedStorageQuery, + common::{ + events::{ + ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageEvent, StorageQuery, + }, + storage::{QueryResult, StorageSubscriptionClient}, }, hex_string, MethodResult, SubscriptionTaskExecutor, }; @@ -259,47 +262,53 @@ where fn archive_unstable_storage( &self, + pending: PendingSubscriptionSink, hash: Block::Hash, - items: Vec>, + items: Vec>, child_trie: Option, - ) -> RpcResult { - let items = items - .into_iter() - .map(|query| { - let key = StorageKey(parse_hex_param(query.key)?); - let pagination_start_key = query - .pagination_start_key - .map(|key| parse_hex_param(key).map(|key| StorageKey(key))) - .transpose()?; - - // Paginated start key is only supported - if pagination_start_key.is_some() && !query.query_type.is_descendant_query() { - return Err(ArchiveError::InvalidParam( - "Pagination start key is only supported for descendants queries" - .to_string(), - )) - } + ) { + let mut storage_client = + StorageSubscriptionClient::::new(self.client.clone()); + + let fut = async move { + let Ok(mut sink) = pending.accept().await.map(Subscription::from) else { return }; - Ok(PaginatedStorageQuery { - key, - query_type: query.query_type, - pagination_start_key, + let items = match items + .into_iter() + .map(|query| { + let key = StorageKey(parse_hex_param(query.key)?); + Ok(StorageQuery { key, query_type: query.query_type }) }) - }) - .collect::, ArchiveError>>()?; + .collect::, ArchiveError>>() + { + Ok(items) => items, + Err(error) => { + let _ = sink.send(&ArchiveStorageEvent::err(error.to_string())); + return + }, + }; - let child_trie = child_trie - .map(|child_trie| parse_hex_param(child_trie)) - .transpose()? - .map(ChildInfo::new_default_from_vec); + let child_trie = child_trie.map(|child_trie| parse_hex_param(child_trie)).transpose(); + let child_trie = match child_trie { + Ok(child_trie) => child_trie.map(ChildInfo::new_default_from_vec), + Err(error) => { + let _ = sink.send(&ArchiveStorageEvent::err(error.to_string())); + return + }, + }; + + let (tx, mut rx) = tokio::sync::mpsc::channel(STORAGE_QUERY_BUF); + let storage_fut = storage_client.generate_events(hash, items, child_trie, tx); - let storage_client = ArchiveStorage::new( - self.client.clone(), - self.storage_max_descendant_responses, - self.storage_max_queried_items, - ); + // We don't care about the return value of this join: + // - process_events might encounter an error (if the client disconnected) + // - storage_fut might encounter an error while processing a trie queries and + // the error is propagated via the sink. + let _ = futures::future::join(storage_fut, process_storage_events(&mut rx, &mut sink)) + .await; + }; - Ok(storage_client.handle_query(hash, items, child_trie)) + self.executor.spawn("substrate-rpc-subscription", Some("rpc"), fut.boxed()); } fn archive_unstable_storage_diff( @@ -371,3 +380,24 @@ async fn process_events(rx: &mut mpsc::Receiver, sink: } } } + +/// Sends all the events to the sink. +async fn process_storage_events(rx: &mut mpsc::Receiver, sink: &mut Subscription) { + while let Some(event) = rx.recv().await { + match event { + Ok(None) => continue, + + Ok(Some(event)) => + if sink.send(&ArchiveStorageEvent::result(event)).await.is_err() { + return + }, + + Err(error) => + if sink.send(&ArchiveStorageEvent::err(error)).await.is_err() { + return + }, + } + } + + let _ = sink.send(&ArchiveStorageEvent::StorageDone).await; +} diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 1d6da33450ce..5f57b9c6f900 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -33,114 +33,13 @@ use crate::{ common::{ events::{ ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageDiffOperationType, - ArchiveStorageDiffResult, ArchiveStorageDiffType, ArchiveStorageEvent, - PaginatedStorageQuery, StorageQueryType, StorageResult, + ArchiveStorageDiffResult, ArchiveStorageDiffType, StorageResult, }, - storage::{IterQueryType, QueryIter, Storage}, + storage::Storage, }, }; use tokio::sync::mpsc; -/// Generates the events of the `archive_storage` method. -pub struct ArchiveStorage { - /// Storage client. - client: Storage, - /// The maximum number of responses the API can return for a descendant query at a time. - storage_max_descendant_responses: usize, - /// The maximum number of queried items allowed for the `archive_storage` at a time. - storage_max_queried_items: usize, -} - -impl ArchiveStorage { - /// Constructs a new [`ArchiveStorage`]. - pub fn new( - client: Arc, - storage_max_descendant_responses: usize, - storage_max_queried_items: usize, - ) -> Self { - Self { - client: Storage::new(client), - storage_max_descendant_responses, - storage_max_queried_items, - } - } -} - -impl ArchiveStorage -where - Block: BlockT + 'static, - BE: Backend + 'static, - Client: StorageProvider + 'static, -{ - /// Generate the response of the `archive_storage` method. - pub fn handle_query( - &self, - hash: Block::Hash, - mut items: Vec>, - child_key: Option, - ) -> ArchiveStorageEvent { - let discarded_items = items.len().saturating_sub(self.storage_max_queried_items); - items.truncate(self.storage_max_queried_items); - - let mut storage_results = Vec::with_capacity(items.len()); - for item in items { - match item.query_type { - StorageQueryType::Value => { - match self.client.query_value(hash, &item.key, child_key.as_ref()) { - Ok(Some(value)) => storage_results.push(value), - Ok(None) => continue, - Err(error) => return ArchiveStorageEvent::err(error), - } - }, - StorageQueryType::Hash => - match self.client.query_hash(hash, &item.key, child_key.as_ref()) { - Ok(Some(value)) => storage_results.push(value), - Ok(None) => continue, - Err(error) => return ArchiveStorageEvent::err(error), - }, - StorageQueryType::ClosestDescendantMerkleValue => - match self.client.query_merkle_value(hash, &item.key, child_key.as_ref()) { - Ok(Some(value)) => storage_results.push(value), - Ok(None) => continue, - Err(error) => return ArchiveStorageEvent::err(error), - }, - StorageQueryType::DescendantsValues => { - match self.client.query_iter_pagination( - QueryIter { - query_key: item.key, - ty: IterQueryType::Value, - pagination_start_key: item.pagination_start_key, - }, - hash, - child_key.as_ref(), - self.storage_max_descendant_responses, - ) { - Ok((results, _)) => storage_results.extend(results), - Err(error) => return ArchiveStorageEvent::err(error), - } - }, - StorageQueryType::DescendantsHashes => { - match self.client.query_iter_pagination( - QueryIter { - query_key: item.key, - ty: IterQueryType::Hash, - pagination_start_key: item.pagination_start_key, - }, - hash, - child_key.as_ref(), - self.storage_max_descendant_responses, - ) { - Ok((results, _)) => storage_results.extend(results), - Err(error) => return ArchiveStorageEvent::err(error), - } - }, - }; - } - - ArchiveStorageEvent::ok(storage_results, discarded_items) - } -} - /// Parse hex-encoded string parameter as raw bytes. /// /// If the parsing fails, returns an error propagated to the RPC method. From 19176726e05db21773a6180f0355f29a467b3ac6 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 14 Nov 2024 15:47:16 +0200 Subject: [PATCH 51/70] archive: Remove config for storage parameters and use backpressure instead Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/archive.rs | 46 +------------------ .../client/rpc-spec-v2/src/archive/mod.rs | 2 +- substrate/client/service/src/builder.rs | 2 - 3 files changed, 2 insertions(+), 48 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index f17479e15839..0d7b2106315a 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -59,42 +59,12 @@ use tokio::sync::mpsc; pub(crate) const LOG_TARGET: &str = "rpc-spec-v2::archive"; -/// The configuration of [`Archive`]. -pub struct ArchiveConfig { - /// The maximum number of items the `archive_storage` can return for a descendant query before - /// pagination is required. - pub max_descendant_responses: usize, - /// The maximum number of queried items allowed for the `archive_storage` at a time. - pub max_queried_items: usize, -} - -/// The maximum number of items the `archive_storage` can return for a descendant query before -/// pagination is required. -/// -/// Note: this is identical to the `chainHead` value. -const MAX_DESCENDANT_RESPONSES: usize = 5; - -/// The maximum number of queried items allowed for the `archive_storage` at a time. -/// -/// Note: A queried item can also be a descendant query which can return up to -/// `MAX_DESCENDANT_RESPONSES`. -const MAX_QUERIED_ITEMS: usize = 8; - /// The buffer capacity for each storage query. /// /// This is small because the underlying JSON-RPC server has /// its down buffer capacity per connection as well. const STORAGE_QUERY_BUF: usize = 16; -impl Default for ArchiveConfig { - fn default() -> Self { - Self { - max_descendant_responses: MAX_DESCENDANT_RESPONSES, - max_queried_items: MAX_QUERIED_ITEMS, - } - } -} - /// An API for archive RPC calls. pub struct Archive, Block: BlockT, Client> { /// Substrate client. @@ -105,11 +75,6 @@ pub struct Archive, Block: BlockT, Client> { executor: SubscriptionTaskExecutor, /// The hexadecimal encoded hash of the genesis block. genesis_hash: String, - /// The maximum number of items the `archive_storage` can return for a descendant query before - /// pagination is required. - storage_max_descendant_responses: usize, - /// The maximum number of queried items allowed for the `archive_storage` at a time. - storage_max_queried_items: usize, /// Phantom member to pin the block type. _phantom: PhantomData, } @@ -121,18 +86,9 @@ impl, Block: BlockT, Client> Archive { backend: Arc, genesis_hash: GenesisHash, executor: SubscriptionTaskExecutor, - config: ArchiveConfig, ) -> Self { let genesis_hash = hex_string(&genesis_hash.as_ref()); - Self { - client, - backend, - executor, - genesis_hash, - storage_max_descendant_responses: config.max_descendant_responses, - storage_max_queried_items: config.max_queried_items, - _phantom: PhantomData, - } + Self { client, backend, executor, genesis_hash, _phantom: PhantomData } } } diff --git a/substrate/client/rpc-spec-v2/src/archive/mod.rs b/substrate/client/rpc-spec-v2/src/archive/mod.rs index 5f020c203eab..14fa104c113a 100644 --- a/substrate/client/rpc-spec-v2/src/archive/mod.rs +++ b/substrate/client/rpc-spec-v2/src/archive/mod.rs @@ -32,4 +32,4 @@ pub mod archive; pub mod error; pub use api::ArchiveApiServer; -pub use archive::{Archive, ArchiveConfig}; +pub use archive::Archive; diff --git a/substrate/client/service/src/builder.rs b/substrate/client/service/src/builder.rs index b4ea8be183bf..df991e84c576 100644 --- a/substrate/client/service/src/builder.rs +++ b/substrate/client/service/src/builder.rs @@ -732,8 +732,6 @@ where backend.clone(), genesis_hash, task_executor.clone(), - // Defaults to sensible limits for the `Archive`. - sc_rpc_spec_v2::archive::ArchiveConfig::default(), ) .into_rpc(); rpc_api.merge(archive_v2).map_err(|e| Error::Application(e.into()))?; From 702f52175d1f47e926cb3d5bfc126b8bb7ba2421 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 14 Nov 2024 17:24:32 +0200 Subject: [PATCH 52/70] archive/storage: Adjust testing to subscription based approach Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/tests.rs | 448 ++++++------------ .../client/rpc-spec-v2/src/common/events.rs | 5 + 2 files changed, 150 insertions(+), 303 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index 8a08ec84e2a3..7e240dd4cd3d 100644 --- a/substrate/client/rpc-spec-v2/src/archive/tests.rs +++ b/substrate/client/rpc-spec-v2/src/archive/tests.rs @@ -19,16 +19,13 @@ use crate::{ common::events::{ ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageDiffOperationType, - ArchiveStorageDiffResult, ArchiveStorageDiffType, ArchiveStorageMethodOk, - ArchiveStorageEvent, PaginatedStorageQuery, StorageQueryType, StorageResultType, + ArchiveStorageDiffResult, ArchiveStorageDiffType, ArchiveStorageEvent, StorageQuery, + StorageQueryType, StorageResult, StorageResultType, }, hex_string, MethodResult, }; -use super::{ - archive::{Archive, ArchiveConfig}, - *, -}; +use super::{archive::Archive, *}; use assert_matches::assert_matches; use codec::{Decode, Encode}; @@ -55,8 +52,6 @@ use substrate_test_runtime_client::{ const CHAIN_GENESIS: [u8; 32] = [0; 32]; const INVALID_HASH: [u8; 32] = [1; 32]; -const MAX_PAGINATION_LIMIT: usize = 5; -const MAX_QUERIED_LIMIT: usize = 5; const KEY: &[u8] = b":mock"; const VALUE: &[u8] = b"hello world"; const CHILD_STORAGE_KEY: &[u8] = b"child"; @@ -65,10 +60,7 @@ const CHILD_VALUE: &[u8] = b"child value"; type Header = substrate_test_runtime_client::runtime::Header; type Block = substrate_test_runtime_client::runtime::Block; -fn setup_api( - max_descendant_responses: usize, - max_queried_items: usize, -) -> (Arc>, RpcModule>>) { +fn setup_api() -> (Arc>, RpcModule>>) { let child_info = ChildInfo::new_default(CHILD_STORAGE_KEY); let builder = TestClientBuilder::new().add_extra_child_storage( &child_info, @@ -83,7 +75,6 @@ fn setup_api( backend, CHAIN_GENESIS, Arc::new(TokioTestExecutor::default()), - ArchiveConfig { max_descendant_responses, max_queried_items }, ) .into_rpc(); @@ -101,7 +92,7 @@ async fn get_next_event(sub: &mut RpcSubscriptio #[tokio::test] async fn archive_genesis() { - let (_client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); + let (_client, api) = setup_api(); let genesis: String = api.call("archive_unstable_genesisHash", EmptyParams::new()).await.unwrap(); @@ -110,7 +101,7 @@ async fn archive_genesis() { #[tokio::test] async fn archive_body() { - let (client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); + let (client, api) = setup_api(); // Invalid block hash. let invalid_hash = hex_string(&INVALID_HASH); @@ -144,7 +135,7 @@ async fn archive_body() { #[tokio::test] async fn archive_header() { - let (client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); + let (client, api) = setup_api(); // Invalid block hash. let invalid_hash = hex_string(&INVALID_HASH); @@ -178,7 +169,7 @@ async fn archive_header() { #[tokio::test] async fn archive_finalized_height() { - let (client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); + let (client, api) = setup_api(); let client_height: u32 = client.info().finalized_number.saturated_into(); @@ -190,7 +181,7 @@ async fn archive_finalized_height() { #[tokio::test] async fn archive_hash_by_height() { - let (client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); + let (client, api) = setup_api(); // Genesis height. let hashes: Vec = api.call("archive_unstable_hashByHeight", [0]).await.unwrap(); @@ -296,7 +287,7 @@ async fn archive_hash_by_height() { #[tokio::test] async fn archive_call() { - let (client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); + let (client, api) = setup_api(); let invalid_hash = hex_string(&INVALID_HASH); // Invalid parameter (non-hex). @@ -355,7 +346,7 @@ async fn archive_call() { #[tokio::test] async fn archive_storage_hashes_values() { - let (client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); + let (client, api) = setup_api(); let block = BlockBuilderBuilder::new(&*client) .on_parent_block(client.chain_info().genesis_hash) @@ -369,42 +360,23 @@ async fn archive_storage_hashes_values() { let block_hash = format!("{:?}", block.header.hash()); let key = hex_string(&KEY); - let items: Vec> = vec![ - PaginatedStorageQuery { - key: key.clone(), - query_type: StorageQueryType::DescendantsHashes, - pagination_start_key: None, - }, - PaginatedStorageQuery { - key: key.clone(), - query_type: StorageQueryType::DescendantsValues, - pagination_start_key: None, - }, - PaginatedStorageQuery { - key: key.clone(), - query_type: StorageQueryType::Hash, - pagination_start_key: None, - }, - PaginatedStorageQuery { - key: key.clone(), - query_type: StorageQueryType::Value, - pagination_start_key: None, - }, + let items: Vec> = vec![ + StorageQuery { key: key.clone(), query_type: StorageQueryType::DescendantsHashes }, + StorageQuery { key: key.clone(), query_type: StorageQueryType::DescendantsValues }, + StorageQuery { key: key.clone(), query_type: StorageQueryType::Hash }, + StorageQuery { key: key.clone(), query_type: StorageQueryType::Value }, ]; - let result: ArchiveStorageEvent = api - .call("archive_unstable_storage", rpc_params![&block_hash, items.clone()]) + let mut sub = api + .subscribe_unbounded("archive_unstable_storage", rpc_params![&block_hash, items.clone()]) .await .unwrap(); - match result { - ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { - // Key has not been imported yet. - assert_eq!(result.len(), 0); - assert_eq!(discarded_items, 0); - }, - _ => panic!("Unexpected result"), - }; + // Key has not been imported yet. + assert_eq!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::StorageDone, + ); // Import a block with the given key value pair. let mut builder = BlockBuilderBuilder::new(&*client) @@ -420,32 +392,52 @@ async fn archive_storage_hashes_values() { let expected_hash = format!("{:?}", Blake2Hasher::hash(&VALUE)); let expected_value = hex_string(&VALUE); - let result: ArchiveStorageEvent = api - .call("archive_unstable_storage", rpc_params![&block_hash, items]) + let mut sub = api + .subscribe_unbounded("archive_unstable_storage", rpc_params![&block_hash, items]) .await .unwrap(); - match result { - ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { - assert_eq!(result.len(), 4); - assert_eq!(discarded_items, 0); - - assert_eq!(result[0].key, key); - assert_eq!(result[0].result, StorageResultType::Hash(expected_hash.clone())); - assert_eq!(result[1].key, key); - assert_eq!(result[1].result, StorageResultType::Value(expected_value.clone())); - assert_eq!(result[2].key, key); - assert_eq!(result[2].result, StorageResultType::Hash(expected_hash)); - assert_eq!(result[3].key, key); - assert_eq!(result[3].result, StorageResultType::Value(expected_value)); - }, - _ => panic!("Unexpected result"), - }; + assert_eq!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::StorageResult(StorageResult { + key: key.clone(), + result: StorageResultType::Hash(expected_hash.clone()), + }), + ); + + assert_eq!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::StorageResult(StorageResult { + key: key.clone(), + result: StorageResultType::Value(expected_value.clone()), + }), + ); + + assert_eq!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::StorageResult(StorageResult { + key: key.clone(), + result: StorageResultType::Hash(expected_hash), + }), + ); + + assert_eq!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::StorageResult(StorageResult { + key: key.clone(), + result: StorageResultType::Value(expected_value), + }), + ); + + assert_matches!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::StorageDone + ); } #[tokio::test] async fn archive_storage_closest_merkle_value() { - let (client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); + let (client, api) = setup_api(); /// The core of this test. /// @@ -457,55 +449,47 @@ async fn archive_storage_closest_merkle_value() { api: &RpcModule>>, block_hash: String, ) -> HashMap { - let result: ArchiveStorageEvent = api - .call( + let mut sub = api + .subscribe_unbounded( "archive_unstable_storage", rpc_params![ &block_hash, vec![ - PaginatedStorageQuery { + StorageQuery { key: hex_string(b":AAAA"), query_type: StorageQueryType::ClosestDescendantMerkleValue, - pagination_start_key: None, }, - PaginatedStorageQuery { + StorageQuery { key: hex_string(b":AAAB"), query_type: StorageQueryType::ClosestDescendantMerkleValue, - pagination_start_key: None, }, // Key with descendant. - PaginatedStorageQuery { + StorageQuery { key: hex_string(b":A"), query_type: StorageQueryType::ClosestDescendantMerkleValue, - pagination_start_key: None, }, - PaginatedStorageQuery { + StorageQuery { key: hex_string(b":AA"), query_type: StorageQueryType::ClosestDescendantMerkleValue, - pagination_start_key: None, }, // Keys below this comment do not produce a result. // Key that exceed the keyspace of the trie. - PaginatedStorageQuery { + StorageQuery { key: hex_string(b":AAAAX"), query_type: StorageQueryType::ClosestDescendantMerkleValue, - pagination_start_key: None, }, - PaginatedStorageQuery { + StorageQuery { key: hex_string(b":AAABX"), query_type: StorageQueryType::ClosestDescendantMerkleValue, - pagination_start_key: None, }, // Key that are not part of the trie. - PaginatedStorageQuery { + StorageQuery { key: hex_string(b":AAX"), query_type: StorageQueryType::ClosestDescendantMerkleValue, - pagination_start_key: None, }, - PaginatedStorageQuery { + StorageQuery { key: hex_string(b":AAAX"), query_type: StorageQueryType::ClosestDescendantMerkleValue, - pagination_start_key: None, }, ] ], @@ -513,19 +497,21 @@ async fn archive_storage_closest_merkle_value() { .await .unwrap(); - let merkle_values: HashMap<_, _> = match result { - ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, .. }) => result - .into_iter() - .map(|res| { - let value = match res.result { + let mut merkle_values = HashMap::new(); + loop { + let event = get_next_event::(&mut sub).await; + match event { + ArchiveStorageEvent::StorageResult(result) => { + let str_result = match result.result { StorageResultType::ClosestDescendantMerkleValue(value) => value, - _ => panic!("Unexpected StorageResultType"), + _ => panic!("Unexpected result type"), }; - (res.key, value) - }) - .collect(), - _ => panic!("Unexpected result"), - }; + merkle_values.insert(result.key, str_result); + }, + ArchiveStorageEvent::StorageErr(err) => panic!("Unexpected error {err:?}"), + ArchiveStorageEvent::StorageDone => break, + } + } // Response for AAAA, AAAB, A and AA. assert_eq!(merkle_values.len(), 4); @@ -604,9 +590,9 @@ async fn archive_storage_closest_merkle_value() { } #[tokio::test] -async fn archive_storage_paginate_iterations() { +async fn archive_storage_iterations() { // 1 iteration allowed before pagination kicks in. - let (client, api) = setup_api(1, MAX_QUERIED_LIMIT); + let (client, api) = setup_api(); // Import a new block with storage changes. let mut builder = BlockBuilderBuilder::new(&*client) @@ -625,237 +611,93 @@ async fn archive_storage_paginate_iterations() { // Calling with an invalid hash. let invalid_hash = hex_string(&INVALID_HASH); - let result: ArchiveStorageEvent = api - .call( + let mut sub = api + .subscribe_unbounded( "archive_unstable_storage", rpc_params![ &invalid_hash, - vec![PaginatedStorageQuery { - key: hex_string(b":m"), - query_type: StorageQueryType::DescendantsValues, - pagination_start_key: None, - }] - ], - ) - .await - .unwrap(); - match result { - ArchiveStorageEvent::Err(_) => (), - _ => panic!("Unexpected result"), - }; - - // Valid call with storage at the key. - let result: ArchiveStorageEvent = api - .call( - "archive_unstable_storage", - rpc_params![ - &block_hash, - vec![PaginatedStorageQuery { - key: hex_string(b":m"), - query_type: StorageQueryType::DescendantsValues, - pagination_start_key: None, - }] - ], - ) - .await - .unwrap(); - match result { - ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { - assert_eq!(result.len(), 1); - assert_eq!(discarded_items, 0); - - assert_eq!(result[0].key, hex_string(b":m")); - assert_eq!(result[0].result, StorageResultType::Value(hex_string(b"a"))); - }, - _ => panic!("Unexpected result"), - }; - - // Continue with pagination. - let result: ArchiveStorageEvent = api - .call( - "archive_unstable_storage", - rpc_params![ - &block_hash, - vec![PaginatedStorageQuery { - key: hex_string(b":m"), - query_type: StorageQueryType::DescendantsValues, - pagination_start_key: Some(hex_string(b":m")), - }] - ], - ) - .await - .unwrap(); - match result { - ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { - assert_eq!(result.len(), 1); - assert_eq!(discarded_items, 0); - - assert_eq!(result[0].key, hex_string(b":mo")); - assert_eq!(result[0].result, StorageResultType::Value(hex_string(b"ab"))); - }, - _ => panic!("Unexpected result"), - }; - - // Continue with pagination. - let result: ArchiveStorageEvent = api - .call( - "archive_unstable_storage", - rpc_params![ - &block_hash, - vec![PaginatedStorageQuery { + vec![StorageQuery { key: hex_string(b":m"), query_type: StorageQueryType::DescendantsValues, - pagination_start_key: Some(hex_string(b":mo")), }] ], ) .await .unwrap(); - match result { - ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { - assert_eq!(result.len(), 1); - assert_eq!(discarded_items, 0); - - assert_eq!(result[0].key, hex_string(b":moD")); - assert_eq!(result[0].result, StorageResultType::Value(hex_string(b"abcmoD"))); - }, - _ => panic!("Unexpected result"), - }; - // Continue with pagination. - let result: ArchiveStorageEvent = api - .call( - "archive_unstable_storage", - rpc_params![ - &block_hash, - vec![PaginatedStorageQuery { - key: hex_string(b":m"), - query_type: StorageQueryType::DescendantsValues, - pagination_start_key: Some(hex_string(b":moD")), - }] - ], - ) - .await - .unwrap(); - match result { - ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { - assert_eq!(result.len(), 1); - assert_eq!(discarded_items, 0); - - assert_eq!(result[0].key, hex_string(b":moc")); - assert_eq!(result[0].result, StorageResultType::Value(hex_string(b"abc"))); - }, - _ => panic!("Unexpected result"), - }; + assert_matches!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::StorageErr(_) + ); + assert_matches!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::StorageDone + ); - // Continue with pagination. - let result: ArchiveStorageEvent = api - .call( + // Valid call with storage at the key. + let mut sub = api + .subscribe_unbounded( "archive_unstable_storage", rpc_params![ &block_hash, - vec![PaginatedStorageQuery { + vec![StorageQuery { key: hex_string(b":m"), query_type: StorageQueryType::DescendantsValues, - pagination_start_key: Some(hex_string(b":moc")), }] ], ) .await .unwrap(); - match result { - ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { - assert_eq!(result.len(), 1); - assert_eq!(discarded_items, 0); - assert_eq!(result[0].key, hex_string(b":mock")); - assert_eq!(result[0].result, StorageResultType::Value(hex_string(b"abcd"))); - }, - _ => panic!("Unexpected result"), - }; + assert_eq!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::StorageResult(StorageResult { + key: hex_string(b":m"), + result: StorageResultType::Value(hex_string(b"a")), + }) + ); - // Continue with pagination until no keys are returned. - let result: ArchiveStorageEvent = api - .call( - "archive_unstable_storage", - rpc_params![ - &block_hash, - vec![PaginatedStorageQuery { - key: hex_string(b":m"), - query_type: StorageQueryType::DescendantsValues, - pagination_start_key: Some(hex_string(b":mock")), - }] - ], - ) - .await - .unwrap(); - match result { - ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { - assert_eq!(result.len(), 0); - assert_eq!(discarded_items, 0); - }, - _ => panic!("Unexpected result"), - }; -} + assert_eq!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::StorageResult(StorageResult { + key: hex_string(b":mo"), + result: StorageResultType::Value(hex_string(b"ab")), + }) + ); -#[tokio::test] -async fn archive_storage_discarded_items() { - // One query at a time - let (client, api) = setup_api(MAX_PAGINATION_LIMIT, 1); + assert_eq!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::StorageResult(StorageResult { + key: hex_string(b":moD"), + result: StorageResultType::Value(hex_string(b"abcmoD")), + }) + ); - // Import a new block with storage changes. - let mut builder = BlockBuilderBuilder::new(&*client) - .on_parent_block(client.chain_info().genesis_hash) - .with_parent_block_number(0) - .build() - .unwrap(); - builder.push_storage_change(b":m".to_vec(), Some(b"a".to_vec())).unwrap(); - let block = builder.build().unwrap().block; - let block_hash = format!("{:?}", block.header.hash()); - client.import(BlockOrigin::Own, block.clone()).await.unwrap(); + assert_eq!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::StorageResult(StorageResult { + key: hex_string(b":moc"), + result: StorageResultType::Value(hex_string(b"abc")), + }) + ); - // Valid call with storage at the key. - let result: ArchiveStorageEvent = api - .call( - "archive_unstable_storage", - rpc_params![ - &block_hash, - vec![ - PaginatedStorageQuery { - key: hex_string(b":m"), - query_type: StorageQueryType::Value, - pagination_start_key: None, - }, - PaginatedStorageQuery { - key: hex_string(b":m"), - query_type: StorageQueryType::Hash, - pagination_start_key: None, - }, - PaginatedStorageQuery { - key: hex_string(b":m"), - query_type: StorageQueryType::Hash, - pagination_start_key: None, - } - ] - ], - ) - .await - .unwrap(); - match result { - ArchiveStorageEvent::Ok(ArchiveStorageMethodOk { result, discarded_items }) => { - assert_eq!(result.len(), 1); - assert_eq!(discarded_items, 2); + assert_eq!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::StorageResult(StorageResult { + key: hex_string(b":mock"), + result: StorageResultType::Value(hex_string(b"abcd")), + }) + ); - assert_eq!(result[0].key, hex_string(b":m")); - assert_eq!(result[0].result, StorageResultType::Value(hex_string(b"a"))); - }, - _ => panic!("Unexpected result"), - }; + assert_matches!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::StorageDone + ); } #[tokio::test] async fn archive_storage_diff_main_trie() { - let (client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); + let (client, api) = setup_api(); let mut builder = BlockBuilderBuilder::new(&*client) .on_parent_block(client.chain_info().genesis_hash) @@ -965,7 +807,7 @@ async fn archive_storage_diff_main_trie() { #[tokio::test] async fn archive_storage_diff_no_changes() { - let (client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); + let (client, api) = setup_api(); // Build 2 identical blocks. let mut builder = BlockBuilderBuilder::new(&*client) @@ -1010,7 +852,7 @@ async fn archive_storage_diff_no_changes() { #[tokio::test] async fn archive_storage_diff_deleted_changes() { - let (client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); + let (client, api) = setup_api(); // Blocks are imported as forks. let mut builder = BlockBuilderBuilder::new(&*client) @@ -1075,7 +917,7 @@ async fn archive_storage_diff_deleted_changes() { #[tokio::test] async fn archive_storage_diff_invalid_params() { let invalid_hash = hex_string(&INVALID_HASH); - let (_, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); + let (_, api) = setup_api(); // Invalid shape for parameters. let items: Vec> = Vec::new(); diff --git a/substrate/client/rpc-spec-v2/src/common/events.rs b/substrate/client/rpc-spec-v2/src/common/events.rs index d01588031a5c..8f589c6cf72b 100644 --- a/substrate/client/rpc-spec-v2/src/common/events.rs +++ b/substrate/client/rpc-spec-v2/src/common/events.rs @@ -136,6 +136,11 @@ impl ArchiveStorageEvent { pub fn is_err(&self) -> bool { matches!(self, Self::StorageErr(_)) } + + /// Checks if the event is a `StorageResult` event. + pub fn is_result(&self) -> bool { + matches!(self, Self::StorageResult(_)) + } } /// The result of a storage call. From 3c2bb7cf547c6a668876efd377322f8cd20a4540 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 26 Nov 2024 13:24:54 +0200 Subject: [PATCH 53/70] archive: Remove variable Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/archive_storage.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 2475435d65b0..1d8a4d1f516d 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -221,13 +221,13 @@ where }, FetchStorageType::Both => { - let value = self.client.query_value(hash, &key, maybe_child_trie.as_ref())?; - let Some(value) = value else { + let Some(value) = self.client.query_value(hash, &key, maybe_child_trie.as_ref())? + else { return Ok(None); }; - let hash = self.client.query_hash(hash, &key, maybe_child_trie.as_ref())?; - let Some(hash) = hash else { + let Some(hash) = self.client.query_hash(hash, &key, maybe_child_trie.as_ref())? + else { return Ok(None); }; From 8f8c0c0a11c5399fd65675ebc1f9d20327adcd21 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 26 Nov 2024 13:28:11 +0200 Subject: [PATCH 54/70] Update cargo lock Signed-off-by: Alexandru Vasile --- Cargo.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 14cb73f7ec0f..98b8d7fa6c2c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -20934,7 +20934,7 @@ checksum = "b544ef1b4eac5dc2db33ea63606ae9ffcfac26c1416a2806ae0bf5f56b201191" dependencies = [ "aho-corasick", "memchr", - "regex-automata 0.4.9", + "regex-automata 0.4.8", "regex-syntax 0.8.5", ] @@ -20955,9 +20955,9 @@ checksum = "fed1ceff11a1dddaee50c9dc8e4938bd106e9d89ae372f192311e7da498e3b69" [[package]] name = "regex-automata" -version = "0.4.9" +version = "0.4.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "809e8dc61f6de73b46c85f4c96486310fe304c434cfa43669d7b40f711150908" +checksum = "368758f23274712b504848e9d5a6f010445cc8b87a7cdb4d7cbee666c1288da3" dependencies = [ "aho-corasick", "memchr", From 07a459cabdb66419b8c3550ab0dfd192e9dd9dd2 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 26 Nov 2024 13:35:49 +0200 Subject: [PATCH 55/70] archive: Mode deduplicate_storage_diff_items to inenr methods Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/archive.rs | 18 ++------------ .../src/archive/archive_storage.rs | 24 +++++++++++++------ 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index 5dd0af8b8786..55054d91d85d 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -20,7 +20,7 @@ use crate::{ archive::{ - archive_storage::{deduplicate_storage_diff_items, ArchiveStorage, ArchiveStorageDiff}, + archive_storage::{ArchiveStorage, ArchiveStorageDiff}, error::Error as ArchiveError, ArchiveApiServer, }, @@ -318,20 +318,6 @@ where let fut = async move { let Ok(mut sink) = pending.accept().await.map(Subscription::from) else { return }; - // Deduplicate the items. - let mut trie_items = match deduplicate_storage_diff_items(items) { - Ok(items) => items, - Err(error) => { - let _ = sink.send(&ArchiveStorageDiffEvent::err(error.to_string())).await; - return - }, - }; - // Default to using the main storage trie if no items are provided. - if trie_items.is_empty() { - trie_items.push(Vec::new()); - } - log::trace!(target: LOG_TARGET, "Storage diff deduplicated items: {:?}", trie_items); - let previous_hash = if let Some(previous_hash) = previous_hash { previous_hash } else { @@ -345,7 +331,7 @@ where let (tx, mut rx) = tokio::sync::mpsc::channel(STORAGE_QUERY_BUF); let storage_fut = - storage_client.handle_trie_queries(hash, previous_hash, trie_items, tx.clone()); + storage_client.handle_trie_queries(hash, items, previous_hash, tx.clone()); // We don't care about the return value of this join: // - process_events might encounter an error (if the client disconnected) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 1d8a4d1f516d..f0150aa4f109 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -423,24 +423,34 @@ where Ok(()) } - /// The items provided to this method are obtained by calling `deduplicate_storage_diff_items`. - /// The deduplication method ensures that all items `Vec` correspond to the same - /// `child_trie_key`. - /// /// This method will iterate over the keys of the main trie or a child trie and fetch the /// given keys. The fetched keys will be sent to the provided `tx` sender to leverage /// the backpressure mechanism. pub async fn handle_trie_queries( &self, hash: Block::Hash, + items: Vec>, previous_hash: Block::Hash, - trie_queries: Vec>, tx: mpsc::Sender, ) -> Result<(), tokio::task::JoinError> { let this = ArchiveStorageDiff { client: self.client.clone() }; tokio::task::spawn_blocking(move || { - for items in trie_queries { + // Deduplicate the items. + let mut trie_items = match deduplicate_storage_diff_items(items) { + Ok(items) => items, + Err(error) => { + let _ = tx.blocking_send(ArchiveStorageDiffEvent::err(error.to_string())); + return + }, + }; + // Default to using the main storage trie if no items are provided. + if trie_items.is_empty() { + trie_items.push(Vec::new()); + } + log::trace!(target: LOG_TARGET, "Storage diff deduplicated items: {:?}", trie_items); + + for items in trie_items { log::trace!( target: LOG_TARGET, "handle_trie_queries: hash={:?}, previous_hash={:?}, items={:?}", @@ -480,7 +490,7 @@ where /// Deduplicate the provided items and return a list of `DiffDetails`. /// /// Each list corresponds to a single child trie or the main trie. -pub fn deduplicate_storage_diff_items( +fn deduplicate_storage_diff_items( items: Vec>, ) -> Result>, ArchiveError> { let mut deduplicated: HashMap, Vec> = HashMap::new(); From a151bf3058f057f08c0942fb1926eca544e6695e Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 26 Nov 2024 13:39:32 +0200 Subject: [PATCH 56/70] archive/tests: Ensure other storage entries are not provided Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/archive/tests.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index 06445d96d125..994c5d28bd61 100644 --- a/substrate/client/rpc-spec-v2/src/archive/tests.rs +++ b/substrate/client/rpc-spec-v2/src/archive/tests.rs @@ -975,6 +975,8 @@ async fn archive_storage_diff_no_changes() { .unwrap(); builder.push_storage_change(b":A".to_vec(), Some(b"B".to_vec())).unwrap(); builder.push_storage_change(b":AA".to_vec(), Some(b"BB".to_vec())).unwrap(); + builder.push_storage_change(b":B".to_vec(), Some(b"CC".to_vec())).unwrap(); + builder.push_storage_change(b":BA".to_vec(), Some(b"CC".to_vec())).unwrap(); let prev_block = builder.build().unwrap().block; let prev_hash = format!("{:?}", prev_block.header.hash()); client.import(BlockOrigin::Own, prev_block.clone()).await.unwrap(); @@ -1020,6 +1022,8 @@ async fn archive_storage_diff_deleted_changes() { .unwrap(); builder.push_storage_change(b":A".to_vec(), Some(b"B".to_vec())).unwrap(); builder.push_storage_change(b":AA".to_vec(), Some(b"BB".to_vec())).unwrap(); + builder.push_storage_change(b":B".to_vec(), Some(b"CC".to_vec())).unwrap(); + builder.push_storage_change(b":BA".to_vec(), Some(b"CC".to_vec())).unwrap(); let prev_block = builder.build().unwrap().block; let prev_hash = format!("{:?}", prev_block.header.hash()); client.import(BlockOrigin::Own, prev_block.clone()).await.unwrap(); From a575b4a77a4b29738debd72218be8b7cc6533733 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 27 Nov 2024 17:12:17 +0200 Subject: [PATCH 57/70] archive/diff: Implement lexicographic_diff Signed-off-by: Alexandru Vasile --- .../src/archive/archive_storage.rs | 100 ++++++++++-------- 1 file changed, 56 insertions(+), 44 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index f0150aa4f109..94346216d5db 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -315,53 +315,17 @@ where // Iterator over the current block and previous block // at the same time to compare the keys. This approach effectively // leverages backpressure to avoid memory consumption. - let mut keys_iter = self.client.raw_keys_iter(hash, maybe_child_trie.clone())?; - let mut previous_keys_iter = + let keys_iter = self.client.raw_keys_iter(hash, maybe_child_trie.clone())?; + let previous_keys_iter = self.client.raw_keys_iter(previous_hash, maybe_child_trie.clone())?; - let mut lhs = keys_iter.next(); - let mut rhs = previous_keys_iter.next(); + let mut diff_iter = lexicographic_diff(keys_iter, previous_keys_iter); - loop { - // Check if the key was added or deleted or modified based on the - // lexicographical order of the keys. - let (operation_type, key) = match (&lhs, &rhs) { - (Some(lhs_key), Some(rhs_key)) => - if lhs_key < rhs_key { - let key = lhs_key.clone(); - - lhs = keys_iter.next(); - - (ArchiveStorageDiffOperationType::Added, key) - } else if lhs_key > rhs_key { - let key = rhs_key.clone(); - - rhs = previous_keys_iter.next(); - - (ArchiveStorageDiffOperationType::Deleted, key) - } else { - let key = lhs_key.clone(); - - lhs = keys_iter.next(); - rhs = previous_keys_iter.next(); - - (ArchiveStorageDiffOperationType::Modified, key) - }, - (Some(lhs_key), None) => { - let key = lhs_key.clone(); - - lhs = keys_iter.next(); - - (ArchiveStorageDiffOperationType::Added, key) - }, - (None, Some(rhs_key)) => { - let key = rhs_key.clone(); - - rhs = previous_keys_iter.next(); - - (ArchiveStorageDiffOperationType::Deleted, key) - }, - (None, None) => break, + while let Some(item) = diff_iter.next() { + let (operation_type, key) = match item { + Diff::Added(key) => (ArchiveStorageDiffOperationType::Added, key), + Diff::Deleted(key) => (ArchiveStorageDiffOperationType::Deleted, key), + Diff::Equal(key) => (ArchiveStorageDiffOperationType::Modified, key), }; let Some(fetch_type) = Self::belongs_to_query(&key, &items) else { @@ -487,6 +451,54 @@ where } } +enum Diff { + Added(T), + Deleted(T), + Equal(T), +} + +fn lexicographic_diff<'a, T, LeftIter, RightIter>( + mut left: LeftIter, + mut right: RightIter, +) -> impl Iterator> +where + T: Ord, + LeftIter: Iterator, + RightIter: Iterator, +{ + let mut a = left.next(); + let mut b = right.next(); + + core::iter::from_fn(move || match (a.take(), b.take()) { + (Some(a_value), Some(b_value)) => + if a_value < b_value { + b = Some(b_value); + a = left.next(); + + Some(Diff::Added(a_value)) + } else if a_value > b_value { + a = Some(a_value); + b = right.next(); + + Some(Diff::Deleted(b_value)) + } else { + a = left.next(); + b = right.next(); + + Some(Diff::Equal(a_value)) + }, + (Some(a_value), None) => { + a = left.next(); + Some(Diff::Added(a_value)) + }, + (None, Some(b_value)) => { + b = right.next(); + Some(Diff::Deleted(b_value)) + }, + (None, None) => None, + }) +} + /// Deduplicate the provided items and return a list of `DiffDetails`. /// /// Each list corresponds to a single child trie or the main trie. From caa75ccf705bb2a5a6fcc43a22ab489e64476e82 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 27 Nov 2024 17:21:43 +0200 Subject: [PATCH 58/70] archive/tests: Check lexicographic diff Signed-off-by: Alexandru Vasile --- .../src/archive/archive_storage.rs | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 94346216d5db..636b703639f3 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -451,12 +451,34 @@ where } } +#[derive(Debug, PartialEq)] enum Diff { Added(T), Deleted(T), Equal(T), } +// impl PartialEq for Diff { +// fn eq(&self, other: &Self) -> bool { +// match (self, other) { +// (Diff::Added(a), Diff::Added(b)) => a == b, +// (Diff::Deleted(a), Diff::Deleted(b)) => a == b, +// (Diff::Equal(a), Diff::Equal(b)) => a == b, +// _ => false, +// } +// } +// } + +// impl std::fmt::Debug for Diff { +// fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { +// match self { +// Diff::Added(value) => write!(f, "Added({:?})", value), +// Diff::Deleted(value) => write!(f, "Deleted({:?})", value), +// Diff::Equal(value) => write!(f, "Equal({:?})", value), +// } +// } +// } + fn lexicographic_diff<'a, T, LeftIter, RightIter>( mut left: LeftIter, mut right: RightIter, @@ -896,4 +918,52 @@ mod tests { assert_eq!(result, expected); } + + #[test] + fn test_lexicographic_diff() { + let left = vec![1, 2, 3, 4, 5]; + let right = vec![2, 3, 4, 5, 6]; + + let diff = lexicographic_diff(left.into_iter(), right.into_iter()).collect::>(); + let expected = vec![ + Diff::Added(1), + Diff::Equal(2), + Diff::Equal(3), + Diff::Equal(4), + Diff::Equal(5), + Diff::Deleted(6), + ]; + assert_eq!(diff, expected); + } + + #[test] + fn test_lexicographic_diff_one_side_empty() { + let left = vec![]; + let right = vec![1, 2, 3, 4, 5, 6]; + + let diff = lexicographic_diff(left.into_iter(), right.into_iter()).collect::>(); + let expected = vec![ + Diff::Deleted(1), + Diff::Deleted(2), + Diff::Deleted(3), + Diff::Deleted(4), + Diff::Deleted(5), + Diff::Deleted(6), + ]; + assert_eq!(diff, expected); + + let left = vec![1, 2, 3, 4, 5, 6]; + let right = vec![]; + + let diff = lexicographic_diff(left.into_iter(), right.into_iter()).collect::>(); + let expected = vec![ + Diff::Added(1), + Diff::Added(2), + Diff::Added(3), + Diff::Added(4), + Diff::Added(5), + Diff::Added(6), + ]; + assert_eq!(diff, expected); + } } From c6e40a0181e805b0e52b7c3eaafbb379522133d4 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 27 Nov 2024 17:22:29 +0200 Subject: [PATCH 59/70] archive: Remove commented code and add documentation Signed-off-by: Alexandru Vasile --- .../src/archive/archive_storage.rs | 23 ++----------------- 1 file changed, 2 insertions(+), 21 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index 636b703639f3..d51aa599d1f4 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -451,6 +451,7 @@ where } } +/// The result of the `lexicographic_diff` method. #[derive(Debug, PartialEq)] enum Diff { Added(T), @@ -458,27 +459,7 @@ enum Diff { Equal(T), } -// impl PartialEq for Diff { -// fn eq(&self, other: &Self) -> bool { -// match (self, other) { -// (Diff::Added(a), Diff::Added(b)) => a == b, -// (Diff::Deleted(a), Diff::Deleted(b)) => a == b, -// (Diff::Equal(a), Diff::Equal(b)) => a == b, -// _ => false, -// } -// } -// } - -// impl std::fmt::Debug for Diff { -// fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { -// match self { -// Diff::Added(value) => write!(f, "Added({:?})", value), -// Diff::Deleted(value) => write!(f, "Deleted({:?})", value), -// Diff::Equal(value) => write!(f, "Equal({:?})", value), -// } -// } -// } - +/// Compare two iterators lexicographically and return the differences. fn lexicographic_diff<'a, T, LeftIter, RightIter>( mut left: LeftIter, mut right: RightIter, From a07b5c49504a4fb6194698d771b211a39cca0641 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Wed, 27 Nov 2024 17:23:17 +0200 Subject: [PATCH 60/70] Update substrate/client/rpc-spec-v2/src/archive/api.rs Co-authored-by: James Wilson --- substrate/client/rpc-spec-v2/src/archive/api.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/api.rs b/substrate/client/rpc-spec-v2/src/archive/api.rs index d3da879fc358..dcfeaecb147b 100644 --- a/substrate/client/rpc-spec-v2/src/archive/api.rs +++ b/substrate/client/rpc-spec-v2/src/archive/api.rs @@ -112,7 +112,7 @@ pub trait ArchiveApi { /// /// # Unstable /// - /// This method is unstable and subject to change in the future. + /// This method is unstable and can change in minor or patch releases. #[subscription( name = "archive_unstable_storageDiff" => "archive_unstable_storageDiffEvent", unsubscribe = "archive_unstable_storageDiff_stopStorageDiff", From ae003275076d07d6536d2c00a404c4081d76f85e Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 27 Nov 2024 17:55:11 +0200 Subject: [PATCH 61/70] archive: Remove unneeded lifetime Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/archive/archive_storage.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs index d51aa599d1f4..5a3920882f00 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -460,7 +460,7 @@ enum Diff { } /// Compare two iterators lexicographically and return the differences. -fn lexicographic_diff<'a, T, LeftIter, RightIter>( +fn lexicographic_diff( mut left: LeftIter, mut right: RightIter, ) -> impl Iterator> From 791831f83fa4e13f9aad7379adb15dd8375df4e7 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 27 Nov 2024 18:14:27 +0200 Subject: [PATCH 62/70] archive: Check sink closed Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/archive.rs | 34 +++++++++++++------ 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index 52aa62c5e12c..79cf99a6756b 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -323,19 +323,31 @@ async fn process_events(rx: &mut mpsc::Receiver, sink: /// Sends all the events to the sink. async fn process_storage_events(rx: &mut mpsc::Receiver, sink: &mut Subscription) { - while let Some(event) = rx.recv().await { - match event { - Ok(None) => continue, + loop { + tokio::select! { + _ = sink.closed() => { + break + } - Ok(Some(event)) => - if sink.send(&ArchiveStorageEvent::result(event)).await.is_err() { - return - }, + maybe_storage = rx.recv() => { + let Some(event) = maybe_storage else { + break; + }; - Err(error) => - if sink.send(&ArchiveStorageEvent::err(error)).await.is_err() { - return - }, + match event { + Ok(None) => continue, + + Ok(Some(event)) => + if sink.send(&ArchiveStorageEvent::result(event)).await.is_err() { + return + }, + + Err(error) => { + let _ = sink.send(&ArchiveStorageEvent::err(error)).await; + return + } + } + } } } From 3ddaf2d7af8706c3131dc5c41a6c65bdc9355e99 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 27 Nov 2024 18:18:33 +0200 Subject: [PATCH 63/70] archive: Refactor process methods Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/archive.rs | 41 +++++++++++++------ 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/archive.rs b/substrate/client/rpc-spec-v2/src/archive/archive.rs index 79cf99a6756b..62e44a016241 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -299,29 +299,46 @@ where // - process_events might encounter an error (if the client disconnected) // - storage_fut might encounter an error while processing a trie queries and // the error is propagated via the sink. - let _ = futures::future::join(storage_fut, process_events(&mut rx, &mut sink)).await; + let _ = + futures::future::join(storage_fut, process_storage_diff_events(&mut rx, &mut sink)) + .await; }; self.executor.spawn("substrate-rpc-subscription", Some("rpc"), fut.boxed()); } } -/// Sends all the events to the sink. -async fn process_events(rx: &mut mpsc::Receiver, sink: &mut Subscription) { - while let Some(event) = rx.recv().await { - if event.is_done() { - log::debug!(target: LOG_TARGET, "Finished processing partial trie query"); - } else if event.is_err() { - log::debug!(target: LOG_TARGET, "Error encountered while processing partial trie query"); - } +/// Sends all the events of the storage_diff method to the sink. +async fn process_storage_diff_events( + rx: &mut mpsc::Receiver, + sink: &mut Subscription, +) { + loop { + tokio::select! { + _ = sink.closed() => { + return + }, + + maybe_event = rx.recv() => { + let Some(event) = maybe_event else { + break; + }; + + if event.is_done() { + log::debug!(target: LOG_TARGET, "Finished processing partial trie query"); + } else if event.is_err() { + log::debug!(target: LOG_TARGET, "Error encountered while processing partial trie query"); + } - if sink.send(&event).await.is_err() { - return + if sink.send(&event).await.is_err() { + return + } + } } } } -/// Sends all the events to the sink. +/// Sends all the events of the storage method to the sink. async fn process_storage_events(rx: &mut mpsc::Receiver, sink: &mut Subscription) { loop { tokio::select! { From 774b525b05401909e497cd3eee8c7017878f9673 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 27 Nov 2024 18:33:02 +0200 Subject: [PATCH 64/70] archive/tests: Adjust to new assumptions wrt StorageDone Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/archive/tests.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index 70460590c5b0..bd0d9f51eed4 100644 --- a/substrate/client/rpc-spec-v2/src/archive/tests.rs +++ b/substrate/client/rpc-spec-v2/src/archive/tests.rs @@ -629,10 +629,6 @@ async fn archive_storage_iterations() { get_next_event::(&mut sub).await, ArchiveStorageEvent::StorageErr(_) ); - assert_matches!( - get_next_event::(&mut sub).await, - ArchiveStorageEvent::StorageDone - ); // Valid call with storage at the key. let mut sub = api From 41474428f97eec7948d495a6732da1c1ab87782f Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 27 Nov 2024 19:22:35 +0200 Subject: [PATCH 65/70] archive: Rename StorageResult to Storage Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/tests.rs | 20 +++++++++---------- .../client/rpc-spec-v2/src/common/events.rs | 6 +++--- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index bd0d9f51eed4..a5e44e96ffe8 100644 --- a/substrate/client/rpc-spec-v2/src/archive/tests.rs +++ b/substrate/client/rpc-spec-v2/src/archive/tests.rs @@ -399,7 +399,7 @@ async fn archive_storage_hashes_values() { assert_eq!( get_next_event::(&mut sub).await, - ArchiveStorageEvent::StorageResult(StorageResult { + ArchiveStorageEvent::Storage(StorageResult { key: key.clone(), result: StorageResultType::Hash(expected_hash.clone()), }), @@ -407,7 +407,7 @@ async fn archive_storage_hashes_values() { assert_eq!( get_next_event::(&mut sub).await, - ArchiveStorageEvent::StorageResult(StorageResult { + ArchiveStorageEvent::Storage(StorageResult { key: key.clone(), result: StorageResultType::Value(expected_value.clone()), }), @@ -415,7 +415,7 @@ async fn archive_storage_hashes_values() { assert_eq!( get_next_event::(&mut sub).await, - ArchiveStorageEvent::StorageResult(StorageResult { + ArchiveStorageEvent::Storage(StorageResult { key: key.clone(), result: StorageResultType::Hash(expected_hash), }), @@ -423,7 +423,7 @@ async fn archive_storage_hashes_values() { assert_eq!( get_next_event::(&mut sub).await, - ArchiveStorageEvent::StorageResult(StorageResult { + ArchiveStorageEvent::Storage(StorageResult { key: key.clone(), result: StorageResultType::Value(expected_value), }), @@ -501,7 +501,7 @@ async fn archive_storage_closest_merkle_value() { loop { let event = get_next_event::(&mut sub).await; match event { - ArchiveStorageEvent::StorageResult(result) => { + ArchiveStorageEvent::Storage(result) => { let str_result = match result.result { StorageResultType::ClosestDescendantMerkleValue(value) => value, _ => panic!("Unexpected result type"), @@ -647,7 +647,7 @@ async fn archive_storage_iterations() { assert_eq!( get_next_event::(&mut sub).await, - ArchiveStorageEvent::StorageResult(StorageResult { + ArchiveStorageEvent::Storage(StorageResult { key: hex_string(b":m"), result: StorageResultType::Value(hex_string(b"a")), }) @@ -655,7 +655,7 @@ async fn archive_storage_iterations() { assert_eq!( get_next_event::(&mut sub).await, - ArchiveStorageEvent::StorageResult(StorageResult { + ArchiveStorageEvent::Storage(StorageResult { key: hex_string(b":mo"), result: StorageResultType::Value(hex_string(b"ab")), }) @@ -663,7 +663,7 @@ async fn archive_storage_iterations() { assert_eq!( get_next_event::(&mut sub).await, - ArchiveStorageEvent::StorageResult(StorageResult { + ArchiveStorageEvent::Storage(StorageResult { key: hex_string(b":moD"), result: StorageResultType::Value(hex_string(b"abcmoD")), }) @@ -671,7 +671,7 @@ async fn archive_storage_iterations() { assert_eq!( get_next_event::(&mut sub).await, - ArchiveStorageEvent::StorageResult(StorageResult { + ArchiveStorageEvent::Storage(StorageResult { key: hex_string(b":moc"), result: StorageResultType::Value(hex_string(b"abc")), }) @@ -679,7 +679,7 @@ async fn archive_storage_iterations() { assert_eq!( get_next_event::(&mut sub).await, - ArchiveStorageEvent::StorageResult(StorageResult { + ArchiveStorageEvent::Storage(StorageResult { key: hex_string(b":mock"), result: StorageResultType::Value(hex_string(b"abcd")), }) diff --git a/substrate/client/rpc-spec-v2/src/common/events.rs b/substrate/client/rpc-spec-v2/src/common/events.rs index 8f589c6cf72b..b0ba4af0755f 100644 --- a/substrate/client/rpc-spec-v2/src/common/events.rs +++ b/substrate/client/rpc-spec-v2/src/common/events.rs @@ -109,7 +109,7 @@ pub struct StorageResultErr { #[serde(tag = "event")] pub enum ArchiveStorageEvent { /// Query generated a result. - StorageResult(StorageResult), + Storage(StorageResult), /// Query encountered an error. StorageErr(ArchiveStorageMethodErr), /// Operation storage is done. @@ -124,7 +124,7 @@ impl ArchiveStorageEvent { /// Create a new `ArchiveStorageEvent::StorageResult` event. pub fn result(result: StorageResult) -> Self { - Self::StorageResult(result) + Self::Storage(result) } /// Checks if the event is a `StorageDone` event. @@ -139,7 +139,7 @@ impl ArchiveStorageEvent { /// Checks if the event is a `StorageResult` event. pub fn is_result(&self) -> bool { - matches!(self, Self::StorageResult(_)) + matches!(self, Self::Storage(_)) } } From c0f5ded8009bce876e50e818fb1eb4767e36ab6d Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 27 Nov 2024 19:30:27 +0200 Subject: [PATCH 66/70] storage/events: Include child trie key Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/common/events.rs | 4 ++++ substrate/client/rpc-spec-v2/src/common/storage.rs | 3 +++ 2 files changed, 7 insertions(+) diff --git a/substrate/client/rpc-spec-v2/src/common/events.rs b/substrate/client/rpc-spec-v2/src/common/events.rs index b0ba4af0755f..20d4d1009fde 100644 --- a/substrate/client/rpc-spec-v2/src/common/events.rs +++ b/substrate/client/rpc-spec-v2/src/common/events.rs @@ -78,6 +78,10 @@ pub struct StorageResult { /// The result of the query. #[serde(flatten)] pub result: StorageResultType, + /// The child trie key if provided. + #[serde(skip_serializing_if = "Option::is_none")] + #[serde(default)] + pub child_trie_key: Option, } /// The type of the storage query. diff --git a/substrate/client/rpc-spec-v2/src/common/storage.rs b/substrate/client/rpc-spec-v2/src/common/storage.rs index 4affec265097..a1e34d51530e 100644 --- a/substrate/client/rpc-spec-v2/src/common/storage.rs +++ b/substrate/client/rpc-spec-v2/src/common/storage.rs @@ -94,6 +94,7 @@ where QueryResult::Ok(opt.map(|storage_data| StorageResult { key: hex_string(&key.0), result: StorageResultType::Value(hex_string(&storage_data.0)), + child_trie_key: child_key.map(|c| hex_string(&c.storage_key())), })) }) .unwrap_or_else(|error| QueryResult::Err(error.to_string())) @@ -117,6 +118,7 @@ where QueryResult::Ok(opt.map(|storage_data| StorageResult { key: hex_string(&key.0), result: StorageResultType::Hash(hex_string(&storage_data.as_ref())), + child_trie_key: child_key.map(|c| hex_string(&c.storage_key())), })) }) .unwrap_or_else(|error| QueryResult::Err(error.to_string())) @@ -146,6 +148,7 @@ where StorageResult { key: hex_string(&key.0), result: StorageResultType::ClosestDescendantMerkleValue(result), + child_trie_key: child_key.map(|c| hex_string(&c.storage_key())), } })) }) From 39aab7a3c4c65b7acd4f09e02b86166d1e37088e Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 27 Nov 2024 19:31:24 +0200 Subject: [PATCH 67/70] archive/tests: Adjust testing Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/archive/tests.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index a5e44e96ffe8..ad2a163bd67f 100644 --- a/substrate/client/rpc-spec-v2/src/archive/tests.rs +++ b/substrate/client/rpc-spec-v2/src/archive/tests.rs @@ -402,6 +402,7 @@ async fn archive_storage_hashes_values() { ArchiveStorageEvent::Storage(StorageResult { key: key.clone(), result: StorageResultType::Hash(expected_hash.clone()), + child_trie_key: None, }), ); @@ -410,6 +411,7 @@ async fn archive_storage_hashes_values() { ArchiveStorageEvent::Storage(StorageResult { key: key.clone(), result: StorageResultType::Value(expected_value.clone()), + child_trie_key: None, }), ); @@ -418,6 +420,7 @@ async fn archive_storage_hashes_values() { ArchiveStorageEvent::Storage(StorageResult { key: key.clone(), result: StorageResultType::Hash(expected_hash), + child_trie_key: None, }), ); @@ -426,6 +429,7 @@ async fn archive_storage_hashes_values() { ArchiveStorageEvent::Storage(StorageResult { key: key.clone(), result: StorageResultType::Value(expected_value), + child_trie_key: None, }), ); @@ -650,6 +654,7 @@ async fn archive_storage_iterations() { ArchiveStorageEvent::Storage(StorageResult { key: hex_string(b":m"), result: StorageResultType::Value(hex_string(b"a")), + child_trie_key: None, }) ); @@ -658,6 +663,7 @@ async fn archive_storage_iterations() { ArchiveStorageEvent::Storage(StorageResult { key: hex_string(b":mo"), result: StorageResultType::Value(hex_string(b"ab")), + child_trie_key: None, }) ); @@ -666,6 +672,7 @@ async fn archive_storage_iterations() { ArchiveStorageEvent::Storage(StorageResult { key: hex_string(b":moD"), result: StorageResultType::Value(hex_string(b"abcmoD")), + child_trie_key: None, }) ); @@ -674,6 +681,7 @@ async fn archive_storage_iterations() { ArchiveStorageEvent::Storage(StorageResult { key: hex_string(b":moc"), result: StorageResultType::Value(hex_string(b"abc")), + child_trie_key: None, }) ); @@ -682,6 +690,7 @@ async fn archive_storage_iterations() { ArchiveStorageEvent::Storage(StorageResult { key: hex_string(b":mock"), result: StorageResultType::Value(hex_string(b"abcd")), + child_trie_key: None, }) ); From b14a4bdc353aa261b324daf6df18ad6ca68bfc9a Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 27 Nov 2024 19:41:33 +0200 Subject: [PATCH 68/70] archive/tests: Check childtrie results Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/tests.rs | 47 +++++++++++++++++++ .../rpc-spec-v2/src/chain_head/event.rs | 1 + .../client/rpc-spec-v2/src/common/events.rs | 15 ++++-- 3 files changed, 59 insertions(+), 4 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index ad2a163bd67f..e4b063c46f83 100644 --- a/substrate/client/rpc-spec-v2/src/archive/tests.rs +++ b/substrate/client/rpc-spec-v2/src/archive/tests.rs @@ -439,6 +439,53 @@ async fn archive_storage_hashes_values() { ); } +#[tokio::test] +async fn archive_storage_hashes_values_child_trie() { + let (client, api) = setup_api(); + + // Get child storage values set in `setup_api`. + let child_info = hex_string(&CHILD_STORAGE_KEY); + let key = hex_string(&KEY); + let genesis_hash = format!("{:?}", client.genesis_hash()); + let expected_hash = format!("{:?}", Blake2Hasher::hash(&CHILD_VALUE)); + let expected_value = hex_string(&CHILD_VALUE); + + let items: Vec> = vec![ + StorageQuery { key: key.clone(), query_type: StorageQueryType::DescendantsHashes }, + StorageQuery { key: key.clone(), query_type: StorageQueryType::DescendantsValues }, + ]; + let mut sub = api + .subscribe_unbounded( + "archive_unstable_storage", + rpc_params![&genesis_hash, items, &child_info], + ) + .await + .unwrap(); + + assert_eq!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::Storage(StorageResult { + key: key.clone(), + result: StorageResultType::Hash(expected_hash.clone()), + child_trie_key: Some(child_info.clone()), + }) + ); + + assert_eq!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::Storage(StorageResult { + key: key.clone(), + result: StorageResultType::Value(expected_value.clone()), + child_trie_key: Some(child_info.clone()), + }) + ); + + assert_eq!( + get_next_event::(&mut sub).await, + ArchiveStorageEvent::StorageDone, + ); +} + #[tokio::test] async fn archive_storage_closest_merkle_value() { let (client, api) = setup_api(); diff --git a/substrate/client/rpc-spec-v2/src/chain_head/event.rs b/substrate/client/rpc-spec-v2/src/chain_head/event.rs index b5571e5fbea7..de74145a3f08 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/event.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/event.rs @@ -536,6 +536,7 @@ mod tests { items: vec![StorageResult { key: "0x1".into(), result: StorageResultType::Value("0x123".to_string()), + child_trie_key: None, }], }); diff --git a/substrate/client/rpc-spec-v2/src/common/events.rs b/substrate/client/rpc-spec-v2/src/common/events.rs index 20d4d1009fde..5015f6dc48de 100644 --- a/substrate/client/rpc-spec-v2/src/common/events.rs +++ b/substrate/client/rpc-spec-v2/src/common/events.rs @@ -376,8 +376,11 @@ mod tests { #[test] fn storage_result() { // Item with Value. - let item = - StorageResult { key: "0x1".into(), result: StorageResultType::Value("res".into()) }; + let item = StorageResult { + key: "0x1".into(), + result: StorageResultType::Value("res".into()), + child_trie_key: None, + }; // Encode let ser = serde_json::to_string(&item).unwrap(); let exp = r#"{"key":"0x1","value":"res"}"#; @@ -387,8 +390,11 @@ mod tests { assert_eq!(dec, item); // Item with Hash. - let item = - StorageResult { key: "0x1".into(), result: StorageResultType::Hash("res".into()) }; + let item = StorageResult { + key: "0x1".into(), + result: StorageResultType::Hash("res".into()), + child_trie_key: None, + }; // Encode let ser = serde_json::to_string(&item).unwrap(); let exp = r#"{"key":"0x1","hash":"res"}"#; @@ -401,6 +407,7 @@ mod tests { let item = StorageResult { key: "0x1".into(), result: StorageResultType::ClosestDescendantMerkleValue("res".into()), + child_trie_key: None, }; // Encode let ser = serde_json::to_string(&item).unwrap(); From ff09c7ed0b5b7e0e8d54fefc7ff1d0ac2ae9ce1a Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 28 Nov 2024 13:32:06 +0200 Subject: [PATCH 69/70] archive/tests: Fix wrong merge Signed-off-by: Alexandru Vasile --- .../client/rpc-spec-v2/src/archive/tests.rs | 257 ------------------ 1 file changed, 257 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index b1d3998140c4..e4b063c46f83 100644 --- a/substrate/client/rpc-spec-v2/src/archive/tests.rs +++ b/substrate/client/rpc-spec-v2/src/archive/tests.rs @@ -1003,260 +1003,3 @@ async fn archive_storage_diff_invalid_params() { ArchiveStorageDiffEvent::StorageDiffError(ref err) if err.error.contains("Header was not found") ); } - -#[tokio::test] -async fn archive_storage_diff_main_trie() { - let (client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); - - let mut builder = BlockBuilderBuilder::new(&*client) - .on_parent_block(client.chain_info().genesis_hash) - .with_parent_block_number(0) - .build() - .unwrap(); - builder.push_storage_change(b":A".to_vec(), Some(b"B".to_vec())).unwrap(); - builder.push_storage_change(b":AA".to_vec(), Some(b"BB".to_vec())).unwrap(); - let prev_block = builder.build().unwrap().block; - let prev_hash = format!("{:?}", prev_block.header.hash()); - client.import(BlockOrigin::Own, prev_block.clone()).await.unwrap(); - - let mut builder = BlockBuilderBuilder::new(&*client) - .on_parent_block(prev_block.hash()) - .with_parent_block_number(1) - .build() - .unwrap(); - builder.push_storage_change(b":A".to_vec(), Some(b"11".to_vec())).unwrap(); - builder.push_storage_change(b":AA".to_vec(), Some(b"22".to_vec())).unwrap(); - builder.push_storage_change(b":AAA".to_vec(), Some(b"222".to_vec())).unwrap(); - let block = builder.build().unwrap().block; - let block_hash = format!("{:?}", block.header.hash()); - client.import(BlockOrigin::Own, block.clone()).await.unwrap(); - - // Search for items in the main trie: - // - values of keys under ":A" - // - hashes of keys under ":AA" - let items = vec![ - ArchiveStorageDiffItem:: { - key: hex_string(b":A"), - return_type: ArchiveStorageDiffType::Value, - child_trie_key: None, - }, - ArchiveStorageDiffItem:: { - key: hex_string(b":AA"), - return_type: ArchiveStorageDiffType::Hash, - child_trie_key: None, - }, - ]; - let mut sub = api - .subscribe_unbounded( - "archive_unstable_storageDiff", - rpc_params![&block_hash, items.clone(), &prev_hash], - ) - .await - .unwrap(); - - let event = get_next_event::(&mut sub).await; - assert_eq!( - ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { - key: hex_string(b":A"), - result: StorageResultType::Value(hex_string(b"11")), - operation_type: ArchiveStorageDiffOperationType::Modified, - child_trie_key: None, - }), - event, - ); - - let event = get_next_event::(&mut sub).await; - assert_eq!( - ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { - key: hex_string(b":AA"), - result: StorageResultType::Value(hex_string(b"22")), - operation_type: ArchiveStorageDiffOperationType::Modified, - child_trie_key: None, - }), - event, - ); - - let event = get_next_event::(&mut sub).await; - assert_eq!( - ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { - key: hex_string(b":AA"), - result: StorageResultType::Hash(format!("{:?}", Blake2Hasher::hash(b"22"))), - operation_type: ArchiveStorageDiffOperationType::Modified, - child_trie_key: None, - }), - event, - ); - - // Added key. - let event = get_next_event::(&mut sub).await; - assert_eq!( - ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { - key: hex_string(b":AAA"), - result: StorageResultType::Value(hex_string(b"222")), - operation_type: ArchiveStorageDiffOperationType::Added, - child_trie_key: None, - }), - event, - ); - - let event = get_next_event::(&mut sub).await; - assert_eq!( - ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { - key: hex_string(b":AAA"), - result: StorageResultType::Hash(format!("{:?}", Blake2Hasher::hash(b"222"))), - operation_type: ArchiveStorageDiffOperationType::Added, - child_trie_key: None, - }), - event, - ); - - let event = get_next_event::(&mut sub).await; - assert_eq!(ArchiveStorageDiffEvent::StorageDiffDone, event); -} - -#[tokio::test] -async fn archive_storage_diff_no_changes() { - let (client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); - - // Build 2 identical blocks. - let mut builder = BlockBuilderBuilder::new(&*client) - .on_parent_block(client.chain_info().genesis_hash) - .with_parent_block_number(0) - .build() - .unwrap(); - builder.push_storage_change(b":A".to_vec(), Some(b"B".to_vec())).unwrap(); - builder.push_storage_change(b":AA".to_vec(), Some(b"BB".to_vec())).unwrap(); - builder.push_storage_change(b":B".to_vec(), Some(b"CC".to_vec())).unwrap(); - builder.push_storage_change(b":BA".to_vec(), Some(b"CC".to_vec())).unwrap(); - let prev_block = builder.build().unwrap().block; - let prev_hash = format!("{:?}", prev_block.header.hash()); - client.import(BlockOrigin::Own, prev_block.clone()).await.unwrap(); - - let mut builder = BlockBuilderBuilder::new(&*client) - .on_parent_block(prev_block.hash()) - .with_parent_block_number(1) - .build() - .unwrap(); - builder.push_storage_change(b":A".to_vec(), Some(b"B".to_vec())).unwrap(); - builder.push_storage_change(b":AA".to_vec(), Some(b"BB".to_vec())).unwrap(); - let block = builder.build().unwrap().block; - let block_hash = format!("{:?}", block.header.hash()); - client.import(BlockOrigin::Own, block.clone()).await.unwrap(); - - // Search for items in the main trie with keys prefixed with ":A". - let items = vec![ArchiveStorageDiffItem:: { - key: hex_string(b":A"), - return_type: ArchiveStorageDiffType::Value, - child_trie_key: None, - }]; - let mut sub = api - .subscribe_unbounded( - "archive_unstable_storageDiff", - rpc_params![&block_hash, items.clone(), &prev_hash], - ) - .await - .unwrap(); - - let event = get_next_event::(&mut sub).await; - assert_eq!(ArchiveStorageDiffEvent::StorageDiffDone, event); -} - -#[tokio::test] -async fn archive_storage_diff_deleted_changes() { - let (client, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); - - // Blocks are imported as forks. - let mut builder = BlockBuilderBuilder::new(&*client) - .on_parent_block(client.chain_info().genesis_hash) - .with_parent_block_number(0) - .build() - .unwrap(); - builder.push_storage_change(b":A".to_vec(), Some(b"B".to_vec())).unwrap(); - builder.push_storage_change(b":AA".to_vec(), Some(b"BB".to_vec())).unwrap(); - builder.push_storage_change(b":B".to_vec(), Some(b"CC".to_vec())).unwrap(); - builder.push_storage_change(b":BA".to_vec(), Some(b"CC".to_vec())).unwrap(); - let prev_block = builder.build().unwrap().block; - let prev_hash = format!("{:?}", prev_block.header.hash()); - client.import(BlockOrigin::Own, prev_block.clone()).await.unwrap(); - - let mut builder = BlockBuilderBuilder::new(&*client) - .on_parent_block(client.chain_info().genesis_hash) - .with_parent_block_number(0) - .build() - .unwrap(); - builder - .push_transfer(Transfer { - from: AccountKeyring::Alice.into(), - to: AccountKeyring::Ferdie.into(), - amount: 41, - nonce: 0, - }) - .unwrap(); - builder.push_storage_change(b":A".to_vec(), Some(b"B".to_vec())).unwrap(); - let block = builder.build().unwrap().block; - let block_hash = format!("{:?}", block.header.hash()); - client.import(BlockOrigin::Own, block.clone()).await.unwrap(); - - // Search for items in the main trie with keys prefixed with ":A". - let items = vec![ArchiveStorageDiffItem:: { - key: hex_string(b":A"), - return_type: ArchiveStorageDiffType::Value, - child_trie_key: None, - }]; - - let mut sub = api - .subscribe_unbounded( - "archive_unstable_storageDiff", - rpc_params![&block_hash, items.clone(), &prev_hash], - ) - .await - .unwrap(); - - let event = get_next_event::(&mut sub).await; - assert_eq!( - ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { - key: hex_string(b":AA"), - result: StorageResultType::Value(hex_string(b"BB")), - operation_type: ArchiveStorageDiffOperationType::Deleted, - child_trie_key: None, - }), - event, - ); - - let event = get_next_event::(&mut sub).await; - assert_eq!(ArchiveStorageDiffEvent::StorageDiffDone, event); -} - -#[tokio::test] -async fn archive_storage_diff_invalid_params() { - let invalid_hash = hex_string(&INVALID_HASH); - let (_, api) = setup_api(MAX_PAGINATION_LIMIT, MAX_QUERIED_LIMIT); - - // Invalid shape for parameters. - let items: Vec> = Vec::new(); - let err = api - .subscribe_unbounded( - "archive_unstable_storageDiff", - rpc_params!["123", items.clone(), &invalid_hash], - ) - .await - .unwrap_err(); - assert_matches!(err, - Error::JsonRpc(ref err) if err.code() == crate::chain_head::error::json_rpc_spec::INVALID_PARAM_ERROR && err.message() == "Invalid params" - ); - - // The shape is right, but the block hash is invalid. - let items: Vec> = Vec::new(); - let mut sub = api - .subscribe_unbounded( - "archive_unstable_storageDiff", - rpc_params![&invalid_hash, items.clone(), &invalid_hash], - ) - .await - .unwrap(); - - let event = get_next_event::(&mut sub).await; - assert_matches!(event, - ArchiveStorageDiffEvent::StorageDiffError(ref err) if err.error.contains("Header was not found") - ); -} From 087f0e70ee7639ac50efa27c877937612ed694c1 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Thu, 28 Nov 2024 18:36:28 +0200 Subject: [PATCH 70/70] archive/events: Align with spec Signed-off-by: Alexandru Vasile --- substrate/client/rpc-spec-v2/src/archive/tests.rs | 4 ++-- substrate/client/rpc-spec-v2/src/common/events.rs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index e4b063c46f83..cddaafde6659 100644 --- a/substrate/client/rpc-spec-v2/src/archive/tests.rs +++ b/substrate/client/rpc-spec-v2/src/archive/tests.rs @@ -559,7 +559,7 @@ async fn archive_storage_closest_merkle_value() { }; merkle_values.insert(result.key, str_result); }, - ArchiveStorageEvent::StorageErr(err) => panic!("Unexpected error {err:?}"), + ArchiveStorageEvent::StorageError(err) => panic!("Unexpected error {err:?}"), ArchiveStorageEvent::StorageDone => break, } } @@ -678,7 +678,7 @@ async fn archive_storage_iterations() { assert_matches!( get_next_event::(&mut sub).await, - ArchiveStorageEvent::StorageErr(_) + ArchiveStorageEvent::StorageError(_) ); // Valid call with storage at the key. diff --git a/substrate/client/rpc-spec-v2/src/common/events.rs b/substrate/client/rpc-spec-v2/src/common/events.rs index 5015f6dc48de..44f722c0c61b 100644 --- a/substrate/client/rpc-spec-v2/src/common/events.rs +++ b/substrate/client/rpc-spec-v2/src/common/events.rs @@ -115,7 +115,7 @@ pub enum ArchiveStorageEvent { /// Query generated a result. Storage(StorageResult), /// Query encountered an error. - StorageErr(ArchiveStorageMethodErr), + StorageError(ArchiveStorageMethodErr), /// Operation storage is done. StorageDone, } @@ -123,7 +123,7 @@ pub enum ArchiveStorageEvent { impl ArchiveStorageEvent { /// Create a new `ArchiveStorageEvent::StorageErr` event. pub fn err(error: String) -> Self { - Self::StorageErr(ArchiveStorageMethodErr { error }) + Self::StorageError(ArchiveStorageMethodErr { error }) } /// Create a new `ArchiveStorageEvent::StorageResult` event. @@ -138,7 +138,7 @@ impl ArchiveStorageEvent { /// Checks if the event is a `StorageErr` event. pub fn is_err(&self) -> bool { - matches!(self, Self::StorageErr(_)) + matches!(self, Self::StorageError(_)) } /// Checks if the event is a `StorageResult` event.