-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rewrite network::ChainNetwork
to not use the peers
module
#1200
Conversation
d54e54b
to
1bb23bc
Compare
f98dffe
to
52d11c9
Compare
I've more or less finished designing the API of the new service. A few changes have been made compared to the current service:
When it comes to peer addresses management, I'm going to keep the same algorithm(s) as right now and made changes to it after this PR has landed. |
After reflection, I'm going to remove Kademlia and the address book from the service state machine in |
52d11c9
to
038cc75
Compare
The code in this PR is working surprisingly well. |
connection_id: service::ConnectionId, | ||
mut connection_task: service::SingleStreamConnectionTask<Instant>, | ||
mut coordinator_to_connection: channel::Receiver<service::CoordinatorToConnection>, | ||
connection_to_coordinator: channel::Sender<super::ToBackground>, | ||
) { | ||
// Finishing ongoing connection process. | ||
let socket = match socket.await.map_err(|_| ()) { |
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.
This removes the timeout under the idea that the ChainNetwork
will handle the timeout. This is true, except that this code doesn't actually process the messages from the ChainNetwork
.
unreachable!() | ||
}; | ||
|
||
let mut socket = pin::pin!(match platform.connect_stream(address).await { |
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.
Same idea here
full-node/src/network_service.rs
Outdated
basic_peering_strategy::AssignOutSlotOutcome::Assigned(peer_id) => { | ||
peer_id.clone() | ||
} | ||
basic_peering_strategy::AssignOutSlotOutcome::AllPeersBanned { .. } // TODO: handle `AllPeersBanned` by waking up when a ban expires |
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.
Should be done before merging
Another thing to change: we don't actually need to track "in slots", as we can consider all the peers that we are connected to but don't have an "out slot" as automatically having an "in slot". EDIT: done |
I'm going to merge this PR 🎉 and open issues for the remaining things to fix, as these things to fix require some changes in other parts of the code and I'd rather not put changes to many different components in a single PR. |
Close #478
Close #111
Close #1090
Close #381
Doesn't tackle #44, as it is a bit tricky to get right. This will be done in a further PR.
This PR also assumes that #391 will be merged at some point. It is unclear at the moment whether #391 needs to be merged beforehand.
Remains to do, at the time of opening of this draft:
peers.rs
and the old service (very easy).