From e37c7c2e7872284093eabc765a4ddafb9e56f690 Mon Sep 17 00:00:00 2001 From: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Date: Sun, 13 Nov 2022 05:02:05 +0100 Subject: [PATCH] feat: remove unnecessary conversion from Multiaddr to IP (#369) * Remove unnecessary conversion from Multiaddr to IP * Uncomment un-implemented code * chore: use proper change:multiaddrs event handler * chore: add multiaddr as dependency Co-authored-by: Cayman --- package.json | 2 +- src/index.ts | 53 +++++++++++++-- src/score/compute-score.ts | 2 +- src/score/peer-score.ts | 136 +++++++++++-------------------------- src/score/peer-stats.ts | 4 +- src/score/scoreMetrics.ts | 2 +- src/utils/multiaddr.ts | 23 +++++++ src/utils/set.ts | 15 ++++ test/peer-score.spec.ts | 48 ++++++------- test/scoreMetrics.spec.ts | 7 +- 10 files changed, 150 insertions(+), 142 deletions(-) create mode 100644 src/utils/multiaddr.ts diff --git a/package.json b/package.json index 18ce24c7..a6f1e2f8 100644 --- a/package.json +++ b/package.json @@ -79,6 +79,7 @@ "@libp2p/peer-record": "^4.0.1", "@libp2p/pubsub": "^5.0.0", "@libp2p/topology": "^3.0.0", + "@multiformats/multiaddr": "^11.0.0", "abortable-iterator": "^4.0.2", "denque": "^1.5.0", "err-code": "^3.0.1", @@ -98,7 +99,6 @@ "@libp2p/interface-pubsub-compliance-tests": "^4.0.0", "@libp2p/peer-id-factory": "^1.0.18", "@libp2p/peer-store": "^5.0.0", - "@multiformats/multiaddr": "^11.0.0", "@types/node": "^17.0.21", "@typescript-eslint/eslint-plugin": "^3.0.2", "@typescript-eslint/parser": "^3.0.2", diff --git a/src/index.ts b/src/index.ts index d009e726..eeee7dbe 100644 --- a/src/index.ts +++ b/src/index.ts @@ -78,7 +78,9 @@ import { InboundStream, OutboundStream } from './stream.js' import { Uint8ArrayList } from 'uint8arraylist' import { decodeRpc, DecodeRPCLimits, defaultDecodeRpcLimits } from './message/decodeRpc.js' import { ConnectionManager } from '@libp2p/interface-connection-manager' -import { PeerStore } from '@libp2p/interface-peer-store' +import { PeerMultiaddrsChangeData, PeerStore } from '@libp2p/interface-peer-store' +import { Multiaddr } from '@multiformats/multiaddr' +import { multiaddrToIPStr } from './utils/multiaddr.js' type ConnectionDirection = 'inbound' | 'outbound' @@ -495,7 +497,7 @@ export class GossipSub extends EventEmitter implements PubSub implements PubSub implements PubSub registrar.unregister(id)) @@ -684,7 +690,7 @@ export class GossipSub extends EventEmitter implements PubSub implements PubSub implements PubSub implements PubSub implements PubSub): void { + const { peerId, multiaddrs, oldMultiaddrs } = evt.detail + const newIps = new Set() + const oldIps = new Set() + for (const mu of multiaddrs) { + const ipStr = multiaddrToIPStr(mu) + if (ipStr) { + newIps.add(ipStr) + } + } + for (const mu of oldMultiaddrs) { + const ipStr = multiaddrToIPStr(mu) + if (ipStr) { + // Remove multiaddrs that aren't new + if (newIps.has(ipStr)) { + newIps.delete(ipStr) + } else { + oldIps.add(ipStr) + } + } + } + const id = peerId.toString() + for (const ipStr of oldIps) { + this.score.removeIP(id, ipStr) + } + for (const ipStr of newIps) { + this.score.addIP(id, ipStr) + } + } + // API METHODS get started(): boolean { diff --git a/src/score/compute-score.ts b/src/score/compute-score.ts index 785032da..f2c6f1a7 100644 --- a/src/score/compute-score.ts +++ b/src/score/compute-score.ts @@ -70,7 +70,7 @@ export function computeScore( score += p5 * params.appSpecificWeight // P6: IP colocation factor - pstats.ips.forEach((ip) => { + pstats.knownIPs.forEach((ip) => { if (params.IPColocationFactorWhitelist.has(ip)) { return } diff --git a/src/score/peer-score.ts b/src/score/peer-score.ts index 859f9e39..ee8a9cbf 100644 --- a/src/score/peer-score.ts +++ b/src/score/peer-score.ts @@ -5,8 +5,7 @@ import { MessageDeliveries, DeliveryRecordStatus } from './message-deliveries.js import { logger } from '@libp2p/logger' import { MsgIdStr, PeerIdStr, RejectReason, TopicStr, IPStr } from '../types.js' import type { Metrics, ScorePenalty } from '../metrics.js' -import { ConnectionManager } from '@libp2p/interface-connection-manager' -import { peerIdFromString } from '@libp2p/peer-id' +import { MapDef } from '../utils/set.js' const log = logger('libp2p:gossipsub:score') @@ -28,10 +27,6 @@ interface ScoreCacheEntry { export type PeerScoreStatsDump = Record -export interface PeerScoreComponents { - connectionManager: ConnectionManager -} - export class PeerScore { /** * Per-peer stats for score calculation @@ -40,7 +35,7 @@ export class PeerScore { /** * IP colocation tracking; maps IP => set of peers. */ - readonly peerIPs = new Map>() + readonly peerIPs = new MapDef>(() => new Set()) /** * Cache score up to decayInterval if topic stats are unchanged. */ @@ -53,17 +48,10 @@ export class PeerScore { _backgroundInterval?: ReturnType private readonly scoreCacheValidityMs: number - private readonly components: PeerScoreComponents private readonly computeScore: typeof computeScore - constructor( - components: PeerScoreComponents, - readonly params: PeerScoreParams, - private readonly metrics: Metrics | null, - opts: PeerScoreOpts - ) { + constructor(readonly params: PeerScoreParams, private readonly metrics: Metrics | null, opts: PeerScoreOpts) { validatePeerScoreParams(params) - this.components = components this.scoreCacheValidityMs = opts.scoreCacheValidityMs this.computeScore = opts.computeScore ?? computeScore } @@ -105,7 +93,6 @@ export class PeerScore { */ background(): void { this.refreshScores() - this.updateIPs() this.deliveryRecords.gc() } @@ -125,7 +112,7 @@ export class PeerScore { // has the retention period expired? if (now > pstats.expire) { // yes, throw it away (but clean up the IP tracking first) - this.removeIPs(id, pstats.ips) + this.removeIPsForPeer(id, pstats.knownIPs) this.peerStats.delete(id) this.scoreCache.delete(id) } @@ -236,15 +223,36 @@ export class PeerScore { connected: true, expire: 0, topics: {}, - ips: [], + knownIPs: new Set(), behaviourPenalty: 0 } this.peerStats.set(id, pstats) + } + + /** Adds a new IP to a peer, if the peer is not known the update is ignored */ + addIP(id: PeerIdStr, ip: string): void { + const pstats = this.peerStats.get(id) + if (pstats) { + pstats.knownIPs.add(ip) + } - // get + update peer IPs - const ips = this.getIPs(id) - this.setIPs(id, ips, pstats.ips) - pstats.ips = ips + this.peerIPs.getOrDefault(ip).add(id) + } + + /** Remove peer association with IP */ + removeIP(id: PeerIdStr, ip: string): void { + const pstats = this.peerStats.get(id) + if (pstats) { + pstats.knownIPs.delete(ip) + } + + const peersWithIP = this.peerIPs.get(ip) + if (peersWithIP) { + peersWithIP.delete(id) + if (peersWithIP.size === 0) { + this.peerIPs.delete(ip) + } + } } removePeer(id: PeerIdStr): void { @@ -256,7 +264,7 @@ export class PeerScore { // decide whether to retain the score; this currently only retains non-positive scores // to dissuade attacks on the score function. if (this.score(id) > 0) { - this.removeIPs(id, pstats.ips) + this.removeIPsForPeer(id, pstats.knownIPs) this.peerStats.delete(id) return } @@ -501,84 +509,18 @@ export class PeerScore { } /** - * Gets the current IPs for a peer. - */ - private getIPs(id: PeerIdStr): IPStr[] { - return this.components.connectionManager - .getConnections(peerIdFromString(id)) - .map((c) => c.remoteAddr.toOptions().host) - } - - /** - * Adds tracking for the new IPs in the list, and removes tracking from the obsolete IPs. + * Removes an IP list from the tracking list for a peer. */ - public setIPs(id: PeerIdStr, newIPs: IPStr[], oldIPs: IPStr[]): void { - // add the new IPs to the tracking - // eslint-disable-next-line no-labels - addNewIPs: for (const ip of newIPs) { - // check if it is in the old ips list - for (const xip of oldIPs) { - if (ip === xip) { - // eslint-disable-next-line no-labels - continue addNewIPs + private removeIPsForPeer(id: PeerIdStr, ipsToRemove: Set): void { + for (const ipToRemove of ipsToRemove) { + const peerSet = this.peerIPs.get(ipToRemove) + if (peerSet) { + peerSet.delete(id) + if (peerSet.size === 0) { + this.peerIPs.delete(ipToRemove) } } - // no, it's a new one -- add it to the tracker - let peers = this.peerIPs.get(ip) - if (!peers) { - peers = new Set() - this.peerIPs.set(ip, peers) - } - peers.add(id) } - // remove the obsolete old IPs from the tracking - // eslint-disable-next-line no-labels - removeOldIPs: for (const ip of oldIPs) { - // check if it is in the new ips list - for (const xip of newIPs) { - if (ip === xip) { - // eslint-disable-next-line no-labels - continue removeOldIPs - } - } - // no, its obselete -- remove it from the tracker - const peers = this.peerIPs.get(ip) - if (!peers) { - continue - } - peers.delete(id) - if (!peers.size) { - this.peerIPs.delete(ip) - } - } - } - - /** - * Removes an IP list from the tracking list for a peer. - */ - public removeIPs(id: PeerIdStr, ips: IPStr[]): void { - ips.forEach((ip) => { - const peers = this.peerIPs.get(ip) - if (!peers) { - return - } - - peers.delete(id) - if (!peers.size) { - this.peerIPs.delete(ip) - } - }) - } - - /** - * Update all peer IPs to currently open connections - */ - public updateIPs(): void { - this.peerStats.forEach((pstats, id) => { - const newIPs = this.getIPs(id) - this.setIPs(id, newIPs, pstats.ips) - pstats.ips = newIPs - }) } /** diff --git a/src/score/peer-stats.ts b/src/score/peer-stats.ts index cd63e504..34011685 100644 --- a/src/score/peer-stats.ts +++ b/src/score/peer-stats.ts @@ -7,8 +7,8 @@ export interface PeerStats { expire: number /** per topic stats */ topics: Record - /** IP tracking; store as string for easy processing */ - ips: string[] + /** IP tracking; store as set for easy processing */ + knownIPs: Set /** behavioural pattern penalties (applied by the router) */ behaviourPenalty: number } diff --git a/src/score/scoreMetrics.ts b/src/score/scoreMetrics.ts index a7787e8d..299e232c 100644 --- a/src/score/scoreMetrics.ts +++ b/src/score/scoreMetrics.ts @@ -127,7 +127,7 @@ export function computeScoreWeights( p5w += p5 * params.appSpecificWeight // P6: IP colocation factor - pstats.ips.forEach((ip) => { + pstats.knownIPs.forEach((ip) => { if (params.IPColocationFactorWhitelist.has(ip)) { return } diff --git a/src/utils/multiaddr.ts b/src/utils/multiaddr.ts new file mode 100644 index 00000000..e3303506 --- /dev/null +++ b/src/utils/multiaddr.ts @@ -0,0 +1,23 @@ +import { Multiaddr } from '@multiformats/multiaddr' +import { convertToString } from '@multiformats/multiaddr/convert' + +// Protocols https://github.com/multiformats/multiaddr/blob/master/protocols.csv +// code size name +// 4 32 ip4 +// 41 128 ip6 +enum Protocol { + ip4 = 4, + ip6 = 41 +} + +export function multiaddrToIPStr(multiaddr: Multiaddr): string | null { + for (const tuple of multiaddr.tuples()) { + switch (tuple[0]) { + case Protocol.ip4: + case Protocol.ip6: + return convertToString(tuple[0], tuple[1]!) + } + } + + return null +} diff --git a/src/utils/set.ts b/src/utils/set.ts index dc3da86c..9687f210 100644 --- a/src/utils/set.ts +++ b/src/utils/set.ts @@ -26,3 +26,18 @@ export function removeItemsFromSet( export function removeFirstNItemsFromSet(superSet: Set, ineed: number): Set { return removeItemsFromSet(superSet, ineed, () => true) } + +export class MapDef extends Map { + constructor(private readonly getDefault: () => V) { + super() + } + + getOrDefault(key: K): V { + let value = super.get(key) + if (value === undefined) { + value = this.getDefault() + this.set(key, value) + } + return value + } +} diff --git a/test/peer-score.spec.ts b/test/peer-score.spec.ts index a7c071bc..c9474794 100644 --- a/test/peer-score.spec.ts +++ b/test/peer-score.spec.ts @@ -1,19 +1,14 @@ import sinon from 'sinon' import { expect } from 'aegir/chai' import delay from 'delay' -import type { ConnectionManager } from '@libp2p/interface-connection-manager' import { PeerScore, createPeerScoreParams, createTopicScoreParams } from '../src/score/index.js' import { getMsgIdStr, makeTestMessage } from './utils/index.js' import { RejectReason } from '../src/types.js' import { ScorePenalty } from '../src/metrics.js' -import { stubInterface } from 'ts-sinon' import { createEd25519PeerId } from '@libp2p/peer-id-factory' import { PeerStats } from '../src/score/peer-stats.js' import type { PeerScoreParams, TopicScoreParams } from '../src/score/peer-score-params.js' -const connectionManager = stubInterface() -connectionManager.getConnections.returns([]) - /** Placeholder for some ScorePenalty value, only used for metrics */ const scorePenaltyAny = ScorePenalty.BrokenPromise @@ -32,7 +27,7 @@ describe('PeerScore', () => { })) const peerA = (await createEd25519PeerId()).toString() // Peer score should start at 0 - const ps = new PeerScore({ connectionManager }, params, null, { scoreCacheValidityMs: 0 }) + const ps = new PeerScore(params, null, { scoreCacheValidityMs: 0 }) ps.addPeer(peerA) let aScore = ps.score(peerA) @@ -61,7 +56,7 @@ describe('PeerScore', () => { })) const peerA = (await createEd25519PeerId()).toString() // Peer score should start at 0 - const ps = new PeerScore({ connectionManager }, params, null, { scoreCacheValidityMs: 0 }) + const ps = new PeerScore(params, null, { scoreCacheValidityMs: 0 }) ps.addPeer(peerA) let aScore = ps.score(peerA) @@ -93,7 +88,7 @@ describe('PeerScore', () => { })) const peerA = (await createEd25519PeerId()).toString() // Peer score should start at 0 - const ps = new PeerScore({ connectionManager }, params, null, { scoreCacheValidityMs: 0 }) + const ps = new PeerScore(params, null, { scoreCacheValidityMs: 0 }) ps.addPeer(peerA) ps.graft(peerA, mytopic) @@ -128,7 +123,7 @@ describe('PeerScore', () => { })) const peerA = (await createEd25519PeerId()).toString() // Peer score should start at 0 - const ps = new PeerScore({ connectionManager }, params, null, { scoreCacheValidityMs: 0 }) + const ps = new PeerScore(params, null, { scoreCacheValidityMs: 0 }) ps.addPeer(peerA) let aScore = ps.score(peerA) @@ -171,7 +166,7 @@ describe('PeerScore', () => { })) const peerA = (await createEd25519PeerId()).toString() // Peer score should start at 0 - const ps = new PeerScore({ connectionManager }, params, null, { scoreCacheValidityMs: 0 }) + const ps = new PeerScore(params, null, { scoreCacheValidityMs: 0 }) ps.addPeer(peerA) let aScore = ps.score(peerA) @@ -234,7 +229,7 @@ describe('PeerScore', () => { const peerC = (await createEd25519PeerId()).toString() const peers = [peerA, peerB, peerC] // Peer score should start at 0 - const ps = new PeerScore({ connectionManager }, params, null, { scoreCacheValidityMs: 0 }) + const ps = new PeerScore(params, null, { scoreCacheValidityMs: 0 }) peers.forEach((p) => { ps.addPeer(p) ps.graft(p, mytopic) @@ -295,7 +290,7 @@ describe('PeerScore', () => { })) const peerA = (await createEd25519PeerId()).toString() // Peer score should start at 0 - const ps = new PeerScore({ connectionManager }, params, null, { scoreCacheValidityMs: 0 }) + const ps = new PeerScore(params, null, { scoreCacheValidityMs: 0 }) ps.addPeer(peerA) ps.graft(peerA, mytopic) @@ -355,7 +350,7 @@ describe('PeerScore', () => { const peerA = (await createEd25519PeerId()).toString() const peerB = (await createEd25519PeerId()).toString() const peers = [peerA, peerB] - const ps = new PeerScore({ connectionManager }, params, null, { scoreCacheValidityMs: 0 }) + const ps = new PeerScore(params, null, { scoreCacheValidityMs: 0 }) peers.forEach((p) => { ps.addPeer(p) ps.graft(p, mytopic) @@ -403,7 +398,7 @@ describe('PeerScore', () => { timeInMeshWeight: 0 })) const peerA = (await createEd25519PeerId()).toString() - const ps = new PeerScore({ connectionManager }, params, null, { scoreCacheValidityMs: 0 }) + const ps = new PeerScore(params, null, { scoreCacheValidityMs: 0 }) ps.addPeer(peerA) ps.graft(peerA, mytopic) @@ -434,7 +429,7 @@ describe('PeerScore', () => { timeInMeshWeight: 0 })) const peerA = (await createEd25519PeerId()).toString() - const ps = new PeerScore({ connectionManager }, params, null, { scoreCacheValidityMs: 0 }) + const ps = new PeerScore(params, null, { scoreCacheValidityMs: 0 }) ps.addPeer(peerA) ps.graft(peerA, mytopic) @@ -474,7 +469,7 @@ describe('PeerScore', () => { }) const peerA = (await createEd25519PeerId()).toString() const peerB = (await createEd25519PeerId()).toString() - const ps = new PeerScore({ connectionManager }, params, null, { scoreCacheValidityMs: 0 }) + const ps = new PeerScore(params, null, { scoreCacheValidityMs: 0 }) ps.addPeer(peerA) ps.addPeer(peerB) @@ -552,7 +547,7 @@ describe('PeerScore', () => { appSpecificWeight: 0.5 }) const peerA = (await createEd25519PeerId()).toString() - const ps = new PeerScore({ connectionManager }, params, null, { scoreCacheValidityMs: 0 }) + const ps = new PeerScore(params, null, { scoreCacheValidityMs: 0 }) ps.addPeer(peerA) ps.graft(peerA, mytopic) @@ -577,18 +572,15 @@ describe('PeerScore', () => { const peerD = (await createEd25519PeerId()).toString() const peers = [peerA, peerB, peerC, peerD] - const ps = new PeerScore({ connectionManager }, params, null, { scoreCacheValidityMs: 0 }) + const ps = new PeerScore(params, null, { scoreCacheValidityMs: 0 }) peers.forEach((p) => { ps.addPeer(p) ps.graft(p, mytopic) }) const setIPsForPeer = (p: string, ips: string[]) => { - ps.setIPs(p, ips, []) - const pstats = ps.peerStats.get(p) - - if (pstats != null) { - pstats.ips = ips + for (const ip of ips) { + ps.addIP(p, ip) } } // peerA should have no penalty, but B, C, and D should be penalized for sharing an IP @@ -621,7 +613,7 @@ describe('PeerScore', () => { }) const peerA = (await createEd25519PeerId()).toString() - const ps = new PeerScore({ connectionManager }, params, null, { scoreCacheValidityMs: 0 }) + const ps = new PeerScore(params, null, { scoreCacheValidityMs: 0 }) // add penalty on a non-existent peer ps.addPenalty(peerA, 1, ScorePenalty.MessageDeficit) @@ -657,7 +649,7 @@ describe('PeerScore', () => { }) const peerA = (await createEd25519PeerId()).toString() - const ps = new PeerScore({ connectionManager }, params, null, { scoreCacheValidityMs: 0 }) + const ps = new PeerScore(params, null, { scoreCacheValidityMs: 0 }) ps.addPeer(peerA) ps.graft(peerA, mytopic) // score should equal -1000 (app-specific score) @@ -703,7 +695,7 @@ describe.skip('PeerScore score cache', function () { sandbox.useFakeTimers(now) computeStoreStub = sinon.stub<[string, PeerStats, PeerScoreParams, Map>], number>() - ps2 = new PeerScore({ connectionManager }, params, null, { + ps2 = new PeerScore(params, null, { scoreCacheValidityMs: 10, computeScore: computeStoreStub }) @@ -733,9 +725,7 @@ describe.skip('PeerScore score cache', function () { { name: 'markInvalidMessageDelivery', fun: () => ps2.markInvalidMessageDelivery(peerA, 'a') }, { name: 'markFirstMessageDelivery', fun: () => ps2.markFirstMessageDelivery(peerA, 'a') }, { name: 'markDuplicateMessageDelivery', fun: () => ps2.markDuplicateMessageDelivery(peerA, 'a') }, - { name: 'setIPs', fun: () => ps2.setIPs(peerA, [], ['127.0.0.1']) }, - { name: 'removeIPs', fun: () => ps2.removeIPs(peerA, ['127.0.0.1']) }, - { name: 'updateIPs', fun: () => ps2.updateIPs() } + { name: 'removeIPs', fun: () => ps2.removeIP(peerA, '127.0.0.1') } ] for (const { name, fun } of testCases) { diff --git a/test/scoreMetrics.spec.ts b/test/scoreMetrics.spec.ts index d5da065b..197f3482 100644 --- a/test/scoreMetrics.spec.ts +++ b/test/scoreMetrics.spec.ts @@ -1,14 +1,9 @@ -import type { ConnectionManager } from '@libp2p/interface-connection-manager' import { computeAllPeersScoreWeights } from '../src/score/scoreMetrics.js' import { createPeerScoreParams, createTopicScoreParams, PeerScore } from '../src/score/index.js' import { ScorePenalty } from '../src/metrics.js' import { expect } from 'aegir/chai' -import { stubInterface } from 'ts-sinon' import { createEd25519PeerId } from '@libp2p/peer-id-factory' -const connectionManager = stubInterface() -connectionManager.getConnections.returns([]) - describe('score / scoreMetrics', () => { it('computeScoreWeights', async () => { // Create parameters with reasonable default values @@ -30,7 +25,7 @@ describe('score / scoreMetrics', () => { const peerA = (await createEd25519PeerId()).toString() // Peer score should start at 0 - const ps = new PeerScore({ connectionManager }, params, null, { scoreCacheValidityMs: 0 }) + const ps = new PeerScore(params, null, { scoreCacheValidityMs: 0 }) ps.addPeer(peerA) // Do some actions that penalize the peer