Substrate: --max-heap-pages has no effect

[cmichi]

The --max-heap-pages argument does not seem to have an effect. It exists as an argument (here: https://github.com/paritytech/substrate/blob/master/core/cli/src/params.rs#L216-L218), but the setting is never actually picked up anywhere in the code.

From the discussion in https://github.com/paritytech/substrate/pull/639 I gather that this argument was removed in the past. It seems to have made it back into the code somehow though.

Is it supposed to be implemented or removed?

[bkchr]

If it not used and also not present in the configuration struct, please create a pr to remove it :)

[gavofyork]

Actually, there still is a default maximum which this CLI param should change here:

https://github.com/paritytech/substrate/blob/92c5f50f8424b4aadb165c91e45add9a85e1c79b/core/executor/src/native_executor.rs

[cmichi]

IIUC the code allows to have the number of heap pages also to be set by the runtime, on chain: https://github.com/paritytech/substrate/blob/5fc4ef6abe49c985238c4dd6f6d9724754f91cdb/core/executor/src/native_executor.rs#L66-L69

What should the behavior then be if a node is started with a --max-heap-pages count lower than the one defined on chain?

[gavofyork]

chain takes precedence; this is just about the default.

[cmichi]

Ok, then I suggest to rename the flag to --default-heap-pages, otherwise the naming can be misleading, especially since the description currently reads The maximum number of 64KB pages to ever allocate for Wasm execution..

[stanislav-tkach]

I would like to take a look at this issue. Do I understand correctly that the NativeExecutor and NativeExecutionDispatch constructors should be extended in order to pass this additional parameter?

[cmichi]

Hi @DarkEld3r, I'm also just getting into the codebase, so take it with a grain of salt. But yeah, I took a look at your current PR and that's also the approach I would have gone with.

[stanislav-tkach]

@cmichi Thanks for help!
I have updated the pull request.