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

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

Merged
merged 5 commits into from
Aug 12, 2024

Conversation

eskimor
Copy link
Member

@eskimor eskimor commented Jul 31, 2024

Should be safe on all production network.

I noticed that Paseo needs to be updated, it is lacking behind in a couple of things.

Execution environment parameters should be updated to those of Polkadot:

[
      {
        MaxMemoryPages: 8,192
      }
      {
        PvfExecTimeout: [
          Backing
          2,500
        ]
      }
      {
        PvfExecTimeout: [
          Approval
          15,000
        ]
      }
    ]
  ]

@@ -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.

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 🙂

@@ -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 it should be fine, AFAIK nobody is actually fully saturating the execution.

@@ -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.

@eskimor eskimor added the T9-cumulus This PR/Issue is related to cumulus. label Jul 31, 2024
@hbulgarini
Copy link
Contributor

Hi! thank you very much for reporting this.

We have already applied this change in our internal test instance and we will be doing in Paseo very soon.

paseo-network/runtimes#111

@sandreim
Copy link
Contributor

The executor params are now set on westend

@hbulgarini
Copy link
Contributor

Hi! The executor params were also applied in Paseo.

@bkchr bkchr added this pull request to the merge queue Aug 12, 2024
Merged via the queue into master with commit 819a581 Aug 12, 2024
160 of 162 checks passed
@bkchr bkchr deleted the rk-authoring-duration-2000 branch August 12, 2024 21:06
ordian added a commit that referenced this pull request Aug 14, 2024
* master: (35 commits)
  Fix OurViewChange small race (#5356)
  Make ticket non-optional and add ensure_successful method to Consideration trait (#5359)
  [tests] dedup test code, add more tests, improve naming and docs (#5338)
  Stop running the wishlist workflow on forks (#5297)
  Migrate foreign assets v3::Location to v4::Location (#4129)
  Minor clean up (#5284)
  [Pools] Ensure members can always exit the pool gracefully (#4998)
  StorageWeightReclaim: set to node pov size if higher (#5281)
  [Bot] Add prdoc generation (#5331)
  Small nits found accidentally along the way (#5341)
  Create subsystem-benchmarks.yml (#5325)
  Bump libp2p-identity from 0.2.8 to 0.2.9 (#5232)
  Bump authoring duration for async backing to 2s. (#5195)
  Fix spelling issues (#5206)
  Bump the known_good_semver group across 1 directory with 3 updates (#5315)
  `polkadot-node-core-pvf-common`: Fix test compilation error (#5310)
  ci: Paused `cmd-action` commenter (#5287)
  Remove unnecessary mut (#5318)
  chain-spec: minor clarification on the genesis config patch (#5324)
  fix av-distribution Jaeger spans mem leak (#5321)
  ...
ordian added a commit that referenced this pull request Aug 14, 2024
* master: (35 commits)
  Fix OurViewChange small race (#5356)
  Make ticket non-optional and add ensure_successful method to Consideration trait (#5359)
  [tests] dedup test code, add more tests, improve naming and docs (#5338)
  Stop running the wishlist workflow on forks (#5297)
  Migrate foreign assets v3::Location to v4::Location (#4129)
  Minor clean up (#5284)
  [Pools] Ensure members can always exit the pool gracefully (#4998)
  StorageWeightReclaim: set to node pov size if higher (#5281)
  [Bot] Add prdoc generation (#5331)
  Small nits found accidentally along the way (#5341)
  Create subsystem-benchmarks.yml (#5325)
  Bump libp2p-identity from 0.2.8 to 0.2.9 (#5232)
  Bump authoring duration for async backing to 2s. (#5195)
  Fix spelling issues (#5206)
  Bump the known_good_semver group across 1 directory with 3 updates (#5315)
  `polkadot-node-core-pvf-common`: Fix test compilation error (#5310)
  ci: Paused `cmd-action` commenter (#5287)
  Remove unnecessary mut (#5318)
  chain-spec: minor clarification on the genesis config patch (#5324)
  fix av-distribution Jaeger spans mem leak (#5321)
  ...
ordian added a commit that referenced this pull request Aug 15, 2024
* master: (167 commits)
  Upgrade accidentally downgraded deps (#5365)
  [Pools] Fix issues with member migration to `DelegateStake` (#4822)
  Unify `no_genesis` check (#5360)
  [CI] Fix prdoc command (#5358)
  Beefy: add benchmarks for `report_fork_voting()` (#5188)
  Fix OurViewChange small race (#5356)
  Make ticket non-optional and add ensure_successful method to Consideration trait (#5359)
  [tests] dedup test code, add more tests, improve naming and docs (#5338)
  Stop running the wishlist workflow on forks (#5297)
  Migrate foreign assets v3::Location to v4::Location (#4129)
  Minor clean up (#5284)
  [Pools] Ensure members can always exit the pool gracefully (#4998)
  StorageWeightReclaim: set to node pov size if higher (#5281)
  [Bot] Add prdoc generation (#5331)
  Small nits found accidentally along the way (#5341)
  Create subsystem-benchmarks.yml (#5325)
  Bump libp2p-identity from 0.2.8 to 0.2.9 (#5232)
  Bump authoring duration for async backing to 2s. (#5195)
  Fix spelling issues (#5206)
  Bump the known_good_semver group across 1 directory with 3 updates (#5315)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T9-cumulus This PR/Issue is related to cumulus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants