Add host function that enforces transaction nesting depth

[athei]

The client does not enforce any limit on the nesting depth when the runtime spawns a new storage transaction. Without a limit a user could construct (without other barriers) a Call like batch(batch(batch(write_many_keys))).

We clearly want a limit here. @shawntabrizi and me came to the conclusion that the most efficient solution would be to fuse this operation with when we are calling into the client to create a new transaction anyways.

This adds:

/// Start a new nested transaction while enforcing a maximum nesting depth.
///
/// This function is equal to [`Self::start_transaction`] with the exception that it
/// will return an error then it would otherwise violate the supplied `max_depth`. The
/// parameter describes the maximum allowed nesting depth.
fn start_transaction_with_limit(&mut self, max_depth: u32) -> Result<(), ()> {
	self.storage_start_transaction_with_limit(max_depth)
}

Needed for #10806

[bkchr]

Why not enforce this in the runtime?

[athei]

So there is a PR that does this but it uses runtime storage to hold the current depth which would add two host functions calls for every transaction spawned: #10808

Another way would be to somehow leverage the environmental crate to hold this global state of the current nesting depth within the runtime. But there is no precedence for using this within the runtime.

[bkchr]

So there is a PR that does this but it uses runtime storage to hold the current depth which would add two host functions calls for every transaction spawned: #10808

Yeah that is what confused me ;) Good point about the two host calls.

Another way would be to somehow leverage the environmental crate to hold this global state of the current nesting depth within the runtime. But there is no precedence for using this within the runtime.

I can help with this if wanted, should not be that hard. environmental is for example used by Parachains in their runtime already.

I first had some problem with this because they client could just ignore the max_depth argument, but yeah then other nodes would fail on import of the block. So, this should be fine.

The only real difference between this approach and the environmental approach would be that releasing a new host functions is a "little bit" more complicated. (Because parachain teams need to upgrade their stuff before etc).

[athei]

Yes if you could make it work with environmental within the runtime that would be preferable so we can save us some hassle.

The main challenge would be to find the correct location to put the environmental::using. I guess you need to wrap all entry points into the wasm blob with it?

[bkchr]

The main challenge would be to find the correct location to put the environmental::using. I guess you need to wrap all entry points into the wasm blob with it?

Exactly.

Here should be the position you are searching for: https://github.com/paritytech/substrate/blob/master/frame/support/procedural/src/construct_runtime/expand/call.rs#L122-L127

[bkchr]

Here should be the position you are searching for: https://github.com/paritytech/substrate/blob/master/frame/support/procedural/src/construct_runtime/expand/call.rs#L122-L127

Okay, maybe not the best position, because we can also reach this position if we are running nested transactions. I mean we could check before if there is already a context and reuse this context.

But, we could also add this here: https://github.com/paritytech/substrate/blob/master/primitives/runtime/src/generic/checked_extrinsic.rs#L81

[athei]

Okay, maybe not the best position, because we can also reach this position if we are running nested transactions. I mean we could check before if there is already a context and reuse this context.

I think this is where Shawn put it in his first test and it failed for this reason.

But, we could also add this here: https://github.com/paritytech/substrate/blob/master/primitives/runtime/src/generic/checked_extrinsic.rs#L81

This seems like a better position. I can't really tell if this is the only position dispatchables can be called from, though.

[tomaka]

which would add two host functions calls for every transaction spawned: #10808

I just want to point out that we do tons of unnecessary host function calls. Every storage access calls the twox host function twice, even though it could be computed at compile time, and every single memory allocation in the runtime (and there are tons of memory allocation in the runtime) does a call to malloc and (hopefully) a call to free. If reducing the number of host function calls is an objective, there are tons of low hanging fruits.

I don't think that we have any policy regarding new host functions, but adding a new host function is a huge commitment. Host functions, once added, can never ever be removed. Adding a new host function just because it "seems" like an overhead, without this overhead even being measured, seems a bit eager to me.

[athei]

I agree. We should have a formal process around adding new host functions. And for this PR especially I think it should be solved within the runtime.