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

cumulus: bump PARENT_SEARCH_DEPTH and add test for 12-core elastic scaling #6983

Merged
merged 52 commits into from
Jan 28, 2025

Conversation

alindima
Copy link
Contributor

On top of #6757

Fixes #6858 by bumping the PARENT_SEARCH_DEPTH constant to a larger value (30) and adds a zombienet-sdk test that exercises the 12-core scenario.

This is a node-side limit that restricts the number of allowed pending availability candidates when choosing the parent parablock during authoring.
This limit is rather redundant, as the parachain runtime already restricts the unincluded segment length to the configured value in the FixedVelocityConsensusHook (which ideally should be equal to this PARENT_SEARCH_DEPTH).

For 12 cores, a value of 24 should be enough, but I bumped it to 30 to have some extra buffer.

There are two other potential ways of fixing this:

  • remove this constant altogether, as the parachain runtime already makes those guarantees. Chose not to do this, as it can't hurt to have an extra safeguard
  • set this value to be equal to the uninlcuded segment size. This value however is not exposed to the node-side and would require a new runtime API, which seems overkill for a redundant check.

@alindima alindima added the T9-cumulus This PR/Issue is related to cumulus. label Dec 23, 2024
@alindima alindima requested a review from a team as a code owner December 23, 2024 09:49
@alindima alindima requested a review from skunert December 23, 2024 09:49
Base automatically changed from alindima/zombienet-sdk-rewrite to master January 7, 2025 14:06
@alindima alindima requested a review from sandreim January 9, 2025 12:20
Comment on lines 119 to 124
#[cfg(feature = "elastic-scaling-500ms")]
const UNINCLUDED_SEGMENT_CAPACITY: u32 = 30;
#[cfg(feature = "elastic-scaling-500ms")]
const BLOCK_PROCESSING_VELOCITY: u32 = 12;
#[cfg(feature = "elastic-scaling-500ms")]
pub const MILLISECS_PER_BLOCK: u64 = 500;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just define MILLISECS_PER_BLOCK and then compute the BLOCK_PROCESSING_VELOCITY and UNINCLUDED_SEGMENT_CAPACITY based on it ? It would also reduce the amount cluttering due to feature flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, done

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12950456722
Failed job name: build-rustdoc

Copy link
Contributor

@sandreim sandreim left a comment

Choose a reason for hiding this comment

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

LGTM! Nice!

@alindima alindima added this pull request to the merge queue Jan 28, 2025
Merged via the queue into master with commit e6aad5b Jan 28, 2025
203 of 207 checks passed
@alindima alindima deleted the alindima/update-parent-search-depth branch January 28, 2025 09:10
ordian added a commit that referenced this pull request Jan 30, 2025
* master: (58 commits)
  [pallet-revive] pack exceeding syscall arguments into registers (#7319)
  cumulus: bump PARENT_SEARCH_DEPTH and add test for 12-core elastic scaling (#6983)
  xcm: fix for DenyThenTry Barrier (#7169)
  Migrating polkadot-runtime-common slots benchmarking to v2 (#6614)
  Add development chain-spec file for minimal/parachain templates for Omni Node compatibility (#6529)
  `Arc` definition in `TransactionPool` (#7042)
  [sync] Let new subscribers know about already connected peers (backward-compatible) (#7344)
  Removed unused dependencies (partial progress) (#7329)
  Improve debugging by using `#[track_caller]` in system `assert_last_event` and `assert_has_event` (#7142)
  `set_validation_data` register weight manually, do not use refund when the pre dispatch is zero. (#7327)
  Fix the link to the chain snapshots (#7330)
  revive: Fix compilation of `uapi` crate when `unstable-hostfn` is not set (#7318)
  [pallet-revive] eth-rpc minor fixes (#7325)
  sync-templates: enable syncing from stable release patches (#7227)
  Bridges: emulated tests small nits/improvements (#7322)
  fix(cmd bench-omni): build omni-bencher with production profile (#7299)
  Nits for collectives-westend XCM benchmarks setup (#7215)
  bench all weekly - and fix for pallet_multisig lib (#6789)
  Deprecate ParaBackingState API (#6867)
  Fix setting the image properly (#7315)
  ...
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.

Elastic scaling: use up to 12 cores
4 participants