Skip to content
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

Merged
merged 78 commits into from
Oct 27, 2023

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Sep 28, 2023

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:

  • Kademlia requests and Kademlia in general.
  • The address book, which I'm not sure how to design.
  • Transition the full-node and light-base network services to use the new service (should be very easy, as the API is mostly the same).
  • Remove peers.rs and the old service (very easy).

@tomaka tomaka force-pushed the service-rewrite branch 6 times, most recently from d54e54b to 1bb23bc Compare October 10, 2023 16:01
@tomaka tomaka force-pushed the service-rewrite branch 3 times, most recently from f98dffe to 52d11c9 Compare October 16, 2023 13:33
@tomaka
Copy link
Contributor Author

tomaka commented Oct 16, 2023

I've more or less finished designing the API of the new service.

A few changes have been made compared to the current service:

  • In and out slots are no longer handled directly by the service. The API user has to manually accept or deny each incoming gossip request, and "out slots" are now called "gossip desired".

  • There's no concept of "pending connection" anymore. This previously designated connections that are still in their pre-libp2p initialization phase. There's actually no need to differentiate this phase, and we can just treat them as connections that are still handshaking. After all, the TCP handshake for example is also a handshake, just not handled directly by smoldot. If necessary, whether the remote is reachable can be determined by whether at least one byte has been received, but I don't really see why there would be a need to differentiate connections that are unreachable from connections that can't finish a handshake.

  • OutRequest and InRequest are now SubstreamId.

  • KademliaOperationId is now OperationId.

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.

@tomaka
Copy link
Contributor Author

tomaka commented Oct 18, 2023

After reflection, I'm going to remove Kademlia and the address book from the service state machine in lib, and put them directly in the high-level network services.

@tomaka
Copy link
Contributor Author

tomaka commented Oct 25, 2023

The code in this PR is working surprisingly well.
The main thing that is broken is more or less Kademlia. Before this PR, we perform discovery by picking a random PeerId then querying the peer closest to this PeerId. After this PR, we pick a random PeerId but then ask any peer, not necessarily the closest. This leads to suboptimal results, but it's fine for the time being.

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(|_| ()) {
Copy link
Contributor Author

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 {
Copy link
Contributor Author

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 Show resolved Hide resolved
full-node/src/network_service.rs Outdated Show resolved Hide resolved
full-node/src/network_service.rs Show resolved Hide resolved
full-node/src/network_service.rs Outdated Show resolved Hide resolved
full-node/src/network_service.rs Show resolved Hide resolved
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
Copy link
Contributor Author

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

full-node/src/network_service.rs Show resolved Hide resolved
full-node/src/network_service.rs Show resolved Hide resolved
@tomaka
Copy link
Contributor Author

tomaka commented Oct 26, 2023

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".
This can lead to a simplification of the peering strategy code.

EDIT: done

@tomaka
Copy link
Contributor Author

tomaka commented Oct 27, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant