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

introduce peer manager #31

Merged
merged 3 commits into from
Jul 5, 2022
Merged

introduce peer manager #31

merged 3 commits into from
Jul 5, 2022

Conversation

Liuhaai
Copy link
Member

@Liuhaai Liuhaai commented Apr 23, 2022

Description

  • The new component peerManager is introduced for peer discovering, peer connecting, and peer managing. In the peerManager, the Rendezvous protocol is used for peer discovery, which replaces the old Neighbors method

  • To interact with peerManager externally, the methods Advertise, FindPeers, BlockPeer, AddBootstrap and ConnectedPeers are added in Host class.

  • Code clean work for NewHost func

peerManager.go Outdated Show resolved Hide resolved
p2p.go Outdated Show resolved Hide resolved
peerManager.go Outdated
continue
}
if err := ma.host.Connect(ctx, peer); err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should count the new connection success / total, instead of return err on a single connection? something like

newConn = len(peerChan) - len(ma.host.Network().Conns())
for {
    if err := ma.host.Connect(ctx, peer); err == nil {
        success++
    }
}
if success < newConn * 2/3 {
    return err
}

p2p.go Show resolved Hide resolved
@Liuhaai Liuhaai force-pushed the peerManager branch 3 times, most recently from d850ed1 to 047757e Compare May 3, 2022 06:30
@Liuhaai Liuhaai marked this pull request as ready for review May 19, 2022 23:20
p2p.go Outdated
@@ -66,6 +65,8 @@ type (
EnableRateLimit bool `yaml:"enableRateLimit"`
RateLimit RateLimitConfig `yaml:"rateLimit"`
PrivateNetworkPSK string `yaml:"privateNetworkPSK"`
GroupID string `yaml:"groupID"`
ServerMode bool `yaml:"serverMode"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. remove ServerMode? we discussed it needs to be server mode, so no need to change regarding this
  2. add a blacklist timeout value, see comment below

p2p.go Outdated
return nil
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

p2p.go Outdated
dhtOpt = append(dhtOpt, dht.Mode(dht.ModeServer))
} else {
dhtOpt = append(dhtOpt, dht.Mode(dht.ModeClient))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

p2p.go Outdated Show resolved Hide resolved
p2p.go Outdated Show resolved Hide resolved
}

// AddBootstrap adds bootnode into peermanager
func (h *Host) AddBootstrap(bootNodeAddrs []multiaddr.Multiaddr) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this func? it is not used

peerManager.go Outdated
return err
}

func (ma *peerManager) AddBootstrap(ids ...core.PeerID) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this func? it is not used

peerManager.go Outdated
@@ -157,6 +157,11 @@ func (ma *peerManager) BlockPeer(pr peer.ID) {
}
}

func (ma *peerManager) BlockPeer(pr peer.ID) {
Copy link
Member

@dustinxie dustinxie May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: call this RemovePeer?

routing: routing,
ns: ns,
bootstrap: make(map[peer.ID]bool),
advertiseInterval: 3 * time.Hour, // DHT provider record is republished every 3hrs by default
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The group id will expire in 24 hrs. So it's advertised forcely every 3 hrs.

p2p.go Outdated Show resolved Hide resolved
peerManager.go Outdated Show resolved Hide resolved
return pm
}

func (pm *peerManager) JoinOverlay() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the go-routine only needs to be triggered once? but not start a new go-routine every time this is called?

type peerManager struct {
    advertise sync.Once
}

func (pm *peerManager) JoinOverlay(
    pm.advertise.Do(func() {
        for {
             select
             ...
        }
    })
)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion

}(pm)
}

func (pm *peerManager) Advertise() error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also we don't need this? in JoinOverlay(), ticker.C will fire every 3 hours to keep advertising itself, right? maybe we can reduce to interval to like 30 min

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the func triggerred externally

peerManager.go Outdated
go func(pm *peerManager) {
for {
select {
case ns := <-pm.advertiseQueue:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also this is not needed? since it serves the same purpose as ticker.C

return pm.host.Network().ClosePeer(pr)
}

func (pm *peerManager) TryBlockPeer(pr peer.ID) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this name is not accurate

Copy link
Member Author

@Liuhaai Liuhaai Jun 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any suggestion?

pm.blacklist.Set(pr, val.(int)+1)
if val.(int)+1 >= pm.blacklistTolerance {
pm.host.Network().ClosePeer(pr)
}
Copy link
Member

@dustinxie dustinxie Jun 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if a peer has been set more than pm.blacklistTolerance times, then after that it will always go to this if, and keep calling ClosePeer(), is that intended? maybe should remove the peer after reaching pm.blacklistTolerance times (give him a chance to start over)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The peer will be filtered out by blocked(), so the node can't get this peer by ConnectedPeers() . And the id will be removed from the map when BlacklistTimeout reaches.

@Liuhaai Liuhaai merged commit 5fe315f into master Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants