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

p2p: design transport agnostic peer abstraction #692

Closed
1 task
melekes opened this issue Nov 20, 2020 · 8 comments · Fixed by #904
Closed
1 task

p2p: design transport agnostic peer abstraction #692

melekes opened this issue Nov 20, 2020 · 8 comments · Fixed by #904
Assignees
Labels
p2p Peer-to-peer networking structure High level repo-wide structural concerns

Comments

@melekes
Copy link
Contributor

melekes commented Nov 20, 2020

To enable flexibility with regards to transport implementations - concerning layer4, encryption, handshake - a proper abstraction should be designed. Aiming for clear boundaries into the core domain and enabling node implementations built on top of it to be future proof should a migration to other transports take place. A comparable effort is under way in the Go implementation of Tendermint in ADR 62.

Important consideration

  • single connection, multiplexed streams
  • attached metadata for
    • capability tracking
    • potential hole punching
    • potential QoS and rate limiting

Acceptance criteria

  • peer-reviewed ADR
@xla xla added p2p Peer-to-peer networking dependencies Pull requests that update a dependency file structure High level repo-wide structural concerns and removed dependencies Pull requests that update a dependency file labels Nov 20, 2020
@xla xla changed the title p2p: design a transport abstraction p2p: deisgn peer abstraction Nov 20, 2020
@xla xla changed the title p2p: deisgn peer abstraction p2p: design peer abstraction Nov 20, 2020
@xla xla changed the title p2p: design peer abstraction p2p: design transport agnostic peer abstraction Nov 20, 2020
@xla xla self-assigned this Nov 20, 2020
@thanethomson
Copy link
Contributor

Looking at #708, is the idea to implement a non-async P2P layer?

@xla
Copy link
Contributor

xla commented Dec 11, 2020

Looking at #708, is the idea to implement a non-async P2P layer?

In short yes. In long also yes. Would it be appreciated to outline the rational for this at this stage?

@thanethomson
Copy link
Contributor

thanethomson commented Dec 11, 2020

In short yes. In long also yes. Would it be appreciated to outline the rational for this at this stage?

It might be worthwhile sync'ing (🧀) up about this sometime. Right now I'm looking at options for ABCI (server and client), and if we're going to vertically integrate all these components eventually it could be painful to mix sync and async networking code.

@xla
Copy link
Contributor

xla commented Dec 11, 2020

It might be worthwhile sync'ing (cheese) up about this sometime. Right now I'm looking at options for ABCI (server and client), and if we're going to vertically integrate all these components eventually it could be painful to mix sync and async networking code.

Sounds great! Would it be possible to do this in a structured form, as in what we expected outcomes to be? Also was there a lasting decision for the asyncing? I remember this being an agenda item for technical decision making for the repository.

Thanks for raising this point early.

@thanethomson
Copy link
Contributor

Sounds great! Would it be possible to do this in a structured form, as in what we expected outcomes to be? Also was there a lasting decision for the asyncing? I remember this being an agenda item for technical decision making for the repository.

So #318 was the primary issue that triggered this debate, and the ideal outcome would be to have code that plays nicely with both sync and async code. We've managed to figure out how to mix the two for the Light Client, but I'm concerned that as we build up the full stack we're going to be introducing unnecessary complexity by trying to support both.

Is your current aim to eventually be able to support both? If so, the outcome of the discussion should be a clear technical strategy as to how to accomplish that. Otherwise, the outcome needs to be a decision as to which of the two approaches to choose to apply to the entire stack.

This comment, I felt, was quite pertinent to the broader discussion regarding the sync/async question.

@xla
Copy link
Contributor

xla commented Dec 13, 2020

There is so much to unpack here and I think an actual sync of the project maintainers would be beneficial. To make it productive there should be a basis for the discussion, for that I'd like to prepare an outline of the current thinking and hopefully capture the trade-offs for different approaches. How does that sound?

So #318 was the primary issue that triggered this debate, and the ideal outcome would be to have code that plays nicely with both sync and async code. We've managed to figure out how to mix the two for the Light Client, but I'm concerned that as we build up the full stack we're going to be introducing unnecessary complexity by trying to support both.

Ideally the call if certain amount of complexity is necessary can only come after establishing a shared understanding of what this part of the codebase will be optimised for. It will likely have quit a different set of requirements/constraints compared to the Light Client.

Is your current aim to eventually be able to support both? If so, the outcome of the discussion should be a clear technical strategy as to how to accomplish that. Otherwise, the outcome needs to be a decision as to which of the two approaches to choose to apply to the entire stack.

Do you see these things being dependent? As in the decision that is made in this context will inform the entire codebase?

This comment, I felt, was quite pertinent to the broader discussion regarding the sync/async question.

While the comment is insightful and correct for many reasons, the whole discussion is disguised as a comparison between futures driven by tokio against a threaded implementation. At no point are actual problems with async mentioned, nor is there a proper probing into what the right I/O model might be, which really is where the discussion should start, before jumping to the conclusion how to schedule it.

All of this points into a similar direction as your comment about a technical strategy and it might be an opportune point before building the basis of networking for node implementations to come.

@tony-iqlusion
Copy link
Collaborator

tony-iqlusion commented Dec 14, 2020

Just a quick note on the TMKMS side of things: per Tendermint ADR 063, we plan on migrating the "privval" connection to gRPC (set to ship in Tendermint v0.35), in which case it will be one less thing to worry about re: P2P connections.

@thanethomson
Copy link
Contributor

what the right I/O model might be

This is definitely the best way to frame the discussion. I'd really dig to hear your perspectives on this @xla and @melekes, and @tony-iqlusion, if you want to weigh in on that discussion it'd be incredibly valuable!

I'll try to coordinate some synchronous discussion time for this. It may only happen once everyone's back from vacation in January though, but this discussion need not be a show-stopper at all. I feel I'm getting better at refactoring Rust networking code/abstractions, so I'm quite comfortable with changing directions if/when necessary 😁

@thanethomson thanethomson added this to the Seed Node milestone Apr 7, 2021
@xla xla mentioned this issue Jun 3, 2021
6 tasks
xla added a commit that referenced this issue Jun 13, 2021
Describes broadly a new set of abstractions to support a transport
agnostic p2p stack. Bundled together are the types and traits in
`p2p::transport`. Other parts like the `Supervisor` will land in
follow-up change-sets.

Closes #692

Signed-off-by: Alexander Simmerl <[email protected]>
thanethomson pushed a commit that referenced this issue Jun 15, 2021
Describes broadly a new set of abstractions to support a transport
agnostic p2p stack. Bundled together are the types and traits in
`p2p::transport`. Other parts like the `Supervisor` will land in
follow-up change-sets.

Closes #692

Signed-off-by: Alexander Simmerl <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2p Peer-to-peer networking structure High level repo-wide structural concerns
Projects
None yet
4 participants