paritytech
Convert `Peerset` into a shared sync struct
Resolves https://github.com/paritytech/substrate/issues/11922.
Follow-up
Things could be simplified further by removing Message::Accept and Message::Reject variants from sc_peerset::Message and making Peerset::incoming() return result synchronously. Unfortunately, this requires modifying Notifications, and as agreed with @altonen and @bkchr should be done only after it's better covered by tests.
The CI pipeline was cancelled due to failure one of the required jobs. Job name: test-linux-stable Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2202882
A note to not forget what is happening here. Before this PR, Peerset had a channel for incoming commands sent via PeersetHandle (rx). These commands were only processes in poll() after all the outgoing events from message_queue with commands for Notifications were sent out. This way Notifications received consistent stream of events from Peerset.
After removing the channel for incoming commands and making the interface for incoming commands synchronous, it became possible that the command from Notifications and the event-reaction to it are interleaved by old out-events not yet dispatched from message_queue. As a result, the interface contract between Notifications and Peerset is broken.
One possible solution would be to make the Notifications-looking part of the Peerset interface also synchronous. E.g., instead of pushing the out-event with Accept and Reject to the end of the queue, we can return the decision immediately from Peerset::incoming(). This requires changing the Notifications code and is blocked by https://github.com/paritytech/substrate/pull/13033.
Nevertheless, it's unclear whether this would be sufficient, because Connect and Drop out-events would still be buffered in the message_queue and only delivered to Notifications during polling.
Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.
![stale[bot] avatar](https://avatars.githubusercontent.com/in/1724?v=4 "Published by a pub.dev verified publisher"){kind=small}
Yes, I'm going to continue working on this.
Btw, based on this comment, there may be some quite significant changes to the peerset at some point.
Btw, based on this comment, there may be some quite significant changes to the peerset at some point.
Yeah, the race conditions @tomaka is mentioning are also in the Peerset, and exactly the reason why this refactoring was not finished in one go. May be if we manage to make Peerset synchronous, it would be easier to transit to synchronous outer (external) API. Or we can try to crystallize the solution with outer API and skip the step of making Peerset synchronous completely.
Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.
Superseded by https://github.com/paritytech/substrate/pull/13611. Closing this PR.