Rewrite collator assignment to support parathreads #385

tmpolaczyk · 2024-01-11T12:23:14Z

The existing algorithm was getting too complex to support parathreads. This new algorithm will first calculate how many collators we need in total, removing some container chains if we don't have enough collators, and then assign collators giving priority to those that were already assigned to the same chains.

In most cases this will result in the same assignment, expect that now all the orchestrator collators will be added all at the same time instead of first min and then max (see test assign_collators_set_zero_per_container).

Additionally, it allows each container chain to have a different number of collators, and even a min/max similar to the orchestrator chain. This is not used yet but it will be needed to support parathreads, which will have a different number of collators than container chains.

This fixes some bugs related to the case where there are not enough collators, see tests assign_collators_remove_from_orchestator_when_all_assigned and assign_collators_invulnerables_priority_orchestrator_reassigned.

This PR will not add parathread support, that will come in future PRs.

And fix some bugs when there are not enough collators

github-actions · 2024-01-11T15:03:11Z

Coverage Report

(master)

@@                       Coverage Diff                        @@
##           master   tomasz-assign-to-parathreads      +/-   ##
================================================================
+ Coverage   76.41%                         76.92%   +0.51%     
+ Files         101                            105       +4     
+ Lines       25488                          26036     +548     
================================================================
+ Hits        19475                          20026     +551     
- Misses       6013                           6010       -3

Files Changed	Coverage
/client/consensus/src/consensus_orchestrator.rs	77.90% (+0.44%)	🔼
/pallets/collator-assignment/src/lib.rs	94.63% (-1.07%)	🔽
/pallets/configuration/src/lib.rs	85.25% (+0.57%)	🔼
/pallets/inflation-rewards/src/lib.rs	93.18% (+2.27%)	🔼
/pallets/registrar/src/lib.rs	92.01% (+0.08%)	🔼
/pallets/services-payment/src/lib.rs	83.89% (+1.34%)	🔼
/runtime/dancebox/src/lib.rs	81.32% (-0.11%)	🔽
/runtime/flashbox/src/lib.rs	43.87% (-0.13%)	🔽

Coverage generated Mon Jan 22 12:24:46 UTC 2024

tmpolaczyk · 2024-01-12T17:35:02Z

~~There are some unwraps/panics that I would like to remove~~, but aside from that this should be ready for review

Edit: Removed all unwraps and panics, if you find one please add a comment

girazoki · 2024-01-15T17:17:29Z

pallets/collator-assignment/src/assignment.rs

+                available_collators -= cc.min_collators;
+                container_chains_with_collators.push(*cc);
+            } else {
+                // Do not break here because we want to push all the remaining para_ids


here you mean parathreads?

Yes, clarified the comment

girazoki · 2024-01-15T17:18:51Z

pallets/collator-assignment/src/assignment.rs

+    /// # Panics
+    ///
+    /// * If `container_chains` is empty, or if the first element of `container_chains` does not have `para_id == SelfParaId::get()`.
+    pub fn prioritize_invulnerables(


I love this refactor BTW. Much cleaner than having it in the pallet

girazoki · 2024-01-15T17:20:17Z

test/scripts/build-spec-warp-sync.sh

@@ -8,4 +8,4 @@ cd $(dirname $0)/..

 mkdir -p specs
 ../target/release/container-chain-template-simple-node build-spec --disable-default-bootnode --add-bootnode "/ip4/127.0.0.1/tcp/33049/ws/p2p/12D3KooWHVMhQDHBpj9vQmssgyfspYecgV6e3hH1dQVDUkUbCYC9" --parachain-id 2000 --raw > specs/warp-sync-template-container-2000.json
-../target/release/tanssi-node build-spec --chain dancebox-local --parachain-id 1000 --add-container-chain specs/warp-sync-template-container-2000.json --invulnerable "Collator1000-01" --invulnerable "Collator1000-02" --invulnerable "Collator2000-01" --invulnerable "Collator2000-02" --invulnerable "Collator1000-03" > specs/warp-sync-tanssi-1000.json
+../target/release/tanssi-node build-spec --chain dancebox-local --parachain-id 1000 --add-container-chain specs/warp-sync-template-container-2000.json --invulnerable "Collator1000-01" --invulnerable "Collator1000-02" --invulnerable "Collator1000-03" --invulnerable "Collator2000-01" --invulnerable "Collator2000-02" > specs/warp-sync-tanssi-1000.json


what is the reason for this?

Intentional, it's the order change mentioned in the description (see test assign_collators_set_zero_per_container for an example)

girazoki · 2024-01-15T17:21:01Z

test/scripts/build-spec.sh

@@ -10,4 +10,4 @@ mkdir -p specs
 ../target/release/container-chain-template-simple-node build-spec --disable-default-bootnode --add-bootnode "/ip4/127.0.0.1/tcp/33049/ws/p2p/12D3KooWHVMhQDHBpj9vQmssgyfspYecgV6e3hH1dQVDUkUbCYC9" --parachain-id 2000 --raw > specs/template-container-2000.json
 ../target/release/container-chain-template-frontier-node build-spec --disable-default-bootnode --add-bootnode "/ip4/127.0.0.1/tcp/33050/ws/p2p/12D3KooWFGaw1rxB6MSuN3ucuBm7hMq5pBFJbEoqTyth4cG483Cc" --parachain-id 2001 --raw > specs/template-container-2001.json
 ../target/release/container-chain-template-simple-node build-spec --disable-default-bootnode --parachain-id 2002 --raw > specs/template-container-2002.json
-../target/release/tanssi-node build-spec --chain dancebox-local --parachain-id 1000 --add-container-chain specs/template-container-2000.json --add-container-chain specs/template-container-2001.json --invulnerable "Collator1000-01" --invulnerable "Collator1000-02" --invulnerable "Collator2000-01" --invulnerable "Collator2000-02" --invulnerable "Collator2001-01" --invulnerable "Collator2001-02" --invulnerable "Collator2002-01" --invulnerable "Collator2002-02" > specs/tanssi-1000.json
+../target/release/tanssi-node build-spec --chain dancebox-local --parachain-id 1000 --add-container-chain specs/template-container-2000.json --add-container-chain specs/template-container-2001.json --invulnerable "Collator1000-01" --invulnerable "Collator1000-02" --invulnerable "Collator2002-01" --invulnerable "Collator2002-02" --invulnerable "Collator2000-01" --invulnerable "Collator2000-02" --invulnerable "Collator2001-01" --invulnerable "Collator2001-02" > specs/tanssi-1000.json


And for these, it only changes the order no?

Yes, to assign collators to the same chains as before the refactor

girazoki · 2024-01-16T08:44:36Z

pallets/collator-assignment/src/lib.rs

+            }];
+            let collators_per_container =
+                T::HostConfiguration::collators_per_container(target_session_index);
+            for para_id in &container_chain_ids {


Can you put a TODO here indicating that this is provisional and that the order should be done based on tipping in the future?

Sure! I imagine that the priority will be done instead of shuffling here, so the rest of the algorithm should not change when we add tipping.

girazoki · 2024-01-16T08:53:47Z

pallets/collator-assignment/src/lib.rs

+            // Chains will not be assigned less than `min_collators`, except the orchestrator chain.
+            // First all chains will be assigned `min_collators`, and then the first one will be assigned up to `max`,
+            // then the second one, and so on.
+            let mut container_chains = vec![ContainerChain {


I am not entirely sure I like calling this a containerChain when it really isnt :(. I would rather call it Chain, but I also dont like it too much, maybe we can have a dual enum here that for now have the same parameters?

Renamed to ChainNumCollators and added docs saying that this could be any kind of chain

girazoki · 2024-01-18T16:46:41Z

LGTM!

fgamundi

LGTM

…rathreads

tmpolaczyk added 2 commits January 11, 2024 13:09

Refactor collator assignment to support parathreads

6fdb179

And fix some bugs when there are not enough collators

nostd

060733d

tmpolaczyk added 6 commits January 11, 2024 19:18

Refactor, add select_chains_with_collators and docs

a8c8d59

Fix typescript tests

428a7ec

Before 999 change

328c5df

Refactor into assignment.rs and add tests

92a5603

Add copyright and fmt

8c03ca7

More refactor

32c2378

tmpolaczyk marked this pull request as ready for review January 12, 2024 17:35

tmpolaczyk added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes labels Jan 12, 2024

clippy and TODO

40e91d6

girazoki reviewed Jan 15, 2024

View reviewed changes

girazoki reviewed Jan 16, 2024

View reviewed changes

tmpolaczyk added 2 commits January 17, 2024 12:27

Refactor, remove some panics

3a71979

Return error instead of panic, add safeguards to avoid bricking chain

011e4ed

girazoki approved these changes Jan 18, 2024

View reviewed changes

fgamundi approved these changes Jan 22, 2024

View reviewed changes

tmpolaczyk added 2 commits January 22, 2024 12:08

Merge remote-tracking branch 'origin/master' into tomasz-assign-to-pa…

c830d53

…rathreads

Integrate config changes

dbee04c

tmpolaczyk merged commit 496d044 into master Jan 22, 2024
30 checks passed

tmpolaczyk deleted the tomasz-assign-to-parathreads branch January 22, 2024 14:41

tmpolaczyk mentioned this pull request Jan 24, 2024

Avoid error log "ZeroCollators" when running cargo test #394

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite collator assignment to support parathreads #385

Rewrite collator assignment to support parathreads #385

tmpolaczyk commented Jan 11, 2024

github-actions bot commented Jan 11, 2024 •

edited

Loading

tmpolaczyk commented Jan 12, 2024 •

edited

Loading

girazoki Jan 15, 2024

tmpolaczyk Jan 17, 2024

girazoki Jan 15, 2024

girazoki Jan 15, 2024

tmpolaczyk Jan 15, 2024

girazoki Jan 15, 2024

tmpolaczyk Jan 15, 2024

girazoki Jan 16, 2024

tmpolaczyk Jan 16, 2024

girazoki Jan 16, 2024

tmpolaczyk Jan 17, 2024

girazoki commented Jan 18, 2024

fgamundi left a comment

Rewrite collator assignment to support parathreads #385

Rewrite collator assignment to support parathreads #385

Conversation

tmpolaczyk commented Jan 11, 2024

github-actions bot commented Jan 11, 2024 • edited Loading

tmpolaczyk commented Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girazoki commented Jan 18, 2024

fgamundi left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 11, 2024 •

edited

Loading

tmpolaczyk commented Jan 12, 2024 •

edited

Loading