Adding CallFilter to calling pallets

[bugrazoid]

Description

When using pallets that can make calls to other pallets, having a mechanism to filter out these calls would be very helpful.

Motivation

It may be necessary to filter out unwanted calls with unwanted effects. An example of such filtering is already present in pallet_utility::batch_all which does not allow itself to be called. pallet_utility::batch is often used by a Scheduler, which executes the items in its agenda in on_initialize. Strict limitation and predictability of runtime are critical. Otherwise, if a dynamically-sized call that’s consistently taking longer than what the execution allows gets on the agenda, the network will come to a STOP since every new attempted block is getting rejected. Because of this, we need a filtering mechanism in pallet_utility::batch to disallow dynamically-timed calls. In a more general but similar case, this will help when using Democracy to avoid calling potentially dangerous extras, such as pallet_evm::call, which can cause the block to consistently overflow on the on_initialize method when called through pallet_scheduler, thereby stopping the network.

Solution

Adding CallFilter into pallet_utility (PR: https://github.com/paritytech/substrate/pull/13523) and pallet_scheduler (PR: https://github.com/paritytech/substrate/pull/13527)

[bkchr]

if a dynamically-sized call that’s consistently taking longer than what the execution allows gets on the agenda, the network will come to a STOP since every new attempted block is getting rejected.

This is already the problem. Calls should not be dynamically sized. The scheduler is also ensuring using weights that a call isn't too heavy to be executed.

[CertainLach]

In an ideal world, calls should not be dynamically sized. But currently, pallets such as frontier's pallet-evm have poor support for benchmarking because their weight is based on gas and not actual weight. In the case of pallet-evm, no direct mapping is possible (Weight is measured in time and PoV size, but gas is measured in good ol' imperial units, chosen arbitrarily (Through RFC process) by Ethereum developers), and it doesn't even support PoV benchmarking yet. Even worse, if your chain supports calling substrate methods from evm, as this weird mapping (weight <=> gas) is executed twice, and the PoV part of the weight is being garbled.

In the future, everything will be implemented correctly. Still, it would be better to prevent those calls from being executed in on_initialize to prevent network stalls caused by PoV size overflow, we can at least block extrinsic calls.

[xlc]

https://github.com/paritytech/polkadot-sdk/issues/206 this is the proper solution.

[bugrazoid]

paritytech/polkadot-sdk#206 this is the proper solution.

We are aware of it, and it would be a good solution, but it's far from being finished. However, in the meanwhile, the problem can be readily resolved with this PRs which also allows for more flexible behavior.

[bkchr]

But who/what is adding stuff to the scheduler? The scheduler should not be accessible by normal users.

[bugrazoid]

But who/what is adding stuff to the scheduler? The scheduler should not be accessible by normal users.

It could be Democracy, it could be anything else with root origin. In the case of Democracy, a reckless or malicious user with enough careless backing could bring down the whole chain irreversibly - an unlikely outcome, however, certainly possible. It is important to highlight that if we are dealing with potentially chain-breaking stuff, it's best to increase odds of defense against it as much as possible.

And this filtering, of course, remains optional for those who'd like to use it, and gives no additional problems to those who don't.

[xlc]

For Democracy case people can always propose a malicious wasm and filter cannot prevent such case.

[bugrazoid]

You are correct! :) I'm not saying the filter is a cure-all for all pallets. But it is still able to help us and others. In Democracy, a user could propose an incorrect wasm and a faulty scheduled evm-call - however, with this filter we'd at least be able to sift out 50% of the problems rather than 0%, so to speak. The crux of this particular use-case is still in the combination of the scheduler and evm pallets, and the lack of sufficient safety checks that evm would warrant in scheduler's on_initialize. Whoever has access to the scheduler, this filter would help. There are surely more use-cases where this filter is beneficial and is the solution to other chains' problems.

[xlc]

It doesn't actually changes anything. For malicious attack, close 100 out of 101 door is the same as have all the doors open. The malicious person will just use the open door to walk in. For other scenario, if it is possible to break your chain by luck, it will be used by exploiters so there is no other scenario.

[xlc]

I am not against this (although I also don't support this). I just want to point out this doesn't increase the security with the example you provided. It can have positive effect but if you don't list them and I can't think of them, it is non existent for me.

[juangirini]

@bugrazoid could you please share a detailed example of an attack that this should prevent? cc @KiChjang @ggwpez