Skip to content

Commit 07719a0

Browse files
ggwpezjoepetrowski
andauthored
[FRAME] Warn on unchecked weight witness (paritytech#1818)
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: <paritytech#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>
1 parent 2151bf6 commit 07719a0

File tree

16 files changed

+190
-329
lines changed

16 files changed

+190
-329
lines changed

substrate/frame/elections-phragmen/src/benchmarking.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,7 @@ benchmarks! {
379379
let root = RawOrigin::Root;
380380
}: _(root, v, d)
381381
verify {
382-
assert_eq!(<Voting<T>>::iter().count() as u32, 0);
382+
assert_eq!(<Voting<T>>::iter().count() as u32, v - d);
383383
}
384384

385385
election_phragmen {

substrate/frame/elections-phragmen/src/lib.rs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -591,15 +591,18 @@ pub mod pallet {
591591
/// ## Complexity
592592
/// - Check is_defunct_voter() details.
593593
#[pallet::call_index(5)]
594-
#[pallet::weight(T::WeightInfo::clean_defunct_voters(*_num_voters, *_num_defunct))]
594+
#[pallet::weight(T::WeightInfo::clean_defunct_voters(*num_voters, *num_defunct))]
595595
pub fn clean_defunct_voters(
596596
origin: OriginFor<T>,
597-
_num_voters: u32,
598-
_num_defunct: u32,
597+
num_voters: u32,
598+
num_defunct: u32,
599599
) -> DispatchResult {
600600
let _ = ensure_root(origin)?;
601+
601602
<Voting<T>>::iter()
603+
.take(num_voters as usize)
602604
.filter(|(_, x)| Self::is_defunct_voter(&x.votes))
605+
.take(num_defunct as usize)
603606
.for_each(|(dv, _)| Self::do_remove_voter(&dv));
604607

605608
Ok(())

substrate/frame/root-testing/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ use sp_runtime::Perbill;
2929

3030
pub use pallet::*;
3131

32-
#[frame_support::pallet]
32+
#[frame_support::pallet(dev_mode)]
3333
pub mod pallet {
3434
use super::*;
3535
use frame_support::pallet_prelude::*;

substrate/frame/sudo/src/lib.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -204,14 +204,15 @@ pub mod pallet {
204204
/// ## Complexity
205205
/// - O(1).
206206
#[pallet::call_index(1)]
207-
#[pallet::weight((*_weight, call.get_dispatch_info().class))]
207+
#[pallet::weight((*weight, call.get_dispatch_info().class))]
208208
pub fn sudo_unchecked_weight(
209209
origin: OriginFor<T>,
210210
call: Box<<T as Config>::RuntimeCall>,
211-
_weight: Weight,
211+
weight: Weight,
212212
) -> DispatchResultWithPostInfo {
213213
// This is a public call, so we ensure that the origin is some signed account.
214214
let sender = ensure_signed(origin)?;
215+
let _ = weight; // We don't check the weight witness since it is a root call.
215216
ensure!(Self::key().map_or(false, |k| sender == k), Error::<T>::RequireSudo);
216217

217218
let res = call.dispatch_bypass_filter(frame_system::RawOrigin::Root.into());

substrate/frame/support/procedural/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ proc-macro2 = "1.0.56"
2323
quote = "1.0.28"
2424
syn = { version = "2.0.38", features = ["full"] }
2525
frame-support-procedural-tools = { path = "tools" }
26-
proc-macro-warning = { version = "0.4.2", default-features = false }
26+
proc-macro-warning = { version = "1.0.0", default-features = false }
2727
macro_magic = { version = "0.4.2", features = ["proc_support"] }
2828
expander = "2.0.0"
2929
sp-core-hashing = { path = "../../../primitives/core/hashing" }

substrate/frame/support/procedural/src/construct_runtime/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -414,7 +414,7 @@ fn construct_runtime_final_expansion(
414414
)
415415
.help_links(&["https://github.com/paritytech/substrate/pull/14437"])
416416
.span(where_section.span)
417-
.build(),
417+
.build_or_panic(),
418418
)
419419
});
420420

substrate/frame/support/procedural/src/pallet/expand/call.rs

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,14 @@
1717

1818
use crate::{
1919
pallet::{
20+
expand::warnings::{weight_constant_warning, weight_witness_warning},
2021
parse::call::{CallVariantDef, CallWeightDef},
2122
Def,
2223
},
2324
COUNTER,
2425
};
2526
use proc_macro2::TokenStream as TokenStream2;
27+
use proc_macro_warning::Warning;
2628
use quote::{quote, ToTokens};
2729
use syn::spanned::Spanned;
2830

@@ -68,7 +70,7 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream {
6870
continue
6971
}
7072

71-
let warning = proc_macro_warning::Warning::new_deprecated("ImplicitCallIndex")
73+
let warning = Warning::new_deprecated("ImplicitCallIndex")
7274
.index(call_index_warnings.len())
7375
.old("use implicit call indices")
7476
.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 {
7779
"https://github.com/paritytech/substrate/pull/11381"
7880
])
7981
.span(method.name.span())
80-
.build();
82+
.build_or_panic();
8183
call_index_warnings.push(warning);
8284
}
8385

@@ -86,18 +88,12 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream {
8688
for method in &methods {
8789
match &method.weight {
8890
CallWeightDef::DevModeDefault => fn_weight.push(syn::parse_quote!(0)),
89-
CallWeightDef::Immediate(e @ syn::Expr::Lit(lit)) if !def.dev_mode => {
90-
let warning = proc_macro_warning::Warning::new_deprecated("ConstantWeight")
91-
.index(weight_warnings.len())
92-
.old("use hard-coded constant as call weight")
93-
.new("benchmark all calls or put the pallet into `dev` mode")
94-
.help_link("https://github.com/paritytech/substrate/pull/13798")
95-
.span(lit.span())
96-
.build();
97-
weight_warnings.push(warning);
91+
CallWeightDef::Immediate(e) => {
92+
weight_constant_warning(e, def.dev_mode, &mut weight_warnings);
93+
weight_witness_warning(method, def.dev_mode, &mut weight_warnings);
94+
9895
fn_weight.push(e.into_token_stream());
9996
},
100-
CallWeightDef::Immediate(e) => fn_weight.push(e.into_token_stream()),
10197
CallWeightDef::Inherited => {
10298
let pallet_weight = def
10399
.call

substrate/frame/support/procedural/src/pallet/expand/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ mod store_trait;
3434
mod tt_default_parts;
3535
mod type_value;
3636
mod validate_unsigned;
37+
mod warnings;
3738

3839
use crate::pallet::Def;
3940
use quote::ToTokens;
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
// This file is part of Substrate.
2+
3+
// Copyright (C) Parity Technologies (UK) Ltd.
4+
// SPDX-License-Identifier: Apache-2.0
5+
6+
// Licensed under the Apache License, Version 2.0 (the "License");
7+
// you may not use this file except in compliance with the License.
8+
// You may obtain a copy of the License at
9+
//
10+
// http://www.apache.org/licenses/LICENSE-2.0
11+
//
12+
// Unless required by applicable law or agreed to in writing, software
13+
// distributed under the License is distributed on an "AS IS" BASIS,
14+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
// See the License for the specific language governing permissions and
16+
// limitations under the License.
17+
18+
//! Generates warnings for undesirable pallet code.
19+
20+
use crate::pallet::parse::call::{CallVariantDef, CallWeightDef};
21+
use proc_macro_warning::Warning;
22+
use syn::{
23+
spanned::Spanned,
24+
visit::{self, Visit},
25+
};
26+
27+
/// Warn if any of the call arguments starts with a underscore and is used in a weight formula.
28+
pub(crate) fn weight_witness_warning(
29+
method: &CallVariantDef,
30+
dev_mode: bool,
31+
warnings: &mut Vec<Warning>,
32+
) {
33+
if dev_mode {
34+
return
35+
}
36+
let CallWeightDef::Immediate(w) = &method.weight else {
37+
return;
38+
};
39+
40+
let partial_warning = Warning::new_deprecated("UncheckedWeightWitness")
41+
.old("not check weight witness data")
42+
.new("ensure that all witness data for weight calculation is checked before usage")
43+
.help_link("https://github.com/paritytech/polkadot-sdk/pull/1818");
44+
45+
for (_, arg_ident, _) in method.args.iter() {
46+
if !arg_ident.to_string().starts_with('_') || !contains_ident(w.clone(), &arg_ident) {
47+
continue
48+
}
49+
50+
let warning = partial_warning
51+
.clone()
52+
.index(warnings.len())
53+
.span(arg_ident.span())
54+
.build_or_panic();
55+
56+
warnings.push(warning);
57+
}
58+
}
59+
60+
/// Warn if the weight is a constant and the pallet not in `dev_mode`.
61+
pub(crate) fn weight_constant_warning(
62+
weight: &syn::Expr,
63+
dev_mode: bool,
64+
warnings: &mut Vec<Warning>,
65+
) {
66+
if dev_mode {
67+
return
68+
}
69+
let syn::Expr::Lit(lit) = weight else {
70+
return;
71+
};
72+
73+
let warning = Warning::new_deprecated("ConstantWeight")
74+
.index(warnings.len())
75+
.old("use hard-coded constant as call weight")
76+
.new("benchmark all calls or put the pallet into `dev` mode")
77+
.help_link("https://github.com/paritytech/substrate/pull/13798")
78+
.span(lit.span())
79+
.build_or_panic();
80+
81+
warnings.push(warning);
82+
}
83+
84+
/// Returns whether `expr` contains `ident`.
85+
fn contains_ident(mut expr: syn::Expr, ident: &syn::Ident) -> bool {
86+
struct ContainsIdent {
87+
ident: syn::Ident,
88+
found: bool,
89+
}
90+
91+
impl<'a> Visit<'a> for ContainsIdent {
92+
fn visit_ident(&mut self, i: &syn::Ident) {
93+
if *i == self.ident {
94+
self.found = true;
95+
}
96+
}
97+
}
98+
99+
let mut visitor = ContainsIdent { ident: ident.clone(), found: false };
100+
visit::visit_expr(&mut visitor, &mut expr);
101+
visitor.found
102+
}

substrate/frame/support/test/tests/pallet.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -210,12 +210,13 @@ pub mod pallet {
210210
{
211211
/// Doc comment put in metadata
212212
#[pallet::call_index(0)]
213-
#[pallet::weight(Weight::from_parts(*_foo as u64, 0))]
213+
#[pallet::weight(Weight::from_parts(*foo as u64, 0))]
214214
pub fn foo(
215215
origin: OriginFor<T>,
216-
#[pallet::compact] _foo: u32,
216+
#[pallet::compact] foo: u32,
217217
_bar: u32,
218218
) -> DispatchResultWithPostInfo {
219+
let _ = foo;
219220
let _ = T::AccountId::from(SomeType1); // Test for where clause
220221
let _ = T::AccountId::from(SomeType3); // Test for where clause
221222
let _ = origin;

0 commit comments

Comments
 (0)