ethresearch / sharding-p2p-poc

Proof of Concept of Ethereum Serenity Peer-to-Peer Layer on libp2p PubSub System
40 stars 19 forks source link

Connection(peer) management for the underlying overlay #19

Open mhchia opened 6 years ago

mhchia commented 6 years ago

What is wrong?

Despite the fact that gossipsub chooses mesh peers for the overlay of each topic for us, we still need a way to control the connections of the node.

How can it be fixed

Possibly can be solved through ConnManager. Should further decide

Related issue:

mhchia commented 6 years ago

Note: my thoughts to manage connections for pubsub is, if our node is currently subscribed to a specific topic, we might want it keeps its connections with its "mesh peers" in that topic, prevent the connections from being closed.

raulk commented 5 years ago

Hey @mhchia, evolving and maturing the connection manager in libp2p is on my personal roadmap for the next weeks/months, so I would love to chat to you about this. Currently gossipsub/floodsub attaches to the host as a Notifee and gets informed of connections and disconnections:

https://github.com/libp2p/go-floodsub/blob/e7b1fe6e75c377fda9f928f11faddcb7e5d842bf/notify.go#L18

We could consider tagging the connections of the peers on meshsub (topics we're subscribed to) to prevent them from being killed.

That said, the connection manager is a bit naïve at the moment; it simply deals with linear scores and closes connections in batches. We've started discussing some topics like protocol weights, traffic shaping, temporary allowances, connection draining, etc. here:

https://github.com/libp2p/go-libp2p-connmgr/issues/19 https://github.com/libp2p/go-libp2p-connmgr/issues/20

They are very early discussions, but I think you guys have a lot to contribute.

NIC619 commented 5 years ago

Hi @raulk thanks for the great introduction on connection manager. I'm currently looking into it, play around and have a few questions regarding trimming connection:

  1. cm.peers(or cm.connCount) is only updated when being notified(Connected/Disconnected) but in getConnsToClose the list of connections to be trimmed is derived from cm.peers: https://github.com/libp2p/go-libp2p-connmgr/blob/master/connmgr.go#L99-L101

    My observation was that this could lead to trimming connections down to below low watermark or even 0 if no Disconnected notification was made and manager keep trimming(triggered by TrimOpenConns or new connections). Is this scenario possible or expected?

  2. Currently gossipsub/floodsub attaches to the host as a Notifee and gets informed of connections and disconnections

    Just to make clear. Do you mean gossipsub/floodsub would notify the host about connections and disconnections or the other way around?

Thanks!

raulk commented 5 years ago

@NIC619 Answers:

  1. When trimming, connections are first sorted based on score. Then, only N connections are trimmed, where N is the number of connections to make the final count == lowWatermark. There's also a check above to prohibit the trimming from running altogether if the number of connections is lower than the low watermark. So I don't think the scenario you described is possible, but I may be wrong, and happy to continue looking if you still think it can happen.

  2. It's the other way around. The current implementation of gossipsub/floodsub doesn't establish connections to peers out of its own accord. It attaches a notifee to the host network, and receives callbacks as connections are opened. On each connection, it attempts to open a new stream for pubsub, and if it succeeds, the peer becomes a pubsub peer.

NIC619 commented 5 years ago

@raulk

  1. I don't know if the scenario below makes sense:
    
    // Basically copied from `TestConnTrimming`
    cm := NewConnManager(10, 20, 0)
    not := cm.Notifee()

var conns []inet.Conn for i := 0; i < 30; i++ { rc := randConn(t) conns = append(conns, rc) not.Connected(nil, rc) }

// wait for a few seconds time.Sleep(time.Second * 5)


but in this setup eventually all connections are trimmed(closed).
And if some of the peers were tagged with score > 0, their connection will remain opened.

It seems to me that `cm.peers` should be updated(i.e., remove the peer from `cm.peers` if it's connections are closed) in `TrimOpenConns`? But I might have missed something.