Make Dispatchable return the actual weight consumed

[athei]

Related Items

#5130: The GCD is an older alternate approach to the same problem. This PR seeks to replace it.
#5032: In this PR we lay the ground work that will eventually allow refunding. The linked PR therefore depends on the work done here. However, the linked PR is currently stand alone and will probably be rewritten given the approach in this PR is deemed viable.

Overview

Introduce an associated type PostInfo to the Dispatchable trait that becomes part of its return type and implement it in frame so that it is used to return the a posteriori actual weight of a call.

It is done in a way that requires very little code changes to the existing frame pallets. The only pallets that need to be (minimally) changed are those that do dispatching (sudo, recovery, ...) because they need to deal with the new return typ of Dispatchable::dispatch.

Motivation

We want dispatchables to be able to refund portions it their static a priori weight maximum. The reason for that is that there are dispatchables where the average weight consumed differs from the maximum. Without a refund we are charging too much weight in some cases. An example is Balance::transfer which is heavier when it needs to create the target account. The average case might be that the account already exists where we are then overcharging.

Having dispatchables returning their a posteriori actual weight allows us (in a different PR) to do a refund of the difference.

High Level Changes

Make DispatchResultWithInfo generic over the PostInfo

pub trait Dispatchable {
    ...
    type PostInfo;
    fn dispatch(self, origin: Self::Origin) -> DispatchResultWithPostInfo<Self::PostInfo>;
}

pub type DispatchResultWithInfo<T> = Result<T, DispatchErrorWithPostInfo<T>>;

pub struct DispatchErrorWithPostInfo<Info>
{
    pub post_info: Info,
    pub error: DispatchError,
}

The rationale for having the PostInfo duplicated to both variants and not having a containing struct is so that the result stays a Result for convenient map, map_err and ? function calls.

Implement Dispatchable in frame

pub struct PostDispatchInfo {
    /// Actual weight consumed by a call or `None` which stands for the worst case static weight.
    pub actual_weight: Option<Weight>,
}

pub type DispatchResultWithPostInfo = sp_runtime::DispatchResultWithPostInfo<PostDispatchInfo>;

frame_support re-exports DispatchResultWithPostInfo so that all of frame uses the concrete PostDispatchInfo we chose for frame. Existing frame code still uses the flat DispatchResultwhich is automatically converted (explained later).

The rationale for wrapping the actual_weight with a struct is that returning a plain Weight feels semantically weak to me: Is it a correction to the weight or the actual weight? Having the field makes it unambiguous.

Change decl_module! so that existing dispatchables still work

decl_module! calls Into at the appropriate places in order to allow preexisting frame code which returns DispatchResult to remain unchanged. The actual Dispatchable implementation generated by the macro returns the DispatchResultWithPostInfo either from the default conversion or a user specified PostDispatchInfo.

A runtime Developer can decide to explicitly return a DispatchResultWithPostInfo in order to override the default value of "all weight was used".

Usage

A runtime Developer can return Ok(Some(actual_weight).into()) when specifying DispatchResultWithPostInfo as the return type.

Here is a simplified example from the identity crate to show usage:

#[weight = T::DbWeight::get().reads_writes(1, 1)
	+ 700_000 * (T::MaxRegistrars::get() as Weight)
]
fn set_identity(origin, info: IdentityInfo) -> DispatchResultWithPostInfo {
	// [...]
	let judgements = id.judgements.len();
	Ok(Some(T::DbWeight::get().reads_writes(1, 1)
		+ 700_000 * judgements as Weight).into())
}

Add WithPostDispatchInfo trait

For easy augmentation of module specific errors with weight information we add a trait and automatically implement it for everything that is convertible into a DispatchError so that it can be used in the following way:

let who = ensure_signed(origin).map_err(|e| e.with_weight(100))?;
ensure!(who == me, Error::<T>::NotMe.with_weight(200_000));

Future Work

The following items will be implemented in the near future and are not part of this PR on purpose

Weight Refund

Actually evaluate the PostDispatchInfo in one way or another in order to do the weight refund. The working idea is to pass it as an additional Parameter to SignedExtension::post_dispatch where the refund can then happen.

Return precise actual weight from frame calls

This will probably be done in multiple smaller PRs by the respective code owners or in a big PR by one awesome individual. The point is that this can be an incremental process if necessary.

This includes changing the return type to DispatchResultWithPostInfo and deal with the consequences.

TODO

• Make the weight return opt-in by matching for the return type in decl_module!
• Update decl_module! documentation

[parity-cla-bot]

It looks like @athei signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[kianenigma]

I don't have much knowledge about the error details so can't help much, but can say that I think this approach doesn't need to blocked because of that and we should find a way around it. ensure_with_weight! seems good to me as usually when we return error with some ensure!, we are early-rejecting something and should consume less.

[athei]

I don't have much knowledge about the error details so can't help much, but can say that I think this approach doesn't need to blocked because of that and we should find a way around it. ensure_with_weight! seems good to me as usually when we return error with some ensure!, we are early-rejecting something and should consume less.

It would be most convenient to defer the solution the a followup PR. However, for this PR to gain traction I should at least explore if changing the ensure! would be a viable. I will try to do that.

[xlc]

This make code looks ugly and breaks every pallets, including one that not in Substrate.

Can we make this opt-in?

Instead of change the return type, how about make it something like

pub fn transfer(
  origin,
  dest: <T::Lookup as StaticLookup>::Source,
  #[compact] value: T::Balance,
  mut& weight_meter: WeightMeter<T>
) {
 // ...body
  weight_meter.add(100)
  // or
  weight_meter.set(100)
  Ok(())
}

And we can use macro magic to make it non-breaking for existing code.

[athei]

This design is the result of a longer discussion where it deemed not the nicest but the most pragmatic approach. See the comments in #5130

This make code looks ugly

How exactly does this make the code more ugly than littering your code base with mutable arguments and weight_meter.add(_) calls?

And we can use macro magic to make it non-breaking for existing code.

My feeling is that adding implicit arguments to functions won't find a consensus.

[xlc]

How exactly does this make the code more ugly than littering your code base with mutable arguments and weight_meter.add(_) calls?

It is non-breaking and optional. And we already doing something similar in contracts pallet for gas meter.

Also the real return type for those dispatchables is (DispatchResult, Weight), Not Result<Weight, DispatchErrorWithInfo>. I understand why you choose later, but it just not feeling right. Image to refactor a helper method that adds a weight and can return Result<Balance, Balance>? Make it Result<(Balance, Weight), (Balance, Weight)>?

And we can use macro magic to make it non-breaking for existing code.

My feeling is that adding implicit arguments to functions won't find a consensus.

Alternate approach is just add add_weight to system::Trait and in code can just be T::add_weight.

I am just providing alternative solutions which hopefully you guys finding helpful.

And please try to avoid adding more breaking changes if possible. 🙏

[athei]

It is non-breaking and optional. And we already doing something similar in contracts pallet for gas meter.

We might be able to achieve that even with the current approach in this PR. It would require some amount of macro magic in both cases. I will put some thought into that. Thanks for your remarks.

Also the real return type for those dispatchables is (DispatchResult, Weight), Not Result<Weight, DispatchErrorWithInfo>. I understand why you choose later, but it just not feeling right. Image to refactor a helper method that adds a weight and can return Result<Balance, Balance>? Make it Result<(Balance, Weight), (Balance, Weight)>?

Think about it like this: You already have to calculate the whole a priori weight for every dispatchable. When you need to bubble up weight information through a cascade of helper functions you have deeper problems. This should be an easy calculation mostly done in the dispatchable. Helper functions are not dispatchables. If they where you had the needed information returned (actual_weight). Having the weight accumulated through a call graph would be too dynamic for this model (remember you still have to do the static weight annotation for the whole thing).

Alternate approach is just add add_weight to system::Trait and in code can just be T::add_weight.

This would require some central instance that does the bookkeeping. This is similar to the approach of the linked GCD PR. This approach was already dismissed for various reasons.

I am just providing alternative solutions which hopefully you guys finding helpful.

And please try to avoid adding more breaking changes if possible. 🙏

Thanks for your help. Please keep in mind that these changes target substrate 3.0 and will not brake your projects against the current substrate 2.0 candidate. That said, we still try to avoid brakes if possible without implementing sub par solutions. It is a difficult trade off to make.

[athei]

I reworked the PR so that it is no longer breaking all the pallets. It substantially lessens the amount of changes. The PR description is now up-to-date. Ready for review.