Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FRAME] Warn on unchecked weight witness #1818

Merged
merged 20 commits into from
Oct 10, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

40 changes: 33 additions & 7 deletions polkadot/runtime/parachains/src/hrmp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}
Expand All @@ -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(())
Expand All @@ -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(())
}
Expand Down
16 changes: 16 additions & 0 deletions prdoc/pr_1818.prdoc
Original file line number Diff line number Diff line change
@@ -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
Comment on lines +8 to +16
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still the default schema?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, i used the prdoc schema and prdoc check.

9 changes: 6 additions & 3 deletions substrate/frame/elections-phragmen/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/root-testing/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down
5 changes: 3 additions & 2 deletions substrate/frame/sudo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/support/procedural/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
)
});

Expand Down
20 changes: 8 additions & 12 deletions substrate/frame/support/procedural/src/pallet/expand/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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")
Expand All @@ -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);
}

Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
102 changes: 102 additions & 0 deletions substrate/frame/support/procedural/src/pallet/expand/warnings.rs
Original file line number Diff line number Diff line change
@@ -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")
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
.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
}
5 changes: 3 additions & 2 deletions substrate/frame/support/test/tests/pallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 3 additions & 2 deletions substrate/frame/support/test/tests/pallet_instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
Expand Down
Loading