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

Bump authoring duration for async backing to 2s. #5195

Merged
merged 5 commits into from
Aug 12, 2024
Merged

Bump authoring duration for async backing to 2s. #5195

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1902a85
Bump authoring duration for async backing to 2s.
Jul 31, 2024
a19f6c8
Add prdoc
Jul 31, 2024
80a8094
Update service file as well.
Jul 31, 2024
fe00b62
Add affected crates
Jul 31, 2024
2eb92ef
Merge branch 'master' into rk-authoring-duration-2000
eskimor Aug 1, 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
4 changes: 2 additions & 2 deletions docs/sdk/src/guides/async_backing_guide.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@
//! - In the `para_client` field, pass in a cloned para client rather than the original
//! - Add a `para_backend` parameter after `para_client`, passing in our para backend
//! - Provide a `code_hash_provider` closure like that shown below
//! - Increase `authoring_duration` from 500 milliseconds to 1500
//! - Increase `authoring_duration` from 500 milliseconds to 2000
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make it clear how this value is related to the configuration on the relay chain.

//! ```ignore
//! let params = AuraParams {
//! ..
Expand All @@ -185,7 +185,7 @@
//! client.code_at(block_hash).ok().map(|c| ValidationCode::from(c).hash())
//! },
//! ..
//! authoring_duration: Duration::from_millis(1500),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this one needs to be increased as well: https://github.com/paritytech/polkadot-sdk/blob/master/cumulus/polkadot-parachain/src/service.rs#L835

Right @s0me0ne-unkn0wn ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Westend still has 2sec backing timeout, so we should bump it on Westend first, and then we may bump it there.

It also should be considered that this change would affect all the system parachains as they're using polkadot-parachain to run collators, so, although it should be safe to bump it, I'd try it on our collators first, just in case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be fine, AFAIK nobody is actually fully saturating the execution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, some runtimes do saturate by design (like HydraDX) so they may start having problems on Westend. On the other hand, I doubt if they're using polkadot-parachain to collate 🤔
Overall, yes, should be totally fine, but one cannot be too paranoid with a change affecting system parachains 🙂

//! authoring_duration: Duration::from_millis(2000),
//! ..
//! };
//! ```
Expand Down
Loading