From 445c1c80d438d5b937c347b04e4daa66ad25d878 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Wed, 27 Nov 2024 10:28:26 +0100 Subject: [PATCH 01/10] Add stable2412 to target_branches for command-backport.yml (#6666) The backport bot opens PR for `A4-needs-backport` only for stable2407 stable2409, but we have already stable2412. The question is, when should we append a new `stable*` branch here? Should it be done when a new `stable*` branch is created? Can we automate this process somehow? --- .github/workflows/command-backport.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/command-backport.yml b/.github/workflows/command-backport.yml index 8f23bcd75f01..eecf0ac72d2c 100644 --- a/.github/workflows/command-backport.yml +++ b/.github/workflows/command-backport.yml @@ -40,7 +40,7 @@ jobs: uses: korthout/backport-action@v3 id: backport with: - target_branches: stable2407 stable2409 + target_branches: stable2407 stable2409 stable2412 merge_commits: skip github_token: ${{ steps.generate_token.outputs.token }} pull_description: | From 2a0b2680b8df9c18d2543f82629917f759827e1c Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Wed, 27 Nov 2024 13:11:01 +0200 Subject: [PATCH 02/10] litep2p/req-resp: Always provide main protocol name in responses (#6603) Request responses are initialized with a main protocol name, and optional protocol names as a fallback. Running litep2p in kusama as a validator has surfaced a `debug_asserts` coming from the sync component: https://github.com/paritytech/polkadot-sdk/blob/3906c578c96d97a8a099a4bdac4685acbe375a7c/substrate/client/network/sync/src/strategy/chain_sync.rs#L640-L646 The issue is that we initiate a request-response over the main protocol name `/genesis/sync/2` but receive a response over the legacy procotol `ksm/sync/2`. This behavior is correct because litep2p propagates to the higher levels the protocol that responded. In contrast, libp2p provides the main protocol name regardless of negotiating a legacy protocol. Because of this, higher level components assume that only the main protocol name will respond. To not break this assumption, this PR alings litep2p shim layer with the libp2p behavior. Closes: https://github.com/paritytech/polkadot-sdk/issues/6581 --------- Signed-off-by: Alexandru Vasile Co-authored-by: Dmitry Markin --- prdoc/pr_6603.prdoc | 16 ++++++++++++++++ .../src/litep2p/shim/request_response/mod.rs | 7 ++----- 2 files changed, 18 insertions(+), 5 deletions(-) create mode 100644 prdoc/pr_6603.prdoc diff --git a/prdoc/pr_6603.prdoc b/prdoc/pr_6603.prdoc new file mode 100644 index 000000000000..20c5e7294dfa --- /dev/null +++ b/prdoc/pr_6603.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: Always provide main protocol name in litep2p responses + +doc: + - audience: [ Node Dev, Node Operator ] + description: | + This PR aligns litep2p behavior with libp2p. Previously, litep2p network backend + would provide the actual negotiated request-response protocol that produced a + response message. After this PR, only the main protocol name is reported to other + subsystems. + +crates: + - name: sc-network + bump: patch diff --git a/substrate/client/network/src/litep2p/shim/request_response/mod.rs b/substrate/client/network/src/litep2p/shim/request_response/mod.rs index bfd7a60ef9fe..146f2e4add97 100644 --- a/substrate/client/network/src/litep2p/shim/request_response/mod.rs +++ b/substrate/client/network/src/litep2p/shim/request_response/mod.rs @@ -320,7 +320,7 @@ impl RequestResponseProtocol { &mut self, peer: litep2p::PeerId, request_id: RequestId, - fallback: Option, + _fallback: Option, response: Vec, ) { match self.pending_inbound_responses.remove(&request_id) { @@ -337,10 +337,7 @@ impl RequestResponseProtocol { response.len(), ); - let _ = tx.send(Ok(( - response, - fallback.map_or_else(|| self.protocol.clone(), Into::into), - ))); + let _ = tx.send(Ok((response, self.protocol.clone()))); self.metrics.register_outbound_request_success(started.elapsed()); }, } From 5b1b34db0c03057c7962eb9682b4d581c324ae26 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Wed, 27 Nov 2024 14:10:36 +0200 Subject: [PATCH 03/10] v16: Expose the unstable metadata v16 (#5732) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR exposes the *unstable* metadata V16. The metadata is exposed under the unstable u32::MAX number. Developers can start experimenting with the new features of the metadata v16. *Please note that this metadata is under development and expect breaking changes until stabilization.* The `ExtrinsicMetadata` trait receives a breaking change. Its associated type `VERSION` is rename to `VERSIONS` and now supports a constant static list of metadata versions. The versions implemented for `UncheckedExtrinsic` are v4 (legacy version) and v5 (new version). For metadata collection, it is assumed that all `TransactionExtensions` are under version 0. Builds on top of: https://github.com/paritytech/polkadot-sdk/pull/5274 Closes: https://github.com/paritytech/polkadot-sdk/issues/5980 Closes: https://github.com/paritytech/polkadot-sdk/issues/5347 Closes: https://github.com/paritytech/polkadot-sdk/issues/5285 cc @paritytech/subxt-team --------- Signed-off-by: Alexandru Vasile Co-authored-by: Niklas Adolfsson Co-authored-by: Bastian Köcher Co-authored-by: James Wilson Co-authored-by: GitHub Action --- Cargo.lock | 28 ++- Cargo.toml | 4 +- prdoc/pr_5732.prdoc | 29 +++ .../frame/metadata-hash-extension/Cargo.toml | 2 +- substrate/frame/revive/src/evm/runtime.rs | 8 +- substrate/frame/support/Cargo.toml | 1 + .../src/construct_runtime/expand/metadata.rs | 2 +- substrate/frame/support/test/Cargo.toml | 2 +- substrate/frame/support/test/tests/pallet.rs | 8 +- substrate/primitives/metadata-ir/Cargo.toml | 2 +- substrate/primitives/metadata-ir/src/lib.rs | 25 ++- substrate/primitives/metadata-ir/src/types.rs | 6 +- .../primitives/metadata-ir/src/unstable.rs | 211 ++++++++++++++++++ substrate/primitives/metadata-ir/src/v14.rs | 5 +- substrate/primitives/metadata-ir/src/v15.rs | 2 +- .../src/generic/unchecked_extrinsic.rs | 3 +- .../primitives/runtime/src/traits/mod.rs | 6 +- .../src/overhead/remark_builder.rs | 8 +- .../src/overhead/runtime_utilities.rs | 15 +- substrate/utils/wasm-builder/Cargo.toml | 2 +- 20 files changed, 329 insertions(+), 40 deletions(-) create mode 100644 prdoc/pr_5732.prdoc create mode 100644 substrate/primitives/metadata-ir/src/unstable.rs diff --git a/Cargo.lock b/Cargo.lock index 58b8b222cce4..2c938ec17bd0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7224,6 +7224,18 @@ dependencies = [ "serde", ] +[[package]] +name = "frame-metadata" +version = "18.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "daaf440c68eb2c3d88e5760fe8c7af3f9fee9181fab6c2f2c4e7cc48dcc40bb8" +dependencies = [ + "cfg-if", + "parity-scale-codec", + "scale-info", + "serde", +] + [[package]] name = "frame-metadata-hash-extension" version = "0.1.0" @@ -7231,7 +7243,7 @@ dependencies = [ "array-bytes", "const-hex", "docify", - "frame-metadata 16.0.0", + "frame-metadata 18.0.0", "frame-support 28.0.0", "frame-system 28.0.0", "log", @@ -7316,7 +7328,7 @@ dependencies = [ "bitflags 1.3.2", "docify", "environmental", - "frame-metadata 16.0.0", + "frame-metadata 18.0.0", "frame-support-procedural 23.0.0", "frame-system 28.0.0", "impl-trait-for-tuples", @@ -7494,7 +7506,7 @@ version = "3.0.0" dependencies = [ "frame-benchmarking 28.0.0", "frame-executive 28.0.0", - "frame-metadata 16.0.0", + "frame-metadata 18.0.0", "frame-support 28.0.0", "frame-support-test-pallet", "frame-system 28.0.0", @@ -10520,13 +10532,13 @@ dependencies = [ [[package]] name = "merkleized-metadata" -version = "0.1.0" +version = "0.1.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f313fcff1d2a4bcaa2deeaa00bf7530d77d5f7bd0467a117dde2e29a75a7a17a" +checksum = "943f6d92804ed0100803d51fa9b21fd9432b5d122ba4c713dc26fe6d2f619cf6" dependencies = [ "array-bytes", "blake3", - "frame-metadata 16.0.0", + "frame-metadata 18.0.0", "parity-scale-codec", "scale-decode 0.13.1", "scale-info", @@ -26664,7 +26676,7 @@ dependencies = [ name = "sp-metadata-ir" version = "0.6.0" dependencies = [ - "frame-metadata 16.0.0", + "frame-metadata 18.0.0", "parity-scale-codec", "scale-info", ] @@ -28601,7 +28613,7 @@ dependencies = [ "cargo_metadata", "console", "filetime", - "frame-metadata 16.0.0", + "frame-metadata 18.0.0", "jobserver", "merkleized-metadata", "parity-scale-codec", diff --git a/Cargo.toml b/Cargo.toml index 53f95406e79a..b1a52712e736 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -779,7 +779,7 @@ frame-benchmarking-pallet-pov = { default-features = false, path = "substrate/fr frame-election-provider-solution-type = { path = "substrate/frame/election-provider-support/solution-type", default-features = false } frame-election-provider-support = { path = "substrate/frame/election-provider-support", default-features = false } frame-executive = { path = "substrate/frame/executive", default-features = false } -frame-metadata = { version = "16.0.0", default-features = false } +frame-metadata = { version = "18.0.0", default-features = false } frame-metadata-hash-extension = { path = "substrate/frame/metadata-hash-extension", default-features = false } frame-support = { path = "substrate/frame/support", default-features = false } frame-support-procedural = { path = "substrate/frame/support/procedural", default-features = false } @@ -854,7 +854,7 @@ macro_magic = { version = "0.5.1" } maplit = { version = "1.0.2" } memmap2 = { version = "0.9.3" } memory-db = { version = "0.32.0", default-features = false } -merkleized-metadata = { version = "0.1.0" } +merkleized-metadata = { version = "0.1.2" } merlin = { version = "3.0", default-features = false } messages-relay = { path = "bridges/relays/messages" } metered = { version = "0.6.1", default-features = false, package = "prioritized-metered-channel" } diff --git a/prdoc/pr_5732.prdoc b/prdoc/pr_5732.prdoc new file mode 100644 index 000000000000..6f3f9b8a1668 --- /dev/null +++ b/prdoc/pr_5732.prdoc @@ -0,0 +1,29 @@ +title: Expose the unstable metadata v16 +doc: +- audience: Node Dev + description: | + This PR exposes the *unstable* metadata V16. The metadata is exposed under the unstable u32::MAX number. + Developers can start experimenting with the new features of the metadata v16. *Please note that this metadata is under development and expect breaking changes until stabilization.* + The `ExtrinsicMetadata` trait receives a breaking change. Its associated type `VERSION` is rename to `VERSIONS` and now supports a constant static list of metadata versions. + The versions implemented for `UncheckedExtrinsic` are v4 (legacy version) and v5 (new version). + For metadata collection, it is assumed that all `TransactionExtensions` are under version 0. + +crates: + - name: sp-metadata-ir + bump: major + - name: frame-support-procedural + bump: patch + - name: frame-support + bump: minor + - name: frame-support-test + bump: major + - name: frame-metadata-hash-extension + bump: patch + - name: substrate-wasm-builder + bump: minor + - name: pallet-revive + bump: minor + - name: sp-runtime + bump: major + - name: frame-benchmarking-cli + bump: patch diff --git a/substrate/frame/metadata-hash-extension/Cargo.toml b/substrate/frame/metadata-hash-extension/Cargo.toml index bca2c3ffb198..8f4ba922984c 100644 --- a/substrate/frame/metadata-hash-extension/Cargo.toml +++ b/substrate/frame/metadata-hash-extension/Cargo.toml @@ -25,7 +25,7 @@ substrate-test-runtime-client = { workspace = true } sp-api = { workspace = true, default-features = true } sp-transaction-pool = { workspace = true, default-features = true } merkleized-metadata = { workspace = true } -frame-metadata = { features = ["current"], workspace = true, default-features = true } +frame-metadata = { features = ["current", "unstable"], workspace = true, default-features = true } sp-tracing = { workspace = true, default-features = true } [features] diff --git a/substrate/frame/revive/src/evm/runtime.rs b/substrate/frame/revive/src/evm/runtime.rs index 40c210304ca2..b5dc9a36065b 100644 --- a/substrate/frame/revive/src/evm/runtime.rs +++ b/substrate/frame/revive/src/evm/runtime.rs @@ -92,8 +92,12 @@ impl ExtrinsicLike impl ExtrinsicMetadata for UncheckedExtrinsic { - const VERSION: u8 = - generic::UncheckedExtrinsic::, Signature, E::Extension>::VERSION; + const VERSIONS: &'static [u8] = generic::UncheckedExtrinsic::< + Address, + CallOf, + Signature, + E::Extension, + >::VERSIONS; type TransactionExtensions = E::Extension; } diff --git a/substrate/frame/support/Cargo.toml b/substrate/frame/support/Cargo.toml index d7da034b3492..d48c80510581 100644 --- a/substrate/frame/support/Cargo.toml +++ b/substrate/frame/support/Cargo.toml @@ -28,6 +28,7 @@ scale-info = { features = [ ], workspace = true } frame-metadata = { features = [ "current", + "unstable", ], workspace = true } sp-api = { features = [ "frame-metadata", diff --git a/substrate/frame/support/procedural/src/construct_runtime/expand/metadata.rs b/substrate/frame/support/procedural/src/construct_runtime/expand/metadata.rs index c12fc20bc8b8..4590a3a7f490 100644 --- a/substrate/frame/support/procedural/src/construct_runtime/expand/metadata.rs +++ b/substrate/frame/support/procedural/src/construct_runtime/expand/metadata.rs @@ -117,7 +117,7 @@ pub fn expand_runtime_metadata( pallets: #scrate::__private::vec![ #(#pallets),* ], extrinsic: #scrate::__private::metadata_ir::ExtrinsicMetadataIR { ty, - version: <#extrinsic as #scrate::sp_runtime::traits::ExtrinsicMetadata>::VERSION, + versions: <#extrinsic as #scrate::sp_runtime::traits::ExtrinsicMetadata>::VERSIONS.into_iter().map(|ref_version| *ref_version).collect(), address_ty, call_ty, signature_ty, diff --git a/substrate/frame/support/test/Cargo.toml b/substrate/frame/support/test/Cargo.toml index 2187ee22b395..17ee3130b741 100644 --- a/substrate/frame/support/test/Cargo.toml +++ b/substrate/frame/support/test/Cargo.toml @@ -19,7 +19,7 @@ static_assertions = { workspace = true, default-features = true } serde = { features = ["derive"], workspace = true } codec = { features = ["derive"], workspace = true } scale-info = { features = ["derive"], workspace = true } -frame-metadata = { features = ["current"], workspace = true } +frame-metadata = { features = ["current", "unstable"], workspace = true } sp-api = { workspace = true } sp-arithmetic = { workspace = true } sp-io = { workspace = true } diff --git a/substrate/frame/support/test/tests/pallet.rs b/substrate/frame/support/test/tests/pallet.rs index de7f7eb4bc97..9df1f461bba2 100644 --- a/substrate/frame/support/test/tests/pallet.rs +++ b/substrate/frame/support/test/tests/pallet.rs @@ -53,6 +53,9 @@ parameter_types! { /// Latest stable metadata version used for testing. const LATEST_METADATA_VERSION: u32 = 15; +/// Unstable metadata version. +const UNSTABLE_METADATA_VERSION: u32 = u32::MAX; + pub struct SomeType1; impl From for u64 { fn from(_t: SomeType1) -> Self { @@ -1977,7 +1980,10 @@ fn metadata_at_version() { #[test] fn metadata_versions() { - assert_eq!(vec![14, LATEST_METADATA_VERSION], Runtime::metadata_versions()); + assert_eq!( + vec![14, LATEST_METADATA_VERSION, UNSTABLE_METADATA_VERSION], + Runtime::metadata_versions() + ); } #[test] diff --git a/substrate/primitives/metadata-ir/Cargo.toml b/substrate/primitives/metadata-ir/Cargo.toml index d7786347dd02..046441104b88 100644 --- a/substrate/primitives/metadata-ir/Cargo.toml +++ b/substrate/primitives/metadata-ir/Cargo.toml @@ -17,7 +17,7 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] codec = { workspace = true } -frame-metadata = { features = ["current"], workspace = true } +frame-metadata = { features = ["current", "unstable"], workspace = true } scale-info = { features = ["derive"], workspace = true } [features] diff --git a/substrate/primitives/metadata-ir/src/lib.rs b/substrate/primitives/metadata-ir/src/lib.rs index 4bd13b935afd..bf234432a1a6 100644 --- a/substrate/primitives/metadata-ir/src/lib.rs +++ b/substrate/primitives/metadata-ir/src/lib.rs @@ -30,6 +30,7 @@ mod types; use frame_metadata::RuntimeMetadataPrefixed; pub use types::*; +mod unstable; mod v14; mod v15; @@ -39,23 +40,33 @@ const V14: u32 = 14; /// Metadata V15. const V15: u32 = 15; +/// Unstable metadata V16. +const UNSTABLE_V16: u32 = u32::MAX; + /// Transform the IR to the specified version. /// /// Use [`supported_versions`] to find supported versions. pub fn into_version(metadata: MetadataIR, version: u32) -> Option { // Note: Unstable metadata version is `u32::MAX` until stabilized. match version { - // Latest stable version. + // Version V14. This needs to be around until the + // deprecation of the `Metadata_metadata` runtime call in favor of + // `Metadata_metadata_at_version. V14 => Some(into_v14(metadata)), - // Unstable metadata. + + // Version V15 - latest stable. V15 => Some(into_latest(metadata)), + + // Unstable metadata under `u32::MAX`. + UNSTABLE_V16 => Some(into_unstable(metadata)), + _ => None, } } /// Returns the supported metadata versions. pub fn supported_versions() -> alloc::vec::Vec { - alloc::vec![V14, V15] + alloc::vec![V14, V15, UNSTABLE_V16] } /// Transform the IR to the latest stable metadata version. @@ -70,6 +81,12 @@ pub fn into_v14(metadata: MetadataIR) -> RuntimeMetadataPrefixed { latest.into() } +/// Transform the IR to unstable metadata version 16. +pub fn into_unstable(metadata: MetadataIR) -> RuntimeMetadataPrefixed { + let latest: frame_metadata::v16::RuntimeMetadataV16 = metadata.into(); + latest.into() +} + #[cfg(test)] mod test { use super::*; @@ -81,7 +98,7 @@ mod test { pallets: vec![], extrinsic: ExtrinsicMetadataIR { ty: meta_type::<()>(), - version: 0, + versions: vec![0], address_ty: meta_type::<()>(), call_ty: meta_type::<()>(), signature_ty: meta_type::<()>(), diff --git a/substrate/primitives/metadata-ir/src/types.rs b/substrate/primitives/metadata-ir/src/types.rs index 199b692fbd8c..af217ffe16ee 100644 --- a/substrate/primitives/metadata-ir/src/types.rs +++ b/substrate/primitives/metadata-ir/src/types.rs @@ -170,8 +170,8 @@ pub struct ExtrinsicMetadataIR { /// /// Note: Field used for metadata V14 only. pub ty: T::Type, - /// Extrinsic version. - pub version: u8, + /// Extrinsic versions. + pub versions: Vec, /// The type of the address that signs the extrinsic pub address_ty: T::Type, /// The type of the outermost Call enum. @@ -191,7 +191,7 @@ impl IntoPortable for ExtrinsicMetadataIR { fn into_portable(self, registry: &mut Registry) -> Self::Output { ExtrinsicMetadataIR { ty: registry.register_type(&self.ty), - version: self.version, + versions: self.versions, address_ty: registry.register_type(&self.address_ty), call_ty: registry.register_type(&self.call_ty), signature_ty: registry.register_type(&self.signature_ty), diff --git a/substrate/primitives/metadata-ir/src/unstable.rs b/substrate/primitives/metadata-ir/src/unstable.rs new file mode 100644 index 000000000000..d46ce3ec6a7d --- /dev/null +++ b/substrate/primitives/metadata-ir/src/unstable.rs @@ -0,0 +1,211 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Convert the IR to V16 metadata. + +use crate::{ + DeprecationInfoIR, DeprecationStatusIR, OuterEnumsIR, PalletAssociatedTypeMetadataIR, + PalletCallMetadataIR, PalletConstantMetadataIR, PalletErrorMetadataIR, PalletEventMetadataIR, + PalletStorageMetadataIR, StorageEntryMetadataIR, +}; + +use super::types::{ + ExtrinsicMetadataIR, MetadataIR, PalletMetadataIR, RuntimeApiMetadataIR, + RuntimeApiMethodMetadataIR, RuntimeApiMethodParamMetadataIR, TransactionExtensionMetadataIR, +}; + +use frame_metadata::v16::{ + CustomMetadata, DeprecationInfo, DeprecationStatus, ExtrinsicMetadata, OuterEnums, + PalletAssociatedTypeMetadata, PalletCallMetadata, PalletConstantMetadata, PalletErrorMetadata, + PalletEventMetadata, PalletMetadata, PalletStorageMetadata, RuntimeApiMetadata, + RuntimeApiMethodMetadata, RuntimeApiMethodParamMetadata, RuntimeMetadataV16, + StorageEntryMetadata, TransactionExtensionMetadata, +}; + +impl From for RuntimeMetadataV16 { + fn from(ir: MetadataIR) -> Self { + RuntimeMetadataV16::new( + ir.pallets.into_iter().map(Into::into).collect(), + ir.extrinsic.into(), + ir.apis.into_iter().map(Into::into).collect(), + ir.outer_enums.into(), + // Substrate does not collect yet the custom metadata fields. + // This allows us to extend the V16 easily. + CustomMetadata { map: Default::default() }, + ) + } +} + +impl From for RuntimeApiMetadata { + fn from(ir: RuntimeApiMetadataIR) -> Self { + RuntimeApiMetadata { + name: ir.name, + methods: ir.methods.into_iter().map(Into::into).collect(), + docs: ir.docs, + deprecation_info: ir.deprecation_info.into(), + } + } +} + +impl From for RuntimeApiMethodMetadata { + fn from(ir: RuntimeApiMethodMetadataIR) -> Self { + RuntimeApiMethodMetadata { + name: ir.name, + inputs: ir.inputs.into_iter().map(Into::into).collect(), + output: ir.output, + docs: ir.docs, + deprecation_info: ir.deprecation_info.into(), + } + } +} + +impl From for RuntimeApiMethodParamMetadata { + fn from(ir: RuntimeApiMethodParamMetadataIR) -> Self { + RuntimeApiMethodParamMetadata { name: ir.name, ty: ir.ty } + } +} + +impl From for PalletMetadata { + fn from(ir: PalletMetadataIR) -> Self { + PalletMetadata { + name: ir.name, + storage: ir.storage.map(Into::into), + calls: ir.calls.map(Into::into), + event: ir.event.map(Into::into), + constants: ir.constants.into_iter().map(Into::into).collect(), + error: ir.error.map(Into::into), + index: ir.index, + docs: ir.docs, + associated_types: ir.associated_types.into_iter().map(Into::into).collect(), + deprecation_info: ir.deprecation_info.into(), + } + } +} + +impl From for PalletStorageMetadata { + fn from(ir: PalletStorageMetadataIR) -> Self { + PalletStorageMetadata { + prefix: ir.prefix, + entries: ir.entries.into_iter().map(Into::into).collect(), + } + } +} + +impl From for StorageEntryMetadata { + fn from(ir: StorageEntryMetadataIR) -> Self { + StorageEntryMetadata { + name: ir.name, + modifier: ir.modifier.into(), + ty: ir.ty.into(), + default: ir.default, + docs: ir.docs, + deprecation_info: ir.deprecation_info.into(), + } + } +} + +impl From for PalletAssociatedTypeMetadata { + fn from(ir: PalletAssociatedTypeMetadataIR) -> Self { + PalletAssociatedTypeMetadata { name: ir.name, ty: ir.ty, docs: ir.docs } + } +} + +impl From for PalletErrorMetadata { + fn from(ir: PalletErrorMetadataIR) -> Self { + PalletErrorMetadata { ty: ir.ty, deprecation_info: ir.deprecation_info.into() } + } +} + +impl From for PalletEventMetadata { + fn from(ir: PalletEventMetadataIR) -> Self { + PalletEventMetadata { ty: ir.ty, deprecation_info: ir.deprecation_info.into() } + } +} + +impl From for PalletCallMetadata { + fn from(ir: PalletCallMetadataIR) -> Self { + PalletCallMetadata { ty: ir.ty, deprecation_info: ir.deprecation_info.into() } + } +} + +impl From for PalletConstantMetadata { + fn from(ir: PalletConstantMetadataIR) -> Self { + PalletConstantMetadata { + name: ir.name, + ty: ir.ty, + value: ir.value, + docs: ir.docs, + deprecation_info: ir.deprecation_info.into(), + } + } +} + +impl From for TransactionExtensionMetadata { + fn from(ir: TransactionExtensionMetadataIR) -> Self { + TransactionExtensionMetadata { identifier: ir.identifier, ty: ir.ty, implicit: ir.implicit } + } +} + +impl From for ExtrinsicMetadata { + fn from(ir: ExtrinsicMetadataIR) -> Self { + // Assume version 0 for all extensions. + let indexes = (0..ir.extensions.len()).map(|index| index as u32).collect(); + let transaction_extensions_by_version = [(0, indexes)].iter().cloned().collect(); + + ExtrinsicMetadata { + versions: ir.versions, + address_ty: ir.address_ty, + signature_ty: ir.signature_ty, + transaction_extensions_by_version, + transaction_extensions: ir.extensions.into_iter().map(Into::into).collect(), + } + } +} + +impl From for OuterEnums { + fn from(ir: OuterEnumsIR) -> Self { + OuterEnums { + call_enum_ty: ir.call_enum_ty, + event_enum_ty: ir.event_enum_ty, + error_enum_ty: ir.error_enum_ty, + } + } +} + +impl From for DeprecationStatus { + fn from(ir: DeprecationStatusIR) -> Self { + match ir { + DeprecationStatusIR::NotDeprecated => DeprecationStatus::NotDeprecated, + DeprecationStatusIR::DeprecatedWithoutNote => DeprecationStatus::DeprecatedWithoutNote, + DeprecationStatusIR::Deprecated { since, note } => + DeprecationStatus::Deprecated { since, note }, + } + } +} + +impl From for DeprecationInfo { + fn from(ir: DeprecationInfoIR) -> Self { + match ir { + DeprecationInfoIR::NotDeprecated => DeprecationInfo::NotDeprecated, + DeprecationInfoIR::ItemDeprecated(status) => + DeprecationInfo::ItemDeprecated(status.into()), + DeprecationInfoIR::VariantsDeprecated(btree) => DeprecationInfo::VariantsDeprecated( + btree.into_iter().map(|(key, value)| (key.0, value.into())).collect(), + ), + } + } +} diff --git a/substrate/primitives/metadata-ir/src/v14.rs b/substrate/primitives/metadata-ir/src/v14.rs index 70e84532add9..f3cb5973f5bd 100644 --- a/substrate/primitives/metadata-ir/src/v14.rs +++ b/substrate/primitives/metadata-ir/src/v14.rs @@ -149,9 +149,12 @@ impl From for SignedExtensionMetadata { impl From for ExtrinsicMetadata { fn from(ir: ExtrinsicMetadataIR) -> Self { + let lowest_supported_version = + ir.versions.iter().min().expect("Metadata V14 supports one version; qed"); + ExtrinsicMetadata { ty: ir.ty, - version: ir.version, + version: *lowest_supported_version, signed_extensions: ir.extensions.into_iter().map(Into::into).collect(), } } diff --git a/substrate/primitives/metadata-ir/src/v15.rs b/substrate/primitives/metadata-ir/src/v15.rs index 4b3b6106d27f..ed315a31e6dc 100644 --- a/substrate/primitives/metadata-ir/src/v15.rs +++ b/substrate/primitives/metadata-ir/src/v15.rs @@ -100,7 +100,7 @@ impl From for SignedExtensionMetadata { impl From for ExtrinsicMetadata { fn from(ir: ExtrinsicMetadataIR) -> Self { ExtrinsicMetadata { - version: ir.version, + version: *ir.versions.iter().min().expect("Metadata V15 supports only one version"), address_ty: ir.address_ty, call_ty: ir.call_ty, signature_ty: ir.signature_ty, diff --git a/substrate/primitives/runtime/src/generic/unchecked_extrinsic.rs b/substrate/primitives/runtime/src/generic/unchecked_extrinsic.rs index 91ba37451909..d8510a60a789 100644 --- a/substrate/primitives/runtime/src/generic/unchecked_extrinsic.rs +++ b/substrate/primitives/runtime/src/generic/unchecked_extrinsic.rs @@ -389,8 +389,7 @@ where impl> ExtrinsicMetadata for UncheckedExtrinsic { - // TODO: Expose both version 4 and version 5 in metadata v16. - const VERSION: u8 = LEGACY_EXTRINSIC_FORMAT_VERSION; + const VERSIONS: &'static [u8] = &[LEGACY_EXTRINSIC_FORMAT_VERSION, EXTRINSIC_FORMAT_VERSION]; type TransactionExtensions = Extension; } diff --git a/substrate/primitives/runtime/src/traits/mod.rs b/substrate/primitives/runtime/src/traits/mod.rs index 02bc7adc8ba5..cfcc3e5a354d 100644 --- a/substrate/primitives/runtime/src/traits/mod.rs +++ b/substrate/primitives/runtime/src/traits/mod.rs @@ -1410,10 +1410,10 @@ impl SignaturePayload for () { /// Implementor is an [`Extrinsic`] and provides metadata about this extrinsic. pub trait ExtrinsicMetadata { - /// The format version of the `Extrinsic`. + /// The format versions of the `Extrinsic`. /// - /// By format is meant the encoded representation of the `Extrinsic`. - const VERSION: u8; + /// By format we mean the encoded representation of the `Extrinsic`. + const VERSIONS: &'static [u8]; /// Transaction extensions attached to this `Extrinsic`. type TransactionExtensions; diff --git a/substrate/utils/frame/benchmarking-cli/src/overhead/remark_builder.rs b/substrate/utils/frame/benchmarking-cli/src/overhead/remark_builder.rs index a1d5f282d9f8..3a2d8776d1e1 100644 --- a/substrate/utils/frame/benchmarking-cli/src/overhead/remark_builder.rs +++ b/substrate/utils/frame/benchmarking-cli/src/overhead/remark_builder.rs @@ -54,13 +54,15 @@ impl> DynamicRemarkBuilder { log::debug!("Found metadata API version {}.", metadata_api_version); let opaque_metadata = if metadata_api_version > 1 { - let Ok(mut supported_metadata_versions) = api.metadata_versions(genesis) else { + let Ok(supported_metadata_versions) = api.metadata_versions(genesis) else { return Err("Unable to fetch metadata versions".to_string().into()); }; let latest = supported_metadata_versions - .pop() - .ok_or("No metadata version supported".to_string())?; + .into_iter() + .filter(|v| *v != u32::MAX) + .max() + .ok_or("No stable metadata versions supported".to_string())?; api.metadata_at_version(genesis, latest) .map_err(|e| format!("Unable to fetch metadata: {:?}", e))? diff --git a/substrate/utils/frame/benchmarking-cli/src/overhead/runtime_utilities.rs b/substrate/utils/frame/benchmarking-cli/src/overhead/runtime_utilities.rs index c498da38afb0..3081197dc033 100644 --- a/substrate/utils/frame/benchmarking-cli/src/overhead/runtime_utilities.rs +++ b/substrate/utils/frame/benchmarking-cli/src/overhead/runtime_utilities.rs @@ -35,14 +35,19 @@ pub fn fetch_latest_metadata_from_code_blob( let opaque_metadata: OpaqueMetadata = match version_result { Ok(supported_versions) => { - let latest_version = Vec::::decode(&mut supported_versions.as_slice()) - .map_err(|e| format!("Unable to decode version list: {e}"))? - .pop() - .ok_or("No metadata versions supported".to_string())?; + let supported_versions = Vec::::decode(&mut supported_versions.as_slice()) + .map_err(|e| format!("Unable to decode version list: {e}"))?; + + let latest_stable = supported_versions + .into_iter() + .filter(|v| *v != u32::MAX) + .max() + .ok_or("No stable metadata versions supported".to_string())?; let encoded = runtime_caller - .call("Metadata_metadata_at_version", latest_version) + .call("Metadata_metadata_at_version", latest_stable) .map_err(|_| "Unable to fetch metadata from blob".to_string())?; + Option::::decode(&mut encoded.as_slice())? .ok_or_else(|| "Metadata not found".to_string())? }, diff --git a/substrate/utils/wasm-builder/Cargo.toml b/substrate/utils/wasm-builder/Cargo.toml index 8f0e8a23e54a..fb15e8619a38 100644 --- a/substrate/utils/wasm-builder/Cargo.toml +++ b/substrate/utils/wasm-builder/Cargo.toml @@ -35,7 +35,7 @@ sc-executor = { optional = true, workspace = true, default-features = true } sp-core = { optional = true, workspace = true, default-features = true } sp-io = { optional = true, workspace = true, default-features = true } sp-version = { optional = true, workspace = true, default-features = true } -frame-metadata = { features = ["current"], optional = true, workspace = true, default-features = true } +frame-metadata = { features = ["current", "unstable"], optional = true, workspace = true, default-features = true } codec = { optional = true, workspace = true, default-features = true } array-bytes = { optional = true, workspace = true, default-features = true } sp-tracing = { optional = true, workspace = true, default-features = true } From afd065fa7267494246a9a8d767dfd030e2681fce Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Wed, 27 Nov 2024 20:12:39 +0200 Subject: [PATCH 04/10] rpc-v2: Implement `archive_unstable_storageDiff` (#5997) This PR implements the `archive_unstable_storageDiff`. The implementation follows the rpc-v2 spec from: - https://github.com/paritytech/json-rpc-interface-spec/pull/159. - builds on top of https://github.com/paritytech/json-rpc-interface-spec/pull/161 cc @paritytech/subxt-team --------- Signed-off-by: Alexandru Vasile Co-authored-by: James Wilson --- Cargo.lock | 7 +- prdoc/pr_5997.prdoc | 18 + substrate/client/rpc-spec-v2/Cargo.toml | 1 + .../client/rpc-spec-v2/src/archive/api.rs | 22 +- .../client/rpc-spec-v2/src/archive/archive.rs | 89 +- .../src/archive/archive_storage.rs | 829 +++++++++++++++++- .../client/rpc-spec-v2/src/archive/tests.rs | 277 +++++- .../client/rpc-spec-v2/src/common/events.rs | 208 ++++- .../client/rpc-spec-v2/src/common/storage.rs | 15 + substrate/client/service/src/builder.rs | 1 + 10 files changed, 1449 insertions(+), 18 deletions(-) create mode 100644 prdoc/pr_5997.prdoc diff --git a/Cargo.lock b/Cargo.lock index 2c938ec17bd0..12e642bc9d06 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -20959,7 +20959,7 @@ checksum = "b544ef1b4eac5dc2db33ea63606ae9ffcfac26c1416a2806ae0bf5f56b201191" dependencies = [ "aho-corasick", "memchr", - "regex-automata 0.4.9", + "regex-automata 0.4.8", "regex-syntax 0.8.5", ] @@ -20980,9 +20980,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", @@ -23277,6 +23277,7 @@ dependencies = [ "futures", "futures-util", "hex", + "itertools 0.11.0", "jsonrpsee", "log", "parity-scale-codec", diff --git a/prdoc/pr_5997.prdoc b/prdoc/pr_5997.prdoc new file mode 100644 index 000000000000..6bac36a44586 --- /dev/null +++ b/prdoc/pr_5997.prdoc @@ -0,0 +1,18 @@ +# 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: major + - name: sc-service + bump: patch diff --git a/substrate/client/rpc-spec-v2/Cargo.toml b/substrate/client/rpc-spec-v2/Cargo.toml index daa805912fb9..b304bc905925 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] async-trait = { workspace = true } diff --git a/substrate/client/rpc-spec-v2/src/archive/api.rs b/substrate/client/rpc-spec-v2/src/archive/api.rs index b19738304000..dcfeaecb147b 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::{ + ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageResult, + PaginatedStorageQuery, + }, MethodResult, }; use jsonrpsee::{core::RpcResult, proc_macros::rpc}; @@ -104,4 +107,21 @@ pub trait ArchiveApi { items: Vec>, child_trie: Option, ) -> RpcResult; + + /// Returns the storage difference between two blocks. + /// + /// # Unstable + /// + /// 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", + item = ArchiveStorageDiffEvent, + )] + fn archive_unstable_storage_diff( + &self, + hash: Hash, + 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 dd6c566a76ed..55054d91d85d 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive.rs @@ -19,17 +19,29 @@ //! API implementation for `archive`. use crate::{ - archive::{error::Error as ArchiveError, ArchiveApiServer}, - common::events::{ArchiveStorageResult, PaginatedStorageQuery}, - hex_string, MethodResult, + archive::{ + archive_storage::{ArchiveStorage, ArchiveStorageDiff}, + error::Error as ArchiveError, + ArchiveApiServer, + }, + common::events::{ + ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageResult, + PaginatedStorageQuery, + }, + 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, @@ -41,7 +53,9 @@ use sp_runtime::{ }; use std::{collections::HashSet, marker::PhantomData, sync::Arc}; -use super::archive_storage::ArchiveStorage; +use tokio::sync::mpsc; + +pub(crate) const LOG_TARGET: &str = "rpc-spec-v2::archive"; /// The configuration of [`Archive`]. pub struct ArchiveConfig { @@ -64,6 +78,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 { @@ -79,6 +99,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 @@ -96,12 +118,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, @@ -278,4 +302,59 @@ where Ok(storage_client.handle_query(hash, items, child_trie)) } + + fn archive_unstable_storage_diff( + &self, + pending: PendingSubscriptionSink, + hash: Block::Hash, + items: Vec>, + previous_hash: Option, + ) { + 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 { + let Ok(mut sink) = pending.accept().await.map(Subscription::from) else { return }; + + let previous_hash = if let Some(previous_hash) = previous_hash { + previous_hash + } else { + let Ok(Some(current_header)) = client.header(hash) else { + let message = format!("Block header is not present: {hash}"); + let _ = sink.send(&ArchiveStorageDiffEvent::err(message)).await; + return + }; + *current_header.parent_hash() + }; + + let (tx, mut rx) = tokio::sync::mpsc::channel(STORAGE_QUERY_BUF); + let storage_fut = + 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) + // - 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()); + } +} + +/// 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"); + } + + if sink.send(&event).await.is_err() { + return + } + } } 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..5a3920882f00 100644 --- a/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs +++ b/substrate/client/rpc-spec-v2/src/archive/archive_storage.rs @@ -18,15 +18,28 @@ //! Implementation of the `archive_storage` method. -use std::sync::Arc; +use std::{ + collections::{hash_map::Entry, HashMap}, + sync::Arc, +}; +use itertools::Itertools; use sc_client_api::{Backend, ChildInfo, StorageKey, StorageProvider}; use sp_runtime::traits::Block as BlockT; -use crate::common::{ - events::{ArchiveStorageResult, PaginatedStorageQuery, StorageQueryType}, - storage::{IterQueryType, QueryIter, Storage}, +use super::error::Error as ArchiveError; +use crate::{ + archive::archive::LOG_TARGET, + common::{ + events::{ + ArchiveStorageDiffEvent, ArchiveStorageDiffItem, ArchiveStorageDiffOperationType, + ArchiveStorageDiffResult, ArchiveStorageDiffType, ArchiveStorageResult, + PaginatedStorageQuery, StorageQueryType, StorageResult, + }, + storage::{IterQueryType, QueryIter, Storage}, + }, }; +use tokio::sync::mpsc; /// Generates the events of the `archive_storage` method. pub struct ArchiveStorage { @@ -127,3 +140,811 @@ 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, +} + +/// 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, +} + +/// 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, +} + +impl ArchiveStorageDiff { + pub fn new(client: Arc) -> Self { + Self { client: Storage::new(client) } + } +} + +impl ArchiveStorageDiff +where + Block: BlockT + 'static, + BE: Backend + 'static, + Client: StorageProvider + Send + Sync + 'static, +{ + /// Fetch the storage from the given key. + fn fetch_storage( + &self, + hash: Block::Hash, + key: StorageKey, + maybe_child_trie: Option, + ty: FetchStorageType, + ) -> Result, String> { + match ty { + FetchStorageType::Value => { + let result = self.client.query_value(hash, &key, maybe_child_trie.as_ref())?; + + Ok(result.map(FetchedStorage::Value)) + }, + + FetchStorageType::Hash => { + let result = self.client.query_hash(hash, &key, maybe_child_trie.as_ref())?; + + Ok(result.map(FetchedStorage::Hash)) + }, + + FetchStorageType::Both => { + let Some(value) = self.client.query_value(hash, &key, maybe_child_trie.as_ref())? + else { + return Ok(None); + }; + + let Some(hash) = self.client.query_hash(hash, &key, maybe_child_trie.as_ref())? + else { + return Ok(None); + }; + + Ok(Some(FetchedStorage::Both { value, hash })) + }, + } + } + + /// Check if the key belongs to the provided query items. + /// + /// 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) + } + + let mut value = false; + let mut hash = false; + + for item in items { + if key.as_ref().starts_with(&item.key.as_ref()) { + match item.return_type { + ArchiveStorageDiffType::Value => value = true, + ArchiveStorageDiffType::Hash => hash = true, + } + } + } + + match (value, hash) { + (true, true) => Some(FetchStorageType::Both), + (true, false) => Some(FetchStorageType::Value), + (false, true) => Some(FetchStorageType::Hash), + (false, false) => None, + } + } + + /// Send the provided result to the `tx` sender. + /// + /// Returns `false` if the sender has been closed. + fn send_result( + tx: &mpsc::Sender, + result: FetchedStorage, + operation_type: ArchiveStorageDiffOperationType, + child_trie_key: Option, + ) -> bool { + let items = match result { + FetchedStorage::Value(storage_result) | FetchedStorage::Hash(storage_result) => + vec![storage_result], + FetchedStorage::Both { value, hash } => vec![value, hash], + }; + + for item in items { + let res = ArchiveStorageDiffEvent::StorageDiff(ArchiveStorageDiffResult { + key: item.key, + result: item.result, + operation_type, + child_trie_key: child_trie_key.clone(), + }); + 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, + tx: &mpsc::Sender, + ) -> Result<(), String> { + // 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 = + items.first().and_then(|item| item.child_trie_key_string.clone()); + + // 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 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 diff_iter = lexicographic_diff(keys_iter, previous_keys_iter); + + 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 { + // The key does not belong the the query items. + continue; + }; + + 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 + }; + + // For modified records we need to check the actual storage values. + if storage_result == previous_storage_result { + continue + } + + Some(storage_result) + }, + }; + + if let Some(storage_result) = maybe_result { + if !Self::send_result( + &tx, + storage_result, + operation_type, + maybe_child_trie_str.clone(), + ) { + return Ok(()) + } + } + } + + Ok(()) + } + + /// 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, + tx: mpsc::Sender, + ) -> Result<(), tokio::task::JoinError> { + let this = ArchiveStorageDiff { client: self.client.clone() }; + + tokio::task::spawn_blocking(move || { + // 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={:?}", + hash, + previous_hash, + items + ); + + 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)); + + return + } else { + log::trace!( + target: LOG_TARGET, + "handle_trie_queries: sending storage diff done", + ); + } + } + + let _ = tx.blocking_send(ArchiveStorageDiffEvent::StorageDiffDone); + }) + .await?; + + Ok(()) + } +} + +/// The result of the `lexicographic_diff` method. +#[derive(Debug, PartialEq)] +enum Diff { + Added(T), + Deleted(T), + Equal(T), +} + +/// Compare two iterators lexicographically and return the differences. +fn lexicographic_diff( + 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. +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 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 + } + + // 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; + } + } + + if should_insert { + entry.get_mut().push(diff_item); + } + }, + Entry::Vacant(entry) => { + entry.insert(vec![diff_item]); + }, + } + } + + 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); + } + + #[test] + fn dedup_with_shorter_key_reverse_order() { + 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); + } + + #[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); + } + + #[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); + } +} diff --git a/substrate/client/rpc-spec-v2/src/archive/tests.rs b/substrate/client/rpc-spec-v2/src/archive/tests.rs index 078016f5b3e2..994c5d28bd61 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,10 +33,13 @@ 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; use sp_blockchain::HeaderBackend; use sp_consensus::BlockOrigin; use sp_core::{Blake2Hasher, Hasher}; @@ -78,6 +82,7 @@ fn setup_api( client.clone(), backend, CHAIN_GENESIS, + Arc::new(TokioTestExecutor::default()), ArchiveConfig { max_descendant_responses, max_queried_items }, ) .into_rpc(); @@ -85,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); @@ -838,3 +852,260 @@ 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(); + 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") + ); +} diff --git a/substrate/client/rpc-spec-v2/src/common/events.rs b/substrate/client/rpc-spec-v2/src/common/events.rs index b1627d74c844..198a60bf4cac 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,17 +136,221 @@ 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. pub error: String, } +/// The type of the archive storage difference query. +#[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. + 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, Copy, PartialEq, Eq, 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, Eq, 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, +} + +/// 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, Eq, 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, +} + +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::*; + #[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. 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()) + } } diff --git a/substrate/client/service/src/builder.rs b/substrate/client/service/src/builder.rs index ac9371a8941b..027a444012af 100644 --- a/substrate/client/service/src/builder.rs +++ b/substrate/client/service/src/builder.rs @@ -755,6 +755,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 2ef2723126584dfcd6d2a9272282ee78375dbcd3 Mon Sep 17 00:00:00 2001 From: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> Date: Wed, 27 Nov 2024 21:40:23 +0100 Subject: [PATCH 05/10] chain-spec-guide-runtime: path to wasm blob fixed (#6673) In `chain-spec-guide-runtime` crate's tests, there was assumption that release version of wasm blob exists. This PR uses `chain_spec_guide_runtime::runtime::WASM_BINARY_PATH` const to use correct path to runtime blob. --------- Co-authored-by: GitHub Action --- .../tests/chain_spec_builder_tests.rs | 14 ++++++++------ prdoc/pr_6673.prdoc | 7 +++++++ 2 files changed, 15 insertions(+), 6 deletions(-) create mode 100644 prdoc/pr_6673.prdoc diff --git a/docs/sdk/src/reference_docs/chain_spec_runtime/tests/chain_spec_builder_tests.rs b/docs/sdk/src/reference_docs/chain_spec_runtime/tests/chain_spec_builder_tests.rs index c2fe5a6727e6..df400b68f79d 100644 --- a/docs/sdk/src/reference_docs/chain_spec_runtime/tests/chain_spec_builder_tests.rs +++ b/docs/sdk/src/reference_docs/chain_spec_runtime/tests/chain_spec_builder_tests.rs @@ -1,8 +1,10 @@ use serde_json::{json, Value}; use std::{process::Command, str}; -const WASM_FILE_PATH: &str = - "../../../../../target/release/wbuild/chain-spec-guide-runtime/chain_spec_guide_runtime.wasm"; +fn wasm_file_path() -> &'static str { + chain_spec_guide_runtime::runtime::WASM_BINARY_PATH + .expect("chain_spec_guide_runtime wasm should exist. qed") +} const CHAIN_SPEC_BUILDER_PATH: &str = "../../../../../target/release/chain-spec-builder"; @@ -26,7 +28,7 @@ fn list_presets() { let output = Command::new(get_chain_spec_builder_path()) .arg("list-presets") .arg("-r") - .arg(WASM_FILE_PATH) + .arg(wasm_file_path()) .output() .expect("Failed to execute command"); @@ -50,7 +52,7 @@ fn get_preset() { let output = Command::new(get_chain_spec_builder_path()) .arg("display-preset") .arg("-r") - .arg(WASM_FILE_PATH) + .arg(wasm_file_path()) .arg("-p") .arg("preset_2") .output() @@ -83,7 +85,7 @@ fn generate_chain_spec() { .arg("/dev/stdout") .arg("create") .arg("-r") - .arg(WASM_FILE_PATH) + .arg(wasm_file_path()) .arg("named-preset") .arg("preset_2") .output() @@ -140,7 +142,7 @@ fn generate_para_chain_spec() { .arg("-p") .arg("1000") .arg("-r") - .arg(WASM_FILE_PATH) + .arg(wasm_file_path()) .arg("named-preset") .arg("preset_2") .output() diff --git a/prdoc/pr_6673.prdoc b/prdoc/pr_6673.prdoc new file mode 100644 index 000000000000..d2ca3c61ff39 --- /dev/null +++ b/prdoc/pr_6673.prdoc @@ -0,0 +1,7 @@ +title: 'chain-spec-guide-runtime: path to wasm blob fixed' +doc: +- audience: Runtime Dev + description: In `chain-spec-guide-runtime` crate's tests, there was assumption that + release version of wasm blob exists. This PR uses `chain_spec_guide_runtime::runtime::WASM_BINARY_PATH` + const to use correct path to runtime blob. +crates: [] From 51c3e95a05c528b3869ad267aeb5c356551e38db Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Thu, 28 Nov 2024 11:16:06 +0200 Subject: [PATCH 06/10] chore: Update litep2p to v0.8.2 (#6677) This includes a critical fix for debug release versions of litep2p (which are running in Kusama as validators). While at it, have stopped the oncall pain of alerts around `incoming_connections_total`. We can rethink the metric expose of litep2p in Q1. ## [0.8.2] - 2024-11-27 This release ensures that the provided peer identity is verified at the crypto/noise protocol level, enhancing security and preventing potential misuses. The release also includes a fix that caused `TransportService` component to panic on debug builds. ### Fixed - req-resp: Fix panic on connection closed for substream open failure ([#291](https://github.com/paritytech/litep2p/pull/291)) - crypto/noise: Verify crypto/noise signature payload ([#278](https://github.com/paritytech/litep2p/pull/278)) ### Changed - transport_service/logs: Provide less details for trace logs ([#292](https://github.com/paritytech/litep2p/pull/292)) ## Testing Done This has been extensively tested in Kusama on all validators, that are now running litep2p. Deployed PR: https://github.com/paritytech/polkadot-sdk/pull/6638 ### Litep2p Dashboards ![Screenshot 2024-11-26 at 19 19 41](https://github.com/user-attachments/assets/e00b2b2b-7e64-4d96-ab26-165e2b8d0dc9) ### Libp2p vs Litep2p CPU usage After deploying litep2p we have reduced CPU usage from around 300-400% to 200%, this is a significant boost in performance, freeing resources for other subcomponents to function more optimally. ![image(1)](https://github.com/user-attachments/assets/fa793df5-4d58-4601-963d-246e56dd2a26) cc @paritytech/sdk-node --------- Signed-off-by: Alexandru Vasile Co-authored-by: GitHub Action --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- prdoc/pr_6677.prdoc | 11 +++++++++++ substrate/client/network/src/litep2p/mod.rs | 11 ++++++++++- 4 files changed, 24 insertions(+), 4 deletions(-) create mode 100644 prdoc/pr_6677.prdoc diff --git a/Cargo.lock b/Cargo.lock index 12e642bc9d06..84477cd05416 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10216,9 +10216,9 @@ dependencies = [ [[package]] name = "litep2p" -version = "0.8.1" +version = "0.8.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5b67484b8ac41e1cfdf012f65fa81e88c2ef5f8a7d6dec0e2678c2d06dc04530" +checksum = "569e7dbec8a0d4b08d30f4942cd579cfe8db5d3f83f8604abe61697c38d17e73" dependencies = [ "async-trait", "bs58", diff --git a/Cargo.toml b/Cargo.toml index b1a52712e736..964964908a9b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -848,7 +848,7 @@ linked-hash-map = { version = "0.5.4" } linked_hash_set = { version = "0.1.4" } linregress = { version = "0.5.1" } lite-json = { version = "0.2.0", default-features = false } -litep2p = { version = "0.8.1", features = ["websocket"] } +litep2p = { version = "0.8.2", features = ["websocket"] } log = { version = "0.4.22", default-features = false } macro_magic = { version = "0.5.1" } maplit = { version = "1.0.2" } diff --git a/prdoc/pr_6677.prdoc b/prdoc/pr_6677.prdoc new file mode 100644 index 000000000000..c6766889e68d --- /dev/null +++ b/prdoc/pr_6677.prdoc @@ -0,0 +1,11 @@ +title: 'chore: Update litep2p to v0.8.2' +doc: +- audience: Node Dev + description: |- + This includes a critical fix for debug release versions of litep2p (which are running in Kusama as validators). + + While at it, have stopped the oncall pain of alerts around `incoming_connections_total`. We can rethink the metric expose of litep2p in Q1. + +crates: +- name: sc-network + bump: minor diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs index 10cf9f4da36d..6d3575fc2b6b 100644 --- a/substrate/client/network/src/litep2p/mod.rs +++ b/substrate/client/network/src/litep2p/mod.rs @@ -986,7 +986,15 @@ impl NetworkBackend for Litep2pNetworkBac let direction = match endpoint { Endpoint::Dialer { .. } => "out", - Endpoint::Listener { .. } => "in", + Endpoint::Listener { .. } => { + // Increment incoming connections counter. + // + // Note: For litep2p these are represented by established negotiated connections, + // while for libp2p (legacy) these represent not-yet-negotiated connections. + metrics.incoming_connections_total.inc(); + + "in" + }, }; metrics.connections_opened_total.with_label_values(&[direction]).inc(); @@ -1058,6 +1066,7 @@ impl NetworkBackend for Litep2pNetworkBac NegotiationError::ParseError(_) => "parse-error", NegotiationError::IoError(_) => "io-error", NegotiationError::WebSocket(_) => "webscoket-error", + NegotiationError::BadSignature => "bad-signature", } }; From 9ec8009c5a8fdf89499fcd2a40df0292d3950efa Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Thu, 28 Nov 2024 12:41:09 +0100 Subject: [PATCH 07/10] Multiple instances for pallet-bridge-relayers fix (#6684) Previously, we added multi-instance pallet support for `pallet-bridge-relayers`, but missed fixing it in this one place. --------- Co-authored-by: GitHub Action Co-authored-by: Adrian Catangiu --- bridges/modules/relayers/src/extension/mod.rs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/bridges/modules/relayers/src/extension/mod.rs b/bridges/modules/relayers/src/extension/mod.rs index 34d280d26d6e..d562ed9bcd0e 100644 --- a/bridges/modules/relayers/src/extension/mod.rs +++ b/bridges/modules/relayers/src/extension/mod.rs @@ -129,7 +129,7 @@ pub struct BridgeRelayersTransactionExtension( impl BridgeRelayersTransactionExtension where Self: 'static + Send + Sync, - R: RelayersConfig + R: RelayersConfig + BridgeMessagesConfig + TransactionPaymentConfig, C: ExtensionConfig, @@ -250,7 +250,7 @@ where // let's also replace the weight of slashing relayer with the weight of rewarding relayer if call_info.is_receive_messages_proof_call() { post_info_weight = post_info_weight.saturating_sub( - ::WeightInfo::extra_weight_of_successful_receive_messages_proof_call(), + >::WeightInfo::extra_weight_of_successful_receive_messages_proof_call(), ); } @@ -278,7 +278,7 @@ impl TransactionExtension for BridgeRelayersTransactionExtension where Self: 'static + Send + Sync, - R: RelayersConfig + R: RelayersConfig + BridgeMessagesConfig + TransactionPaymentConfig, C: ExtensionConfig, @@ -326,7 +326,9 @@ where }; // we only boost priority if relayer has staked required balance - if !RelayersPallet::::is_registration_active(&data.relayer) { + if !RelayersPallet::::is_registration_active( + &data.relayer, + ) { return Ok((Default::default(), Some(data), origin)) } @@ -382,7 +384,11 @@ where match call_result { RelayerAccountAction::None => (), RelayerAccountAction::Reward(relayer, reward_account, reward) => { - RelayersPallet::::register_relayer_reward(reward_account, &relayer, reward); + RelayersPallet::::register_relayer_reward( + reward_account, + &relayer, + reward, + ); log::trace!( target: LOG_TARGET, @@ -394,7 +400,7 @@ where ); }, RelayerAccountAction::Slash(relayer, slash_account) => - RelayersPallet::::slash_and_deregister( + RelayersPallet::::slash_and_deregister( &relayer, ExplicitOrAccountParams::Params(slash_account), ), From 23369acd34411cfae924718a2834df1a7ea8bc05 Mon Sep 17 00:00:00 2001 From: Ludovic_Domingues Date: Thu, 28 Nov 2024 15:27:49 +0100 Subject: [PATCH 08/10] Migrating pallet-xcm-benchmarks to V2 (#6618) # Description Migrated pallet-xcm-benchmarks to benchmaking syntax V2 This is part of #6202 --------- Co-authored-by: Giuseppe Re --- .../src/generic/benchmarking.rs | 652 +++++++++++------- 1 file changed, 394 insertions(+), 258 deletions(-) diff --git a/polkadot/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs b/polkadot/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs index 87bf27e4ff18..f4836b7cdde1 100644 --- a/polkadot/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs +++ b/polkadot/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs @@ -13,12 +13,13 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . +#![cfg(feature = "runtime-benchmarks")] use super::*; use crate::{account_and_location, new_executor, EnsureDelivery, XcmCallOf}; use alloc::{vec, vec::Vec}; use codec::Encode; -use frame_benchmarking::{benchmarks, BenchmarkError}; +use frame_benchmarking::v2::*; use frame_support::traits::fungible::Inspect; use xcm::{ latest::{prelude::*, MaxDispatchErrorLen, MaybeErrorCode, Weight, MAX_ITEMS_IN_ASSETS}, @@ -29,16 +30,21 @@ use xcm_executor::{ ExecutorError, FeesMode, }; -benchmarks! { - report_holding { +#[benchmarks] +mod benchmarks { + use super::*; + + #[benchmark] + fn report_holding() -> Result<(), BenchmarkError> { let (sender_account, sender_location) = account_and_location::(1); let destination = T::valid_destination().map_err(|_| BenchmarkError::Skip)?; - let (expected_fees_mode, expected_assets_in_holding) = T::DeliveryHelper::ensure_successful_delivery( - &sender_location, - &destination, - FeeReason::Report, - ); + let (expected_fees_mode, expected_assets_in_holding) = + T::DeliveryHelper::ensure_successful_delivery( + &sender_location, + &destination, + FeeReason::Report, + ); let sender_account_balance_before = T::TransactAsset::balance(&sender_account); // generate holding and add possible required fees @@ -64,21 +70,33 @@ benchmarks! { query_id: Default::default(), max_weight: Weight::MAX, }, - // Worst case is looking through all holdings for every asset explicitly - respecting the limit `MAX_ITEMS_IN_ASSETS`. - assets: Definite(holding.into_inner().into_iter().take(MAX_ITEMS_IN_ASSETS).collect::>().into()), + // Worst case is looking through all holdings for every asset explicitly - respecting + // the limit `MAX_ITEMS_IN_ASSETS`. + assets: Definite( + holding + .into_inner() + .into_iter() + .take(MAX_ITEMS_IN_ASSETS) + .collect::>() + .into(), + ), }; let xcm = Xcm(vec![instruction]); - } : { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } // Check we charged the delivery fees assert!(T::TransactAsset::balance(&sender_account) <= sender_account_balance_before); + + Ok(()) } // This benchmark does not use any additional orders or instructions. This should be managed // by the `deep` and `shallow` implementation. - buy_execution { + #[benchmark] + fn buy_execution() -> Result<(), BenchmarkError> { let holding = T::worst_case_holding(0).into(); let mut executor = new_executor::(Default::default()); @@ -92,13 +110,16 @@ benchmarks! { }; let xcm = Xcm(vec![instruction]); - } : { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } + Ok(()) } - pay_fees { + #[benchmark] + fn pay_fees() -> Result<(), BenchmarkError> { let holding = T::worst_case_holding(0).into(); let mut executor = new_executor::(Default::default()); @@ -111,40 +132,53 @@ benchmarks! { let instruction = Instruction::>::PayFees { asset: fee_asset }; let xcm = Xcm(vec![instruction]); - } : { - executor.bench_process(xcm)?; - } verify {} + #[block] + { + executor.bench_process(xcm)?; + } + Ok(()) + } - set_asset_claimer { + #[benchmark] + fn set_asset_claimer() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); let (_, sender_location) = account_and_location::(1); - let instruction = Instruction::SetAssetClaimer{ location:sender_location.clone() }; + let instruction = Instruction::SetAssetClaimer { location: sender_location.clone() }; let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } assert_eq!(executor.asset_claimer(), Some(sender_location.clone())); + + Ok(()) } - query_response { + #[benchmark] + fn query_response() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); let (query_id, response) = T::worst_case_response(); let max_weight = Weight::MAX; let querier: Option = Some(Here.into()); let instruction = Instruction::QueryResponse { query_id, response, max_weight, querier }; let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + + #[block] + { + executor.bench_process(xcm)?; + } // The assert above is enough to show this XCM succeeded + + Ok(()) } // We don't care about the call itself, since that is accounted for in the weight parameter // and included in the final weight calculation. So this is just the overhead of submitting // a noop call. - transact { + #[benchmark] + fn transact() -> Result<(), BenchmarkError> { let (origin, noop_call) = T::transact_origin_and_runtime_call()?; let mut executor = new_executor::(origin); let double_encoded_noop_call: DoubleEncoded<_> = noop_call.encode().into(); @@ -154,119 +188,145 @@ benchmarks! { call: double_encoded_noop_call, }; let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } // TODO Make the assertion configurable? + + Ok(()) } - refund_surplus { + #[benchmark] + fn refund_surplus() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); let holding_assets = T::worst_case_holding(1); // We can already buy execution since we'll load the holding register manually let asset_for_fees = T::fee_asset().unwrap(); - let previous_xcm = Xcm(vec![BuyExecution { fees: asset_for_fees, weight_limit: Limited(Weight::from_parts(1337, 1337)) }]); + let previous_xcm = Xcm(vec![BuyExecution { + fees: asset_for_fees, + weight_limit: Limited(Weight::from_parts(1337, 1337)), + }]); executor.set_holding(holding_assets.into()); executor.set_total_surplus(Weight::from_parts(1337, 1337)); executor.set_total_refunded(Weight::zero()); - executor.bench_process(previous_xcm).expect("Holding has been loaded, so we can buy execution here"); + executor + .bench_process(previous_xcm) + .expect("Holding has been loaded, so we can buy execution here"); let instruction = Instruction::>::RefundSurplus; let xcm = Xcm(vec![instruction]); - } : { - let result = executor.bench_process(xcm)?; - } verify { + #[block] + { + let _result = executor.bench_process(xcm)?; + } assert_eq!(executor.total_surplus(), &Weight::from_parts(1337, 1337)); assert_eq!(executor.total_refunded(), &Weight::from_parts(1337, 1337)); + + Ok(()) } - set_error_handler { + #[benchmark] + fn set_error_handler() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); let instruction = Instruction::>::SetErrorHandler(Xcm(vec![])); let xcm = Xcm(vec![instruction]); - } : { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } assert_eq!(executor.error_handler(), &Xcm(vec![])); + + Ok(()) } - set_appendix { + #[benchmark] + fn set_appendix() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); let appendix = Xcm(vec![]); let instruction = Instruction::>::SetAppendix(appendix); let xcm = Xcm(vec![instruction]); - } : { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } assert_eq!(executor.appendix(), &Xcm(vec![])); + Ok(()) } - clear_error { + #[benchmark] + fn clear_error() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); executor.set_error(Some((5u32, XcmError::Overflow))); let instruction = Instruction::>::ClearError; let xcm = Xcm(vec![instruction]); - } : { - executor.bench_process(xcm)?; - } verify { - assert!(executor.error().is_none()) + #[block] + { + executor.bench_process(xcm)?; + } + assert!(executor.error().is_none()); + Ok(()) } - descend_origin { + #[benchmark] + fn descend_origin() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); let who = Junctions::from([OnlyChild, OnlyChild]); let instruction = Instruction::DescendOrigin(who.clone()); let xcm = Xcm(vec![instruction]); - } : { - executor.bench_process(xcm)?; - } verify { - assert_eq!( - executor.origin(), - &Some(Location { - parents: 0, - interior: who, - }), - ); + #[block] + { + executor.bench_process(xcm)?; + } + assert_eq!(executor.origin(), &Some(Location { parents: 0, interior: who }),); + + Ok(()) } - execute_with_origin { + #[benchmark] + fn execute_with_origin() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); let who: Junctions = Junctions::from([AccountId32 { id: [0u8; 32], network: None }]); - let instruction = Instruction::ExecuteWithOrigin { descendant_origin: Some(who.clone()), xcm: Xcm(vec![]) }; + let instruction = Instruction::ExecuteWithOrigin { + descendant_origin: Some(who.clone()), + xcm: Xcm(vec![]), + }; let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { - assert_eq!( - executor.origin(), - &Some(Location { - parents: 0, - interior: Here, - }), - ); + #[block] + { + executor.bench_process(xcm)?; + } + assert_eq!(executor.origin(), &Some(Location { parents: 0, interior: Here }),); + + Ok(()) } - clear_origin { + #[benchmark] + fn clear_origin() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); let instruction = Instruction::ClearOrigin; let xcm = Xcm(vec![instruction]); - } : { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } assert_eq!(executor.origin(), &None); + Ok(()) } - report_error { + #[benchmark] + fn report_error() -> Result<(), BenchmarkError> { let (sender_account, sender_location) = account_and_location::(1); let query_id = Default::default(); let max_weight = Default::default(); let destination = T::valid_destination().map_err(|_| BenchmarkError::Skip)?; - let (expected_fees_mode, expected_assets_in_holding) = T::DeliveryHelper::ensure_successful_delivery( - &sender_location, - &destination, - FeeReason::Report, - ); + let (expected_fees_mode, expected_assets_in_holding) = + T::DeliveryHelper::ensure_successful_delivery( + &sender_location, + &destination, + FeeReason::Report, + ); let sender_account_balance_before = T::TransactAsset::balance(&sender_account); let mut executor = new_executor::(sender_location); @@ -278,18 +338,21 @@ benchmarks! { } executor.set_error(Some((0u32, XcmError::Unimplemented))); - let instruction = Instruction::ReportError(QueryResponseInfo { - query_id, destination, max_weight - }); + let instruction = + Instruction::ReportError(QueryResponseInfo { query_id, destination, max_weight }); let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } // Check we charged the delivery fees assert!(T::TransactAsset::balance(&sender_account) <= sender_account_balance_before); + + Ok(()) } - claim_asset { + #[benchmark] + fn claim_asset() -> Result<(), BenchmarkError> { use xcm_executor::traits::DropAssets; let (origin, ticket, assets) = T::claimable_asset()?; @@ -298,11 +361,7 @@ benchmarks! { ::AssetTrap::drop_assets( &origin, assets.clone().into(), - &XcmContext { - origin: Some(origin.clone()), - message_id: [0; 32], - topic: None, - }, + &XcmContext { origin: Some(origin.clone()), message_id: [0; 32], topic: None }, ); // Assets should be in the trap now. @@ -310,28 +369,32 @@ benchmarks! { let mut executor = new_executor::(origin); let instruction = Instruction::ClaimAsset { assets: assets.clone(), ticket }; let xcm = Xcm(vec![instruction]); - } :{ - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } assert!(executor.holding().ensure_contains(&assets).is_ok()); + Ok(()) } - trap { + #[benchmark] + fn trap() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); let instruction = Instruction::Trap(10); let xcm = Xcm(vec![instruction]); // In order to access result in the verification below, it needs to be defined here. - let mut _result = Ok(()); - } : { - _result = executor.bench_process(xcm); - } verify { - assert!(matches!(_result, Err(ExecutorError { - xcm_error: XcmError::Trap(10), - .. - }))); + let result; + #[block] + { + result = executor.bench_process(xcm); + } + assert!(matches!(result, Err(ExecutorError { xcm_error: XcmError::Trap(10), .. }))); + + Ok(()) } - subscribe_version { + #[benchmark] + fn subscribe_version() -> Result<(), BenchmarkError> { use xcm_executor::traits::VersionChangeNotifier; let origin = T::subscribe_origin()?; let query_id = Default::default(); @@ -339,13 +402,18 @@ benchmarks! { let mut executor = new_executor::(origin.clone()); let instruction = Instruction::SubscribeVersion { query_id, max_response_weight }; let xcm = Xcm(vec![instruction]); - } : { - executor.bench_process(xcm)?; - } verify { - assert!(::SubscriptionService::is_subscribed(&origin)); + #[block] + { + executor.bench_process(xcm)?; + } + assert!(::SubscriptionService::is_subscribed( + &origin + )); + Ok(()) } - unsubscribe_version { + #[benchmark] + fn unsubscribe_version() -> Result<(), BenchmarkError> { use xcm_executor::traits::VersionChangeNotifier; // First we need to subscribe to notifications. let (origin, _) = T::transact_origin_and_runtime_call()?; @@ -355,24 +423,28 @@ benchmarks! { &origin, query_id, max_response_weight, - &XcmContext { - origin: Some(origin.clone()), - message_id: [0; 32], - topic: None, - }, - ).map_err(|_| "Could not start subscription")?; - assert!(::SubscriptionService::is_subscribed(&origin)); + &XcmContext { origin: Some(origin.clone()), message_id: [0; 32], topic: None }, + ) + .map_err(|_| "Could not start subscription")?; + assert!(::SubscriptionService::is_subscribed( + &origin + )); let mut executor = new_executor::(origin.clone()); let instruction = Instruction::UnsubscribeVersion; let xcm = Xcm(vec![instruction]); - } : { - executor.bench_process(xcm)?; - } verify { - assert!(!::SubscriptionService::is_subscribed(&origin)); + #[block] + { + executor.bench_process(xcm)?; + } + assert!(!::SubscriptionService::is_subscribed( + &origin + )); + Ok(()) } - burn_asset { + #[benchmark] + fn burn_asset() -> Result<(), BenchmarkError> { let holding = T::worst_case_holding(0); let assets = holding.clone(); @@ -381,13 +453,16 @@ benchmarks! { let instruction = Instruction::BurnAsset(assets.into()); let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } assert!(executor.holding().is_empty()); + Ok(()) } - expect_asset { + #[benchmark] + fn expect_asset() -> Result<(), BenchmarkError> { let holding = T::worst_case_holding(0); let assets = holding.clone(); @@ -396,71 +471,86 @@ benchmarks! { let instruction = Instruction::ExpectAsset(assets.into()); let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } // `execute` completing successfully is as good as we can check. + + Ok(()) } - expect_origin { + #[benchmark] + fn expect_origin() -> Result<(), BenchmarkError> { let expected_origin = Parent.into(); let mut executor = new_executor::(Default::default()); let instruction = Instruction::ExpectOrigin(Some(expected_origin)); let xcm = Xcm(vec![instruction]); let mut _result = Ok(()); - }: { - _result = executor.bench_process(xcm); - } verify { - assert!(matches!(_result, Err(ExecutorError { - xcm_error: XcmError::ExpectationFalse, - .. - }))); + #[block] + { + _result = executor.bench_process(xcm); + } + assert!(matches!( + _result, + Err(ExecutorError { xcm_error: XcmError::ExpectationFalse, .. }) + )); + + Ok(()) } - expect_error { + #[benchmark] + fn expect_error() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); executor.set_error(Some((3u32, XcmError::Overflow))); let instruction = Instruction::ExpectError(None); let xcm = Xcm(vec![instruction]); let mut _result = Ok(()); - }: { - _result = executor.bench_process(xcm); - } verify { - assert!(matches!(_result, Err(ExecutorError { - xcm_error: XcmError::ExpectationFalse, - .. - }))); + #[block] + { + _result = executor.bench_process(xcm); + } + assert!(matches!( + _result, + Err(ExecutorError { xcm_error: XcmError::ExpectationFalse, .. }) + )); + + Ok(()) } - expect_transact_status { + #[benchmark] + fn expect_transact_status() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); - let worst_error = || -> MaybeErrorCode { - vec![0; MaxDispatchErrorLen::get() as usize].into() - }; + let worst_error = + || -> MaybeErrorCode { vec![0; MaxDispatchErrorLen::get() as usize].into() }; executor.set_transact_status(worst_error()); let instruction = Instruction::ExpectTransactStatus(worst_error()); let xcm = Xcm(vec![instruction]); let mut _result = Ok(()); - }: { - _result = executor.bench_process(xcm); - } verify { + #[block] + { + _result = executor.bench_process(xcm); + } assert!(matches!(_result, Ok(..))); + Ok(()) } - query_pallet { + #[benchmark] + fn query_pallet() -> Result<(), BenchmarkError> { let (sender_account, sender_location) = account_and_location::(1); let query_id = Default::default(); let destination = T::valid_destination().map_err(|_| BenchmarkError::Skip)?; let max_weight = Default::default(); - let (expected_fees_mode, expected_assets_in_holding) = T::DeliveryHelper::ensure_successful_delivery( - &sender_location, - &destination, - FeeReason::QueryPallet, - ); + let (expected_fees_mode, expected_assets_in_holding) = + T::DeliveryHelper::ensure_successful_delivery( + &sender_location, + &destination, + FeeReason::QueryPallet, + ); let sender_account_balance_before = T::TransactAsset::balance(&sender_account); let mut executor = new_executor::(sender_location); if let Some(expected_fees_mode) = expected_fees_mode { @@ -476,15 +566,19 @@ benchmarks! { response_info: QueryResponseInfo { destination, query_id, max_weight }, }; let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } // Check we charged the delivery fees assert!(T::TransactAsset::balance(&sender_account) <= sender_account_balance_before); // TODO: Potentially add new trait to XcmSender to detect a queued outgoing message. #4426 + + Ok(()) } - expect_pallet { + #[benchmark] + fn expect_pallet() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); let valid_pallet = T::valid_pallet(); let instruction = Instruction::ExpectPallet { @@ -495,23 +589,27 @@ benchmarks! { min_crate_minor: valid_pallet.crate_version.minor.into(), }; let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } // the execution succeeding is all we need to verify this xcm was successful + Ok(()) } - report_transact_status { + #[benchmark] + fn report_transact_status() -> Result<(), BenchmarkError> { let (sender_account, sender_location) = account_and_location::(1); let query_id = Default::default(); let destination = T::valid_destination().map_err(|_| BenchmarkError::Skip)?; let max_weight = Default::default(); - let (expected_fees_mode, expected_assets_in_holding) = T::DeliveryHelper::ensure_successful_delivery( - &sender_location, - &destination, - FeeReason::Report, - ); + let (expected_fees_mode, expected_assets_in_holding) = + T::DeliveryHelper::ensure_successful_delivery( + &sender_location, + &destination, + FeeReason::Report, + ); let sender_account_balance_before = T::TransactAsset::balance(&sender_account); let mut executor = new_executor::(sender_location); @@ -529,84 +627,102 @@ benchmarks! { max_weight, }); let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } // Check we charged the delivery fees assert!(T::TransactAsset::balance(&sender_account) <= sender_account_balance_before); // TODO: Potentially add new trait to XcmSender to detect a queued outgoing message. #4426 + Ok(()) } - clear_transact_status { + #[benchmark] + fn clear_transact_status() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); executor.set_transact_status(b"MyError".to_vec().into()); let instruction = Instruction::ClearTransactStatus; let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } assert_eq!(executor.transact_status(), &MaybeErrorCode::Success); + Ok(()) } - set_topic { + #[benchmark] + fn set_topic() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); let instruction = Instruction::SetTopic([1; 32]); let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } assert_eq!(executor.topic(), &Some([1; 32])); + Ok(()) } - clear_topic { + #[benchmark] + fn clear_topic() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); executor.set_topic(Some([2; 32])); let instruction = Instruction::ClearTopic; let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } assert_eq!(executor.topic(), &None); + Ok(()) } - exchange_asset { + #[benchmark] + fn exchange_asset() -> Result<(), BenchmarkError> { let (give, want) = T::worst_case_asset_exchange().map_err(|_| BenchmarkError::Skip)?; let assets = give.clone(); let mut executor = new_executor::(Default::default()); executor.set_holding(give.into()); - let instruction = Instruction::ExchangeAsset { - give: assets.into(), - want: want.clone(), - maximal: true, - }; + let instruction = + Instruction::ExchangeAsset { give: assets.into(), want: want.clone(), maximal: true }; let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } assert_eq!(executor.holding(), &want.into()); + Ok(()) } - universal_origin { + #[benchmark] + fn universal_origin() -> Result<(), BenchmarkError> { let (origin, alias) = T::universal_alias().map_err(|_| BenchmarkError::Skip)?; let mut executor = new_executor::(origin); let instruction = Instruction::UniversalOrigin(alias); let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } use frame_support::traits::Get; let universal_location = ::UniversalLocation::get(); - assert_eq!(executor.origin(), &Some(Junctions::from([alias]).relative_to(&universal_location))); + assert_eq!( + executor.origin(), + &Some(Junctions::from([alias]).relative_to(&universal_location)) + ); + + Ok(()) } - export_message { - let x in 1 .. 1000; + #[benchmark] + fn export_message(x: Linear<1, 1000>) -> Result<(), BenchmarkError> { // The `inner_xcm` influences `ExportMessage` total weight based on // `inner_xcm.encoded_size()`, so for this benchmark use smallest encoded instruction // to approximate weight per "unit" of encoded size; then actual weight can be estimated @@ -616,11 +732,12 @@ benchmarks! { // Get `origin`, `network` and `destination` from configured runtime. let (origin, network, destination) = T::export_message_origin_and_destination()?; - let (expected_fees_mode, expected_assets_in_holding) = T::DeliveryHelper::ensure_successful_delivery( - &origin, - &destination.clone().into(), - FeeReason::Export { network, destination: destination.clone() }, - ); + let (expected_fees_mode, expected_assets_in_holding) = + T::DeliveryHelper::ensure_successful_delivery( + &origin, + &destination.clone().into(), + FeeReason::Export { network, destination: destination.clone() }, + ); let sender_account = T::AccountIdConverter::convert_location(&origin).unwrap(); let sender_account_balance_before = T::TransactAsset::balance(&sender_account); @@ -631,37 +748,39 @@ benchmarks! { if let Some(expected_assets_in_holding) = expected_assets_in_holding { executor.set_holding(expected_assets_in_holding.into()); } - let xcm = Xcm(vec![ExportMessage { - network, destination: destination.clone(), xcm: inner_xcm, - }]); - }: { - executor.bench_process(xcm)?; - } verify { + let xcm = + Xcm(vec![ExportMessage { network, destination: destination.clone(), xcm: inner_xcm }]); + #[block] + { + executor.bench_process(xcm)?; + } // Check we charged the delivery fees assert!(T::TransactAsset::balance(&sender_account) <= sender_account_balance_before); // TODO: Potentially add new trait to XcmSender to detect a queued outgoing message. #4426 + Ok(()) } - set_fees_mode { + #[benchmark] + fn set_fees_mode() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); executor.set_fees_mode(FeesMode { jit_withdraw: false }); let instruction = Instruction::SetFeesMode { jit_withdraw: true }; let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } assert_eq!(executor.fees_mode(), &FeesMode { jit_withdraw: true }); + Ok(()) } - lock_asset { + #[benchmark] + fn lock_asset() -> Result<(), BenchmarkError> { let (unlocker, owner, asset) = T::unlockable_asset()?; - let (expected_fees_mode, expected_assets_in_holding) = T::DeliveryHelper::ensure_successful_delivery( - &owner, - &unlocker, - FeeReason::LockAsset, - ); + let (expected_fees_mode, expected_assets_in_holding) = + T::DeliveryHelper::ensure_successful_delivery(&owner, &unlocker, FeeReason::LockAsset); let sender_account = T::AccountIdConverter::convert_location(&owner).unwrap(); let sender_account_balance_before = T::TransactAsset::balance(&sender_account); @@ -681,15 +800,18 @@ benchmarks! { let instruction = Instruction::LockAsset { asset, unlocker }; let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } // Check delivery fees assert!(T::TransactAsset::balance(&sender_account) <= sender_account_balance_before); // TODO: Potentially add new trait to XcmSender to detect a queued outgoing message. #4426 + Ok(()) } - unlock_asset { + #[benchmark] + fn unlock_asset() -> Result<(), BenchmarkError> { use xcm_executor::traits::{AssetLock, Enact}; let (unlocker, owner, asset) = T::unlockable_asset()?; @@ -709,13 +831,15 @@ benchmarks! { // ... then unlock them with the UnlockAsset instruction. let instruction = Instruction::UnlockAsset { asset, target: owner }; let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { - + #[block] + { + executor.bench_process(xcm)?; + } + Ok(()) } - note_unlockable { + #[benchmark] + fn note_unlockable() -> Result<(), BenchmarkError> { use xcm_executor::traits::{AssetLock, Enact}; let (unlocker, owner, asset) = T::unlockable_asset()?; @@ -735,13 +859,15 @@ benchmarks! { // ... then note them as unlockable with the NoteUnlockable instruction. let instruction = Instruction::NoteUnlockable { asset, owner }; let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { - + #[block] + { + executor.bench_process(xcm)?; + } + Ok(()) } - request_unlock { + #[benchmark] + fn request_unlock() -> Result<(), BenchmarkError> { use xcm_executor::traits::{AssetLock, Enact}; let (locker, owner, asset) = T::unlockable_asset()?; @@ -756,11 +882,12 @@ benchmarks! { .enact() .map_err(|_| BenchmarkError::Skip)?; - let (expected_fees_mode, expected_assets_in_holding) = T::DeliveryHelper::ensure_successful_delivery( - &owner, - &locker, - FeeReason::RequestUnlock, - ); + let (expected_fees_mode, expected_assets_in_holding) = + T::DeliveryHelper::ensure_successful_delivery( + &owner, + &locker, + FeeReason::RequestUnlock, + ); let sender_account = T::AccountIdConverter::convert_location(&owner).unwrap(); let sender_account_balance_before = T::TransactAsset::balance(&sender_account); @@ -774,15 +901,18 @@ benchmarks! { } let instruction = Instruction::RequestUnlock { asset, locker }; let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } // Check we charged the delivery fees assert!(T::TransactAsset::balance(&sender_account) <= sender_account_balance_before); // TODO: Potentially add new trait to XcmSender to detect a queued outgoing message. #4426 + Ok(()) } - unpaid_execution { + #[benchmark] + fn unpaid_execution() -> Result<(), BenchmarkError> { let mut executor = new_executor::(Default::default()); executor.set_origin(Some(Here.into())); @@ -792,21 +922,27 @@ benchmarks! { }; let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; + #[block] + { + executor.bench_process(xcm)?; + } + Ok(()) } - alias_origin { + #[benchmark] + fn alias_origin() -> Result<(), BenchmarkError> { let (origin, target) = T::alias_origin().map_err(|_| BenchmarkError::Skip)?; let mut executor = new_executor::(origin); let instruction = Instruction::AliasOrigin(target.clone()); let xcm = Xcm(vec![instruction]); - }: { - executor.bench_process(xcm)?; - } verify { + #[block] + { + executor.bench_process(xcm)?; + } assert_eq!(executor.origin(), &Some(target)); + Ok(()) } impl_benchmark_test_suite!( From fdb264d0df6fdbed32f001ba43c3282a01dd3d65 Mon Sep 17 00:00:00 2001 From: Ludovic_Domingues Date: Thu, 28 Nov 2024 17:18:30 +0100 Subject: [PATCH 09/10] Migrating pallet-state-trie-migration to benchmarking V2 (#6617) # Description Migrated pallet-state-trie-migration benchmarking to the new benchmarking syntax v2. This is part of #6202 Co-authored-by: Shawn Tabrizi Co-authored-by: Giuseppe Re --- .../frame/state-trie-migration/src/lib.rs | 176 +++++++++++------- 1 file changed, 108 insertions(+), 68 deletions(-) diff --git a/substrate/frame/state-trie-migration/src/lib.rs b/substrate/frame/state-trie-migration/src/lib.rs index 3fe5abb81031..61323b70b33d 100644 --- a/substrate/frame/state-trie-migration/src/lib.rs +++ b/substrate/frame/state-trie-migration/src/lib.rs @@ -249,13 +249,13 @@ pub mod pallet { if limits.item.is_zero() || limits.size.is_zero() { // handle this minor edge case, else we would call `migrate_tick` at least once. log!(warn, "limits are zero. stopping"); - return Ok(()) + return Ok(()); } while !self.exhausted(limits) && !self.finished() { if let Err(e) = self.migrate_tick() { log!(error, "migrate_until_exhaustion failed: {:?}", e); - return Err(e) + return Err(e); } } @@ -332,7 +332,7 @@ pub mod pallet { _ => { // defensive: there must be an ongoing top migration. frame_support::defensive!("cannot migrate child key."); - return Ok(()) + return Ok(()); }, }; @@ -374,7 +374,7 @@ pub mod pallet { Progress::Complete => { // defensive: there must be an ongoing top migration. frame_support::defensive!("cannot migrate top key."); - return Ok(()) + return Ok(()); }, }; @@ -669,7 +669,7 @@ pub mod pallet { // ensure that the migration witness data was correct. if real_size_upper < task.dyn_size { Self::slash(who, deposit)?; - return Ok(().into()) + return Ok(().into()); } Self::deposit_event(Event::::Migrated { @@ -957,6 +957,7 @@ pub mod pallet { mod benchmarks { use super::{pallet::Pallet as StateTrieMigration, *}; use alloc::vec; + use frame_benchmarking::v2::*; use frame_support::traits::fungible::{Inspect, Mutate}; // The size of the key seemingly makes no difference in the read/write time, so we make it @@ -970,8 +971,12 @@ mod benchmarks { stash } - frame_benchmarking::benchmarks! { - continue_migrate { + #[benchmarks] + mod inner_benchmarks { + use super::*; + + #[benchmark] + fn continue_migrate() -> Result<(), BenchmarkError> { // note that this benchmark should migrate nothing, as we only want the overhead weight // of the bookkeeping, and the migration cost itself is noted via the `dynamic_weight` // function. @@ -980,116 +985,151 @@ mod benchmarks { let stash = set_balance_for_deposit::(&caller, null.item); // Allow signed migrations. SignedMigrationMaxLimits::::put(MigrationLimits { size: 1024, item: 5 }); - }: _(frame_system::RawOrigin::Signed(caller.clone()), null, 0, StateTrieMigration::::migration_process()) - verify { + + #[extrinsic_call] + _( + frame_system::RawOrigin::Signed(caller.clone()), + null, + 0, + StateTrieMigration::::migration_process(), + ); + assert_eq!(StateTrieMigration::::migration_process(), Default::default()); - assert_eq!(T::Currency::balance(&caller), stash) + assert_eq!(T::Currency::balance(&caller), stash); + + Ok(()) } - continue_migrate_wrong_witness { + #[benchmark] + fn continue_migrate_wrong_witness() -> Result<(), BenchmarkError> { let null = MigrationLimits::default(); let caller = frame_benchmarking::whitelisted_caller(); - let bad_witness = MigrationTask { progress_top: Progress::LastKey(vec![1u8].try_into().unwrap()), ..Default::default() }; - }: { - assert!( - StateTrieMigration::::continue_migrate( + let bad_witness = MigrationTask { + progress_top: Progress::LastKey(vec![1u8].try_into().unwrap()), + ..Default::default() + }; + #[block] + { + assert!(StateTrieMigration::::continue_migrate( frame_system::RawOrigin::Signed(caller).into(), null, 0, bad_witness, ) - .is_err() - ) - } - verify { - assert_eq!(StateTrieMigration::::migration_process(), Default::default()) + .is_err()); + } + + assert_eq!(StateTrieMigration::::migration_process(), Default::default()); + + Ok(()) } - migrate_custom_top_success { + #[benchmark] + fn migrate_custom_top_success() -> Result<(), BenchmarkError> { let null = MigrationLimits::default(); let caller: T::AccountId = frame_benchmarking::whitelisted_caller(); let stash = set_balance_for_deposit::(&caller, null.item); - }: migrate_custom_top(frame_system::RawOrigin::Signed(caller.clone()), Default::default(), 0) - verify { + #[extrinsic_call] + migrate_custom_top( + frame_system::RawOrigin::Signed(caller.clone()), + Default::default(), + 0, + ); + assert_eq!(StateTrieMigration::::migration_process(), Default::default()); - assert_eq!(T::Currency::balance(&caller), stash) + assert_eq!(T::Currency::balance(&caller), stash); + Ok(()) } - migrate_custom_top_fail { + #[benchmark] + fn migrate_custom_top_fail() -> Result<(), BenchmarkError> { let null = MigrationLimits::default(); let caller: T::AccountId = frame_benchmarking::whitelisted_caller(); let stash = set_balance_for_deposit::(&caller, null.item); // for tests, we need to make sure there is _something_ in storage that is being // migrated. - sp_io::storage::set(b"foo", vec![1u8;33].as_ref()); - }: { - assert!( - StateTrieMigration::::migrate_custom_top( + sp_io::storage::set(b"foo", vec![1u8; 33].as_ref()); + #[block] + { + assert!(StateTrieMigration::::migrate_custom_top( frame_system::RawOrigin::Signed(caller.clone()).into(), vec![b"foo".to_vec()], 1, - ).is_ok() - ); + ) + .is_ok()); + + frame_system::Pallet::::assert_last_event( + ::RuntimeEvent::from(crate::Event::Slashed { + who: caller.clone(), + amount: StateTrieMigration::::calculate_deposit_for(1u32), + }) + .into(), + ); + } - frame_system::Pallet::::assert_last_event( - ::RuntimeEvent::from(crate::Event::Slashed { - who: caller.clone(), - amount: StateTrieMigration::::calculate_deposit_for(1u32), - }).into(), - ); - } - verify { assert_eq!(StateTrieMigration::::migration_process(), Default::default()); // must have gotten slashed - assert!(T::Currency::balance(&caller) < stash) + assert!(T::Currency::balance(&caller) < stash); + + Ok(()) } - migrate_custom_child_success { + #[benchmark] + fn migrate_custom_child_success() -> Result<(), BenchmarkError> { let caller: T::AccountId = frame_benchmarking::whitelisted_caller(); let stash = set_balance_for_deposit::(&caller, 0); - }: migrate_custom_child( - frame_system::RawOrigin::Signed(caller.clone()), - StateTrieMigration::::childify(Default::default()), - Default::default(), - 0 - ) - verify { + + #[extrinsic_call] + migrate_custom_child( + frame_system::RawOrigin::Signed(caller.clone()), + StateTrieMigration::::childify(Default::default()), + Default::default(), + 0, + ); + assert_eq!(StateTrieMigration::::migration_process(), Default::default()); assert_eq!(T::Currency::balance(&caller), stash); + + Ok(()) } - migrate_custom_child_fail { + #[benchmark] + fn migrate_custom_child_fail() -> Result<(), BenchmarkError> { let caller: T::AccountId = frame_benchmarking::whitelisted_caller(); let stash = set_balance_for_deposit::(&caller, 1); // for tests, we need to make sure there is _something_ in storage that is being // migrated. - sp_io::default_child_storage::set(b"top", b"foo", vec![1u8;33].as_ref()); - }: { - assert!( - StateTrieMigration::::migrate_custom_child( + sp_io::default_child_storage::set(b"top", b"foo", vec![1u8; 33].as_ref()); + + #[block] + { + assert!(StateTrieMigration::::migrate_custom_child( frame_system::RawOrigin::Signed(caller.clone()).into(), StateTrieMigration::::childify("top"), vec![b"foo".to_vec()], 1, - ).is_ok() - ) - } - verify { + ) + .is_ok()); + } assert_eq!(StateTrieMigration::::migration_process(), Default::default()); // must have gotten slashed - assert!(T::Currency::balance(&caller) < stash) + assert!(T::Currency::balance(&caller) < stash); + Ok(()) } - process_top_key { - let v in 1 .. (4 * 1024 * 1024); - + #[benchmark] + fn process_top_key(v: Linear<1, { 4 * 1024 * 1024 }>) -> Result<(), BenchmarkError> { let value = alloc::vec![1u8; v as usize]; sp_io::storage::set(KEY, &value); - }: { - let data = sp_io::storage::get(KEY).unwrap(); - sp_io::storage::set(KEY, &data); - let _next = sp_io::storage::next_key(KEY); - assert_eq!(data, value); + #[block] + { + let data = sp_io::storage::get(KEY).unwrap(); + sp_io::storage::set(KEY, &data); + let _next = sp_io::storage::next_key(KEY); + assert_eq!(data, value); + } + + Ok(()) } impl_benchmark_test_suite!( @@ -1741,7 +1781,7 @@ pub(crate) mod remote_tests { let ((finished, weight), proof) = ext.execute_and_prove(|| { let weight = run_to_block::(now + One::one()).1; if StateTrieMigration::::migration_process().finished() { - return (true, weight) + return (true, weight); } duration += One::one(); now += One::one(); @@ -1768,7 +1808,7 @@ pub(crate) mod remote_tests { ext.commit_all().unwrap(); if finished { - break + break; } } From 6416b280a7d0032ba3c265e4506504c6d6536637 Mon Sep 17 00:00:00 2001 From: Cyrill Leutwiler Date: Thu, 28 Nov 2024 19:01:41 +0100 Subject: [PATCH 10/10] [pallet-revive] bugfix decoding 64bit args in the decoder (#6695) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The argument index of the next argument is dictated by the size of the current one. --------- Signed-off-by: xermicus Co-authored-by: GitHub Action Co-authored-by: Alexander Theißen --- prdoc/pr_6695.prdoc | 8 ++++++++ substrate/frame/revive/proc-macro/src/lib.rs | 4 +++- 2 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 prdoc/pr_6695.prdoc diff --git a/prdoc/pr_6695.prdoc b/prdoc/pr_6695.prdoc new file mode 100644 index 000000000000..7a950e8546cd --- /dev/null +++ b/prdoc/pr_6695.prdoc @@ -0,0 +1,8 @@ +title: '[pallet-revive] bugfix decoding 64bit args in the decoder' +doc: +- audience: Runtime Dev + description: The argument index of the next argument is dictated by the size of + the current one. +crates: +- name: pallet-revive-proc-macro + bump: patch diff --git a/substrate/frame/revive/proc-macro/src/lib.rs b/substrate/frame/revive/proc-macro/src/lib.rs index 012b4bfab9a9..7232c6342824 100644 --- a/substrate/frame/revive/proc-macro/src/lib.rs +++ b/substrate/frame/revive/proc-macro/src/lib.rs @@ -342,7 +342,8 @@ where const ALLOWED_REGISTERS: u32 = 6; let mut registers_used = 0; let mut bindings = vec![]; - for (idx, (name, ty)) in param_names.clone().zip(param_types.clone()).enumerate() { + let mut idx = 0; + for (name, ty) in param_names.clone().zip(param_types.clone()) { let syn::Type::Path(path) = &**ty else { panic!("Type needs to be path"); }; @@ -380,6 +381,7 @@ where } }; bindings.push(binding); + idx += size; } quote! { #( #bindings )*