-
Notifications
You must be signed in to change notification settings - Fork 743
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
collator protocol changes for elastic scaling (validator side) #3302
Conversation
@@ -107,6 +107,9 @@ pub struct Params<BI, CIDP, Client, Backend, RClient, CHP, SO, Proposer, CS> { | |||
pub authoring_duration: Duration, | |||
/// Whether we should reinitialize the collator config (i.e. we are transitioning to aura). | |||
pub reinitialize: bool, | |||
/// Whether elastic scaling is enabled for this collation. | |||
/// If it is, the collator will send the parent-head data along with the collation. | |||
pub with_elastic_scaling: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please get rid of this with_elastic_scaling
everywhere in this pr? This is just "dirty". The collation generation logic internally can just decide based on the fact if the head
is passed or not if it wants to use the new message.
From all the collator implementations we want to set the head
to None
for now and then later do this properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, pushed 94ba49b
The collation generation logic internally can just decide based on the fact if the head is passed or not if it wants to use the new message
could you elaborate on this? passing the head from where?
i mean we could also detect elastic scaling by checking all the assignments for the para_id in collation-generation (or somewhere else)
we could also set to Some unconditionally (which seems wasteful)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/paritytech/polkadot-sdk/pull/3302/files#diff-14f4394aa7a3666c1a78fb28ccf076505323c497acf8dae788d3fc28a9cc6a18R862 you are already doing here what I meant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then give maybe_parent_head
better docs to explain on when to set this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently, in this PR, maybe_parent_head_data
is being set by collation-generation - it's hardcoded to None
: https://github.com/paritytech/polkadot-sdk/pull/3302/files#diff-d7298fb050b10e14df59080885ebee73861d85b2c4722cc00089fc91a703d63eR543. I've tried to make it configurable with with_elastic_scaling
param, but you seem to think it's a hack. So my question is, do you have a suggestion on how it should be configured properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make maybe_parent_head_data
part of
pub struct Collation<BlockNumber = polkadot_primitives::BlockNumber> { |
CollatorFn
if that the right way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make
maybe_parent_head_data
part of
Maybe we don't need this, because CollatorFn
is sort of deprecated I would say. I mean the lookahead collator isn't using this anymore and any feature work will also not use CollatorFn
anymore. SubmitCollationParams
is the struct that should be updated to take maybe_parent_head_data
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think I misunderstood you a bit and after chatting with @skunert, settled on approach where collators don't configure anything and elastic scaling is detected automatically (not done in this PR).
Updated the PR and description. PTAL 🙏
* master: (41 commits) Add Coretime to Westend (#3319) removed `pallet::getter` from `pallet-sudo` (#3370) gossip-support: add unittests for update authorities (#3258) [FRAME Core] remove unnecessary overrides while using derive_impl for frame_system (#3317) Update coretime-westend bootnodes (#3380) `im-online` removal cleanup: remove off-chain storage (#2290) Bump the known_good_semver group with 1 update (#3379) Fix documentation dead link (#3372) Make `sp-keystore` `no_std`-compatible and fix the `build-runtimes-polkavm` CI job (#3363) Remove unused `im-online` weights (#3373) Ensure referenda `TracksInfo` is sorted (#3325) rpc server: add rate limiting middleware (#3301) do not block finality for "disabled" disputes (#3358) fix(zombienet): docker `img` version to use in merge queues for bridges (#3337) Various nits and alignments for SP testnets found during bumping `polkadot-fellows` repo (#3359) Add broker pallet to `coretime-westend` (#3272) remove recursion limit (#3348) Update subkey README.md (#3355) Bump the known_good_semver group with 6 updates (#3347) Document LocalKeystore insert method (#3336) ...
…head-data * origin/master: (51 commits) Runtime Upgrade ref docs and Single Block Migration example pallet (#1554) Collator overseer builder unification (#3335) Introduce storage attr macro `#[disable_try_decode_storage]` and set it on `System::Events` and `ParachainSystem::HostConfiguration` (#3454) Add Polkadotters bootnoders per IBP application (#3423) Add documentation around FRAME Origin (#3362) Bridge zombienet tests: Check amount received at destination (#3490) Snowbridge benchmark tests fix (#3424) fix(zombienet): increase timeout in download artifacts (#3376) Cleanup String::from_utf8 (#3446) [prdoc] Validate crate names (#3467) Limit max execution time for `test-linux-stable` CI jobs (#3483) Introduce Notification block pinning limit (#2935) frame-support: Improve error reporting when having too many pallets (#3478) add Encointer as trusted teleporter for Westend (#3411) [pallet-xcm] Adjust benchmarks (teleport_assets/reserve_transfer_assets) not relying on ED (#3464) Add more debug logs to understand if statement-distribution misbehaving (#3419) Remove redundant parachains assigner pallet. (#3457) Use generic hash for runtime wasm in resolve_state_version_from_wasm (#3447) Runtime: allow backing multiple candidates of same parachain on different cores (#3231) Bridge zombienet tests: move all "framework" files under one folder (#3462) ...
Opened a separate #3521 for actually sending the parent head data on the collator side. |
…head-data * origin/master: Fix call enum's metadata regression (#3513) Enable elastic scaling node feature in local testnets genesis (#3509) update development setup in sdk-docs (#3506) Fix accidental no-shows on node restart (#3277) Remove `AssignmentProviderConfig` and use parameters from `HostConfiguration` instead (#3181) [Deprecation] Remove sp_weights::OldWeight (#3491) Fixup multi-collator parachain transition to async backing (#3510) Multi-Block-Migrations, `poll` hook and new System callbacks (#1781) Snowbridge - Extract Ethereum Chain ID (#3501) PVF: re-preparing artifact on failed runtime construction (#3187) Add documentation around FRAME Offchain workers (#3463) [prdoc] Optional SemVer bumps and Docs (#3441) rpc-v2/tx/tests: Add transaction broadcast tests and check propagated tx status (#3193)
* master: Finish documenting `#[pallet::xxx]` macros (#2638) Remove `as frame_system::DefaultConfig` from the required syntax in `derive_impl` (#3505) provisioner: allow multiple cores assigned to the same para (#3233) subsystem-bench: add regression tests for availability read and write (#3311) make SelfParaId a metadata constant (#3517) Fix crash of synced parachain node run with `--sync=warp` (#3523) [Backport] Node version and spec_version bumps and ordering of the prdoc files from 1.8.0 (#3508) Add `claim_assets` extrinsic to `pallet-xcm` (#3403)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ordian !
* master: (65 commits) collator protocol changes for elastic scaling (validator side) (#3302) Contracts use polkavm workspace deps (#3715) Add elastic scaling support in ParaInherent BenchBuilder (#3690) Removes `as [disambiguation_path]` from `derive_impl` usage (#3652) fix(paseo-spec): New Paseo Bootnodes (#3674) Improve Penpal runtime + emulated tests (#3543) Staking ledger bonding fixes (#3639) DescribeAllTerminal for HashedDescription (#3349) Increase timeout for assertions (#3680) Add subsystems regression tests to CI (#3527) Always print connectivity report (#3677) Revert "FRAME: Create `TransactionExtension` as a replacement for `SignedExtension` (#2280)" (#3665) authority-discovery: Add log for debugging DHT authority records (#3668) Construct Runtime v2 (#1378) Support for `keyring` in runtimes (#2044) Add api-name in `cannot query the runtime API version` warning (#3653) Add a PolkaVM-based executor (#3458) Adds default config for assets pallet (#3637) Bump handlebars from 4.3.7 to 5.1.0 (#3248) [Collator Selection] Fix weight refund for `set_candidacy_bond` (#3643) ...
On top of #3302. We want the validators to upgrade first before we add changes to the collation side to send the new variants, which is why this part is extracted into a separate PR. The detection of when to send the parent head is based on the core assignments at the relay parent of the candidate. We probably want to make it more flexible in the future, but for now, it will work for a simple use case when a para always has multiple cores assigned to it. --------- Signed-off-by: Matteo Muraca <mmuraca247@gmail.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Matteo Muraca <56828990+muraca@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Juan Ignacio Rios <54085674+JuaniRios@users.noreply.github.com> Co-authored-by: Branislav Kontur <bkontur@gmail.com> Co-authored-by: Bastian Köcher <git@kchr.de>
…ytech#3302) Fixes paritytech#3128. This introduces a new variant for the collation response from the collator that includes the parent head data. For now, collators won't send this new variant. We'll need to change the collator side of the collator protocol to detect all the cores assigned to a para and send the parent head data in the case when it's more than 1 core. - [x] validate approach - [x] check head data hash
On top of paritytech#3302. We want the validators to upgrade first before we add changes to the collation side to send the new variants, which is why this part is extracted into a separate PR. The detection of when to send the parent head is based on the core assignments at the relay parent of the candidate. We probably want to make it more flexible in the future, but for now, it will work for a simple use case when a para always has multiple cores assigned to it. --------- Signed-off-by: Matteo Muraca <mmuraca247@gmail.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Matteo Muraca <56828990+muraca@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Juan Ignacio Rios <54085674+JuaniRios@users.noreply.github.com> Co-authored-by: Branislav Kontur <bkontur@gmail.com> Co-authored-by: Bastian Köcher <git@kchr.de>
Properly account for paritytech#3302, cc paritytech#3984.
Fixes #3128.
This introduces a new variant for the collation response from the collator that includes the parent head data. For now, collators won't send this new variant. We'll need to change the collator side of the collator protocol to detect all the cores assigned to a para and send the parent head data in the case when it's more than 1 core.