FRAME: Avoid boilerplate weight macros

[gavofyork]

Currently we specify weights individually for each call. In cases of complex weights, this is fine. But often times pallets have very simple O(1) weights. An example is the new core-fellowship pallet, whose call block looks like this:

#[pallet::call]
impl<T: Config<I>, I: 'static> Pallet<T, I> {
	/// Snip
	#[pallet::weight(T::WeightInfo::set_params())]
	#[pallet::call_index(1)]
	pub fn set_params(/* snip */) -> DispatchResultWithPostInfo {
		/* snip */
	}

	/// Snip
	#[pallet::weight(T::WeightInfo::set_active())]
	#[pallet::call_index(2)]
	pub fn set_active(/* snip */) -> DispatchResultWithPostInfo {
		/* snip */
	}

	/// Snip
	#[pallet::weight(T::WeightInfo::approve())]
	#[pallet::call_index(3)]
	pub fn approve(/* snip */) -> DispatchResultWithPostInfo {
		/* snip */
	}

	/// Snip
	#[pallet::weight(T::WeightInfo::induct())]
	#[pallet::call_index(4)]
	pub fn induct(/* snip */) -> DispatchResultWithPostInfo {
		/* snip */
	}
	
	// etc.
}

This is a bit tedious, but more importantly it's very error-prone. If the line reading:

	#[pallet::weight(T::WeightInfo::set_active())]

were accidentally written:

	#[pallet::weight(T::WeightInfo::set_params())]

Then there would be no build error or warning and everything would appear to proceed well but the chain would be at risk of a potentially devastating DoS attack.

Instead, we should dispense with per-function weights in the general case and instead have a directive at the level of the call-block instructing the default weight to be determined as the function call in the WeightInfo of the same name as the call function. Thus the previous code becomes:

#[pallet::call]
#[pallet::weight_info(T::WeightInfo)]
impl<T: Config<I>, I: 'static> Pallet<T, I> {
	/// Snip
	#[pallet::call_index(1)]
	pub fn set_params(/* snip */) -> DispatchResultWithPostInfo {
		/* snip */
	}

	/// Snip
	#[pallet::call_index(2)]
	pub fn set_active(/* snip */) -> DispatchResultWithPostInfo {
		/* snip */
	}

	/// Snip
	#[pallet::call_index(3)]
	pub fn approve(/* snip */) -> DispatchResultWithPostInfo {
		/* snip */
	}

	/// Snip
	#[pallet::call_index(4)]
	pub fn induct(/* snip */) -> DispatchResultWithPostInfo {
		/* snip */
	}
	
	// etc.
}

Ideally, we could come up with a combination of pallet macros, WeightInfo API/convention and benchmarking macros to allow the complete avoidance of manual weight specification logic in the call block.

For a start, one other pattern which is quite common to see is multi-modal-maximum, whereby a call has two or more benchmarks, each forcing a different control-flow, and the call's weight is the per-component max. See the bump call in the core-fellowship pallet:

/// Snip
#[pallet::weight(
	T::WeightInfo::bump_offboard().max(T::WeightInfo::bump_demote())
)]
#[pallet::call_index(0)]
pub fn bump(/* snip */) -> DispatchResultWithPostInfo {
	/* snip */
}

We might have some better (more declarative, less procedural) means of expressing this:

/// Snip
#[pallet::weight_max_over_prefix]
#[pallet::call_index(0)]
pub fn bump(/* snip */) -> DispatchResultWithPostInfo {
	/* snip */
}

A better way would be to avoid relying on purely string prefixes, and actually have tagged groupings in the benchmarking/WeightInfo APIs but this would be a bigger job.

[sam0x17]

I'll add this to the macros meta issue #264, good one!

[ggwpez]

A better way would be to avoid relying on purely string prefixes, and actually have tagged groupings in the benchmarking/WeightInfo APIs but this would be a bigger job.

Yea or some kind of meta-functions (V2 only) that can be used to do these calculations and then become part of the WeightInfo trait.

mod benchmarks {
	use super::*;
	// other benchmarks…

	#[weight_function]
	fn transfer() -> Weight {
		transfer_worst_case()
			.max(transfer_best_case())
	}
}

These could also accept components.
Although all weight functions with a component would then still need to be called manually, since it is not clear what that component is (eg length of a vector) and pulling in all the real types into the WeightInfo trait is probably stupid.

[kianenigma]

@sam0x17 can this be incorporated as a part of #380, and placed in the roadmap?

All in all, I am not super versed in the latest suggested features for benchmarking and weight. It seems like even #380 is not a good representation of all the items we want to achieve. Perhaps you can re-evaluate everything that you foresee, and make a new tracking issue for the roadmap? happy to help brainstorming if needed.

[ggwpez]

The first part was done here paritytech/substrate#13932.

I implemented the second part locally, but it was quite general. It resulted in possible syntax like this:

#[benchmarks]
mod benches {
    #[aggregated]
    fn my_max() {
        T::WeightInfo::fast()
            .max(T::WeightInfo::slow())
    }
}

Any arbitrary code could be written in aggregated functions. The problem is now that this code gets expanded 1-to-1 in the WeightInfo trait. Now a developer can write arbitrary code which compiles locally, but not anymore after it got expanded into the WeightInfo template.

[sam0x17]

Now a developer can write arbitrary code which compiles locally, but not anymore after it got expanded into the WeightInfo template.

So you just get a really confusing error message basically?

[ggwpez]

So you just get a really confusing error message basically?

Yea basically. The problem is that the code gets pasted into here and the errors when trying to compile it next time.
Maybe it would be possible to somehow restrict the syntax (or add a custom syntax instead).

[sam0x17]

Do we want to restrict what you can do in there, like maybe const fn only? Whitelist of function calls?

[kianenigma]

This is already done.