Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Expose WASM bulk memory extension in execution environment parameters #7008

Merged
merged 1 commit into from
Apr 5, 2023
Merged

Expose WASM bulk memory extension in execution environment parameters #7008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions node/core/pvf/src/executor_intf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ fn params_to_wasmtime_semantics(par: &ExecutorParams) -> Result<Semantics, Strin
HeapAllocStrategy::Dynamic { maximum_pages: Some(*max_pages) },
ExecutorParam::StackLogicalMax(slm) => stack_limit.logical_max = *slm,
ExecutorParam::StackNativeMax(snm) => stack_limit.native_stack_max = *snm,
ExecutorParam::WasmExtBulkMemory => sem.wasm_bulk_memory = true,
ExecutorParam::PrecheckingMaxMemory(_) => (), // TODO: Not implemented yet
ExecutorParam::PvfPrepTimeout(_, _) | ExecutorParam::PvfExecTimeout(_, _) => (), // Not used here
}
Expand Down
3 changes: 3 additions & 0 deletions primitives/src/v4/executor_params.rs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v4 is what is out now right? What happens if some nodes start using the new param and other nodes can't decode? IIRC it will throw an error but is it fatal?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory yes, but in practice the array of params is hardcoded to be empty, hence safe to change

polkadot/runtime/parachains/src/session_info.rs

Lines 42 to 44 in dc230b3

// The order of parameters should be deterministic, that is, one should not reorder them when
// changing the array contents to avoid creating excessive pressure to PVF execution subsys.
const EXECUTOR_PARAMS: [ExecutorParam; 0] = [];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v4 is what is out now right? What happens if some nodes start using the new param and other nodes can't decode? IIRC it will throw an error but is it fatal?

We've been discussing that for some time already, the last discussion was here: paritytech/substrate#13811 (comment).
In short, we won't bump the API version every time something changes in ExecutorParams. Instead, we allow validators with an old node implementation to fail to decode ExecutorParams, resulting in a no-show in pre-checking and execution. But we must be sure it doesn't result in a dispute.
For now, as @slumber correctly noted, we don't have a chance to fall into that situation as ExecutorParams are empty.

Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ pub enum ExecutorParam {
/// PVF execution timeouts, millisec
#[codec(index = 6)]
PvfExecTimeout(PvfExecTimeoutKind, u64),
/// Enables WASM bulk memory proposal
#[codec(index = 7)]
WasmExtBulkMemory,
}

/// Unit type wrapper around [`type@Hash`] that represents an execution parameter set hash.
Expand Down