Set PoV size limit to 10 Mb #5884
Conversation
s0me0ne-unkn0wn
requested review from
eskimor,
bkchr,
dmitry-markin,
acatangiu and
sandreim
|
After a new relay chain runtime version with this value is released and the runtime upgrade is performed, it will theoretically allow for a 10 Mb PoV size, but the on-chain limit will still remain at 5 Mb. Not only the chain also candidate validation will reject it. For the first part: Why don't we already read the initial value from the relay chain config? |
You mean in genesis? I suppose because we treat the genesis state as a static state, and we don't want to make requests to the relay chain when building a chainspec? |
| @@ -426,7 +426,7 @@ pub const MAX_HEAD_DATA_SIZE: u32 = 1 * 1024 * 1024; | |||
| /// * checking updates to this stored runtime configuration do not exceed this limit | |||
| /// * when detecting a PoV decompression bomb in the client | |||
| // NOTE: This value is used in the runtime so be careful when changing it. | |||
| pub const MAX_POV_SIZE: u32 = 5 * 1024 * 1024; | |||
| pub const MAX_POV_SIZE: u32 = 10 * 1024 * 1024; | |||
There was a problem hiding this comment.
Did we validate this works e2e, like have a parachain that generates 10MiB PoV and confirm parachain blocks work at 6s rate with all blocks included on chain ?
There was a problem hiding this comment.
It kind of makes sense to add a glutton test with these 10MB PoVs. Later we should do a more scaled up test on Versi.
There was a problem hiding this comment.
We haven't validated anything yet (beyond running local zombienets to ensure 10 Mb PoVs work in principle). The purpose of this PR is to gather opinions and concerns, we're not merging it before we do a lot of tests anyway.
There was a problem hiding this comment.
beyond running local zombienets to ensure 10 Mb PoVs work in principle
That's what I was asking for :D.
There was a problem hiding this comment.
The purpose of this PR is to gather opinions and concerns, we're not merging it before we do a lot of tests anyway.
But the description of the PRs and my own reasoning tells me that we can merge it. PoV should still be limited by the relay runtime config value at 5MB. To have 10MB in practice you need governance to bump the chain limit and also parachain runtimes to be adjusted.
Okay, enough theory, let's kick it off. Closes #5334.
What are the implications from my point of view right now:
After this PR is merged
All the new chains (including test ones) started from genesis will support 10 Mb PoVs.
If some parachain updates to the SDK version containing this value and then upgrades its runtime, the runtime will allow for 10 Mb PoVs while the relay chain still has a 5 Mb limit. I don't think it's a problem because the collator building a block will be limited by the PoV size limit stored in the persistent validation data (or even half of that limit if the
full-pov-sizefeature is not enabled). That may lead to overshooting some per-dispatch-class limits but not the absolute PoV size limits, which is safe. Still, a malicious collator can build a block that the relay chain will reject. I'm not 100% sure if we need to take care of such a thing as a "malicious collator". If that's a concern, the possibility of temporarily using another constant for parachains is discussible.After this change is propagated to the fellowship runtimes
After a new relay chain runtime version with this value is released and the runtime upgrade is performed, it will theoretically allow for a 10 Mb PoV size, but the on-chain limit will still remain at 5 Mb.
After governance changes the
max_pov_sizeconfig valueEverything supports 10 Mb PoVs now.
@acatangiu please comment on implications for bridges, I see it's used in bridges code, but not sure if any special treatment is needed.