Runtime: allow backing multiple candidates of same parachain on different cores #3231

sandreim · 2024-02-06T16:39:03Z

Builds on top of #3229

Summary

Some preparations for Runtime to support elastic scaling, guarded by config node features bit FeatureIndex::ElasticScalingMVP. This PR introduces a per-candidate CoreIndex but does it in a hacky way to avoid changing CandidateCommitments, CandidateReceipts primitives and networking protocols.

Including `CoreIndex` in `BackedCandidate`

If the ElasticScalingMVP feature bit is enabled then BackedCandidate::validator_indices is extended by 8 bits.
The value stored in these bits represents the assumed core index for the candidate.

It is temporary solution which works by creating a mapping from BackedCandidate to CoreIndex by assuming the CoreIndex can be discovered by checking in which validator group the validator that signed the statement is.

TODO:

fix tests
add new tests
Bump runtime API for Kusama, so we have that node features thing! -> bump ParachainHost to api version 9 polkadot-fellows/runtimes#194

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

…reim/backing_multiple_cores_per_para

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

…:paritytech/polkadot-sdk into sandreim/runtime_core_index_step1

polkadot/runtime/parachains/src/paras_inherent/mod.rs

alindima · 2024-02-12T11:37:56Z

Seeing this new InjectCoreIndex feature made me think about how are we going to enable elastic scaling (on the runtime side). On the node side, all of the changes that I can think of are backwards compatible.

I think we'll need another NodeFeature bit, AllowMultipleCoresPerPara, which will switch on the logic in the runtime for accepting multiple candidates for a parachain.

This way:

if InjectCoreIndex is enabled and AllowMultipleCoresPerPara is enabled, elastic scaling is enabled and working
if InjectCoreIndex is enabled and AllowMultipleCoresPerPara is disabled, parachains can acquire multiple cores but the runtime will only accept one candidate at a time. (what this PR aims to do)
if InjectCoreIndex is disabled and AllowMultipleCoresPerPara is enabled, this makes no sense. Elastic scaling won't work and parachains who acquire multiple cores will be stalled (the current status quo)
if InjectCoreIndex is disabled and AllowMultipleCoresPerPara is disabled. Identical to option 3 above.

Seeing this, I'm wondering if having this InjectCoreIndex makes any sense on its own after all. I guess it does make sense because it'll be disabled once we have the CoreIndex in the receipt.

CC @eskimor

alindima · 2024-02-12T11:48:41Z

Maybe a NodeFeature is not the most suitable name for this, because in this case it enables changes in the runtime as well (not only on the node). Especially the AllowMultipleCoresPerPara I mention above. But I think it's fine

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

…:paritytech/polkadot-sdk into sandreim/runtime_core_index_step1

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

sandreim · 2024-02-13T10:20:50Z

@alindima
I think InjectCoreIndex is sufficient to enable elastic scaling in Runtime as well as node side. Not sure why you want another feature. The soft switch which you seem to be looking for is actually having paras assigned to multiple cores.

alindima · 2024-02-13T10:27:55Z

@alindima I think InjectCoreIndex is sufficient to enable elastic scaling in Runtime as well as node side. Not sure why you want another feature. The soft switch which you seem to be looking for is actually having paras assigned to multiple cores.

I thought we wanted to maintain the possibility of not having elastic scaling but still not bricking a parachain which occupies more cores.
Now that I think about it again, maybe it isn't very helpful. Only for the possibility of disabling elastic scaling to mitigate a bug. If there were some bug with elastic scaling that stalled a parachain, disabling InjectCoreIndex would not unbrick the parachain. Maybe it's not worth the complexity.
Also, in the future, after CoreIndex is present in the candidatereceipt, disabling InjectCoreIndex will no longer disable elastic scaling.

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

sandreim · 2024-02-13T10:50:21Z

I thought we wanted to maintain the possibility of not having elastic scaling but still not bricking a parachain which occupies more cores.

This is included in this PR, they won't break if the InjectCoreIndex feature is enabled, even if parachains have multiple cores assigned and their collators are unupgraded.

alindima · 2024-02-13T11:03:44Z

I thought we wanted to maintain the possibility of not having elastic scaling but still not bricking a parachain which occupies more cores.

This is included in this PR, they won't break if the InjectCoreIndex feature is enabled, even if parachains have multiple cores assigned and their collators are unupgraded.

I am talking about after we enable elastic scaling

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

sandreim · 2024-02-13T13:26:46Z

Anything we implement for elastic scaling must not break parachains that don't make use of this feature, while also not requiring them to upgrade (except of when we add CoreIndex to the commited candidate receipt).

We should prevent parachains (I'll cut a ticket) from acquiring multiple cores as a safety measure and as an enablement mechanism. We initially plan to launch on Rococo, while on other chains the possibility of assigning multiple cores is disabled - hence elastic scaling is disabled.

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

…re_index_step1

eskimor · 2024-02-22T10:35:36Z

polkadot/zombienet_tests/functional/0012-elastic-scaling-mvp.zndsl

+alice: js-script ./0012-enable-node-feature.js with "1" return is 0 within 600 seconds
+
+# Wait two sessions for the config to be updated.
+sleep 120 seconds


It would be better to wait for the actual session change 😬 - If that is not possible right now, we should add that feature.

polkadot/zombienet_tests/functional/0012-elastic-scaling-mvp.zndsl

tdimitrov

Looks very good @alindima. Left a few nits and a question.

polkadot/runtime/parachains/src/paras_inherent/mod.rs

tdimitrov · 2024-02-23T13:29:05Z

polkadot/runtime/parachains/src/paras_inherent/mod.rs

+		// cases.
+		if context == ProcessInherentDataContext::Enter {
+			ensure!(
+				!dropped_elastic_scaling_candidates,


Unrelated to this PR but why we check if we have dropped 'elastic scaling candidates' here, but we are fine dropping candidates without assigned core (which happens here)?

Thanks for pointing this out!

prior to this PR, we just assumed that one para maps to one core. So we were only checking if the candidate's paraid has one scheduled core.
if it didn't, we'd return an error. if multiple candidates with the same paraid were supplied, we'd only check for one scheduled core.

The logic I added is more comprehensive and will work for proper elastic scaling also. But I decided to be more lenient and just filter out unscheduled candidates instead of erroring.

However, the check you mentioned should be I think extended to all dropped candidates 👍️ I don't see a reason why it should only be for some of them
because they should be all filtered by the block author. the other block importers should not do this

polkadot/runtime/parachains/src/paras_inherent/mod.rs

paritytech-cicd-pr · 2024-02-23T15:26:57Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 2/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5329732

…rent cores (#3231) Fixes #3144 Builds on top of #3229 Some preparations for Runtime to support elastic scaling, guarded by config node features bit `FeatureIndex::ElasticScalingMVP`. This PR introduces a per-candidate `CoreIndex` but does it in a hacky way to avoid changing `CandidateCommitments`, `CandidateReceipts` primitives and networking protocols. If the `ElasticScalingMVP` feature bit is enabled then `BackedCandidate::validator_indices` is extended by 8 bits. The value stored in these bits represents the assumed core index for the candidate. It is temporary solution which works by creating a mapping from `BackedCandidate` to `CoreIndex` by assuming the `CoreIndex` can be discovered by checking in which validator group the validator that signed the statement is. TODO: - [x] fix tests - [x] add new tests - [x] Bump runtime API for Kusama, so we have that node features thing! -> polkadot-fellows/runtimes#194 --------- Signed-off-by: Andrei Sandu <andrei-mihail@parity.io> Signed-off-by: alindima <alin@parity.io> Co-authored-by: alindima <alin@parity.io>

…rent cores (#3231) Fixes #3144 Builds on top of #3229 ### Summary Some preparations for Runtime to support elastic scaling, guarded by config node features bit `FeatureIndex::ElasticScalingMVP`. This PR introduces a per-candidate `CoreIndex` but does it in a hacky way to avoid changing `CandidateCommitments`, `CandidateReceipts` primitives and networking protocols. #### Including `CoreIndex` in `BackedCandidate` If the `ElasticScalingMVP` feature bit is enabled then `BackedCandidate::validator_indices` is extended by 8 bits. The value stored in these bits represents the assumed core index for the candidate. It is temporary solution which works by creating a mapping from `BackedCandidate` to `CoreIndex` by assuming the `CoreIndex` can be discovered by checking in which validator group the validator that signed the statement is. TODO: - [x] fix tests - [x] add new tests - [x] Bump runtime API for Kusama, so we have that node features thing! -> polkadot-fellows/runtimes#194 --------- Signed-off-by: Andrei Sandu <andrei-mihail@parity.io> Signed-off-by: alindima <alin@parity.io> Co-authored-by: alindima <alin@parity.io>

…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) ...

sandreim added 9 commits February 1, 2024 22:29

Switch statement table from ParaId to CoreIndex

cab6ab9

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

cargo lock

d2df658

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

add experimental feature

4a8b8d5

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

inject core_index from statements

9244632

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

temporary provisioner fix

22e017b

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

Support injected CoreIndex

9dc8927

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

Merge branch 'master' of github.com:paritytech/polkadot-sdk into sand…

574b06a

…reim/backing_multiple_cores_per_para

cargo lock

fbb7351

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

Merge branch 'sandreim/backing_multiple_cores_per_para' of github.com…

c6b833e

…:paritytech/polkadot-sdk into sandreim/runtime_core_index_step1

sandreim commented Feb 7, 2024

View reviewed changes

polkadot/runtime/parachains/src/paras_inherent/mod.rs Outdated Show resolved Hide resolved

sandreim added 9 commits February 12, 2024 14:35

It was damn hard to fix these tests

6c72918

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

These tests were easy to fix

fc5c109

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

Fix comment

33351b4

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

clippy was angry

0d994bf

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

A bit refactor and add a test

534c019

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

taplo happy

10d86dd

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

Merge branch 'sandreim/backing_multiple_cores_per_para' of github.com…

d990a65

…:paritytech/polkadot-sdk into sandreim/runtime_core_index_step1

BackedCandidate: make all members private and provide an interface

fbe1ad5

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

refactor based on new BackedCandidate

e74f038

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

sandreim changed the title ~~Elastic scaling: allow backing multiple candidates of same parachain on different cores~~ Runtime: allow backing multiple candidates of same parachain on different cores Feb 13, 2024

Fix all parachain runtime tests affected

d898740

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

fix more broken test on node side

9e40490

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

wip new test

42f46a0

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>

Base automatically changed from sandreim/backing_multiple_cores_per_para to master February 22, 2024 08:47

alindima added 3 commits February 22, 2024 11:38

add more unit tests

bb5968c

Merge remote-tracking branch 'origin/master' into sandreim/runtime_co…

1ca7a70

…re_index_step1

fix clippy

eb345b0

eskimor reviewed Feb 22, 2024

View reviewed changes

alindima added 3 commits February 22, 2024 17:50

refactor

058c0c2

fix some bugs and add more unit tests

d4c58bd

update some comments

cb41758

alindima mentioned this pull request Feb 23, 2024

provisioner: allow multiple cores assigned to the same para #3233

Merged

tdimitrov approved these changes Feb 23, 2024

View reviewed changes

review comments

bffa4e9

fix unit test

5d3a85d

eskimor approved these changes Feb 23, 2024

View reviewed changes

eskimor added this pull request to the merge queue Feb 23, 2024

Merged via the queue into master with commit 2431001 Feb 23, 2024
129 of 131 checks passed

eskimor deleted the sandreim/runtime_core_index_step1 branch February 23, 2024 17:01

This was referenced Feb 26, 2024

Fix multiple cores 1.8 #3477

Merged

Fix multiple cores assignments 1.7 #3469

Merged

sandreim mentioned this pull request Feb 28, 2024

Testnet HostConfiguration migration (v11 to v12) #3503

Closed

patriciobcs self-assigned this Apr 9, 2024

CanonicalJP unassigned patriciobcs Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Runtime: allow backing multiple candidates of same parachain on different cores #3231

Runtime: allow backing multiple candidates of same parachain on different cores #3231

sandreim commented Feb 6, 2024 •

edited by alindima

Loading

alindima commented Feb 12, 2024

alindima commented Feb 12, 2024

sandreim commented Feb 13, 2024

alindima commented Feb 13, 2024 •

edited

Loading

sandreim commented Feb 13, 2024

alindima commented Feb 13, 2024

sandreim commented Feb 13, 2024

eskimor Feb 22, 2024

tdimitrov left a comment

tdimitrov Feb 23, 2024

alindima Feb 23, 2024

alindima Feb 23, 2024

paritytech-cicd-pr commented Feb 23, 2024

Runtime: allow backing multiple candidates of same parachain on different cores #3231

Runtime: allow backing multiple candidates of same parachain on different cores #3231

Conversation

sandreim commented Feb 6, 2024 • edited by alindima Loading

Summary

Including CoreIndex in BackedCandidate

alindima commented Feb 12, 2024

alindima commented Feb 12, 2024

sandreim commented Feb 13, 2024

alindima commented Feb 13, 2024 • edited Loading

sandreim commented Feb 13, 2024

alindima commented Feb 13, 2024

sandreim commented Feb 13, 2024

eskimor Feb 22, 2024

Choose a reason for hiding this comment

tdimitrov left a comment

Choose a reason for hiding this comment

tdimitrov Feb 23, 2024

Choose a reason for hiding this comment

alindima Feb 23, 2024

Choose a reason for hiding this comment

alindima Feb 23, 2024

Choose a reason for hiding this comment

paritytech-cicd-pr commented Feb 23, 2024

sandreim commented Feb 6, 2024 •

edited by alindima

Loading

Including `CoreIndex` in `BackedCandidate`

alindima commented Feb 13, 2024 •

edited

Loading