Conversation
tomaka
added
A0-please_review
| PRIORITY_GROUP.to_owned(), | ||
| // from them until removed from the set | ||
| if let Err(e) = network_service.add_peers_set_reserved( | ||
| super::COLLATION_PROTOCOL_NAME.into(), |
There was a problem hiding this comment.
Here we use the same set for collation peer set and validation peer set, I assume this is not intentional. In the initial design we passed the peer set id in the request (would be a parameter in this on_request function), we can bring it back.
But I still don't understand how validators are supposed to have a collators peer set without knowing their addresses. IIUC a request with COLLATION_PROTOCOL_NAME means that we are collators trying to connect to validators. so that would create a peer set on the collator side, not validator side.
There was a problem hiding this comment.
I still don't understand how validators are supposed to have a collators peer set without knowing their addresses
If by "address" you mean a multiaddr (IP address/port), it's contained in the second parameter of add_peers_set_reserved.
There was a problem hiding this comment.
I still don't understand how validators are supposed to have a collators peer set without knowing their addresses
If by "address" you mean a multiaddr (IP address/port), it's contained in the second parameter of
add_peers_set_reserved.
I thought for some reason these multiaddresses are of validators participating in the collation protocol, not collators. Is that wrong? Do we run authority discovery on all collators?
There was a problem hiding this comment.
Indeed, these are the addresses of the validators, not of the collators. The code is correct when run from the collator's point of view. From the validator's point of view, we rely on inbound slots to accept incoming connections from collators.
The code should eventually change so that in the future validators don't connect to other validators through this protocol, but only through collators.
node/network/bridge/src/lib.rs
Outdated
| in_peers: 0, | ||
| out_peers: 0, | ||
| reserved_nodes: Vec::new(), | ||
| non_reserved_mode: sc_network::config::NonReservedPeerMode::Deny, |
There was a problem hiding this comment.
We definitely need to split this into more fine grained protocols. For availability distribution to be able to connect to lots of peers, but also because of this. NonReservedPeerMode is not appropriate for availability recovery, which might be used by collators or other full nodes as well. Obviously this should not block this PR, as this is work that will build on this.
| tracing::warn!(target: LOG_TARGET, err = ?e, "AuthorityDiscoveryService returned an invalid multiaddress"); | ||
| } | ||
| if let Err(e) = network_service.add_peers_set_reserved( | ||
| super::VALIDATION_PROTOCOL_NAME.into(), |
There was a problem hiding this comment.
@ordian We also add the authorities to the reserved nodes of the validator set here. It makes sense to me to add the authorities to both the collation protocol peer set and the validation protocol peer set.
There was a problem hiding this comment.
The way it is currently implemented, we will have two identical sets (COLLATION_PROTOCOL_NAME and VALIDATION_PROTOCOL_NAME). I think the intention is to pass the PeerSet here from the bridge and add to only one set depending on the PeerSet.
There was a problem hiding this comment.
Ok, got it. Depending on whether the node is a validator or collator it suffices if the authorities are added to one of those sets. That makes sense.
There was a problem hiding this comment.
As far as I understand, this on_request function is called only in response to a NetworkBridgeMessage::ConnectToValidators. Right now this ConnectToValidators message is only ever emitted indirectly from here. Am I supposed to send two messages, once for PeerSet::Collation and once for PeerSet::Validation?
There was a problem hiding this comment.
We can pass the peerset as part of the ConnectToValidators message and connect_to_validators* functions, I think this was the initial design.
|
The collation integration test is failing. I don't really know if the logic of what I'm doing in this PR is correct, so debugging it is kind of tricky for me. If someone has an idea, I'd appreciate. |
|
I'd like to see #2095 (comment) addressed, otherwise looks good. |
Since this PR doesn't modify the existing behaviour nor API, I'd suggest to do this afterwards? |
Sure, we can do it in a separate PR. Technically, we used only one peerset/priority group previously, but behavior should be the same. |
ordian
added
the
A4-companion
|
Waiting for commit status. |
ghost
merged commit 3 participants
Companion for paritytech/substrate#7700