Derive DecodeWithMemTracking for Block

[serban300]

Related to #7360

This PR adds DecodeWithMemTracking as a trait bound for Header, Block and TransactionExtension and
derives it for all the types that implement these traits in polkadot-sdk.

[georgepisaltu]

General comment which isn't blocking for this PR, but wouldn't it make sense to have the Parameter trait require DecodeWithMemTracking? It would get rid of some trait bounds.

[serban300]

General comment which isn't blocking for this PR, but wouldn't it make sense to have the Parameter trait require DecodeWithMemTracking? It would get rid of some trait bounds.

I'm not sure. We could. But I just wanted to avoid having to implement DecodeWithMemTracking for even more structures now. So I tried to add it as a bound in as few places as possible

[franciscoaguirre]

Approved but maybe having a Codec macro would be the best

[gui1117]

It will be a bit difficult for people to upgrade their code, I think they will get a lot of error that won't be very descriptive, saying some bound are not met for calling like DispatchTransaction and nothing precise about DecodeWithMemTracking.

Looking at this I wonder if it wouldn't be actually simpler to release parity-scale-codec 4.0 with DecodeWithMemTracking automatically bounded for HasCompact and also automatically derived directly by Decode derive macro. (not sure of this one)
With both it will be simpler for user to upgrade.

But maybe the PR is also fine as it is.

Do have opinion @kianenigma ?

[gui1117]

If you look at the doc of parameter you get:

/// A type that can be used as a parameter in a dispatchable function.
///
/// When using decl_module all arguments for call functions must implement this trait.

I think Parameter itself should bound DecodeWithMemTracking.

[serban300]

I agree that it would be ideal to have DecodeWithMemTracking implemented for each structure that can be used as a parameter in a dispatchable function. But if we do this, we will have to implement DecodeWithMemTracking for a lot more structures and it will lead to even more work for each runtime when it upgrades to this version of polkadot-sdk. Also it isn't needed. We only want to use DecodeWithMemLimit for Block and Extrinsic for the moment. So personally I would avoid doing this.

[gui1117]

I don't understand, requiring DecodeWithMemTracking to extrinsic implies requiring parameter of dispatchables to bound DecodeWithMemTracking

Do you mean people are using the trait Parameter for other types?
Or do you mean that we don't need to enforce it at the pallet level and only constraint at the runtime definition that call implements DecodeWithMemTracking.

Because Parameter should be the bound for types used in extrinsics as part of call parameters.

[serban300]

Done. Sorry, at that point I hadn't checked exactly. I just tried to do Parameter: DecodeWithMemTracking and saw there were a lot of conflicts. So I thought that people were using DecodeWithMemTracking for other types since we already enforce it for RuntimeCall. But looks like the conflicts were coming from types used in tests. Also had to propagate DecodeWithMemTracking to some more traits.

[gui1117]

I wonder if we shouldn't just add this bound to Nonce directly.
Do we expect use of frame-system without CheckNonce or frame-system-benchmarking.

I agree it is good to be generic, but FRAME a little bit more opinionated would help the user experience I think.

[serban300]

I wouldn't add HasCompact to Nonce since I don't know if it's always needed.

Also this is temporary. It's happening because HasCompact looks like this:

pub trait HasCompact: Sized {
	/// The compact type; this can be
	type Type: for<'a> EncodeAsRef<'a, Self> + Decode + From<Self> + Into<Self>;
}

and we avoided adding Type: DecodeWithMemTracking because it would have been a breaking change. But in parity-scale-codec v4 we can add it.

[gui1117]

I think Nonce already bounds HasCompact through BaseArithmetic

[serban300]

Done

[gui1117]

I think we can remove this line now.

Suggested change

<T::Nonce as codec::HasCompact>::Type: codec::DecodeWithMemTracking

[kianenigma]

regarding @gui1117's comment on this being a breaking change, let's zoom back and ask:

1. why do we need this?
2. if we and only we need it for certain types or a specific project, can we add this elsewhere, and not impose it on anyone that uses trait Header etc?

If for example a bridge related project needs this, it can be implemented for Block/Header type of that specific runtime?

I do agree that this will likely break a lot of solochain or parachain teams that have customized anything in Block/Header realm.

[bkchr]

Why this change?

[serban300]

When working on the prdoc for this PR I realized that I also derived DecodeWithMemTracking in some structures from bridge-hub-westend-runtime in pr #7634. So I guess it should also be patch.

[bkchr]

If for example a bridge related project needs this, it can be implemented for Block/Header type of that specific runtime?

We want this for every runtime. For every user of the SDK.

With both it will be simpler for user to upgrade.

Are you sure? Also the SCALE upgrade would have a some impact for downstream users.

[gui1117]

With both it will be simpler for user to upgrade.

Are you sure? Also the SCALE upgrade would have a some impact for downstream users.

Yes ok, SCALE upgrade is a massive task as well. Maybe the best would be to bound DecodeWithMemTracking in Parameter trait, and in primitives types that bounds HasCompact like BlockNumber trait and Hash trait etc..

That said, bounding DecodeWithMemTracking in BaseArithmetic where HasCompact is bounded doesn't feel so good but I don't know. So only more precise trait seems better.

[serban300]

With both it will be simpler for user to upgrade.

Are you sure? Also the SCALE upgrade would have a some impact for downstream users.

Yes ok, SCALE upgrade is a massive task as well. Maybe the best would be to bound DecodeWithMemTracking in Parameter trait, and in primitives types that bounds HasCompact like BlockNumber trait and Hash trait etc..

I agree. SCALE upgrade would be a massive task. Also bounding DecodeWithMemTracking in Parameter and in primitive types would lead to even more upgrade conflicts, so personally I wouldn't do that either. I would just bound DecodeWithMemTracking just where it's needed and keep the changes and the upgrade conflicts to a minimum.

1. why do we need this?

For context we need this in order to then be able to add a heap memory limit for the blocks and the extrinsics. So we would need this for every runtime.

another thing that we could do in order to reduce the impact of this upgrade is to skip implementing DecodeWithMemTracking for signed extensions and just do something like:

// We consider that the `UncheckedExtrinsic` implements `DecodeWithMemTracking` if the
// `Address`, `Signature` and `Call` implement it.
// The `Call` is the heaviest part of the extrinsic. The `Extension` should be much lighter usually.
impl<Address, Call, Signature, Extension> DecodeWithMemTracking
	for UncheckedExtrinsic<Address, Call, Signature, Extension>
where
	UncheckedExtrinsic<Address, Call, Signature, Extension>: Decode,
	Address: DecodeWithMemTracking,
	Signature: DecodeWithMemTracking,
	Call: DecodeWithMemTracking,
{
}

@gui1117 @kianenigma @bkchr WDYT ?

[gui1117]

Some comments, but I think it looks nicer now, less "where clauses" and just some up-front bounds and derive call.

[gui1117]

I think we can remove this line now.

Suggested change

<T::Nonce as codec::HasCompact>::Type: codec::DecodeWithMemTracking

[serban300]

@gui1117 I think I addressed all your comments. Can you please take another look ?

[gui1117]

Looks good

[athei]

The derive in revive is fine.

[AKJUS]

superb