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

[DNM] Add offchain executor params #5278

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

s0me0ne-unkn0wn
Copy link
Contributor

Do not merge. Not in this form definitely.

TODO: make allocator distinguish between onchain and offchain mode and have different single allocation limits depending on mode

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6934683

@michalkucharczyk
Copy link
Contributor

related: #5419

@bkchr
Copy link
Member

bkchr commented Oct 16, 2024

Generally I don't see any reason to keep this pr. Will not be merged in this form.

@bkchr bkchr closed this Oct 16, 2024
@s0me0ne-unkn0wn
Copy link
Contributor Author

@bkchr, please do not delete the branch at least. Several developers use it for state-size experiments.

@bkchr
Copy link
Member

bkchr commented Oct 16, 2024

@s0me0ne-unkn0wn as you see I did not delete the branch. Even if I would do it, the pr would still be here.

@s0me0ne-unkn0wn
Copy link
Contributor Author

@bkchr a couple of questions:

  1. How do you feel about merging it without allocator-related changes, that is, keeping only node-side changes with configurable executor parameters for offchain execution? That would be useful by itself, even without bumping allocation limits.
  2. What do you think about actually bumping the allocation limit (in a separate PR) by one order of magnitude, exactly as it's currently done in this PR? What are the drawbacks? Some context here is that when we're experimenting with 10 Mb PoVs, we're getting "you're trying to allocate too much" runtime panics quite often. Currently, it's always connected with pushing the chain over some limit, so no direct danger here, I believe, but generally, I have a feeling that we're approaching the point where 10 Mb PoVs will require us to be able to allocate more than 32 MiB in a single chunk.

@bkchr
Copy link
Member

bkchr commented Dec 23, 2024

None of the changes in the pr are required, if we would concentrate our work on moving the collator into the runtime. Apparently the contracts team wants to do the same and there are also more and more issues around the allocator. So, I hope that there is enough traction that finally someone does it. Also the changes should not be that complicated, just takes time for someone to do it.

@s0me0ne-unkn0wn
Copy link
Contributor Author

But even with the allocator moved to the runtime, we'll still have offchain execution, like when building chainspec, etc. My intent here is to be able to parameterize such executions as well as we're currently able to parameterize onchain execution with executor params. Setting the allocation limit is not the only use case possible.

@bkchr
Copy link
Member

bkchr commented Dec 23, 2024

But even with the allocator moved to the runtime, we'll still have offchain execution, like when building chainspec, etc.

When we have an in-runtime allocator, we will not have a per allocation limit anymore. We will only have an total allocation limit, which is already configurable for offchain and onchain.

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12468775127
Failed job name: fmt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants