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

Consider merging service.rs and peers.rs together #478

Closed
tomaka opened this issue Apr 26, 2023 · 3 comments · Fixed by #1200
Closed

Consider merging service.rs and peers.rs together #478

tomaka opened this issue Apr 26, 2023 · 3 comments · Fixed by #1200

Comments

@tomaka
Copy link
Contributor

tomaka commented Apr 26, 2023

When thinking about how to update peers.rs in the context of #111 and #44, it seems to me that the logic peers.rs would be quite intertwined with the one in service.rs.
In other words, the way the logic of the networking is split between peers.rs and service.rs isn't really appropriate anymore.

It would make sense to me to remove peers.rs entirely, and move what it is doing within service.rs.

Of course, we can't just move all the fields of peers.rs within the service, as it would become way too complex, but we can add more state machines that help the service do what it has do to.

@tomaka
Copy link
Contributor Author

tomaka commented Sep 26, 2023

I think that the best course of action is to "soft-rewrite" service.rs.

service.rs is kind of tricky, because it's unclear what parts of the high-level networking behavior should be customizable by the top-level code (in full-node and light-base). In that kind of situation, an appropriate course of action is to have multiple copies of the same module for the different use cases.

Of course, these different copies would share a lot of code.


What we need from the networking service is:

  • Hold and manage the list of active connections.
  • Hold and manage the list of pending connections.
  • Hold and manage Kademlia.
  • Hold and manage a list of discovered peers and their addresses.
  • Hold and manage the list of outgoing peers slots. We don't need incoming peer slots anymore, I think, as this is actually detrimental to the network.
  • For collators and validators, a way to remain connected to specific validators.
  • A way to send strongly-typed notifications and requests to connected peers/validators.
  • A way to answer incoming requests other than Kademlia's.

One problem that service.rs currently suffers from is the fact that failing a connection attempt immediately unassigns the peer slot.
Instead, the peer slot should be marked as "failed to connect" and then manually cleared and re-allocated by the API user.


When it comes to peers and addresses to decide who to connect to, it should be a separate mechanism from Kademlia, just like in Substrate.

Peers and addresses discovered through Kademlia are untrusted. In other words, if Alice sends to Bob the list of addresses of Charlie, Alice might lie and give fake addresses. Charlie should not suffer if that is the case.

It's still unclear to me how to manage these addresses. As a reminder, the algorithm used in Substrate is leaky.

@tomaka
Copy link
Contributor Author

tomaka commented Sep 26, 2023

Also we can do #111 and #44 by rewriting service.rs, as all the necessary primitives are now available in collection.rs.

@tomaka
Copy link
Contributor Author

tomaka commented Sep 27, 2023

Hold and manage the list of active connections.
Hold and manage the list of pending connections.

So there's actually no need to differentiate active from pending connections.
We obviously need to differentiate connections before their handshake vs after their handshake, but when it comes to "pending" vs "active" not only the definition is blurry but this distinction isn't useful in any way.

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

Successfully merging a pull request may close this issue.

1 participant