pallet-revive: Raise contract size limit to one megabyte and raise call depth to 25 #9267
Conversation
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
|
/cmd bench --runtime dev --pallet pallet_revive |
|
Command "bench --runtime dev --pallet pallet_revive" has started 🚀 See logs here |
…--pallet pallet_revive'
|
Command "bench --runtime dev --pallet pallet_revive" has finished ✅ See logs here Subweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
| /// Maximum size of events (including topics) and storage values. | ||
| pub const PAYLOAD_BYTES: u32 = 416; | ||
|
|
||
| /// The maximum size for calldata and return data. |
There was a problem hiding this comment.
IIRC this value matches the current practical limit on Ethereum. Might point that out here?
|
Resolved merge conflicts and updates to the latest version of paritytech/polkavm#316. We will make use of PolkaVMs memory estimation now instead of baking in assumptions into the pallet. Next up: Will try to attack this: Creating a contract that tries to maximize memory usage and call it recursively. Cannot be written as a unit test as those run inside the native runtime. |
|
/cmd fmt |
athei
changed the title
|
Hey @athei ! I have a few planning question related to this work.
If it comes to local allocator usage we got ~55MB peak requested space and ~56MB worth of physical memory used, so slightly better than the host allocator. However, host allocator is doing a good job too. I would not consider them in the danger zone for this heavy contract, and I would say things look good either way, and the above checkbox can be checked if you feel this testing scenario suffices for now.
LMK your thoughts. |
I agree. The wasted space on padding is
If you are logging from the allocator you might just built an infinite loop. Because the logging also allocates.
You should do all your experiments with the default log level. This is because the logging does allocate and it is not representative of the actual memory allocations. AFAIK critical nodes are run without any logging just to be safe. A bit more context: When you log from the runtime the filtering is done at the host. The runtime just requests the maximum log level independent of the target (it uses whatever the host uses). This means turning on debug for any target will make the runtime emit all the debug logs for all the targets. Even though the host will discard most of them you still caused a shit ton of logs (and hence allocations) inside the runtime. This is especially bad for contracts since the PolkaVM debug logs are very verbose. |
|
/cmd prdoc --audience runtime_dev --bump patch |
…time_dev --bump patch'
|
/cmd bench --runtime dev --pallet pallet_revive |
|
Command "bench --runtime dev --pallet pallet_revive" has started 🚀 See logs here |
…--pallet pallet_revive'
|
Command "bench --runtime dev --pallet pallet_revive" has finished ✅ See logs here Subweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
| fn call_with_code_per_byte( | ||
| c: Linear<0, { limits::code::STATIC_MEMORY_BYTES / limits::code::BYTES_PER_INSTRUCTION }>, | ||
| ) -> Result<(), BenchmarkError> { | ||
| fn call_with_code_per_byte(c: Linear<0, { 100 * 1024 }>) -> Result<(), BenchmarkError> { |
There was a problem hiding this comment.
why 100 * 1024 and not BLOB_BYTES?
There was a problem hiding this comment.
This gets really slow. And since it scales linearly we don't need to test with the biggest blob. Slows down running tests.
…ll depth to 25 (#9267) This PR changes the contract code limit from roughly 100KiB to exactly 1MiB. It also raises the call stack depth from 5 to 25. Those limits were in place because of memory constraints within the runtime. We work around them in those ways: 1) Removing the 4x safety margin for allocations which is no longer needed due to the new allocator. 2) Limiting the size of the compilation cache to a fixed size. 3) Resetting the compilation cache and flat map every time we call into a new contract. 4) Limiting the size of calldata and return data to 128KiB (only capped by tx size and RAM before). While this is a breaking change nobody will be affected since Geth effectively limits the call data to 128KiB. This is large enough so that all known contracts won't fail for size issues anymore. The new limit is also much simpler to understand since it does not depend on the number of instructions. Just those two constraints: ``` PVM_BLOB.len() < 1 MiB PVM_BLOB.len() + (rw/ro/stack) < 1MiB + 512KiB ``` This means: 1) A contract is guaranteed to have at least 512KiB of memory available. 2) A contract that is smaller in code can use more memory. 3) Limit is exactly 1MiB unless a user manually increase the memory usage of a contract to be larger than 512KiB. The limit of `5` was problematic because there are use cases which require deeper stacks. With the raise to `25` there should be no benign use cases anymore that won't work. Please note that even with the low limit of `25` contracts are not vulnerable to stack depth exhaustion attacks: We do trap the caller's context when the depth limit is reached. This is different from Eth where this error can be handled and failure to do so leaves the contract vulnerable. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
- #9112 - #9101 - #9416 - #9357 - #9441 - #9267 Those are all the changes we want to get onto the next Kusama release. The new gas mapping and EVM backend will not make it. --------- Co-authored-by: PG Herveou <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Francisco Aguirre <[email protected]> Co-authored-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: sekiseki <[email protected]> Co-authored-by: Michael Müller <[email protected]>
This PR changes the contract code limit from roughly 100KiB to exactly 1MiB. It also raises the call stack depth from 5 to 25.
Those limits were in place because of memory constraints within the runtime. We work around them in those ways:
1MiB contracts
This is large enough so that all known contracts won't fail for size issues anymore.
The new limit is also much simpler to understand since it does not depend on the number of instructions. Just those two constraints:
This means:
Call stack depth
5 -> 25The limit of
5was problematic because there are use cases which require deeper stacks. With the raise to25there should be no benign use cases anymore that won't work.Please note that even with the low limit of
25contracts are not vulnerable to stack depth exhaustion attacks: We do trap the caller's context when the depth limit is reached. This is different from Eth where this error can be handled and failure to do so leaves the contract vulnerable.