status-im / status-go

The Status module that consumes go-ethereum
https://status.im
Mozilla Public License 2.0
727 stars 245 forks source link

Count replacement peers properly #973

Closed dshulyak closed 6 years ago

dshulyak commented 6 years ago

Problem

Currently peer that is found as a replacement for dropped peer won't be added to local peer pool as connected and thus can break limits logic.

cc @adambabik

adambabik commented 6 years ago

Affected method:

func (t *TopicPool) AddPeerFromTable(server *p2p.Server) *discv5.Node {
    t.mu.RLock()
    defer t.mu.RUnlock()

    // The most recently added peer is removed from the queue.
    // If it did not expire yet, it will be added to the server.
    // TODO(adam): investigate if it's worth to keep the peer in the queue
    // until the server confirms it is added and in the meanwhile only adjust its priority.
    peer := t.popFromPeerPool()
    if peer != nil && mclock.Now() < peer.discoveredTime+mclock.AbsTime(expirationPeriod) {
        t.addServerPeer(server, peer)
        return peer.node
    }

    return nil
}

line peer := t.popFromPeerPool().

The peer is removed from the map but later when ConfirmAdded() is called this fragment returns while it should continue executing:

        peer, exist := t.peerPool[discV5NodeID]
    if !exist {
        return
    }
dshulyak commented 6 years ago

Also, while debugging discovery, i realized that it is a safer strategy to reconnecting with all available peers, not the one we discovered last. This way we will find new connections faster and all other peers will be dropped using already available rebalancing.