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

Restore wasmtime's default stack size limit to 1MB #11993

Merged

Conversation

koute
Copy link
Contributor

@koute koute commented Aug 8, 2022

Fixes #11891

This PR restores wasmtime's default stack size limit to 1MB.

Originally wasmtime's default stack size limit was set to 1MB, however in version 0.35 it was changed to 512 KB for reasons which aren't really relevant to us. This flew under the radar until our friends from the Astar parachain noticed that this broke their ability to sync their parachain from scratch.

So this PR simply restores the original limit.

I've also added two extra tests to more closely enforce this limit. Please note that since they test the exact boundary in theory they may also fail if anything else changes related to how much stack space a single frame takes without us modifying the stack limit. This is intended; we don't want this to change under us without us knowing about it.

(We might have to #[cfg]-gate these test by architecture and/or OS, but I didn't want to blindly do it until I'll see a failing CI job and/or someone complaining that it fails on their machine.)

@koute koute added A0-please_review Pull request needs code review. I3-bug The node fails to follow expected behavior. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Aug 8, 2022
@koute koute requested review from pepyakin and a team August 8, 2022 13:47
client/executor/wasmtime/src/runtime.rs Show resolved Hide resolved
client/executor/wasmtime/src/tests.rs Outdated Show resolved Hide resolved
@koute
Copy link
Contributor Author

koute commented Aug 9, 2022

Hmm... okay, this is interesting. Apparently when you compile with --release (and the tests on our CI run with --release) the maximum call depth is bigger.

We potentially might need to slightly rethink these tests if they end up being too flake-y (e.g. only guarantee the minimum call stack depth, but not enforce the maximum), but I'd still like to try having them as is. If nothing else we can learn from this what factors can affect the maximum call stack depth.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Just one minor note:
IIUC the test checks that STACK_SIZE / FRAME_SIZE stays the same and would not detect if both change in lockstep so that the ratio stays the same.
Anyway this should already detect most cases.

@koute
Copy link
Contributor Author

koute commented Aug 9, 2022

Just one minor note: IIUC the test checks that STACK_SIZE / FRAME_SIZE stays the same and would not detect if both change in lockstep so that the ratio stays the same. Anyway this should already detect most cases.

Correct. This isn't supposed to be truly exhaustive (as that would be a lot more involved); I just wanted to have something so that this doesn't accidentally happen again under our noses ever again.

// existing chain from syncing (if it was effectively decreased)

// We need two limits here since depending on whether the code is compiled in debug
// or in release mode the maximum call depth is slightly different.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, that's good to know. Thanks!

@koute
Copy link
Contributor Author

koute commented Aug 9, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 8e600ee into paritytech:master Aug 9, 2022
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
* Restore `wasmtime`'s default stack size limit to 1MB

* Add extra comments

* Enforce different maximum call depth in release mode

* Split the call depth limit in two
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Restore `wasmtime`'s default stack size limit to 1MB

* Add extra comments

* Enforce different maximum call depth in release mode

* Split the call depth limit in two
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit I3-bug The node fails to follow expected behavior.
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

Cannot sync chain from scratch due to block import error
4 participants