Check executor params coherence #1774
Conversation
There was a problem hiding this comment.
Thanks for the contribution, it's highly appreciated! Left some comments on how to improve it.
@eskimor @sandreim @slumber we need some brainstorming here. We want to limit values of backing and approval timeouts that could be set via ExecutorParams to some reasonable range, that is, to forbid setting a value that breaks things. Technically, for backing timeout, we could limit from 0 to block_time, and that's good enough to rule out obvious errors in configuration. Practically, I'd like to have more reasonable values, as setting the backing limit to 0, or, say, 0.01 sec, definitely breaks everything. My first guess is to limit it from 1/12 blocktime to 1/2 blocktime but I may not have a solid enough understanding of underlying logic, especially after async backing has landed. Do you have better ideas?
|
In the mean time, any good next issue I could work on? |
|
/tip small |
|
@mrcnski A small (4 KSM) tip was successfully submitted for @eagr (FvpsvV1GQAAbwqX6oyRjemgdKV11QU5bXsMg9xsonD1FLGK on kusama). https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama-rpc.polkadot.io#/referenda |
| @@ -130,6 +182,126 @@ impl ExecutorParams { | |||
| } | |||
| None | |||
| } | |||
|
|
|||
| /// Check params coherence. | |||
| pub fn check_consistency(&self) -> Result<(), ExecutorParamError> { | |||
There was a problem hiding this comment.
Can we have this logic right in configuration.rs? This file seems to be mainly defining a bunch of types with not much actual logic.
There was a problem hiding this comment.
Yeah, it would be nice to if we could put the checks in one place. But that may require opening up the internal of ExecutorParams, do you think it's worth it? @mrcnski
There was a problem hiding this comment.
Not sure what you mean exactly, but I think it's fine.
There was a problem hiding this comment.
Sorry, I was thinking that we might need to make the internal state public in order to do the checks in another crate, like pub struct ExecutorParams(pub Vec<ExecutorParam>). Then it's possible that other crates may rely on the internal state which is not great.
I just realized that it comes with an iterator :)
But still, for the sake of separation of concerns, I think it wouldn't be a bad idea that the type does its own checks. Like, what if the checks are needed else where (other than configurations.rs)?
| sem.heap_alloc_strategy = | ||
| HeapAllocStrategy::Dynamic { maximum_pages: Some(*max_pages) }, | ||
| sem.heap_alloc_strategy = HeapAllocStrategy::Dynamic { | ||
| maximum_pages: Some((*max_pages).saturating_add(DEFAULT_HEAP_PAGES_ESTIMATE)), |
There was a problem hiding this comment.
Nice! But can we just use the value from sem instead?
There was a problem hiding this comment.
I think we discussed it here
There was a problem hiding this comment.
Oh I misread this code, nevermind. Should it be this instead though?
| maximum_pages: Some((*max_pages).saturating_add(DEFAULT_HEAP_PAGES_ESTIMATE)), | |
| maximum_pages: Some((*max_pages).saturating_add(EXTRA_HEAP_PAGES)), |
And are these values correct? I would have thought the extra pages were 32. @s0me0ne-unkn0wn
const DEFAULT_HEAP_PAGES_ESTIMATE: u32 = 32;
const EXTRA_HEAP_PAGES: u32 = 2048;There was a problem hiding this comment.
No, I believe everything is okay with the current version.
EXTRA_HEAP_PAGES is the number of pages of linear memory the instance is allowed to allocate in addition to its initial memory size.
DEFAULT_HEAP_PAGES_ESTIMATE is the number of pages the instance will use for its own purposes, they are not part of linear memory.
So, the corresponding executor parameter defines the number of linear memory pages the instance is allowed to allocate, and here, we implicitly add those auxiliary pages to that limit (for example, we can set the limit to 0, and Wasm code not using any linear memory should still be functional, but it's not possible without having some space for shadow stack and other bits).
There was a problem hiding this comment.
Would it clarify stuff if we name it SHADOW_STACK_HEAP_PAGES? Then current name may be a bit vague.
There was a problem hiding this comment.
It's not just shadow stack, according to the comment 🤔 But I doubt if the reasoning behind the calculation provided is correct enough. I didn't dig deeper into this aspect of Wasmtime, but it seems obvious that data segment pages shouldn't be allocated implicitly. Wasm source must specify the initial memory size, and it should always be enough to accommodate the data segment at least. Considering that I'm not the author of the original constant naming, I'm not 100% sure what is better, but probably renaming DEFAULT_HEAP_PAGES_ESTIMATE to INITIAL_HEAP_PAGES_ESTIMATE would bring more clarity.
| sem.heap_alloc_strategy = | ||
| HeapAllocStrategy::Dynamic { maximum_pages: Some(*max_pages) }, | ||
| sem.heap_alloc_strategy = HeapAllocStrategy::Dynamic { | ||
| maximum_pages: Some((*max_pages).saturating_add(DEFAULT_HEAP_PAGES_ESTIMATE)), |
There was a problem hiding this comment.
No, I believe everything is okay with the current version.
EXTRA_HEAP_PAGES is the number of pages of linear memory the instance is allowed to allocate in addition to its initial memory size.
DEFAULT_HEAP_PAGES_ESTIMATE is the number of pages the instance will use for its own purposes, they are not part of linear memory.
So, the corresponding executor parameter defines the number of linear memory pages the instance is allowed to allocate, and here, we implicitly add those auxiliary pages to that limit (for example, we can set the limit to 0, and Wasm code not using any linear memory should still be functional, but it's not possible without having some space for shadow stack and other bits).
|
Thank you guys for spending the time to review! Is this good to go now? |
|
@eagr Our CI was recently updated, so some checks here were failing. I merged in master, once CI succeeds we should be good! |
* master: (54 commits) Publish `xcm-emulator` crate (#1881) Adding migrations to clean Rococo Gov 1 storage & reserved funds (#1849) Arkworks Elliptic Curve utils overhaul (#1870) Fix typos (#1878) fix: GoAhead signal only set when runtime upgrade is enacted from parachain side (#1176) Refactor staking ledger (#1484) Paired-key Crypto Scheme (#1705) Include polkadot version in artifact path (#1828) add link to rfc-0001 in broker README (#1862) Discard `Executor` (#1855) Macros to use path instead of ident (#1474) Remove clippy clone-double-ref lint noise (#1860) Refactor alliance benchmarks to v2 (#1868) Check executor params coherence (#1774) frame: use derive-impl for beefy and mmr pallets (#1867) sc-consensus-beefy: improve gossip logic (#1852) Adds instance support for composite enums (#1857) Fix links to implementers' guide (#1865) Disabled validators runtime API (#1257) Adding `try_state` hook for `Treasury` pallet (#1820) ...
…ribution * tsv-disabling-backing: (54 commits) Publish `xcm-emulator` crate (#1881) Adding migrations to clean Rococo Gov 1 storage & reserved funds (#1849) Arkworks Elliptic Curve utils overhaul (#1870) Fix typos (#1878) fix: GoAhead signal only set when runtime upgrade is enacted from parachain side (#1176) Refactor staking ledger (#1484) Paired-key Crypto Scheme (#1705) Include polkadot version in artifact path (#1828) add link to rfc-0001 in broker README (#1862) Discard `Executor` (#1855) Macros to use path instead of ident (#1474) Remove clippy clone-double-ref lint noise (#1860) Refactor alliance benchmarks to v2 (#1868) Check executor params coherence (#1774) frame: use derive-impl for beefy and mmr pallets (#1867) sc-consensus-beefy: improve gossip logic (#1852) Adds instance support for composite enums (#1857) Fix links to implementers' guide (#1865) Disabled validators runtime API (#1257) Adding `try_state` hook for `Treasury` pallet (#1820) ...
closes #1704
Really appreciate the help and patience from @s0me0ne-unkn0wn 🙏
I left some unsolved questions in the FIXME comments, would you take a look?
kusama address: FvpsvV1GQAAbwqX6oyRjemgdKV11QU5bXsMg9xsonD1FLGK