Improve collator side of the collator-protocol #1955
Conversation
This pr improves the collator-protocol implementation of the collator side. Besides cleaning up code and rewriting it, the following changed: - Before on `PeerViewChange` we send an advertisment to every peer, now this only happens for validators. - It also adds a check that we send an advertisment message only once for a connected peer. - If the same validator was part of the current and next group, we requested to be connected to this validator two times. This is also fixed now. - Instead of having only one connection request, we now are being able to store multiple of them. This is required as we can have multiple active leafs at any point of time.
bkchr
added
A0-please_review
That is an expected behaviour. Validator discovery has reference counting for each requested peer to know when to disconnect. |
| async fn distribute_collation<Context>( | ||
| ctx: &mut Context, | ||
| async fn distribute_collation( | ||
| ctx: &mut impl SubsystemContext<Message = CollatorProtocolMessage>, |
There was a problem hiding this comment.
Also, we please not carpet-format things and mix it with other changes 🙏? AFAIK different subsystems atm use both variants of this syntax.
There was a problem hiding this comment.
Nope. When I fix stuff, I'm doing it everywhere. A better solution would anyway to implement all these functions for the state struct. It makes no sense that all these functions are free standing, this is not C.
There was a problem hiding this comment.
It makes no sense that all these functions are free standing, this is not C.
This is a stylistic difference I strongly disagree with you on. Rust excels when used in a functional style, especially in async/await.
There was a problem hiding this comment.
Other sub systems are exactly written in the style I mentioned. I also see no benefit to always pass the same two parameters in state and context to every function.
And async/await doesn't require that you only use free standing functions.
|
Could you update to #1966? |
| res = state.connection_requests.next().fuse() => { | ||
| let (relay_parent, validator_id, peer_id) = match res { | ||
| Some(res) => res, | ||
| None => continue, |
There was a problem hiding this comment.
this is unreachable IIUC, it's fine, but maybe add a comment
There was a problem hiding this comment.
Yeah that is unreachable, another reason I had a Future before :D
There was a problem hiding this comment.
I would say we should write the caller code without relying on any quirkiness of the callee so that when after two years someone changes the behavior of the callee things do not break here. Usually saying something like "malloc does not fail anyway" is not ok. But this is not the hill I am willing to die on at the moment, so ok.
There was a problem hiding this comment.
I agree, ideally we want to enforce it via type system. But there is no abstraction for an infinite stream AFAIK, so emulating it with a stream that never returns a None is our best option so far.
| state.peer_views.remove(&peer_id); | ||
| state.declared_at.remove(&peer_id); |
There was a problem hiding this comment.
do we also need to remove it from peer_ids mapping in our_validator_groups?
There was a problem hiding this comment.
No it is fine to stay in there. our_validator_groups is mainly used to track PeerId -> ValidatorId mapping and will be cleaned up when the corresponding leaf is closed.
| res = state.connection_requests.next().fuse() => { | ||
| let (relay_parent, validator_id, peer_id) = match res { | ||
| Some(res) => res, | ||
| None => continue, |
There was a problem hiding this comment.
I would say we should write the caller code without relying on any quirkiness of the callee so that when after two years someone changes the behavior of the callee things do not break here. Usually saying something like "malloc does not fail anyway" is not ok. But this is not the hill I am willing to die on at the moment, so ok.
* master: Improve collator side of the collator-protocol (#1955)
This pr improves the collator-protocol implementation of the collator
side. Besides cleaning up code and rewriting it, the following changed:
PeerViewChangewe send an advertisment to every peer, nowthis only happens for validators.
for a connected peer.
requested to be connected to this validator two times. This is also
fixed now.
to store multiple of them. This is required as we can have multiple
active leafs at any point of time.