-
Notifications
You must be signed in to change notification settings - Fork 1.6k
validator_discovery: less flexible, but simpler design #3052
Conversation
investigating why adder-collator test if failing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked the formerly modular subsystem independent approach, but as it was only half baked anyway (subsystems would not have received PeerConnected
events if another subsystem had issued connection requests earlier) and we don't need that modularity right now anyway, I think it is a fine simplification.
node/network/availability-distribution/src/pov_requester/mod.rs
Outdated
Show resolved
Hide resolved
Do you mean that subsystem A would issue a connection request before subsystem B even started? I don't think that was the case in practice, because A would have to wait for the first |
It is definitely not an issue in practice. Just conceptually, if one subsystem issues connection requests later than another one, it would miss out on Either way all of this no longer matters with this simplification as we now rely on a single source of connection requests. |
#3055 explains the failing test 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validator_discovery.rs
looks good to me
Note that sc-network
also provides set_reserved_peers
that does the same thing 😅
EDIT: the peerset contains the appropriate logic, but it's actually not exposed in the public API of sc-network
LOL, ok I think we should use that then. At least in a follow up. |
yeah, I think it's not exposed on |
* master: validator_discovery: less flexible, but simpler design (#3052)
This PR implement and alternative design for validator discovery mentioned in #3035 (comment). As a bonus, it makes #3035 simpler to implement.
Instead of doing reference counting, we assume that the request contains the full list of peers all subsystems are interested in (per PeerSet). This is less flexible, but much simpler and hence less prone to bugs like #3038.