collator-protocol: preconnect (v2) #5321

slumber · 2022-04-13T11:30:29Z

In short: whenever possible, a collator should establish a connection to validators group in advance (for a given active leaf prepare for collating on the child)
Stuff to take into account: group rotations, if we're close to it, issue connections to both current and the next group assigned to our core

Assumptions:

When preconnecting we assume that our core is currently occupied, this is not always true for parathreads
This doesn't work on session boundaries. A collator will still issue a connection to the correct group when distributing a collation, but the preconnect will be a waste.
I have a feeling that these changes may be very well reverted when async backing lands. But may help until this time.

rphmeier · 2022-04-14T14:14:09Z

Could you add a PR description summarizing changes, intent, and assumptions?

rphmeier · 2022-04-14T14:24:03Z

I don't have a strong opinion on this change (assuming it helps to some extent on Kusama) but I would like to think ahead to asynchronous backing somewhat. In the future stuff like pre-connect will be less important because the buffering will happen at the validator/backing level. There'll be some changes that might affect things here, particularly related to how collations are generated.

e.g. in #5056 we propose moving the logic of when to schedule a collation entirely to the Cumulus implementation (or other collator implementation). In that model, collation-generation becomes a relatively simple wrapper, which the collator can query for information about the latest relay-chain state and which sends produced collations over to the collator-protocol subsystem.

This changeset isn't very large, so we shouldn't really mind doing some rewriting of these changes if they get us some short-term benefits in real world deployments.

slumber · 2022-04-14T14:33:03Z

@rphmeier I've updated the description

rphmeier · 2022-04-15T13:08:57Z

When preconnecting we assume that our core is currently occupied, this is not always true for parathreads

Wouldn't we assume that our core is currently unoccupied? And why is this assumption necessary?

slumber · 2022-04-15T15:38:49Z

When preconnecting we assume that our core is currently occupied, this is not always true for parathreads

Wouldn't we assume that our core is currently unoccupied? And why is this assumption necessary?

The pipeline is as follows:

At relay parent R our core is scheduled, we distribute a candidate based on R - 1 to our group
At R + 1 the core is occupied, but the collator knows it's going to provide the collation once the core is free again, so it issues preconnect request to collator-protocol.
At R + 2 the core is scheduled and we're already connected, go to 1.

…econnect-v2

rphmeier · 2022-04-19T01:06:59Z

While I think it makes sense to have some preconnection logic for occupied cores, this does still maintain some connection logic for when the core is free, right?

At R + 1 the core is occupied, but the collator knows it's going to provide the collation once the core is free again, so it issues preconnect request to collator-protocol.

In practice, how is this "the collator knows" logic implemented in the Cumulus side? Not all collators author parablocks all the time.

node/primitives/src/lib.rs

node/collation-generation/src/lib.rs

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

rphmeier · 2022-04-19T01:23:21Z

node/network/collator-protocol/src/collator_side/mod.rs

+	/// backed in the grandchild of relay parent, therefore based on the child.
+	/// The core is expected to be occupied.
+	///
+	/// **Important note**: parathread cores very well may happen to be free.


Tangentially related, but as you have studied this part of the code and thought about parathreads, what information do you think the runtime needs to provide about parathread cores in the future? Since we also want to experiment with other kinds of exotic scheduling (2 or 3 chains sharing 1 slot and taking turns, for instance), it would be nice to have a unified abstraction for scheduling information in the runtime API.

For the current case when a parachain takes a core for the whole session duration it's fine to rely on scheduled-occupied-scheduled... behavior

With parathreads a core can happen e.g. to be free-scheduled-occupied, so we need to be aware in advance for which para id the core is going to be scheduled.

On the other hand, with async backing this change will be unnecessary, we could probably just drop it later

rphmeier · 2022-04-19T01:24:54Z

node/network/collator-protocol/src/collator_side/mod.rs

+/// if the collator is aware of its assignment on the child of the given relay parent.\
+/// Connects to both current and the next group if the block is
+/// [close](NEXT_GROUP_PRECONNECT_WINDOW) to the group rotation boundary.
+async fn preconnect<Context>(


Unlike the is_preconnect_allowed counterpart, I think this is cheap enough without refactoring, as it only has to run once per relay-parent at most.

node/subsystem-types/src/messages.rs

rphmeier

I gave light review. Not planning to dive too much deeper. Looks broadly sensible, as in this is behavior that is probably somewhat useful. Thanks!

However, since this is meant to close 5255, I don't really see this doing that. The original plan in #5255 was to query the collator before invoking fn produce_collation with something like a fn will_produce_collation and then connect to the validators of the current group if will_produce_collation is true.

slumber · 2022-04-19T05:02:51Z

I gave light review. Not planning to dive too much deeper. Looks broadly sensible, as in this is behavior that is probably somewhat useful. Thanks!

However, since this is meant to close 5255, I don't really see this doing that. The original plan in #5255 was to query the collator before invoking fn produce_collation with something like a fn will_produce_collation and then connect to the validators of the current group if will_produce_collation is true.

During the discussion with @eskimor we decided that implementing both Connect and Preconnect doesn't make sense since both serve the same purpose. Preconnect is simply one-relay-parent shifted and more efficient.

It's also mentioned in #5255

slumber · 2022-04-19T05:06:45Z

While I think it makes sense to have some preconnection logic for occupied cores, this does still maintain some connection logic for when the core is free, right?

According to my code, preconnect will not work if the core is free. I don't really know how to discover our validators group in this case.

At R + 1 the core is occupied, but the collator knows it's going to provide the collation once the core is free again, so it issues preconnect request to collator-protocol.

In practice, how is this "the collator knows" logic implemented in the Cumulus side? Not all collators author parablocks all the time.

For AuRa we can try to claim slot + 1, if we succeed, return true

rphmeier · 2022-04-19T23:36:23Z

preconnect will not work if the core is free. I don't really know how to discover our validators group in this case.

It should be trivial if the core is scheduled. If the core is free, yeah, nothing to do.

ordian · 2022-04-20T09:11:23Z

Note that #4381 also includes

we should also take care of collators disconnecting once they are done with their collation

which doesn't seem to be taken care of in this PR. To disconnect, we need to issue an empty ConnectToValidators request.

slumber · 2022-04-20T09:40:08Z

Note that #4381 also includes

we should also take care of collators disconnecting once they are done with their collation

which doesn't seem to be taken care of in this PR. To disconnect, we need to issue an empty ConnectToValidators request.

In my opinion it makes sense to do it in a separate task, unless we can come up with a simple solution here. We have to figure out when to disconnect

eskimor

Nice work overall! Thanks!

node/collation-generation/src/lib.rs

eskimor · 2022-04-20T09:48:04Z

node/collation-generation/src/tests.rs

-
-		fn poll(self: Pin<&mut Self>, _cx: &mut FuturesContext) -> Poll<Self::Output> {
-			Poll::Ready(Some(CollationResult { collation: test_collation(), result_sender: None }))
+	#[async_trait::async_trait]


We should add a small test for the pre-connect functionality as well, I think.

node/network/collator-protocol/src/collator_side/mod.rs

eskimor · 2022-04-20T10:44:38Z

node/network/collator-protocol/src/collator_side/mod.rs

+	)
+	.await?;
+
+	connect_to_validators(ctx, validators_group.validators.into_iter().collect()).await;


This stateless connection mechanism can't really work, but the issue was present before already and could be an explanation for short stalls:

If we have two forks with different height, they might be in different backing groups, hence they would disconnect each other. I believe we had this discussion before ... @ordian I believe you had some reasoning, why this is not a problem?

This might be a problem at group rotation boundaries. In #4640 (was reverted) we used a union of those groups. Maybe we should prefer the new group to the old one in this case?

Doesn't making NEXT_GROUP_PRECONNECT_WINDOW big enough cover this issue? How big the spread of block number in our view can be?

That would still lead to stalls, if the shorter fork is actually the better one. We should just make sure to be connected to both - I will have a look at that.

Doesn't making NEXT_GROUP_PRECONNECT_WINDOW big enough cover this issue? How big the spread of block number in our view can be?

it is not really limited, larger spreads are just becoming increasingly unlikely. Indeed for pre-connect it will likely be less of a problem, due to NEXT_GROUP_PRECONNECT_WINDOW.

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

slumber added 3 commits April 12, 2022 18:58

Collator protocol: collator side for preconnect

1213e63

Validator side

94b4715

Spellcheck

6af673f

Validator side tests

997c3e2

slumber added 2 commits April 18, 2022 22:33

Replace CollatorFn with a trait

962e29c

Merge remote-tracking branch 'origin/master' into slumber-collator-pr…

b2bee7f

…econnect-v2