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.

Sign up for GitHub

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

Set PoV size limit to 10 Mb #5884

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Set PoV size limit to 10 Mb #5884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
06108e1
Set PoV size limit to 10 Mb
s0me0ne-unkn0wn Oct 1, 2024
0a9970a
Merge remote-tracking branch 'origin/master' into s0me0ne/max-pov-config
s0me0ne-unkn0wn Nov 28, 2024
f88331e
Fix bridges
s0me0ne-unkn0wn Nov 29, 2024
387f1dc
Merge remote-tracking branch 'origin/master' into s0me0ne/max-pov-config
s0me0ne-unkn0wn Dec 7, 2024
15e91e7
Fix bridges more
s0me0ne-unkn0wn Dec 7, 2024
5452419
Merge remote-tracking branch 'origin/master' into s0me0ne/max-pov-config
s0me0ne-unkn0wn Dec 10, 2024
3095f7f
Restore lost feature propagation
s0me0ne-unkn0wn Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/bridge_to_bulletin_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ parameter_types! {
///
/// It is determined semi-automatically - see `FEE_BOOST_PER_MESSAGE` constant to get the
/// meaning of this value.
pub PriorityBoostPerMessage: u64 = 182_044_444_444_444;
pub PriorityBoostPerMessage: u64 = 364_088_888_888_888;

/// PeopleRococo location
pub PeopleRococoLocation: Location = Location::new(1, [Parachain(rococo_runtime_constants::system_parachain::PEOPLE_ID)]);
Expand Down
2 changes: 1 addition & 1 deletion cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/bridge_to_westend_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ parameter_types! {
// see the `FEE_BOOST_PER_PARACHAIN_HEADER` constant get the meaning of this value
pub PriorityBoostPerParachainHeader: u64 = 1_396_340_903_540_903;
// see the `FEE_BOOST_PER_MESSAGE` constant to get the meaning of this value
pub PriorityBoostPerMessage: u64 = 182_044_444_444_444;
pub PriorityBoostPerMessage: u64 = 364_088_888_888_888;

pub BridgeHubWestendLocation: Location = Location::new(
2,
Expand Down
2 changes: 1 addition & 1 deletion cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/bridge_to_rococo_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ parameter_types! {
// see the `FEE_BOOST_PER_PARACHAIN_HEADER` constant get the meaning of this value
pub PriorityBoostPerParachainHeader: u64 = 1_396_340_903_540_903;
// see the `FEE_BOOST_PER_MESSAGE` constant to get the meaning of this value
pub PriorityBoostPerMessage: u64 = 182_044_444_444_444;
pub PriorityBoostPerMessage: u64 = 364_088_888_888_888;

pub BridgeHubRococoLocation: Location = Location::new(
2,
Expand Down
3 changes: 3 additions & 0 deletions cumulus/polkadot-omni-node/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ substrate-build-script-utils = { workspace = true, default-features = true }

[features]
default = []
full-pov-size = [
"polkadot-omni-node-lib/full-pov-size",
]
runtime-benchmarks = [
"polkadot-omni-node-lib/runtime-benchmarks",
]
Expand Down
3 changes: 3 additions & 0 deletions cumulus/polkadot-omni-node/lib/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ wait-timeout = { workspace = true }

[features]
default = []
full-pov-size = [
"cumulus-client-consensus-aura/full-pov-size",
]
rococo-native = [
"polkadot-cli/rococo-native",
]
Expand Down
3 changes: 3 additions & 0 deletions cumulus/polkadot-parachain/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ substrate-build-script-utils = { workspace = true, default-features = true }

[features]
default = []
full-pov-size = [
"polkadot-omni-node-lib/full-pov-size",
]
runtime-benchmarks = [
"cumulus-primitives-core/runtime-benchmarks",
"parachains-common/runtime-benchmarks",
Expand Down
2 changes: 1 addition & 1 deletion polkadot/primitives/src/v8/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

@alexggh alexggh Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beyond running local zombienets to ensure 10 Mb PoVs work in principle

That's what I was asking for :D.

Copy link
Contributor

@sandreim sandreim Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, okay, I'll put that another way. By this PR, I argue that the given changeset is enough to enable 10 Mb PoVs, and I expect others to argue why it is not. @alexggh's point is that we need more testing before applying the changes, and that's fair. By now, I've finished the local tests with gluttons and will do some stress tests. After that, I'm ready to go to Versi, but I'd like to make it as close to real-life as possible on Versi, that is, to start with a regular network with 5 Mb PoVs and then upgrade it to 10 Mb step by step.

But if the lack of testing is the only argument against merging it in this form, it's beautiful ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexggh's point is that we need more testing before applying the changes, and that's fair.

I was simply asking, because I was curious what tests we've done on this end, the fact that you ran it locally with 10MiB is good enough for me.

I understand that the relay-chain configuration is guarding us against using more than 5MiB, and it is probably safe to merge it without positive evidence that the new maximum is working all the way trough our stack, I was simply trying to avoid the situation where we raise this constant because we know the relay-chain configuration is at 5MiB and it is risk free now, but then 6 months later someone else comes without this context and raises the relay-chain configuration to 10MiB because MAX_POV_SIZE is already 10MiB and it must be safe :D.


/// Default queue size we use for the on-demand order book.
///
Expand Down
Loading