From 56f0cbc7825fdf881a953f7a61c9d8f90a8bc961 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sun, 8 Oct 2023 15:05:07 +0200 Subject: [PATCH 01/19] Warn on unchecked weight witness Signed-off-by: Oliver Tale-Yazdi --- Cargo.lock | 8 +-- substrate/frame/support/procedural/Cargo.toml | 2 +- .../procedural/src/construct_runtime/mod.rs | 2 +- .../procedural/src/pallet/expand/call.rs | 52 +++++++++++++++++-- 4 files changed, 55 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 48ac004a8597..54a99579508b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13203,9 +13203,9 @@ checksum = "dc375e1527247fe1a97d8b7156678dfe7c1af2fc075c9a4db3690ecd2a148068" [[package]] name = "proc-macro-warning" -version = "0.4.2" +version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3d1eaa7fa0aa1929ffdf7eeb6eac234dde6268914a14ad44d23521ab6a9b258e" +checksum = "9b698b0b09d40e9b7c1a47b132d66a8b54bcd20583d9b6d06e4535e383b4405c" dependencies = [ "proc-macro2", "quote", @@ -13214,9 +13214,9 @@ dependencies = [ [[package]] name = "proc-macro2" -version = "1.0.67" +version = "1.0.68" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3d433d9f1a3e8c1263d9456598b16fec66f4acc9a74dacffd35c7bb09b3a1328" +checksum = "5b1106fec09662ec6dd98ccac0f81cef56984d0b49f75c92d8cbad76e20c005c" dependencies = [ "unicode-ident", ] diff --git a/substrate/frame/support/procedural/Cargo.toml b/substrate/frame/support/procedural/Cargo.toml index d2854a2a79f0..9781a8825142 100644 --- a/substrate/frame/support/procedural/Cargo.toml +++ b/substrate/frame/support/procedural/Cargo.toml @@ -23,7 +23,7 @@ proc-macro2 = "1.0.56" quote = "1.0.28" syn = { version = "2.0.38", features = ["full"] } frame-support-procedural-tools = { path = "tools" } -proc-macro-warning = { version = "0.4.2", default-features = false } +proc-macro-warning = { version = "1.0.0", default-features = false } macro_magic = { version = "0.4.2", features = ["proc_support"] } expander = "2.0.0" sp-core-hashing = { path = "../../../primitives/core/hashing" } diff --git a/substrate/frame/support/procedural/src/construct_runtime/mod.rs b/substrate/frame/support/procedural/src/construct_runtime/mod.rs index e8c3c0889214..c3d433643fd7 100644 --- a/substrate/frame/support/procedural/src/construct_runtime/mod.rs +++ b/substrate/frame/support/procedural/src/construct_runtime/mod.rs @@ -414,7 +414,7 @@ fn construct_runtime_final_expansion( ) .help_links(&["https://github.com/paritytech/substrate/pull/14437"]) .span(where_section.span) - .build(), + .build_or_panic(), ) }); diff --git a/substrate/frame/support/procedural/src/pallet/expand/call.rs b/substrate/frame/support/procedural/src/pallet/expand/call.rs index 3ed5509863e9..8f2924e2dcd7 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/call.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/call.rs @@ -77,7 +77,7 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream { "https://github.com/paritytech/substrate/pull/11381" ]) .span(method.name.span()) - .build(); + .build_or_panic(); call_index_warnings.push(warning); } @@ -93,11 +93,14 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream { .new("benchmark all calls or put the pallet into `dev` mode") .help_link("https://github.com/paritytech/substrate/pull/13798") .span(lit.span()) - .build(); + .build_or_panic(); weight_warnings.push(warning); fn_weight.push(e.into_token_stream()); }, - CallWeightDef::Immediate(e) => fn_weight.push(e.into_token_stream()), + CallWeightDef::Immediate(e) => { + fn_weight.push(e.clone().into_token_stream()); + check_weight_witness(method, &mut weight_warnings); + }, CallWeightDef::Inherited => { let pallet_weight = def .call @@ -425,3 +428,46 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream { } ) } + +/// Warn if any of the call arguments starts with a underscore and is used in a weight formula. +fn check_weight_witness(method: &CallVariantDef, warnings: &mut Vec) { + let CallWeightDef::Immediate(imm) = &method.weight else { + return; + }; + let builder = proc_macro_warning::Warning::new_deprecated("UncheckedWeightWitness") + .old("not checked weight witness data") + .new("ensure that all witness data for weight calculation is checked before usage") + .help_links(&["FAIL-CI TODO"]); + + for (_, arg_ident, _) in method.args.iter().skip(1) { + // Unused arguments cannot be used in weight formulas. + if !arg_ident.to_string().starts_with('_') || !contains(&imm, &arg_ident) { + continue + } + + let warning = builder.clone().index(warnings.len()).span(arg_ident.span()).build_or_panic(); + warnings.push(warning); + } +} + +fn contains(expr: &syn::Expr, ident: &syn::Ident) -> bool { + use syn::visit_mut::{self, VisitMut}; + + struct ContainsIdent { + ident: syn::Ident, + found: bool, + } + + impl VisitMut for ContainsIdent { + fn visit_ident_mut(&mut self, i: &mut syn::Ident) { + if *i == self.ident { + self.found = true; + } + } + } + + let mut expr = expr.clone(); + let mut visitor = ContainsIdent { ident: ident.clone(), found: false }; + visit_mut::visit_expr_mut(&mut visitor, &mut expr); + visitor.found +} From 03f8669b4723b8ff8e7780fc9db9ca267abb0c03 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sun, 8 Oct 2023 15:05:51 +0200 Subject: [PATCH 02/19] Remove stupid folder Signed-off-by: Oliver Tale-Yazdi --- substrate/src/src/lib.rs | 297 --------------------------------------- 1 file changed, 297 deletions(-) delete mode 100644 substrate/src/src/lib.rs diff --git a/substrate/src/src/lib.rs b/substrate/src/src/lib.rs deleted file mode 100644 index 16a606778965..000000000000 --- a/substrate/src/src/lib.rs +++ /dev/null @@ -1,297 +0,0 @@ -// This file is part of Substrate. - -// Copyright (C) Parity Technologies (UK) Ltd. -// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 - -// This program is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. - -// This program is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. - -// You should have received a copy of the GNU General Public License -// along with this program. If not, see . - -//! # Substrate -//! -//! Substrate is a Rust framework for building blockchains in a modular and extensible way. While in -//! itself un-opinionated, it is the main engine behind the Polkadot ecosystem. -//! -//! [![github]](https://github.com/paritytech/substrate/) - [![polkadot]](https://polkadot.network) -//! -//! This crate in itself does not contain any code and is just meant ot be a documentation hub for -//! substrate-based crates. -//! -//! ## Overview -//! -//! Substrate approaches blockchain development with an acknowledgement of a few self-evident -//! truths: -//! -//! 1. Society and technology evolves. -//! 2. Humans are fallible. -//! -//! This, specifically, makes the task of designing a correct, safe and long-lasting blockchain -//! system hard. -//! -//! Nonetheless, in order to achieve this goal, substrate embraces the following: -//! -//! 1. Use of **Rust** as a modern, and safe programming language, which limits human error through -//! various means, most notably memory safety. -//! 2. Substrate is written from the ground-up with a generic, modular and extensible design. This -//! ensures that software components can be easily swapped and upgraded. Examples of this is -//! multiple consensus mechanisms provided by Substrate, as listed below. -//! 3. Lastly, the final blockchain system created with the above properties needs to be -//! upgradeable. In order to achieve this, Substrate is designed as a meta-protocol, whereby the -//! application logic of the blockchain (called "Runtime") is encoded as a Wasm blob, and is -//! stored onchain. The rest of the system (called "Client") acts as the executor of the Wasm -//! blob. -//! -//! In essence, the meta-protocol of all Substrate based chains is the "Runtime as Wasm blob" -//! accord. This enables the Runtime to become inherently upgradeable (without forks). The upgrade -//! is merely a matter of the Wasm blob being changed in the chain state, which is, in principle, -//! same as updating an account's balance. -//! -//! To learn more about the substrate architecture using some visuals, see [`substrate_diagram`]. -//! -//! `FRAME`, Substrate's default runtime development library takes the above even further by -//! embracing a declarative programming model whereby correctness is enhanced and the system is -//! highly configurable through parameterization. -//! -//! All in all, this design enables all substrate-based chains to achieve forkless, self-enacting -//! upgrades out of the box. Combined with governance abilities that are shipped with `FRAME`, this -//! enables a chain to survive the test of time. -//! -//! ## How to Get Stared -//! -//! Most developers want to leave the client side code as-is, and focus on the runtime. To do so, -//! look into the [`frame_support`] crate, which is the entry point crate into runtime development -//! with FRAME. -//! -//! > Side note, it is entirely possible to craft a substrate-based runtime without FRAME, an -//! > example of which can be found [here](https://github.com/JoshOrndorff/frameless-node-template). -//! -//! In more broad terms, the following avenues exist into developing with substrate: -//! -//! * **Templates**: A number of substrate-based templates exist and they can be used for various -//! purposes, with zero to little additional code needed. All of these templates contain runtimes -//! that are highly configurable and are likely suitable for basic needs. -//! * `FRAME`: If need, one can customize that runtime even further, by using `FRAME` and developing -//! custom modules. -//! * **Core**: To the contrary, some developers may want to customize the client side software to -//! achieve novel goals such as a new consensus engine, or a new database backend. While -//! Substrate's main configurability is in the runtime, the client is also highly generic and can -//! be customized to a great extent. -//! -//! ## Structure -//! -//! Substrate is a massive cargo workspace with hundreds of crates, therefore it is useful to know -//! how to navigate its crates. -//! -//! In broad terms, it is divided into three categories: -//! -//! * `sc-*` (short for *substrate-client*) crates, located under `./client` folder. These are all -//! the client crates. Notable examples are crates such as [`sc-network`], various consensus -//! crates, [`sc-rpc-api`] and [`sc-client-db`], all of which are expected to reside in the client -//! side. -//! * `sp-*` (short for *substrate-primitives*) crates, located under `./primitives` folder. These -//! are the traits that glue the client and runtime together, but are not opinionated about what -//! framework is using for building the runtime. Notable examples are [`sp-api`] and [`sp-io`], -//! which form the communication bridge between the client and runtime, as explained in -//! [`substrate_diagram`]. -//! * `pallet-*` and `frame-*` crates, located under `./frame` folder. These are the crates related -//! to FRAME. See [`frame_support`] for more information. -//! -//! ### Wasm Build -//! -//! Many of the Substrate crates, such as entire `sp-*`, need to compile to both Wasm (when a Wasm -//! runtime is being generated) and native (for example, when testing). To achieve this, Substrate -//! follows the convention of the Rust community, and uses a `feature = "std"` to signify that a -//! crate is being built with the standard library, and is built for native. Otherwise, it is built -//! for `no_std`. -//! -//! This can be summarized in `#![cfg_attr(not(feature = "std"), no_std)]`, which you can often find -//! in any Substrate-based runtime. -//! -//! Substrate-based runtimes use [`substrate-wasm-builder`] in their `build.rs` to automatically -//! build their Wasm files as a part of normal build commandsOnce built, the wasm file is placed in -//! `./target/{debug|release}/wbuild/{runtime_name}.wasm`. -//! -//! ### Binaries -//! -//! Multiple binaries are shipped with substrate, the most important of which are located in the -//! `./bin` folder. -//! -//! * [`node`] is an extensive substrate node that contains the superset of all runtime and client -//! side features. The corresponding runtime, called [`kitchensink_runtime`] contains all of the -//! modules that are provided with `FRAME`. This node and runtime is only used for testing and -//! demonstration. -//! * [`chain-spec-builder`]: Utility to build more detailed chain-specs for the aforementioned -//! node. Other projects typically contain a `build-spec` subcommand that does the same. -//! * [`node-template`]: a template node that contains a minimal set of features and can act as a -//! starting point of a project. -//! * [`subkey`]: Substrate's key management utility. -//! -//! ### Anatomy of a Binary Crate -//! -//! From the above, [`node`] and [`node-template`] are essentially blueprints of a substrate-based -//! project, as the name of the latter is implying. Each substrate-based project typically contains -//! the following: -//! -//! * Under `./runtime`, a `./runtime/src/lib.rs` which is the top level runtime amalgamator file. -//! This file typically contains the [`frame_support::construct_runtime`] macro, which is the -//! final definition of a runtime. -//! -//! * Under `./node`, a `main.rs`, which is the point, and a `./service.rs`, which contains all the -//! client side components. Skimming this file yields an overview of the networking, database, -//! consensus and similar client side components. -//! -//! > The above two are conventions, not rules. -//! -//! ## Parachain? -//! -//! As noted above, Substrate is the main engine behind the Polkadot ecosystem. One of the ways -//! through which Polkadot can be utilized is by building "parachains", blockchains that are -//! connected to Polkadot's shared security. -//! -//! To build a parachain, one could use [`Cumulus`](https://github.com/paritytech/cumulus/), the -//! library on top of Substrate, empowering any substrate-based chain to be a Polkadot parachain. -//! -//! ## Where To Go Next? -//! -//! Additional noteworthy crates within substrate: -//! -//! - RPC APIs of a Substrate node: [`sc-rpc-api`]/[`sc-rpc`] -//! - CLI Options of a Substrate node: [`sc-cli`] -//! - All of the consensus related crates provided by Substrate: -//! - [`sc-consensus-aura`] -//! - [`sc-consensus-babe`] -//! - [`sc-consensus-grandpa`] -//! - [`sc-consensus-beefy`] -//! - [`sc-consensus-manual-seal`] -//! - [`sc-consensus-pow`] -//! -//! Additional noteworthy external resources: -//! -//! - [Substrate Developer Hub](https://substrate.dev) -//! - [Parity Tech's Documentation Hub](https://paritytech.github.io/) -//! - [Frontier: Substrate's Ethereum Compatibility Library](https://paritytech.github.io/frontier/) -//! - [Polkadot Wiki](https://wiki.polkadot.network/en/) -//! -//! Notable upstream crates: -//! -//! - [`parity-scale-codec`](https://github.com/paritytech/parity-scale-codec) -//! - [`parity-db`](https://github.com/paritytech/parity-db) -//! - [`trie`](https://github.com/paritytech/trie) -//! - [`parity-common`](https://github.com/paritytech/parity-common) -//! -//! Templates: -//! -//! - classic [`substrate-node-template`](https://github.com/substrate-developer-hub/substrate-node-template) -//! - classic [cumulus-parachain-template](https://github.com/substrate-developer-hub/substrate-parachain-template) -//! - [`extended-parachain-template`](https://github.com/paritytech/extended-parachain-template) -//! - [`frontier-parachain-template`](https://github.com/paritytech/frontier-parachain-template) -//! -//! [polkadot]: -//! https://img.shields.io/badge/polkadot-E6007A?style=for-the-badge&logo=polkadot&logoColor=white -//! [github]: -//! https://img.shields.io/badge/github-8da0cb?style=for-the-badge&labelColor=555555&logo=github -//! [`sp-io`]: ../sp_io/index.html -//! [`sp-api`]: ../sp_api/index.html -//! [`sp-api`]: ../sp_api/index.html -//! [`sc-client-db`]: ../sc_client_db/index.html -//! [`sc-network`]: ../sc_network/index.html -//! [`sc-rpc-api`]: ../sc_rpc_api/index.html -//! [`sc-rpc`]: ../sc_rpc/index.html -//! [`sc-cli`]: ../sc_cli/index.html -//! [`sc-consensus-aura`]: ../sc_consensus_aura/index.html -//! [`sc-consensus-babe`]: ../sc_consensus_babe/index.html -//! [`sc-consensus-grandpa`]: ../sc_consensus_grandpa/index.html -//! [`sc-consensus-beefy`]: ../sc_consensus_beefy/index.html -//! [`sc-consensus-manual-seal`]: ../sc_consensus_manual_seal/index.html -//! [`sc-consensus-pow`]: ../sc_consensus_pow/index.html -//! [`node`]: ../node_cli/index.html -//! [`node-template`]: ../node_template/index.html -//! [`kitchensink_runtime`]: ../kitchensink_runtime/index.html -//! [`subkey`]: ../subkey/index.html -//! [`chain-spec-builder`]: ../chain_spec_builder/index.html -//! [`substrate-wasm-builder`]: https://crates.io/crates/substrate-wasm-builder - -#![deny(rustdoc::broken_intra_doc_links)] -#![deny(rustdoc::private_intra_doc_links)] - -#[cfg_attr(doc, aquamarine::aquamarine)] -/// In this module, we explore substrate at a more depth. First, let's establish substrate being -/// divided into a client and runtime. -/// -/// ```mermaid -/// graph TB -/// subgraph Substrate -/// direction LR -/// subgraph Client -/// end -/// subgraph Runtime -/// end -/// end -/// ``` -/// -/// The client and the runtime of course need to communicate. This is done through two concepts: -/// -/// 1. Host functions: a way for the (Wasm) runtime to talk to the client. All host functions are -/// defined in [`sp-io`]. For example, [`sp-io::storage`] are the set of host functions that -/// allow the runtime to read and write data to the on-chain state. -/// 2. Runtime APIs: a way for the client to talk to the Wasm runtime. Runtime APIs are defined -/// using macros and utilities in [`sp-api`]. For example, [`sp-api::Core`] is the most basic -/// runtime API that any blockchain must implement in order to be able to (re) execute blocks. -/// -/// ```mermaid -/// graph TB -/// subgraph Substrate -/// direction LR -/// subgraph Client -/// end -/// subgraph Runtime -/// end -/// Client --runtime-api--> Runtime -/// Runtime --host-functions--> Client -/// end -/// ``` -/// -/// Finally, let's expand the diagram a bit further and look at the internals of each component: -/// -/// ```mermaid -/// graph TB -/// subgraph Substrate -/// direction LR -/// subgraph Client -/// Database -/// Networking -/// Consensus -/// end -/// subgraph Runtime -/// subgraph FRAME -/// direction LR -/// Governance -/// Currency -/// Staking -/// Identity -/// end -/// end -/// Client --runtime-api--> Runtime -/// Runtime --host-functions--> Client -/// end -/// ``` -/// -/// As noted the runtime contains all of the application specific logic of the blockchain. This is -/// usually written with `FRAME`. The client, on the other hand, contains reusable and generic -/// components that are not specific to one single blockchain, such as networking, database, and the -/// consensus engine. -/// -/// [`sp-io`]: ../../sp_io/index.html -/// [`sp-api`]: ../../sp_api/index.html -/// [`sp-io::storage`]: ../../sp_io/storage/index.html -/// [`sp-api::Core`]: ../../sp_api/trait.Core.html -pub mod substrate_diagram {} From ca3e754215629e76cda0c81e39c8e2a12d32b6c3 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sun, 8 Oct 2023 15:06:45 +0200 Subject: [PATCH 03/19] Use key limit in kill_prefix Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/system/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index 84b6dc031457..e201d50f6fb8 100644 --- a/substrate/frame/system/src/lib.rs +++ b/substrate/frame/system/src/lib.rs @@ -495,16 +495,16 @@ pub mod pallet { /// the prefix we are removing to accurately calculate the weight of this function. #[pallet::call_index(6)] #[pallet::weight(( - T::SystemWeightInfo::kill_prefix(_subkeys.saturating_add(1)), + T::SystemWeightInfo::kill_prefix(subkeys.saturating_add(1)), DispatchClass::Operational, ))] pub fn kill_prefix( origin: OriginFor, prefix: Key, - _subkeys: u32, + subkeys: u32, ) -> DispatchResultWithPostInfo { ensure_root(origin)?; - let _ = storage::unhashed::clear_prefix(&prefix, None, None); + let _ = storage::unhashed::clear_prefix(&prefix, Some(subkeys), None); Ok(().into()) } From f3562fb6e8c3ccaa4ae60d8537d16f514fd19f39 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sun, 8 Oct 2023 15:13:58 +0200 Subject: [PATCH 04/19] Put into own file Signed-off-by: Oliver Tale-Yazdi --- .../procedural/src/pallet/expand/call.rs | 64 ++----------- .../procedural/src/pallet/expand/mod.rs | 1 + .../procedural/src/pallet/expand/warnings.rs | 95 +++++++++++++++++++ 3 files changed, 103 insertions(+), 57 deletions(-) create mode 100644 substrate/frame/support/procedural/src/pallet/expand/warnings.rs diff --git a/substrate/frame/support/procedural/src/pallet/expand/call.rs b/substrate/frame/support/procedural/src/pallet/expand/call.rs index 8f2924e2dcd7..89d6f2649a7e 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/call.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/call.rs @@ -17,12 +17,14 @@ use crate::{ pallet::{ + expand::warnings::{weight_constant_warning, weight_witness_warning}, parse::call::{CallVariantDef, CallWeightDef}, Def, }, COUNTER, }; use proc_macro2::TokenStream as TokenStream2; +use proc_macro_warning::Warning; use quote::{quote, ToTokens}; use syn::spanned::Spanned; @@ -68,7 +70,7 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream { continue } - let warning = proc_macro_warning::Warning::new_deprecated("ImplicitCallIndex") + let warning = Warning::new_deprecated("ImplicitCallIndex") .index(call_index_warnings.len()) .old("use implicit call indices") .new("ensure that all calls have a `pallet::call_index` attribute or put the pallet into `dev` mode") @@ -86,20 +88,11 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream { for method in &methods { match &method.weight { CallWeightDef::DevModeDefault => fn_weight.push(syn::parse_quote!(0)), - CallWeightDef::Immediate(e @ syn::Expr::Lit(lit)) if !def.dev_mode => { - let warning = proc_macro_warning::Warning::new_deprecated("ConstantWeight") - .index(weight_warnings.len()) - .old("use hard-coded constant as call weight") - .new("benchmark all calls or put the pallet into `dev` mode") - .help_link("https://github.com/paritytech/substrate/pull/13798") - .span(lit.span()) - .build_or_panic(); - weight_warnings.push(warning); - fn_weight.push(e.into_token_stream()); - }, CallWeightDef::Immediate(e) => { - fn_weight.push(e.clone().into_token_stream()); - check_weight_witness(method, &mut weight_warnings); + weight_constant_warning(e, def.dev_mode, &mut weight_warnings); + weight_witness_warning(method, &mut weight_warnings); + + fn_weight.push(e.into_token_stream()); }, CallWeightDef::Inherited => { let pallet_weight = def @@ -428,46 +421,3 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream { } ) } - -/// Warn if any of the call arguments starts with a underscore and is used in a weight formula. -fn check_weight_witness(method: &CallVariantDef, warnings: &mut Vec) { - let CallWeightDef::Immediate(imm) = &method.weight else { - return; - }; - let builder = proc_macro_warning::Warning::new_deprecated("UncheckedWeightWitness") - .old("not checked weight witness data") - .new("ensure that all witness data for weight calculation is checked before usage") - .help_links(&["FAIL-CI TODO"]); - - for (_, arg_ident, _) in method.args.iter().skip(1) { - // Unused arguments cannot be used in weight formulas. - if !arg_ident.to_string().starts_with('_') || !contains(&imm, &arg_ident) { - continue - } - - let warning = builder.clone().index(warnings.len()).span(arg_ident.span()).build_or_panic(); - warnings.push(warning); - } -} - -fn contains(expr: &syn::Expr, ident: &syn::Ident) -> bool { - use syn::visit_mut::{self, VisitMut}; - - struct ContainsIdent { - ident: syn::Ident, - found: bool, - } - - impl VisitMut for ContainsIdent { - fn visit_ident_mut(&mut self, i: &mut syn::Ident) { - if *i == self.ident { - self.found = true; - } - } - } - - let mut expr = expr.clone(); - let mut visitor = ContainsIdent { ident: ident.clone(), found: false }; - visit_mut::visit_expr_mut(&mut visitor, &mut expr); - visitor.found -} diff --git a/substrate/frame/support/procedural/src/pallet/expand/mod.rs b/substrate/frame/support/procedural/src/pallet/expand/mod.rs index 2b998227c1d8..6f32e5697512 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/mod.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/mod.rs @@ -34,6 +34,7 @@ mod store_trait; mod tt_default_parts; mod type_value; mod validate_unsigned; +mod warnings; use crate::pallet::Def; use quote::ToTokens; diff --git a/substrate/frame/support/procedural/src/pallet/expand/warnings.rs b/substrate/frame/support/procedural/src/pallet/expand/warnings.rs new file mode 100644 index 000000000000..6e373ac75f14 --- /dev/null +++ b/substrate/frame/support/procedural/src/pallet/expand/warnings.rs @@ -0,0 +1,95 @@ +// 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. + +//! Generates warnings for undesirable pallet code. + +use crate::pallet::parse::call::{CallVariantDef, CallWeightDef}; +use proc_macro_warning::Warning; +use syn::spanned::Spanned; + +/// Warn if any of the call arguments starts with a underscore and is used in a weight formula. +pub(crate) fn weight_witness_warning(method: &CallVariantDef, warnings: &mut Vec) { + let CallWeightDef::Immediate(imm) = &method.weight else { + return; + }; + let partial_warning = Warning::new_deprecated("UncheckedWeightWitness") + .old("not checked weight witness data") + .new("ensure that all witness data for weight calculation is checked before usage") + .help_links(&["FAIL-CI TODO"]); + + for (_, arg_ident, _) in method.args.iter().skip(1) { + // Unused arguments cannot be used in weight formulas. + if !arg_ident.to_string().starts_with('_') || !contains_ident(&imm, &arg_ident) { + continue + } + + let warning = partial_warning + .clone() + .index(warnings.len()) + .span(arg_ident.span()) + .build_or_panic(); + + warnings.push(warning); + } +} + +/// Warn if the weight is a constant and the pallet not in `dev_mode`. +pub(crate) fn weight_constant_warning( + weight: &syn::Expr, + dev_mode: bool, + warnings: &mut Vec, +) { + let syn::Expr::Lit(lit) = weight else { + return; + }; + if dev_mode { + return + } + + let warning = Warning::new_deprecated("ConstantWeight") + .index(warnings.len()) + .old("use hard-coded constant as call weight") + .new("benchmark all calls or put the pallet into `dev` mode") + .help_link("https://github.com/paritytech/substrate/pull/13798") + .span(lit.span()) + .build_or_panic(); + + warnings.push(warning); +} + +/// Returns whether `expr` contains `ident`. +fn contains_ident(expr: &syn::Expr, ident: &syn::Ident) -> bool { + use syn::visit_mut::{self, VisitMut}; + + struct ContainsIdent { + ident: syn::Ident, + found: bool, + } + + impl VisitMut for ContainsIdent { + fn visit_ident_mut(&mut self, i: &mut syn::Ident) { + if *i == self.ident { + self.found = true; + } + } + } + + let mut expr = expr.clone(); + let mut visitor = ContainsIdent { ident: ident.clone(), found: false }; + visit_mut::visit_expr_mut(&mut visitor, &mut expr); + visitor.found +} From 52ef3426b94ea079e984f461b85b14b187382477 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sun, 8 Oct 2023 15:22:44 +0200 Subject: [PATCH 05/19] Use new help link Signed-off-by: Oliver Tale-Yazdi --- .../frame/support/procedural/src/pallet/expand/warnings.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/support/procedural/src/pallet/expand/warnings.rs b/substrate/frame/support/procedural/src/pallet/expand/warnings.rs index 6e373ac75f14..a0e1de87f16f 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/warnings.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/warnings.rs @@ -29,7 +29,7 @@ pub(crate) fn weight_witness_warning(method: &CallVariantDef, warnings: &mut Vec let partial_warning = Warning::new_deprecated("UncheckedWeightWitness") .old("not checked weight witness data") .new("ensure that all witness data for weight calculation is checked before usage") - .help_links(&["FAIL-CI TODO"]); + .help_link("https://github.com/paritytech/polkadot-sdk/pull/1818"); for (_, arg_ident, _) in method.args.iter().skip(1) { // Unused arguments cannot be used in weight formulas. From 7ae121c7fd601be735f22f34bfcc9d99112569c2 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sun, 8 Oct 2023 15:25:07 +0200 Subject: [PATCH 06/19] Typo Signed-off-by: Oliver Tale-Yazdi --- .../frame/support/procedural/src/pallet/expand/warnings.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/support/procedural/src/pallet/expand/warnings.rs b/substrate/frame/support/procedural/src/pallet/expand/warnings.rs index a0e1de87f16f..888c0bf6dcc1 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/warnings.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/warnings.rs @@ -27,7 +27,7 @@ pub(crate) fn weight_witness_warning(method: &CallVariantDef, warnings: &mut Vec return; }; let partial_warning = Warning::new_deprecated("UncheckedWeightWitness") - .old("not checked weight witness data") + .old("not check weight witness data") .new("ensure that all witness data for weight calculation is checked before usage") .help_link("https://github.com/paritytech/polkadot-sdk/pull/1818"); From d7ae3e1c3654a3e823956a75748efa4839df0f2e Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sun, 8 Oct 2023 16:55:41 +0200 Subject: [PATCH 07/19] Ignore in dev-mode Signed-off-by: Oliver Tale-Yazdi --- .../support/procedural/src/pallet/expand/call.rs | 2 +- .../support/procedural/src/pallet/expand/warnings.rs | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/substrate/frame/support/procedural/src/pallet/expand/call.rs b/substrate/frame/support/procedural/src/pallet/expand/call.rs index 89d6f2649a7e..ed6335159cd6 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/call.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/call.rs @@ -90,7 +90,7 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream { CallWeightDef::DevModeDefault => fn_weight.push(syn::parse_quote!(0)), CallWeightDef::Immediate(e) => { weight_constant_warning(e, def.dev_mode, &mut weight_warnings); - weight_witness_warning(method, &mut weight_warnings); + weight_witness_warning(method, def.dev_mode, &mut weight_warnings); fn_weight.push(e.into_token_stream()); }, diff --git a/substrate/frame/support/procedural/src/pallet/expand/warnings.rs b/substrate/frame/support/procedural/src/pallet/expand/warnings.rs index 888c0bf6dcc1..c855277afe96 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/warnings.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/warnings.rs @@ -22,10 +22,14 @@ use proc_macro_warning::Warning; use syn::spanned::Spanned; /// Warn if any of the call arguments starts with a underscore and is used in a weight formula. -pub(crate) fn weight_witness_warning(method: &CallVariantDef, warnings: &mut Vec) { +pub(crate) fn weight_witness_warning(method: &CallVariantDef, dev_mode: bool, warnings: &mut Vec) { + if dev_mode { + return + } let CallWeightDef::Immediate(imm) = &method.weight else { return; }; + let partial_warning = Warning::new_deprecated("UncheckedWeightWitness") .old("not check weight witness data") .new("ensure that all witness data for weight calculation is checked before usage") @@ -53,12 +57,12 @@ pub(crate) fn weight_constant_warning( dev_mode: bool, warnings: &mut Vec, ) { - let syn::Expr::Lit(lit) = weight else { - return; - }; if dev_mode { return } + let syn::Expr::Lit(lit) = weight else { + return; + }; let warning = Warning::new_deprecated("ConstantWeight") .index(warnings.len()) From 2a6f5721a0ed9c4e05321847e07f41b513ec1f79 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sun, 8 Oct 2023 16:58:17 +0200 Subject: [PATCH 08/19] Ignore in root calls Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/elections-phragmen/src/lib.rs | 9 ++++++--- substrate/frame/sudo/src/lib.rs | 5 +++-- substrate/frame/utility/src/lib.rs | 6 ++++-- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/substrate/frame/elections-phragmen/src/lib.rs b/substrate/frame/elections-phragmen/src/lib.rs index 6912649bd122..075ed499efd0 100644 --- a/substrate/frame/elections-phragmen/src/lib.rs +++ b/substrate/frame/elections-phragmen/src/lib.rs @@ -591,13 +591,16 @@ pub mod pallet { /// ## Complexity /// - Check is_defunct_voter() details. #[pallet::call_index(5)] - #[pallet::weight(T::WeightInfo::clean_defunct_voters(*_num_voters, *_num_defunct))] + #[pallet::weight(T::WeightInfo::clean_defunct_voters(*num_voters, *num_defunct))] pub fn clean_defunct_voters( origin: OriginFor, - _num_voters: u32, - _num_defunct: u32, + num_voters: u32, + num_defunct: u32, ) -> DispatchResult { let _ = ensure_root(origin)?; + // We don't check the weight witness since it is a root call. + let _ = (num_voters, num_defunct); + >::iter() .filter(|(_, x)| Self::is_defunct_voter(&x.votes)) .for_each(|(dv, _)| Self::do_remove_voter(&dv)); diff --git a/substrate/frame/sudo/src/lib.rs b/substrate/frame/sudo/src/lib.rs index 0c869bec7f07..47dfb7c00967 100644 --- a/substrate/frame/sudo/src/lib.rs +++ b/substrate/frame/sudo/src/lib.rs @@ -204,14 +204,15 @@ pub mod pallet { /// ## Complexity /// - O(1). #[pallet::call_index(1)] - #[pallet::weight((*_weight, call.get_dispatch_info().class))] + #[pallet::weight((*weight, call.get_dispatch_info().class))] pub fn sudo_unchecked_weight( origin: OriginFor, call: Box<::RuntimeCall>, - _weight: Weight, + weight: Weight, ) -> DispatchResultWithPostInfo { // This is a public call, so we ensure that the origin is some signed account. let sender = ensure_signed(origin)?; + let _ = weight; // We don't check the weight witness since it a root call. ensure!(Self::key().map_or(false, |k| sender == k), Error::::RequireSudo); let res = call.dispatch_bypass_filter(frame_system::RawOrigin::Root.into()); diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs index af212a31eb97..7f963e3637d6 100644 --- a/substrate/frame/utility/src/lib.rs +++ b/substrate/frame/utility/src/lib.rs @@ -479,13 +479,15 @@ pub mod pallet { /// /// The dispatch origin for this call must be _Root_. #[pallet::call_index(5)] - #[pallet::weight((*_weight, call.get_dispatch_info().class))] + #[pallet::weight((*weight, call.get_dispatch_info().class))] pub fn with_weight( origin: OriginFor, call: Box<::RuntimeCall>, - _weight: Weight, + weight: Weight, ) -> DispatchResult { ensure_root(origin)?; + let _ = weight; // Explicitly don't check the the weight witness. + let res = call.dispatch_bypass_filter(frame_system::RawOrigin::Root.into()); res.map(|_| ()).map_err(|e| e.error) } From 9c968834987f41ca738003ce98c5ed1cfa910be4 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sun, 8 Oct 2023 18:00:09 +0200 Subject: [PATCH 09/19] Fix HRMP pallet Signed-off-by: Oliver Tale-Yazdi --- polkadot/runtime/parachains/src/hrmp.rs | 40 +++++++++++++++---- substrate/frame/support/procedural/Cargo.toml | 2 +- .../procedural/src/pallet/expand/warnings.rs | 8 +++- substrate/frame/system/src/lib.rs | 5 ++- 4 files changed, 43 insertions(+), 12 deletions(-) diff --git a/polkadot/runtime/parachains/src/hrmp.rs b/polkadot/runtime/parachains/src/hrmp.rs index b3bbcb433c0b..42592d9d9f14 100644 --- a/polkadot/runtime/parachains/src/hrmp.rs +++ b/polkadot/runtime/parachains/src/hrmp.rs @@ -554,14 +554,26 @@ pub mod pallet { /// /// Origin must be the `ChannelManager`. #[pallet::call_index(3)] - #[pallet::weight(::WeightInfo::force_clean_hrmp(*_inbound, *_outbound))] + #[pallet::weight(::WeightInfo::force_clean_hrmp(*num_inbound, *num_outbound))] pub fn force_clean_hrmp( origin: OriginFor, para: ParaId, - _inbound: u32, - _outbound: u32, + num_inbound: u32, + num_outbound: u32, ) -> DispatchResult { T::ChannelManager::ensure_origin(origin)?; + + ensure!( + HrmpIngressChannelsIndex::::decode_len(para).unwrap_or_default() <= + num_inbound as usize, + Error::::WrongWitness + ); + ensure!( + HrmpEgressChannelsIndex::::decode_len(para).unwrap_or_default() <= + num_outbound as usize, + Error::::WrongWitness + ); + Self::clean_hrmp_after_outgoing(¶); Ok(()) } @@ -575,9 +587,16 @@ pub mod pallet { /// /// Origin must be the `ChannelManager`. #[pallet::call_index(4)] - #[pallet::weight(::WeightInfo::force_process_hrmp_open(*_channels))] - pub fn force_process_hrmp_open(origin: OriginFor, _channels: u32) -> DispatchResult { + #[pallet::weight(::WeightInfo::force_process_hrmp_open(*channels))] + pub fn force_process_hrmp_open(origin: OriginFor, channels: u32) -> DispatchResult { T::ChannelManager::ensure_origin(origin)?; + + ensure!( + HrmpOpenChannelRequestsList::::decode_len().unwrap_or_default() as u32 <= + channels, + Error::::WrongWitness + ); + let host_config = configuration::Pallet::::config(); Self::process_hrmp_open_channel_requests(&host_config); Ok(()) @@ -592,9 +611,16 @@ pub mod pallet { /// /// Origin must be the `ChannelManager`. #[pallet::call_index(5)] - #[pallet::weight(::WeightInfo::force_process_hrmp_close(*_channels))] - pub fn force_process_hrmp_close(origin: OriginFor, _channels: u32) -> DispatchResult { + #[pallet::weight(::WeightInfo::force_process_hrmp_close(*channels))] + pub fn force_process_hrmp_close(origin: OriginFor, channels: u32) -> DispatchResult { T::ChannelManager::ensure_origin(origin)?; + + ensure!( + HrmpCloseChannelRequestsList::::decode_len().unwrap_or_default() as u32 <= + channels, + Error::::WrongWitness + ); + Self::process_hrmp_close_channel_requests(); Ok(()) } diff --git a/substrate/frame/support/procedural/Cargo.toml b/substrate/frame/support/procedural/Cargo.toml index 9781a8825142..8ab86aaf9c39 100644 --- a/substrate/frame/support/procedural/Cargo.toml +++ b/substrate/frame/support/procedural/Cargo.toml @@ -21,7 +21,7 @@ cfg-expr = "0.15.5" itertools = "0.10.3" proc-macro2 = "1.0.56" quote = "1.0.28" -syn = { version = "2.0.38", features = ["full"] } +syn = { version = "2.0.38", features = ["full", "visit-mut"] } frame-support-procedural-tools = { path = "tools" } proc-macro-warning = { version = "1.0.0", default-features = false } macro_magic = { version = "0.4.2", features = ["proc_support"] } diff --git a/substrate/frame/support/procedural/src/pallet/expand/warnings.rs b/substrate/frame/support/procedural/src/pallet/expand/warnings.rs index c855277afe96..f5c6f8f7369c 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/warnings.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/warnings.rs @@ -22,7 +22,11 @@ use proc_macro_warning::Warning; use syn::spanned::Spanned; /// Warn if any of the call arguments starts with a underscore and is used in a weight formula. -pub(crate) fn weight_witness_warning(method: &CallVariantDef, dev_mode: bool, warnings: &mut Vec) { +pub(crate) fn weight_witness_warning( + method: &CallVariantDef, + dev_mode: bool, + warnings: &mut Vec, +) { if dev_mode { return } @@ -35,7 +39,7 @@ pub(crate) fn weight_witness_warning(method: &CallVariantDef, dev_mode: bool, wa .new("ensure that all witness data for weight calculation is checked before usage") .help_link("https://github.com/paritytech/polkadot-sdk/pull/1818"); - for (_, arg_ident, _) in method.args.iter().skip(1) { + for (_, arg_ident, _) in method.args.iter() { // Unused arguments cannot be used in weight formulas. if !arg_ident.to_string().starts_with('_') || !contains_ident(&imm, &arg_ident) { continue diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs index e201d50f6fb8..897d3bd7ce91 100644 --- a/substrate/frame/system/src/lib.rs +++ b/substrate/frame/system/src/lib.rs @@ -420,8 +420,9 @@ pub mod pallet { /// /// Can be executed by every `origin`. #[pallet::call_index(0)] - #[pallet::weight(T::SystemWeightInfo::remark(_remark.len() as u32))] - pub fn remark(_origin: OriginFor, _remark: Vec) -> DispatchResultWithPostInfo { + #[pallet::weight(T::SystemWeightInfo::remark(remark.len() as u32))] + pub fn remark(_origin: OriginFor, remark: Vec) -> DispatchResultWithPostInfo { + let _ = remark; // No need to check the weight witness. Ok(().into()) } From f6415d9a3fcd21d169fbe75cbe70a238090c7685 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sun, 8 Oct 2023 20:18:17 +0200 Subject: [PATCH 10/19] Put pallet-root-testing into dev-mode Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/root-testing/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/root-testing/src/lib.rs b/substrate/frame/root-testing/src/lib.rs index e04c7bfa13d2..bbcda09c3065 100644 --- a/substrate/frame/root-testing/src/lib.rs +++ b/substrate/frame/root-testing/src/lib.rs @@ -29,7 +29,7 @@ use sp_runtime::Perbill; pub use pallet::*; -#[frame_support::pallet] +#[frame_support::pallet(dev_mode)] pub mod pallet { use super::*; use frame_support::pallet_prelude::*; From 19f7a127b53165165193953218c60e7ed72e7077 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Sun, 8 Oct 2023 22:17:21 +0200 Subject: [PATCH 11/19] Fix other tests Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/support/test/tests/pallet.rs | 5 +++-- substrate/frame/support/test/tests/pallet_instance.rs | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/substrate/frame/support/test/tests/pallet.rs b/substrate/frame/support/test/tests/pallet.rs index 1898246470c7..83ae5b9253ce 100644 --- a/substrate/frame/support/test/tests/pallet.rs +++ b/substrate/frame/support/test/tests/pallet.rs @@ -210,12 +210,13 @@ pub mod pallet { { /// Doc comment put in metadata #[pallet::call_index(0)] - #[pallet::weight(Weight::from_parts(*_foo as u64, 0))] + #[pallet::weight(Weight::from_parts(*foo as u64, 0))] pub fn foo( origin: OriginFor, - #[pallet::compact] _foo: u32, + #[pallet::compact] foo: u32, _bar: u32, ) -> DispatchResultWithPostInfo { + let _ = foo; let _ = T::AccountId::from(SomeType1); // Test for where clause let _ = T::AccountId::from(SomeType3); // Test for where clause let _ = origin; diff --git a/substrate/frame/support/test/tests/pallet_instance.rs b/substrate/frame/support/test/tests/pallet_instance.rs index 8d2d52d18852..724734ec4fc9 100644 --- a/substrate/frame/support/test/tests/pallet_instance.rs +++ b/substrate/frame/support/test/tests/pallet_instance.rs @@ -87,12 +87,13 @@ pub mod pallet { impl, I: 'static> Pallet { /// Doc comment put in metadata #[pallet::call_index(0)] - #[pallet::weight(Weight::from_parts(*_foo as u64, 0))] + #[pallet::weight(Weight::from_parts(*foo as u64, 0))] pub fn foo( origin: OriginFor, - #[pallet::compact] _foo: u32, + #[pallet::compact] foo: u32, ) -> DispatchResultWithPostInfo { let _ = origin; + let _ = foo; Self::deposit_event(Event::Something(3)); Ok(().into()) } From bc8c5ce1678edd97def1371456df5d5d8d631444 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 9 Oct 2023 12:47:10 +0200 Subject: [PATCH 12/19] Add prdoc Signed-off-by: Oliver Tale-Yazdi --- prdoc/pr_1818.prdoc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 prdoc/pr_1818.prdoc diff --git a/prdoc/pr_1818.prdoc b/prdoc/pr_1818.prdoc new file mode 100644 index 000000000000..18f577145ec7 --- /dev/null +++ b/prdoc/pr_1818.prdoc @@ -0,0 +1,14 @@ +title: FRAME pallets warning for unchecked weight witness + +doc: + - audience: Core Dev + description: | + FRAME pallets now emit a warning when a call uses a function argument that starts with an underscore in its weight declaration. + +migrations: + db: [ ] + runtime: [ ] + +host_functions: [] + +crates: [ ] From 8a075d3d630e117945c9dcaa5c690531e5660308 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 9 Oct 2023 15:29:08 +0200 Subject: [PATCH 13/19] Add minor bump to prdoc Signed-off-by: Oliver Tale-Yazdi --- prdoc/pr_1818.prdoc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/prdoc/pr_1818.prdoc b/prdoc/pr_1818.prdoc index 18f577145ec7..cbafa02f9af5 100644 --- a/prdoc/pr_1818.prdoc +++ b/prdoc/pr_1818.prdoc @@ -11,4 +11,6 @@ migrations: host_functions: [] -crates: [ ] +crates: +- name: "frame-support-procedural" + semver: minor From dee51fd0b718b7be12519f4bfe703efaee59eb38 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 9 Oct 2023 15:54:23 +0200 Subject: [PATCH 14/19] Update substrate/frame/sudo/src/lib.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> --- substrate/frame/sudo/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/sudo/src/lib.rs b/substrate/frame/sudo/src/lib.rs index 47dfb7c00967..fb29c0da42a9 100644 --- a/substrate/frame/sudo/src/lib.rs +++ b/substrate/frame/sudo/src/lib.rs @@ -212,7 +212,7 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { // This is a public call, so we ensure that the origin is some signed account. let sender = ensure_signed(origin)?; - let _ = weight; // We don't check the weight witness since it a root call. + let _ = weight; // We don't check the weight witness since it is a root call. ensure!(Self::key().map_or(false, |k| sender == k), Error::::RequireSudo); let res = call.dispatch_bypass_filter(frame_system::RawOrigin::Root.into()); From c4059c5b7250435078d315b3679eec5162a87490 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 9 Oct 2023 17:53:39 +0200 Subject: [PATCH 15/19] Dont use VisitMut Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/support/procedural/Cargo.toml | 2 +- .../procedural/src/pallet/expand/warnings.rs | 21 +++++++++---------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/substrate/frame/support/procedural/Cargo.toml b/substrate/frame/support/procedural/Cargo.toml index 8ab86aaf9c39..9781a8825142 100644 --- a/substrate/frame/support/procedural/Cargo.toml +++ b/substrate/frame/support/procedural/Cargo.toml @@ -21,7 +21,7 @@ cfg-expr = "0.15.5" itertools = "0.10.3" proc-macro2 = "1.0.56" quote = "1.0.28" -syn = { version = "2.0.38", features = ["full", "visit-mut"] } +syn = { version = "2.0.38", features = ["full"] } frame-support-procedural-tools = { path = "tools" } proc-macro-warning = { version = "1.0.0", default-features = false } macro_magic = { version = "0.4.2", features = ["proc_support"] } diff --git a/substrate/frame/support/procedural/src/pallet/expand/warnings.rs b/substrate/frame/support/procedural/src/pallet/expand/warnings.rs index f5c6f8f7369c..ae5890878a2f 100644 --- a/substrate/frame/support/procedural/src/pallet/expand/warnings.rs +++ b/substrate/frame/support/procedural/src/pallet/expand/warnings.rs @@ -19,7 +19,10 @@ use crate::pallet::parse::call::{CallVariantDef, CallWeightDef}; use proc_macro_warning::Warning; -use syn::spanned::Spanned; +use syn::{ + spanned::Spanned, + visit::{self, Visit}, +}; /// Warn if any of the call arguments starts with a underscore and is used in a weight formula. pub(crate) fn weight_witness_warning( @@ -30,7 +33,7 @@ pub(crate) fn weight_witness_warning( if dev_mode { return } - let CallWeightDef::Immediate(imm) = &method.weight else { + let CallWeightDef::Immediate(w) = &method.weight else { return; }; @@ -40,8 +43,7 @@ pub(crate) fn weight_witness_warning( .help_link("https://github.com/paritytech/polkadot-sdk/pull/1818"); for (_, arg_ident, _) in method.args.iter() { - // Unused arguments cannot be used in weight formulas. - if !arg_ident.to_string().starts_with('_') || !contains_ident(&imm, &arg_ident) { + if !arg_ident.to_string().starts_with('_') || !contains_ident(w.clone(), &arg_ident) { continue } @@ -80,24 +82,21 @@ pub(crate) fn weight_constant_warning( } /// Returns whether `expr` contains `ident`. -fn contains_ident(expr: &syn::Expr, ident: &syn::Ident) -> bool { - use syn::visit_mut::{self, VisitMut}; - +fn contains_ident(mut expr: syn::Expr, ident: &syn::Ident) -> bool { struct ContainsIdent { ident: syn::Ident, found: bool, } - impl VisitMut for ContainsIdent { - fn visit_ident_mut(&mut self, i: &mut syn::Ident) { + impl<'a> Visit<'a> for ContainsIdent { + fn visit_ident(&mut self, i: &syn::Ident) { if *i == self.ident { self.found = true; } } } - let mut expr = expr.clone(); let mut visitor = ContainsIdent { ident: ident.clone(), found: false }; - visit_mut::visit_expr_mut(&mut visitor, &mut expr); + visit::visit_expr(&mut visitor, &mut expr); visitor.found } From e0b5aa536e620a5f3f2d6b15e841826a5985b85b Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 9 Oct 2023 18:24:47 +0200 Subject: [PATCH 16/19] Add pallet-ui test Signed-off-by: Oliver Tale-Yazdi --- .../call_weight_unchecked_warning.rs | 38 +++++++++++++++++++ .../call_weight_unchecked_warning.stderr | 12 ++++++ 2 files changed, 50 insertions(+) create mode 100644 substrate/frame/support/test/tests/pallet_ui/call_weight_unchecked_warning.rs create mode 100644 substrate/frame/support/test/tests/pallet_ui/call_weight_unchecked_warning.stderr diff --git a/substrate/frame/support/test/tests/pallet_ui/call_weight_unchecked_warning.rs b/substrate/frame/support/test/tests/pallet_ui/call_weight_unchecked_warning.rs new file mode 100644 index 000000000000..8d93638f5a51 --- /dev/null +++ b/substrate/frame/support/test/tests/pallet_ui/call_weight_unchecked_warning.rs @@ -0,0 +1,38 @@ +// 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. + +#[frame_support::pallet] +mod pallet { + use frame_support::pallet_prelude::DispatchResult; + use frame_system::pallet_prelude::OriginFor; + + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::pallet] + pub struct Pallet(core::marker::PhantomData); + + #[pallet::call] + impl Pallet { + #[pallet::call_index(0)] + #[pallet::weight(*_unused)] + pub fn foo(_: OriginFor, _unused: u64) -> DispatchResult { Ok(()) } + } +} + +fn main() { +} diff --git a/substrate/frame/support/test/tests/pallet_ui/call_weight_unchecked_warning.stderr b/substrate/frame/support/test/tests/pallet_ui/call_weight_unchecked_warning.stderr new file mode 100644 index 000000000000..89fc1e0820f5 --- /dev/null +++ b/substrate/frame/support/test/tests/pallet_ui/call_weight_unchecked_warning.stderr @@ -0,0 +1,12 @@ +error: use of deprecated constant `pallet::warnings::UncheckedWeightWitness_0::_w`: + It is deprecated to not check weight witness data. + Please instead ensure that all witness data for weight calculation is checked before usage. + + For more info see: + + --> tests/pallet_ui/call_weight_unchecked_warning.rs:33:31 + | +33 | pub fn foo(_: OriginFor, _unused: u64) -> DispatchResult { Ok(()) } + | ^^^^^^^ + | + = note: `-D deprecated` implied by `-D warnings` From d2f893aa65ea930801444fb030ef703bf4f2ab5a Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 9 Oct 2023 21:23:58 +0200 Subject: [PATCH 17/19] Prevent font-run in clean_defunct_voters Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/elections-phragmen/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/elections-phragmen/src/lib.rs b/substrate/frame/elections-phragmen/src/lib.rs index 075ed499efd0..a6a2d542d8ac 100644 --- a/substrate/frame/elections-phragmen/src/lib.rs +++ b/substrate/frame/elections-phragmen/src/lib.rs @@ -598,11 +598,11 @@ pub mod pallet { num_defunct: u32, ) -> DispatchResult { let _ = ensure_root(origin)?; - // We don't check the weight witness since it is a root call. - let _ = (num_voters, num_defunct); >::iter() + .take(num_voters) .filter(|(_, x)| Self::is_defunct_voter(&x.votes)) + .take(num_defunct) .for_each(|(dv, _)| Self::do_remove_voter(&dv)); Ok(()) From 8ff36d32ebfa954800b131eb63849a98618ef535 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 9 Oct 2023 21:30:25 +0200 Subject: [PATCH 18/19] Compile Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/elections-phragmen/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/elections-phragmen/src/lib.rs b/substrate/frame/elections-phragmen/src/lib.rs index a6a2d542d8ac..93f9fc2b6d24 100644 --- a/substrate/frame/elections-phragmen/src/lib.rs +++ b/substrate/frame/elections-phragmen/src/lib.rs @@ -600,9 +600,9 @@ pub mod pallet { let _ = ensure_root(origin)?; >::iter() - .take(num_voters) + .take(num_voters as usize) .filter(|(_, x)| Self::is_defunct_voter(&x.votes)) - .take(num_defunct) + .take(num_defunct as usize) .for_each(|(dv, _)| Self::do_remove_voter(&dv)); Ok(()) From 7ce101989d87f94d8d4d8cca2446cd261b4322ee Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 10 Oct 2023 15:14:25 +0200 Subject: [PATCH 19/19] Fix bench Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/elections-phragmen/src/benchmarking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/elections-phragmen/src/benchmarking.rs b/substrate/frame/elections-phragmen/src/benchmarking.rs index 56ea19578c8f..9878f7fd41c0 100644 --- a/substrate/frame/elections-phragmen/src/benchmarking.rs +++ b/substrate/frame/elections-phragmen/src/benchmarking.rs @@ -379,7 +379,7 @@ benchmarks! { let root = RawOrigin::Root; }: _(root, v, d) verify { - assert_eq!(>::iter().count() as u32, 0); + assert_eq!(>::iter().count() as u32, v - d); } election_phragmen {