Reduce the inclusion inherent's actual weight if the block is already heavy

[coriolinus]

Closes #1555.

This should give us these behaviors:

• the inclusion inherent is prioritized over standard transactions
• but if it's too heavy, i.e. in case of runtime upgrade, it'll be dropped.

It is my belief that allowing the proposer to just not include
this data won't have any adverse effects: it's equivalent to replacing
them with empty versions of themselves, which the ProvideInherent
impl already does.

[pepyakin]

So the problem I find in the outlined description is that you still don't know what extrinsics will be included. The proposer doesn't take all extrinsics from the pool. Also there is no way to tell what extrinsics will be proposed, at least not until the proposer does it job.

Instead, there is a long line of extrinsics waiting to be included in the block and then the proposer adds one by one keeping an eye on the cumulative consumed weight. After it reaches a certain threshold it tries to shove some more extrinsics. So all in all this boils down to the first approach.

There are two observations that may expand possibly ways to explore on how to tackle this:

1. After the substrate proposer initializes the block, it should already know the amount of weight consumed by the on_initialize and on_finalize. Assuming that this is the source of most heavy (i.e. most weight) work - we could already know the amount of weight the on_initialize+on_finalize bracket will perform.
2. In case, we need to handle some extrinsics (I guess some heavy operational extrinsics) then another observation is that inherents could be moved in the extrinsics list. If so, we could have a custom logic that moves the inclusion inherent after on_initialize and heavy operational/mandatory extrinsics. At this point there is enough information to decide whether we want to skip inclusion or not.

As a side note, this can be also performed on the runtime side, btw. In fact, there is already this (a relevant comment)

[bkchr]

There are two observations that may expand possibly ways to explore on how to tackle this:

1. After the substrate proposer initializes the block, it should already know the amount of weight consumed by the `on_initialize` and `on_finalize`. Assuming that this is the source of most heavy (i.e. most weight) work - we could already know the amount of weight the on_initialize+on_finalize bracket will perform.

We can not know the weight of on_finalize before running it and this is executed after all transaction are applied. However, currently most of the important heavy stuff is done in on_initialize. This means, if we see that on_initialize already eat up most of the block weight, we could just skip the inherent when it is applied. But this should be done from inside the runtime.

2. In case, we need to handle some extrinsics (I guess some heavy operational extrinsics) then another observation is that inherents could be moved in the extrinsics list. If so, we could have a custom logic that moves the inclusion inherent after on_initialize and heavy operational/mandatory extrinsics. At this point there is enough information to decide whether we want to skip inclusion or not.

How should the node know what each extrinsic is doing? If you have some important operational extrinsic, you could just schedule it, so that it is executed before the inherents in the next block. However opertional extrinsics also have more weight budget.

As already said in Riot, I don't see this as anything that should be handled from the runtime. Apply the inherent and the inclusion logic will skip the data in the inherent if we already used too much weight.

[rphmeier]

As already said in Riot, I don't see this as anything that should be handled from the runtime. Apply the inherent and the inclusion logic will skip the data in the inherent if we already used too much weight.

Do you mean that we should alter inclusion_inherent in the runtime to perform this purpose? Not a bad idea, although we will waste block space, potentially without necessity.

[pepyakin]

We can not know the weight of on_finalize before running it and this is executed after all transaction are applied.

Isn't the on_initialize returns weight for both on_initialize and on_finalize? Otherwise, you already get into the problem when a block packed tightly and then there is on_finalize on top.

How should the node know what each extrinsic is doing?

I was thinking about special casing this in the node, if needed.

As already said in Riot, I don't see this as anything that should be handled from the runtime. Apply the inherent and the inclusion logic will skip the data in the inherent if we already used too much weight.

Could you point to that discussion?

[bkchr]

As already said in Riot, I don't see this as anything that should be handled from the runtime. Apply the inherent and the inclusion logic will skip the data in the inherent if we already used too much weight.

Do you mean that we should alter inclusion_inherent in the runtime to perform this purpose?

Exactly.

Not a bad idea, although we will waste block space, potentially without necessity.

Ahh yeah, good point.

[bkchr]

We can not know the weight of on_finalize before running it and this is executed after all transaction are applied.

Isn't the on_initialize returns weight for both on_initialize and on_finalize? Otherwise, you already get into the problem when a block packed tightly and then there is on_finalize on top.

Ohh yeah you are right, somehow I thought this wasn't the case. 🤦

How should the node know what each extrinsic is doing?

I was thinking about special casing this in the node, if needed.

Special casing it in which way? Call into the runtime and get back the weights? Extrinsics are anyway negligible, because the inclusion inherent is in 99% of the cases more important.

[bkchr]

Not a bad idea, although we will waste block space, potentially without necessity.

Ahh yeah, good point.

If we want to prevent this, we could just simply add a runtime api that returns the current and max weight and let you decide on this. While this still doesn't give you the weight that the inherent will consume.

[pepyakin]

Special casing it in which way? Call into the runtime and get back the weights? Extrinsics are anyway negligible, because the inclusion inherent is in 99% of the cases more important.

Perhaps, or just straight hardcoding in the node. But yeah, I agree that this perhaps is not required.

[bkchr]

Hardcode what? Sorry, I can not follow ^^

[gui1117]

Do you mean that we should alter inclusion_inherent in the runtime to perform this purpose? Not a bad idea, although we will waste block space, potentially without necessity.

The waste is if we exclude the InclusionInherent due to a heuristic, when it actually would have fit, isn't it ?

As far as I can see what we want ultimately is to have this inherent prioritised with other extrinsics in the transaction pool. This inherent would be of class normal or operational and priority high, thus in case of runtime upgrade and when block is full then it won't be included. But I don't know how difficult it is.

So yes if we execute this inherent before all other non-inherent extrinsics then we can update check if there is enough weight available, and maybe update the used weight of the block. But then this extrinsic has more priority than operational signed/unsigned extrinsics. But considering this is probably what we want most of the time and that Operationals have some reserved weight amount (weight amount available only for operationals extrinsics), then I think it is fine.

[bkchr]

The waste is if we exclude the InclusionInherent due to a heuristic, when it actually would have fit, isn't it ?

He means waste of block space. Because the inherent would still be part of the block extrinsics. However, the size of this probably not that much.

[gui1117]

I'm confused now, what if we just make the call inclusion operational or normal instead of mandatory.

If we do inclusion as operational, then the weight of the extrinsic will be checked, and the transaction validity will be Err(ExhaustRessource). Such an extrinsic should not be included in the block. and this is what we want.

I'm very not familiar about how the client organize the inherent extrinsics. It doesn't use the priority information but it does check the validity no ?

[coriolinus]

So, if it's operational, then the weight stuff gets much easier, because the normal provisioner takes care of including it or not as space is available; we don't need to mess with it here. However, if it's not included, then we need to get appropriate defaults when we read it.

... I can't think of a reason that isn't a good idea. I'm going to try implementing it.

[bkchr]

Please add tests.

[rphmeier]

Processing availability bitfields is probably heavier than you give it credit for. It's possibly even heavier than processing backing. But we can tweak ratios later on.

[rphmeier]

Needs unit tests and to be tested in practice as well.

[coriolinus]

More tests on the way; I just haven't yet figured out all the details of how to implement tests that the inherent weight changes, or not, according to the block weight before execution.

[coriolinus]

@bkchr Have added tests; please let me know if you would like more.

[drahnr]

The changes are in line with the previous comments 👍