Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure parachain blocks are taking the PVF validation memory limit into account #71

Open
bkchr opened this issue May 26, 2023 · 4 comments
Labels
I5-enhancement An additional feature request.

Comments

@bkchr
Copy link
Member

bkchr commented May 26, 2023

When relay chain validators validate a PVF the wasm execution gets a certain amount of memory allocated. If a collator is building a block while using a higher memory limit it may fails at validation. Thus, we should ensure that a honest collator respects these memory limits set on the relay chain. We actually should run at 50% (number is discuss-able) of the memory limit set for the validation. The reason behind this is that the validation needs to keep more stuff in memory (the entire POV, data that is written etc) as while building the block.

The memory limit is stored in the ExecutorParams.

@fgamundi
Copy link
Contributor

Hi. We currently have an on-chain heap pages value that is too high (0xed5c416273747261), producing the error shown in paritytech/substrate#14364 (we avoid it by omitting then on-chain at the client). We are interested in having an upstream permanent fix for it. Given that the proposed Substrate fix isn't valid, can you please guide me on what a more proper solution would be, that also addresses this issue? Thanks

@bkchr
Copy link
Member Author

bkchr commented Jun 15, 2023

The full solution will require some more work which I currently can not real foresee. I have some ideas, but nothing concrete. However, to move forward you could introduce a pr to Substrate that is adding a method to WasmExecutorBuilder ignore_on_chain_heap_pages(). Then internally in the executor if this is set, it just ignores the heap pages set on chain.

@bkchr
Copy link
Member Author

bkchr commented Jun 15, 2023

For the proper solution as outlined in this issue here, we will require more work to support setting the heap pages per instance (which currently doesn't work that good, as we cache instances based on the heap pages). Then we need support on the upper layers (runtime api) to forward the requested heap pages down to the executor. However, I'm still preparing the changes to the runtime api to make this possible. If we have this stuff, we then need to think on how to forward this information to the block builder to respect the heap pages.

@fgamundi
Copy link
Contributor

fgamundi commented Jul 5, 2023

Thanks for the reply! I've created a new PR paritytech/substrate#14508 following your suggestion

@the-right-joyce the-right-joyce transferred this issue from paritytech/cumulus Aug 24, 2023
@the-right-joyce the-right-joyce added I5-enhancement An additional feature request. and removed J0-enhancement labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request.
Projects
Status: backlog
Development

No branches or pull requests

3 participants