fix: use pwasm-utils 0.18 for more correct stack accounting #6051
Conversation
This forward-ports the change from 1.23.1 We upgrade our version of pwasm-utils -- the old one severely undercounted stack usage in some cases. As this is a protocol change, we keep the old implementation around Test Plan ========= * added test for stack overflow behavior for the bad case * added test for protocol upgrade related to stack overflow * fuzzed new pwasm utils implementation locally, confirmed that it only differs for calls which stack overflow
| @@ -128,6 +128,9 @@ pub enum ProtocolFeature { | |||
| /// Make block producers produce chunks for the same block they would later produce to avoid | |||
| /// network delays | |||
| SynchronizeBlockChunkProduction, | |||
| /// Change the algorithm to count WASM stack usage to avoid undercounting in | |||
| /// some cases. | |||
| CorrectStackLimit, | |||
There was a problem hiding this comment.
if we ever need to correct the algorithm again, it'll be hard to come up with a name for it ^^
There was a problem hiding this comment.
This name we can retroactively change in the future, as it is not serialized anywhere, so tried not to over-thing this )
faed89a to
460bfc1
Compare
| @@ -31,6 +31,8 @@ pub struct VMLimitConfig { | |||
| /// See <https://wiki.parity.io/WebAssembly-StackHeight> to find out | |||
| /// how the stack frame cost is calculated. | |||
| pub max_stack_height: u32, | |||
| #[serde(default = "StackLimiterVersion::v0")] | |||
There was a problem hiding this comment.
comment - that we have different stack limiter versions (due to the bugs/issues / different pwasm version etc)
| #[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)] | ||
| pub enum StackLimiterVersion { | ||
| V0, | ||
| V1, |
There was a problem hiding this comment.
can you comment on how they differ?
There was a problem hiding this comment.
Yeah, good call, added some comments.
| @@ -181,7 +184,8 @@ impl ProtocolFeature { | |||
| | ProtocolFeature::LimitContractFunctionsNumber | |||
| | ProtocolFeature::BlockHeaderV3 | |||
| | ProtocolFeature::AliasValidatorSelectionAlgorithm => 49, | |||
| ProtocolFeature::SynchronizeBlockChunkProduction => 50, | |||
| ProtocolFeature::SynchronizeBlockChunkProduction | |||
| | ProtocolFeature::CorrectStackLimit => 50, | |||
There was a problem hiding this comment.
shouldn't we use a new protocol version for this? (or was it already deployed somehow?)
There was a problem hiding this comment.
Yeah, that was deployed as a part of 1.23.1: https://github.com/near/nearcore-ghsa-62f6-rgx9-xcw2/pull/1
| let Type::Function(ref _func_ty) = | ||
| types.get(*type_idx as usize).ok_or(PrepareError::Instantiate)?; | ||
|
|
||
| // TODO: Function type check with Env |
There was a problem hiding this comment.
optional: I'm not a fan of 'orphaned' TODOs ;-) we should either have a github/jira issue that is tracking it (if this is a lot more complex fix), or author name + additional info on why this is complex enough that we're not fixing it in this PR.
| // TODO: Function type check with Env | ||
| /* | ||
|
|
||
| let ext_func = env |
There was a problem hiding this comment.
Yeah, I agree that it's better to just remove wishful-thinking comments from this module. In general, every time I open this file I want to refactor it.
I'd rather not do that in the PR though, as the goal here is to change a bit of the behavior we want to change, and to hold everything else constant.
| // Support for old protocol versions, where we incorrectly didn't | ||
| // account for many locals of the same type. | ||
| // | ||
| // See `test_stack_instrumentation_protocol_upgrade` test. |
There was a problem hiding this comment.
FWIW, there's a cool trick to make such links to tests checkable (ie, the test fails if it doesn't actually touch the relevant code): https://docs.rs/cov-mark/latest/cov_mark/.
I am still on the fence where maintaining such coverage marks helps overall.
This forward-ports the change from 1.23.1 We upgrade our version of pwasm-utils -- the old one severely undercounted stack usage in some cases. As this is a protocol change, we keep the old implementation around Test Plan ========= * added test for stack overflow behavior for the bad case * added test for protocol upgrade related to stack overflow * fuzzed new pwasm utils implementation locally, confirmed that it only differs for calls which stack overflow
This forward-ports the change from 1.23.1
We upgrade our version of pwasm-utils -- the old one severely
undercounted stack usage in some cases. As this is a protocol change, we
keep the old implementation around
Test Plan
differs for calls which stack overflow