Include extrinsics len in PoV size

[bkchr]

With WeightsV2 and storage proof tracking we will also need to take the encoded extrinsics length into account. The point being that the actual PoV size is the size of the block header, the extrinsics and the storage proof.
As we also should include the header, we could use the length of an empty encoded header plus the the size of the digests.

CC @ggwpez

[ggwpez]

We only need to take this into account when building the block and when calculating the resulting weight i think.
So no benchmarking needed, just adding extrinsic.encoded_len() to the Proof weight result should be enough.

[bkchr]

So no benchmarking needed, just adding extrinsic.encoded_len() to the Proof weight result should be enough.

Yeah that should be enough! Could probably be done as part of CheckWeight as this already has all the required data.

[eskimor]

Actually I think for standalone chains like the relay chain we even have to only track extrinsic.encoded_len() and no storage proof size. Otherwise if we take the size/storage proof weight into account we would underutilize the available block length.

[ggwpez]

Actually I think for standalone chains like the relay chain we even have to only track extrinsic.encoded_len() and no storage proof size. Otherwise if we take the size/storage proof weight into account we would underutilize the available block length.

The DotSama relay chains have u64::MAX as proof size, so the proof size is not important at all.

[bkchr]

Yeah and the block size is already tracked independently here: https://github.com/paritytech/substrate/blob/2bf16d831b777d3c44445d869689a66f0c0e64c6/frame/system/src/extensions/check_weight.rs#L73-L86

[eskimor]

The DotSama relay chains have u64::MAX as proof size, so the proof size is not important at all.

That does not help at all. If we are still accounting proof size in the proof size weight, then we cannot use it for tracking block size - and would not know when a block becomes overweight with regards to size/length.

[eskimor]

Yeah and the block size is already tracked independently here: https://github.com/paritytech/substrate/blob/2bf16d831b777d3c44445d869689a66f0c0e64c6/frame/system/src/extensions/check_weight.rs#L73-L86

Copying over my response from the other ticket:

To me this does not look like it would apply to inherents? But even if it did (does it?) it would actually make matters worse, because if would drop all of paras inherent at a hard boundary. So we would need to check size internally even more precisely then.

[bkchr]

To me this does not look like it would apply to inherents?

Inherents are just normal (unsigned) extrinsics and this also applies to them: https://github.com/paritytech/substrate/blob/057f2afd686dceb48c08d8a31f35b338bb5992d3/frame/system/src/extensions/check_weight.rs#L206-L212

So we would need to check size internally even more precisely then.

Not sure what you mean by "internally", but yes. I mean you could do these checks inside create_inherent (inside the runtime) to ensure that the size is not running above the allowed maximum. We should probably design this system you proposed to handle the size usage between inherents. Inherents could expose some kind of minimum size they would need, which in the case for timestamp is just the u64 and for the parachain inherents the encoded size of the empty inherent data. Then when creating the inherents we pass the left over size the inherent can use.

[eskimor]

To me it sounds like, if we do the "proof size weight" proper (name should probably be adjusted then), then we already pretty much have the mechanism in place I was suggesting, but actually better. Because we already do filtering on weight, so in theory if I get benchmarks to work correctly for candidate receipts and we have proof size weights which take into account block size (and only that for standalone chains), then the code that is already there would already have the desired effect: Perfect, just seems to be the right solution to me!

The code we have, queries what is the remaining available weight and then filters out stuff to fit things in. With weights also taking into account size, that code already does all we need.

Shouldn't be to hard to ignore actual proof size for standalone chains?

[bkchr]

Shouldn't be to hard to ignore actual proof size for standalone chains?

I think I proposed this at some point, by making the size an option and set it to None. However, currently we set it to u64::Max which is the same as ignoring it?

[eskimor]

Let's see whether I understand this correctly:

1. The second dimension of weight will/does contain the proof size + the block size used.
2. When we calculate the proof size, we set it u64::Max for standalone chains.
3. Then we calculate the second weight dimension by adding together with overflow the proof size and the used block size, hence for standalone chains we end up only with the used block size?

Did I get this correctly? If so, I am still unsure why we would want to use u64::Max and not 0? Sounds more correct and safer to me. E.g. if we had a bug in benchmarking and used block size was 0, then we would always be overweight.

So I think, I am absolutely not getting what you actually mean. 😬

[ggwpez]

Shouldn't be to hard to ignore actual proof size for standalone chains?

I think I proposed this at some point, by making the size an option and set it to None. However, currently we set it to u64::Max which is the same as ignoring it?

Tried this a while back, but it resulted in a large breaking change where all downstream chains would have to change how they construct their RuntimeBlockWeights: paritytech/substrate#13164
It uses a WeightLimit type that can be set to unlimited (=None). But it is much easier and non-breaking to just rely on the saturating math and u64::MAX.

Did I get this correctly? If so, I am still unsure why we would want to use u64::Max and not 0?

The limit is set to u64::MAX, not the initial value. The parachain limit is 5MiB in comparison. Or do you mean we should set the relay limit also to 0?
Sorry I am neither getting what you mean…

[eskimor]

Ok, that was my initial understanding - that you are talking about the limit, but given @bkchr comment I wasn't so sure anymore.

The problem is for our use case, just setting the limit to u64::MAX is absolutely not what we want. We just don't want to track proof size (as there is none). Instead we only want to track block size and know when we reached the limit. So the limit should not be u64::MAX, but maximum block size - header size.

If we keep tracking the size for the non existing proof for standalone chains, then the second weight component is totally useless here. While if we tracked correctly, it would be very valuable and useful in maintaining the security of the chain.

[eskimor]

I also quite don't get how not adding the proof size in benchmarks (but only counting extrinisic length) for standalone chains should break any parachain.

I am specifically talking about benchmarks. If we benchmark a call for a parachain runtime we get for the second weight component:

accessed state proof size + extrinisic length

While for the relay chain we would get:

extrinsic length

The limits for a parachain would be the PoV size limit. For the relay chain it would be the maximum block size.

[eskimor]

Does this make sense? At least for parachain consensus we do have a couple of things which can be large in size and we need a proper way to ensure we are not producing blocks exceeding the block size limit.

[ggwpez]

At least for parachain consensus we do have a couple of things which can be large in size and we need a proper way to ensure we are not producing blocks exceeding the block size limit.

With "things" do you mean dispatchables? So you have a point in the code where you have X bytes of space left and want to know whether or not to Dispatch something without overflowing that size limit?

The extrinsic length is known by calling encoded_len on it. You can even do this before dispatching and then adding the PoV weight yourself.
I dont get how benchmarking is neccecary. This way is much better than benchmarking, since it is exact.

[eskimor]

Yes it would not be benchmarked, the generated code would just call encoded_len. It would just be part of the benchmarking code gen, not benchmarking itself. Anyhow, in my mind it still makes sense to have it there, but we will start by wrapping WeightInfo calls manually for now and see how it goes.

[ggwpez]

Okay that makes more sense, thanks.

Anyhow, in my mind it still makes sense to have it there, but we will start by wrapping WeightInfo calls manually for now and see how it goes.

I dont think there is a good way around this though. Even if we include it in the WeightInfo output: Dispatchables are not MaxEncodedLen bound. So now we would have to write benchmarks not only in a way that consumes the most resources, but also to hit the maximal possible length. Dont know how this should be done for calls that accept a Vec.
The wrapping approach is superior in my opinion, since you get the exact length. Not some benchmarking approximation.
But it is un-ergonomic… maybe we can get around this with some macros. Would have to see the usage first.

[eskimor]

Oh shoot, I remembered wrongly. Indeed we are already using a wrapper function that takes the actual argument and not the u32 parameters. Indeed since all we have is the u32 values in the WeightInfo functions, we would need to benchmark, which is kinda stupid.

I think I will just override the second weight component in the wrapper function, since we don't care about proof size anyway. Feels a bit hacky, but let's see how it goes.

[librelois]

I found this issue while thinking about how to manage Weight v2 in frontier and moonbeam.

I think the optimal solution is to have two different CheckWeight implementations for the sovereign chain and for the parachains.
Since the transition to Weight v2 will be mainly to manage the PoV limit for parachains, it seems to me more natural and less breakable to adapt the current susbtrate CheckWeight for the sovereign chain, and to create a new CheckWeight (maybe in cumulus) dedicated to the parachains. Thus, only the parachains need to adapt.

The solution which would consist in integrating the size of the tx in the benchmarks does not suit us for frontier, because in frontier we not have benchmarks (we accounting resources usage dynamically).

[librelois]

Seem's to be fixed by #4326

[bkchr]

Yes! :D Ty @librelois for reminding me.