gs1.1 Integrate peer score #87
Conversation
ts/score/peerScore.ts
Outdated
| @@ -77,7 +83,7 @@ export class PeerScore { | |||
| } | |||
| clearInterval(this._backgroundInterval) | |||
| delete this._backgroundInterval | |||
| this._addressBook.off('change:multiaddrs', this._updateIPs) | |||
| this._connectionManager.off('change:multiaddrs', this._updateIPs) | |||
There was a problem hiding this comment.
Should we just remove the IPs instead?
If I understood correctly, this will keep them in the data structure
There was a problem hiding this comment.
ah, yeah, we should probably clear all IPs.
This is also wrong, bc the ConnectionManager emits the 'peer:connect' and 'peer:disconnect' events, not 'change:multiaddrs' events
There was a problem hiding this comment.
it seems that there are some problems with our current approach of updating IPs, and it seems the events currently emitted by the connection manager are not sufficient for tracking IPs to open connections for peers with scores.
issue 1: ConnectionManager doesn't emit on closed connections. ConnectionManager#_maybeDisconnectOne, doesn't emit when a connection is closed, more generally, ConnectionManager isn't tracking/emitting on manually closed connections, but rather only on calls to ConnectionManager#onDisconnect.
issue 2: ConnectionManager emits peer:connect event before storing the emitted connection.
The current approach is to use ConnectionManager#getAll on each peer:connect event for a list of current IPs, but since the event is triggered before the new connection is stored, the call to getAll will not include the new connection unless the event handler awaits the ConnectionManager#onConnect finishing in its entirety. But that potentially also runs into issues re issue 1.
I think, for now, to move forward, we should fall back to calling ConnectionManager#getAll for each peer in the background method here, in the same synchronous fashion that go-libp2p-pubsub does, while we sort out what a good solution for asynchronously tracking open connection IPs would look like.
There was a problem hiding this comment.
whoops, i thought this was #83, i've updated that PR with the idea ^
Once we merge, the diff here should be tiny
There was a problem hiding this comment.
I had a look and I think we can go with this for now! I will create an issue for tracking this connectionManager issues that you mentioned. Hopefully we can get to that in [email protected]. Meanwhile, we should probably create an issue in gossipsub to sign that this should be changed once libp2p connection manager properly supports these needs.
wemeetagain
force-pushed
the
cayman/gs1.1-integrate-peer-score
branch
from
f3fb6d1 to
513b90c
Compare
Codecov Report
@@ Coverage Diff @@
## gsv1.1 #87 +/- ##
=======================================
Coverage 80.00% 80.00%
=======================================
Files 1 1
Lines 10 10
=======================================
Hits 8 8
Misses 2 2
Continue to review full report at Codecov.
|
Building off #83
Minimal integration of peer score
start/stop/_addPeer/_removePeer