Replies: 4 comments
-
I very much appreciate your constructive actionable feedback. That said I don't think I am the right person to respond to most of this feedback. 1. Remove Substream type from StreamMuxerNot the right person to answer this one. 2. Remove ConnectionInfo as a trait, replace it with PeerIdOff the top of my head I agree, but again, I don't think I am the right person here. 3. Use boxing in reasonable places.I agree that dynamic dispatching should not be avoided at all costs and I also think that this mindset is reflected in the recent rust-libp2p changes, e.g. see #1794. In case you have specific components in mind that would benefit from dynamic dispatching, suggestions are most welcome. 5. Move away from custom built combinators for Transport and UpgradeCan you expand on how the current design is restricting you? 6. Make behaviors independent.Yes, the fact that behaviours have to be constructed as hierarchies does make composition hard, your IPFS Kademlia example being one such case. One could run multiple Kademlia instances within the Instead of designing deeply nested behaviour hierarchies, one can already have a flat hierarchy and a single root behaviour passing messages between the sub behaviours. This is a bit similar to your "separate control flow construct" suggestion, just in a synchronous fashion.
The problem I see with structuring behaviours as independent components running in parallel is the difficulty of concurrency. Reasoning about a synchronous behaviour hierarchy with a single control flow is, in my eyes, easier compared to a set of behaviours running concurrently or even in parallel.
Can you expand on how your suggestions would simplify the implementation of libp2p-relay (#1134)? |
Beta Was this translation helpful? Give feedback.
-
This genericity came from the fact that, in Substrate/Polkadot, we wanted to pass to the upper layers the ephemeral public key used during the encryption handshake, as a way to add a per-connection key.
To me the problem of boxing isn't performances, but about having to decide whether to use While this wasn't necessarily obvious in the past, it is now clearer that asynchronous I/O in Rust almost always uses
The issue in doing this is, as often, the lack of existential types in the Rust language. If you use non-temporary closures or futures, then you most likely have to store these closures or futures in a struct or enum at some point. There are two ways to do that:
|
Beta Was this translation helpful? Give feedback.
-
Isn't most stuff already required to be
Doesn't this imply that the decision has already kind of be made? :D |
Beta Was this translation helpful? Give feedback.
-
I gave this a spin here: #1813 |
Beta Was this translation helpful? Give feedback.
-
Hello, I've been lurking around libp2p for some time, doing some hacking, and have summarized my issues here.
If it interests anyone:
While the capabilities of current design are quite impressive, It seems to be overly complex for what it does. It should be possible to work at the application layer without having to interact with complex traits like
Transport
andStreamMuxer
, and their associated types.This makes it hard to modify the library (I've tried several times, and and just got discouraged by the insanity of having to specify 8 generic parameters in some places ).
Several ideas:
1. Remove Substream type from
StreamMuxer
Arc<Mutex>>
2. Remove
ConnectionInfo
as a trait, replace it withPeerId
3. Use boxing in reasonable places.
5. Move away from custom built combinators for
Transport
andUpgrade
6. Make behaviors independent.
Kademlia
behaviour yourself). Structuring this as a hierarchy is not correct. This is a set.a. Listen for network messages,
b. Listen for Internal messages
c. Emit network messages
d. Emit internal mesages to other behaviours
My goal is not to just be negative, but to provide feedback from working with the library, and some directions, in which I'd like to move. I'm going to try to move forward on some of these myself, but I'm not sure how far I'll be able to go.
Let me know whether you're open to some of these changes, I'd be glad to do some work in this direction, I think it'd enable more features (like Relay)
Beta Was this translation helpful? Give feedback.
All reactions