network: hybrid networking #5800
Conversation
| @@ -100,6 +101,7 @@ func MakeService(ctx context.Context, log logging.Logger, cfg config.Local, data | |||
| libp2p.Muxer("/yamux/1.0.0", &ymx), | |||
| libp2p.Peerstore(pstore), | |||
| libp2p.ListenAddrStrings(listenAddr), | |||
| libp2p.Security(noise.ID, noise.New), | |||
There was a problem hiding this comment.
it does not look used except adding a new endpoint here?
There was a problem hiding this comment.
This is setting the configuration for libp2p transport, making explicit more of the default configuration we are relying on (and also disabling TLS 1.3 which is a second-priority fallback by default). It is also possible to configure no encryption at all (like wsNetwork currently uses) and expose that as a config option — we could revisit if we want to consider that for performance reasons.
| @@ -585,6 +585,12 @@ type Local struct { | |||
| // EnableP2P turns on the peer to peer network | |||
| EnableP2P bool `version[31]:"false"` | |||
|
|
|||
| // EnableP2PHybridMode turns on both websockets and P2P networking. | |||
| EnableP2PHybridMode bool `version[31]:"false"` | |||
There was a problem hiding this comment.
move to v32, v31 was released a few days ago
|
|
||
| // Broadcast implements GossipNode | ||
| func (n *HybridP2PNetwork) Broadcast(ctx context.Context, tag protocol.Tag, data []byte, wait bool, except Peer) error { | ||
| return n.runParallel(func(net GossipNode) error { |
There was a problem hiding this comment.
shouldn't we limit type of traffic broadcasted with p2p in hybrid mode? I'm afraid of xN bandwidth increase
There was a problem hiding this comment.
I'm assuming there is a constant number of peers "out there", so there wouldn't just x2 rather than a shifting / splitting of existing bandwidth amounts between different transport/connection types. We will also be able to set limits for total # of connections for each network type.
There was a problem hiding this comment.
I agree about a constant number of peers, but I think we'd still need logic to prevent peers from "doubling up" and connecting to each other over both networks. If that logic's in this PR I haven't identified it yet
Codecov Report
@@ Coverage Diff @@
## master #5800 +/- ##
==========================================
- Coverage 55.57% 55.47% -0.11%
==========================================
Files 475 476 +1
Lines 66838 67006 +168
==========================================
+ Hits 37147 37171 +24
- Misses 27172 27320 +148
+ Partials 2519 2515 -4
... and 13 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -438,15 +463,15 @@ func (n *P2PNetwork) txTopicHandleLoop() { | |||
| // txTopicValidator calls txHandler to validate and process incoming transactions. | |||
| func (n *P2PNetwork) txTopicValidator(ctx context.Context, peerID peer.ID, msg *pubsub.Message) pubsub.ValidationResult { | |||
| inmsg := IncomingMessage{ | |||
| Sender: msg.ReceivedFrom, | |||
| Sender: gossipSubPeer{peerID: msg.ReceivedFrom, net: n}, | |||
There was a problem hiding this comment.
Do you know if msg.ReceivedFrom will be the same as the peerID argument?
There was a problem hiding this comment.
Oh good point. I'm not sure but for this case I think it is best to use the peerID, not the ReceivedFrom.
There was a problem hiding this comment.
AFAICT in go-libp2p-pubsub pubsub.go it sets peer ID to msg.ReceivedFrom (in pushMsg and PushLocal) when calling the validators. Odd.
|
|
||
| // Broadcast implements GossipNode | ||
| func (n *HybridP2PNetwork) Broadcast(ctx context.Context, tag protocol.Tag, data []byte, wait bool, except Peer) error { | ||
| return n.runParallel(func(net GossipNode) error { |
There was a problem hiding this comment.
I agree about a constant number of peers, but I think we'd still need logic to prevent peers from "doubling up" and connecting to each other over both networks. If that logic's in this PR I haven't identified it yet
There was a problem hiding this comment.
This implementations allows node to run on both p2p and ws networks, and appears to be missing stage 1 restrictions for using p2p for discovery only. @cce please comment
|
Merging to feature/p2p, even though this doesn't yet support for using the identityTracker to prevent duplicate connections to the same peer ID on both networks — will follow in a future PR on the feature branch. |
WIP draft PR for GossipNode implementation that combines WebsocketNetwork and P2PNetwork.