[FRAME] Warn on unchecked weight witness (#1818)

ggwpez · joepetrowski · web-flow · commit 64877492c568 · 2023-10-10T16:02:35.000+02:00
Adds a warning to FRAME pallets when a function argument that starts with `_` is used in the weight formula. This is in most cases an error since the weight witness needs to be checked. Example: ```rust #[pallet::call_index(0)] #[pallet::weight(T::SystemWeightInfo::remark(_remark.len() as u32))] pub fn remark(_origin: OriginFor<T>, _remark: Vec<u8>) -> DispatchResultWithPostInfo { Ok(().into()) } ``` Produces this warning: ```pre warning: 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: <#1818> --> substrate/frame/system/src/lib.rs:424:40 | 424 | pub fn remark(_origin: OriginFor<T>, _remark: Vec<u8>) -> DispatchResultWithPostInfo { | ^^^^^^^ | = note: `#[warn(deprecated)]` on by default ``` Can be suppressed like this, since in this case it is legit: ```rust #[pallet::call_index(0)] #[pallet::weight(T::SystemWeightInfo::remark(remark.len() as u32))] pub fn remark(_origin: OriginFor<T>, remark: Vec<u8>) -> DispatchResultWithPostInfo { let _ = remark; // We dont need to check the weight witness. Ok(().into()) } ``` Changes: - Add warning on uncheded weight witness - Respect `subkeys` limit in `System::kill_prefix` - Fix HRMP pallet and other warnings - Update`proc_macro_warning` dependency - Delete random folder `substrate/src/src` 🙈 - Adding Prdoc --------- Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
diff --git a/Cargo.lock b/Cargo.lock
diff --git 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(<T as Config>::WeightInfo::force_clean_hrmp(*_inbound, *_outbound))]
+		#[pallet::weight(<T as Config>::WeightInfo::force_clean_hrmp(*num_inbound, *num_outbound))]
 		pub fn force_clean_hrmp(
 			origin: OriginFor<T>,
 			para: ParaId,
-			_inbound: u32,
-			_outbound: u32,
+			num_inbound: u32,
+			num_outbound: u32,
 		) -> DispatchResult {
 			T::ChannelManager::ensure_origin(origin)?;
+
+			ensure!(
+				HrmpIngressChannelsIndex::<T>::decode_len(para).unwrap_or_default() <=
+					num_inbound as usize,
+				Error::<T>::WrongWitness
+			);
+			ensure!(
+				HrmpEgressChannelsIndex::<T>::decode_len(para).unwrap_or_default() <=
+					num_outbound as usize,
+				Error::<T>::WrongWitness
+			);
+
 			Self::clean_hrmp_after_outgoing(&para);
 			Ok(())
 		}
@@ -575,9 +587,16 @@ pub mod pallet {
 		///
 		/// Origin must be the `ChannelManager`.
 		#[pallet::call_index(4)]
-		#[pallet::weight(<T as Config>::WeightInfo::force_process_hrmp_open(*_channels))]
-		pub fn force_process_hrmp_open(origin: OriginFor<T>, _channels: u32) -> DispatchResult {
+		#[pallet::weight(<T as Config>::WeightInfo::force_process_hrmp_open(*channels))]
+		pub fn force_process_hrmp_open(origin: OriginFor<T>, channels: u32) -> DispatchResult {
 			T::ChannelManager::ensure_origin(origin)?;
+
+			ensure!(
+				HrmpOpenChannelRequestsList::<T>::decode_len().unwrap_or_default() as u32 <=
+					channels,
+				Error::<T>::WrongWitness
+			);
+
 			let host_config = configuration::Pallet::<T>::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(<T as Config>::WeightInfo::force_process_hrmp_close(*_channels))]
-		pub fn force_process_hrmp_close(origin: OriginFor<T>, _channels: u32) -> DispatchResult {
+		#[pallet::weight(<T as Config>::WeightInfo::force_process_hrmp_close(*channels))]
+		pub fn force_process_hrmp_close(origin: OriginFor<T>, channels: u32) -> DispatchResult {
 			T::ChannelManager::ensure_origin(origin)?;
+
+			ensure!(
+				HrmpCloseChannelRequestsList::<T>::decode_len().unwrap_or_default() as u32 <=
+					channels,
+				Error::<T>::WrongWitness
+			);
+
 			Self::process_hrmp_close_channel_requests();
 			Ok(())
 		}
diff --git a/prdoc/pr_1818.prdoc b/prdoc/pr_1818.prdoc
@@ -0,0 +1,16 @@
+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:
+- name: "frame-support-procedural"
+  semver: minor
diff --git 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!(<Voting<T>>::iter().count() as u32, 0);
+		assert_eq!(<Voting<T>>::iter().count() as u32, v - d);
 	}
 
 	election_phragmen {
diff --git a/substrate/frame/elections-phragmen/src/lib.rs b/substrate/frame/elections-phragmen/src/lib.rs
@@ -591,15 +591,18 @@ 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<T>,
-			_num_voters: u32,
-			_num_defunct: u32,
+			num_voters: u32,
+			num_defunct: u32,
 		) -> DispatchResult {
 			let _ = ensure_root(origin)?;
+
 			<Voting<T>>::iter()
+				.take(num_voters as usize)
 				.filter(|(_, x)| Self::is_defunct_voter(&x.votes))
+				.take(num_defunct as usize)
 				.for_each(|(dv, _)| Self::do_remove_voter(&dv));
 
 			Ok(())
diff --git 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::*;
diff --git 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<T>,
 			call: Box<<T as Config>::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 is a root call.
 			ensure!(Self::key().map_or(false, |k| sender == k), Error::<T>::RequireSudo);
 
 			let res = call.dispatch_bypass_filter(frame_system::RawOrigin::Root.into());
diff --git 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
@@ -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
@@ -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")
@@ -77,7 +79,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);
 	}
 
@@ -86,18 +88,12 @@ 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();
-				weight_warnings.push(warning);
+			CallWeightDef::Immediate(e) => {
+				weight_constant_warning(e, def.dev_mode, &mut weight_warnings);
+				weight_witness_warning(method, def.dev_mode, &mut weight_warnings);
+
 				fn_weight.push(e.into_token_stream());
 			},
-			CallWeightDef::Immediate(e) => fn_weight.push(e.into_token_stream()),
 			CallWeightDef::Inherited => {
 				let pallet_weight = def
 					.call
diff --git 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
@@ -0,0 +1,102 @@
+// 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,
+	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(
+	method: &CallVariantDef,
+	dev_mode: bool,
+	warnings: &mut Vec<Warning>,
+) {
+	if dev_mode {
+		return
+	}
+	let CallWeightDef::Immediate(w) = &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")
+		.help_link("https://github.com/paritytech/polkadot-sdk/pull/1818");
+
+	for (_, arg_ident, _) in method.args.iter() {
+		if !arg_ident.to_string().starts_with('_') || !contains_ident(w.clone(), &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<Warning>,
+) {
+	if dev_mode {
+		return
+	}
+	let syn::Expr::Lit(lit) = weight else {
+		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(mut expr: syn::Expr, ident: &syn::Ident) -> bool {
+	struct ContainsIdent {
+		ident: syn::Ident,
+		found: bool,
+	}
+
+	impl<'a> Visit<'a> for ContainsIdent {
+		fn visit_ident(&mut self, i: &syn::Ident) {
+			if *i == self.ident {
+				self.found = true;
+			}
+		}
+	}
+
+	let mut visitor = ContainsIdent { ident: ident.clone(), found: false };
+	visit::visit_expr(&mut visitor, &mut expr);
+	visitor.found
+}
diff --git 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<T>,
-			#[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
@@ -87,12 +87,13 @@ pub mod pallet {
 	impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		/// 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<T>,
-			#[pallet::compact] _foo: u32,
+			#[pallet::compact] foo: u32,
 		) -> DispatchResultWithPostInfo {
 			let _ = origin;
+			let _ = foo;
 			Self::deposit_event(Event::Something(3));
 			Ok(().into())
 		}
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
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
diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs
diff --git a/substrate/frame/utility/src/lib.rs b/substrate/frame/utility/src/lib.rs
diff --git a/substrate/src/src/lib.rs b/substrate/src/src/lib.rs

Original file line number	Diff line number	Diff line change
`@@ -379,7 +379,7 @@ benchmarks! {`
`379`	`379`	`let root = RawOrigin::Root;`
`380`	`380`	`}: _(root, v, d)`
`381`	`381`	`verify {`
`382`		`- assert_eq!(<Voting<T>>::iter().count() as u32, 0);`
	`382`	`+ assert_eq!(<Voting<T>>::iter().count() as u32, v - d);`
`383`	`383`	`}`
`384`	`384`
`385`	`385`	`election_phragmen {`
Original file line number	Diff line number	Diff line change
`@@ -414,7 +414,7 @@ fn construct_runtime_final_expansion(`
`414`	`414`	`)`
`415`	`415`	`.help_links(&["https://github.com/paritytech/substrate/pull/14437"])`
`416`	`416`	`.span(where_section.span)`
`417`		`- .build(),`
	`417`	`+ .build_or_panic(),`
`418`	`418`	`)`
`419`	`419`	`});`
`420`	`420`