feat(vm): limit globals per contract#16047
Conversation
The VM currently has not explicit limit on declared Wasm globals in a contract. However, `MAX_CORE_INSTANCE_SIZE = 1MiB` acts as an implicit limit today. This PR sets the limit to 100k globally, which would add 1.6MB to the core instance size, thus it is less restrictive than the existing limit. A follow-up PR will make this limit stricter and combine it with other limits that contribute to MAX_CORE_INSTANCE_SIZE. But the goal is to introduce each limit in a separate PR before combining them.
|
Fyi, next, I have one more commit ready that adds an explicit limit for the number of escaped functions. After that, all limits are in place to capture all errors in the prepare phase rather than in the loading phase. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #16047 +/- ##
==========================================
+ Coverage 73.19% 73.21% +0.01%
==========================================
Files 856 856
Lines 188394 188415 +21
Branches 188394 188415 +21
==========================================
+ Hits 137904 137943 +39
+ Misses 46070 46049 -21
- Partials 4420 4423 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ssavenko-near
left a comment
ssavenko-near
left a comment
There was a problem hiding this comment.
So what is the issue with letting the loading fail?
I am still not convinced by the value of repeating the internal compiler limits just to avoid it failing in the load phase. We should still be prepared for loading to fail, there's no contract/promise that the phase succeeds TBMK. We probably do not want to keep these limits in sync with upstream, introduce new ones, remove outdated, adjust the values, etc...
Hm, so before we switched to Wasmtime, there were no loading errors. But I guess you are right to challenge the assumption that we need to preserve that. But I think it is actually less about when we surface the error (preparation vs loading) and more about controlling what is accepted as valid code. I expect we will regularly update Wasmtime and if that includes loading errors we don't control, it may cut us in two ways.
By maintaining explicit limits in our code base, together with some tests that check they are in line with Wasmtime's limit, we can be sure we don't accidentally pull in such changes. My more general opinion is that we actually need to keep understanding compiler internals, whether we make it explicit in our code or not. Near offers what I call a "remote compilation service" and hence Near engineers should be deeply familiar with this compilation process. But I'm curious to hear other opinions, @ssavenko-near, @darioush WDYT? |
2 participants
The VM currently has not explicit limit on declared Wasm globals in a contract. However,
MAX_CORE_INSTANCE_SIZE = 1MiBacts as an implicit limit today.This PR sets the limit to 100k globally, which would add 1.6MB to the core instance size, thus it is less restrictive than the existing limit.
A follow-up PR will make this limit stricter and combine it with other limits that contribute to MAX_CORE_INSTANCE_SIZE. But the goal is to introduce each limit in a separate PR before combining them.