Collation fetching fairness #4880

tdimitrov · 2024-06-26T07:13:31Z

Related to #1797

When fetching collations in collator protocol/validator side we need to ensure that each parachain has got a fair core time share depending on its assignments in the claim queue. This means that the number of collations fetched per parachain should ideally be equal to (but definitely not bigger than) the number of claims for the particular parachain in the claim queue.

The current implementation doesn't guarantee such fairness. For each relay parent there is a waiting_queue (PerRelayParent -> Collations -> waiting_queue) which holds any unfetched collations advertised to the validator. The collations are fetched on first in first out principle which means that if two parachains share a core and one of the parachains is more aggresive it might starve the second parachain. How? At each relay parent up to max_candidate_depth candidates are accepted (enforced in fn is_seconded_limit_reached) so if one of the parachains is quick enough to fill in the queue with its advertisements the validator will never fetch anything from the rest of the parachains despite they are scheduled. This doesn't mean that the aggressive parachain will occupy all the core time (this is guaranteed by the runtime) but it will deny the rest of the parachains sharing the same core to have collations backed.

~~The solution I am proposing extends the checks in is_seconded_limit_reached with an additional check.~~ The solution I am proposing is to limit fetches and advertisements based on the state of the claim queue. At each relay parent the claim queue for the core assigned to the validator is fetched. For each parachain a fetch limit is calculated (equal to the number of entries in the claim queue). Advertisements are not fetched for a parachain which has exceeded its claims in the claim queue. This solves the problem with aggressive parachains advertising too much collations.

The second part is in collation fetching logic. The collator will keep track on which collations it has fetched so far. When a new collation needs to be fetched instead of popping the first entry from the waiting_queue the validator examines the claim queue and looks for the earliest claim which hasn't got a corresponding fetch. This way the collator will always try to prioritise the most urgent entries.

polkadot/node/network/collator-protocol/src/validator_side/collation.rs

…al to `allowed_ancestry_len`

tdimitrov · 2024-06-28T09:31:33Z

polkadot/node/network/collator-protocol/src/validator_side/mod.rs

@@ -266,9 +264,6 @@ impl PeerData {
 							let candidates =
 								state.advertisements.entry(on_relay_parent).or_default();

-							if candidates.len() > max_candidate_depth {


This error leads to reporting the peer with COST_UNEXPECTED_MESSAGE. I think we shold relax it to just ignoring the advertisement.

Pros:

with the new logic submitting more elements than scheduled is not such a major offence

old collators won't get punished for not respecting the claim queue

Cons:

we don't punish spammy collators

with the new logic submitting more elements than scheduled is not such a major offence

how come?

I didn't pick the best words here but my point was since we guarantee fair share of core time for each parachain we can close our eyes for collators pushing too much advertisements as they don't cause any harm.

Furthermore at the moment we haven't got a proper collator using the claim queue to decide what advertisements to send (right?) so when this change is live a lot of collators will suffer.

We should certainly not keep inserting blindly. You are right that it is less of an issue since we are now guaranteeing fairness, so we don't have to be super strict here. In particular given how ineffective reputation changes are in practice, it might make sense to set the limit to something we are 100% no honest node will ever reach, but then make it an immediate disconnect (max punishment).

Is the concern about old collators valid? Why would they be spammy?

we should still have some limit here. otherwise we record unlimited advertisements

I agree that without proper spam protection a malicious collator could bombard the validator with advertisements. I've restored the limit but instead of max_candidate_depth I've put the length of the current assignments which is equal to the claim queue of the assigned core.

Current version looks sane to me. Claim queue length should be a decent limit and we retain the punishment to not make it too easy.

polkadot/node/network/collator-protocol/src/validator_side/collation.rs

tdimitrov · 2024-10-14T12:31:50Z

polkadot/node/network/collator-protocol/src/validator_side/mod.rs

+					if let Some(CoreState::Occupied(core_state)) = cores.get(core_now.0 as usize) {
+						core_state.next_up_on_available.as_ref().map(|scheduled_core| {
+							vec![scheduled_core.para_id].into_iter().collect()
+						})
+					} else {
+						None
+					}


I'm not sure if we still need to handle missing claim queue or handle it as an error (the runtime api is released everywhere). Since it's a small addition I decided to keep it here.

Overkillus

Few questions as well as some nits here and there.

Overall looking good!

I see that you reverted to default lookahead value. Is the mystery of the failing zombienet test resolved?

prdoc/pr_4880.prdoc

polkadot/node/network/collator-protocol/src/validator_side/collation.rs

polkadot/node/network/collator-protocol/src/validator_side/mod.rs

Overkillus · 2024-10-14T14:19:19Z

polkadot/node/network/collator-protocol/src/validator_side/mod.rs

+
+	// Checks if another collation can be accepted. The number of collations that can be seconded
+	// per parachain is limited by the entries in claim queue for the `ParaId` in question.
+	if seconded_and_pending_at_ancestors >= claims_for_para {


We return SecondedLimitReached Error. There is still a possibility that the pending collation will not be seconded for some reason. What happens then to the 1 extra slot that could be used for seconding?

If there is a pending collation or not is determined by this function. If the fetching or the validation fails pending will be 0 and we will be able to accept another collation for the same relay parent. But as soon as there is a pending item we are optimistic and reject any new advertisements.

Overkillus · 2024-10-14T14:29:34Z

polkadot/node/network/collator-protocol/src/validator_side/collation.rs

+	/// We are currently fetching a collation for the specified `ParaId`.
+	Fetching(ParaId),
+	/// We are waiting that a collation is being validated for the specified `ParaId`.
+	WaitingOnValidation(ParaId),
 }


Originally and before async backing once we've reached Seconded status we knew we are done here. Right now we can only be satisfied with collations once we reach the seconding limit but I see that we do not have a corresponding status here.

When we reach the seconding limit and no more ocllations are expected what will be the status here?

Or is it nonsensical to even refer to a CollationStatus when we reach the limit? (Dont think so)

We could do it. The lifetime right now is Waiting -> Fetching -> WaitingOnValidation and back to Waiting. We can have an additional Satisfied or something similar which indicates that we can't accept any more collations at this relay parent.

Then on each seconded collation we can call seconded_and_pending_for_para_in_view and if we have reached the claim queue limit we can set the state to Satisfied and know that nothing can be accepted at this relay parent.

The benefits are that once the relay parent 'is complete' (all claim queue entries are filled) we won't need to run any checks in order to reject an incoming collation.

The drawbacks are that on each seconded collation we'll have to run the same function. So unless a collator is spamming us the benefit is minimal. And since we are limiting the number of advertisements per collator we can't get spammed too much.

We can explore this further but it better be a follow up task.

polkadot/node/network/collator-protocol/src/validator_side/mod.rs

Co-authored-by: Maciej <maciej.zyszkiewicz@parity.io>

tdimitrov · 2024-10-15T08:43:25Z

bot fmt

command-bot · 2024-10-15T08:43:30Z

@tdimitrov https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7573708 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 2-32f4f5d2-4847-4304-a72c-c83d5228c964 to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2024-10-15T08:46:26Z

@tdimitrov Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7573708 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7573708/artifacts/download.

tdimitrov · 2024-10-15T09:07:36Z

I see that you reverted to default lookahead value. Is the mystery of the failing zombienet test resolved?

Yes, turned out there is an issue after all: #4880 (comment)

…aim queue

tdimitrov · 2024-10-16T07:54:29Z

polkadot/node/network/collator-protocol/src/validator_side/mod.rs

+				?para_id,
+				"seconded_and_pending_for_para_in_view"
+			);
+			ancestors.iter().take(claim_queue_len).fold(0, |res, anc| {


Please refer to this discussion for details on take(claim_queue_len).

alindima

I still need to do a more in depth re-review but leaving some comments.

We should also add the new test to the yaml file so that it's run in the CI

polkadot/zombienet_tests/functional/0016-coretime-collation-fetching-fairness.zndsl

alindima · 2024-10-16T10:55:49Z

polkadot/zombienet_tests/functional/0016-coretime-collation-fetching-fairness.zndsl

+validator-0: js-script ./0016-verify-included-events.js return is 1 within 120 seconds
+
+# could be flaky feel free to remove if it's causing problems
+validator-2: count of log lines matching regex "Rejected v2 advertisement.*error=SecondedLimitReached" is greater than 1 within 10 seconds


why does the collator attempt to advertise more collations than scheduled?

alindima · 2024-10-16T10:57:30Z

polkadot/zombienet_tests/functional/0016-verify-included-events.js

+            });
+
+            if (block_count == 12) {
+                unsubscribe();


haven't written js in a while but isn't this a recursive call?

I took it from the documentation: https://polkadot.js.org/docs/api/examples/promise/listen-to-blocks

It works but I'm not at all proficient with JS so if you feel it's bad and have a suggestion how to fix it - I'd gladly do it :)

alindima · 2024-10-16T10:58:12Z

polkadot/zombienet_tests/functional/0016-verify-included-events.js

+    });
+
+    console.log(`Result: 2000: ${blocks_per_para[2000]}, 2001: ${blocks_per_para[2001]}`);
+    return (blocks_per_para[2000] == 6 && blocks_per_para[2001] == 1);


this is a 6:1 ratio. why?

Because I was testing some stuff and carelessly committed messed up version of the test 👎

I've fixed the whole test. I'm still worried it will be flaky but let's see.

Collation fetching fairness

f4738dc

tdimitrov added the T8-polkadot This PR/Issue is related to/affects the Polkadot network. label Jun 26, 2024

tdimitrov commented Jun 26, 2024

View reviewed changes

polkadot/node/network/collator-protocol/src/validator_side/collation.rs Outdated Show resolved Hide resolved

Comments

c7074da

tdimitrov commented Jun 26, 2024

View reviewed changes

polkadot/node/network/collator-protocol/src/validator_side/collation.rs Outdated Show resolved Hide resolved

tdimitrov added 4 commits June 26, 2024 16:39

Fix tests and add some logs

73eee87

Fix per para limit calculation in is_collations_limit_reached

fa321ce

Fix default TestState initialization: claim queue len should be equ…

96392a5

…al to `allowed_ancestry_len`

clippy

0f28aa8

tdimitrov force-pushed the tsv-collator-proto-fairness branch from c7f24aa to 0f28aa8 Compare June 28, 2024 08:19

Update is_collations_limit_reached - remove seconded limit

e5ea548

tdimitrov commented Jun 28, 2024

View reviewed changes

tdimitrov added 2 commits July 1, 2024 13:59

Fix pending fetches and more tests

9abc898

Remove unnecessary clone

c07890b

tdimitrov commented Jul 1, 2024

View reviewed changes

polkadot/node/network/collator-protocol/src/validator_side/collation.rs Outdated Show resolved Hide resolved

tdimitrov added 15 commits July 1, 2024 15:20

Comments

e50440e

Better var names

42b05c7

Fix pick_a_collation_to_fetch and add more tests

2f5a466

Fix test: collation_fetching_respects_claim_queue

ff96ef9

Add collation_fetching_fallback_works test + comments

e837689

More tests

91cdd13

Fix collation limit fallback

9f2d59b

Separate claim_queue_support from ProspectiveParachainsMode

a10c86d

Fix comments and add logs

b39858a

Update test: collation_fetching_prefer_entries_earlier_in_claim_queue

b30f340

Fix pick_a_collation_to_fetch and more tests

c0f18b9

Merge branch 'master' into tsv-collator-proto-fairness

703ed6d

Fix pick_a_collation_to_fetch - iter 1

fba7ca6

Fix pick_a_collation_to_fetch - iter 2

d4f4ce2

Remove a redundant runtime version check

5f52712

Consider what's scheduled on the core when determining assignments

cb69361

tdimitrov commented Oct 14, 2024

View reviewed changes

tdimitrov added 2 commits October 14, 2024 15:56

Merge branch 'master' into tsv-collator-proto-fairness

1142a90

Fix a clippy warning

4438349

tdimitrov requested review from alindima and Overkillus October 14, 2024 13:14

Overkillus reviewed Oct 14, 2024

View reviewed changes

tdimitrov and others added 2 commits October 15, 2024 08:47

Update PRdoc

e82c386

Apply suggestions from code review

4b2d4c5

Co-authored-by: Maciej <maciej.zyszkiewicz@parity.io>

".git/.scripts/commands/fmt/fmt.sh"

1c91371

Code review feedback

be34132

tdimitrov added 2 commits October 15, 2024 12:11

Fix a typo in prdoc

5c7b2ac

seconded_and_pending_for_para_in_view looks up to the len of the cl…

62c6473

…aim queue

tdimitrov commented Oct 16, 2024

View reviewed changes

tdimitrov added 2 commits October 16, 2024 13:43

Merge branch 'master' into tsv-collator-proto-fairness

9e3f62d

rerun CI

a4bc21f

alindima reviewed Oct 16, 2024

View reviewed changes

Fix zombienet test

6c103df

tdimitrov requested a review from a team as a code owner October 18, 2024 08:10

tdimitrov added 7 commits October 18, 2024 14:59

Merge branch 'master' into tsv-collator-proto-fairness

15e3a74

Relax expected block counts for each para

d6b35ca

Bump lookahead and decrease timeout

586b56b

Fix ZN pipeline - try 1

a04d480

Fix ZN pipeline - try 2

13d5d15

Fix ZN pipeline - try 3

86870d0

Fix ZN pipeline - try 4

7b822af

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collation fetching fairness #4880

Collation fetching fairness #4880

tdimitrov commented Jun 26, 2024 •

edited

Loading

tdimitrov Jun 28, 2024

alindima Jul 8, 2024

tdimitrov Jul 8, 2024

eskimor Jul 19, 2024

alindima Sep 23, 2024

tdimitrov Oct 3, 2024

Overkillus Oct 14, 2024

tdimitrov Oct 14, 2024

Overkillus left a comment

Overkillus Oct 14, 2024

tdimitrov Oct 15, 2024

Overkillus Oct 14, 2024

tdimitrov Oct 15, 2024

tdimitrov commented Oct 15, 2024

command-bot bot commented Oct 15, 2024 •

edited

Loading

command-bot bot commented Oct 15, 2024

tdimitrov commented Oct 15, 2024

tdimitrov Oct 16, 2024

alindima left a comment

alindima Oct 16, 2024

alindima Oct 16, 2024

tdimitrov Oct 18, 2024

alindima Oct 16, 2024

tdimitrov Oct 18, 2024

Collation fetching fairness #4880

Are you sure you want to change the base?

Collation fetching fairness #4880

Conversation

tdimitrov commented Jun 26, 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

Overkillus left a 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

tdimitrov commented Oct 15, 2024

command-bot bot commented Oct 15, 2024 • edited Loading

command-bot bot commented Oct 15, 2024

tdimitrov commented Oct 15, 2024

Choose a reason for hiding this comment

alindima left a 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

tdimitrov commented Jun 26, 2024 •

edited

Loading

command-bot bot commented Oct 15, 2024 •

edited

Loading