feat: graft/prune events and mesh peer tagging #383

maschad · 2022-12-14T00:56:26Z

Add 'gossipsub:graft' and 'gossipsub:prune' events with the following detail:

export interface MeshPeer {
  peerId: string
  topic: string
  direction: Direction
}

Add new configuration option tagMeshPeers (default to true) which will tag the mesh peers in the peer store (This may be used in turn by the connection manager to rank connections, eg for disconnection purposes)
Closes Tag mesh peers on graft, remove tags on prune #380

src/index.ts

wemeetagain · 2023-01-10T16:30:37Z

comment from @dapplion

Hey! I strongly oppose merging the peer tagging feature before having strong guarantees this won't have a performance penalty.

maschad · 2023-01-10T17:00:10Z

comment from @dapplion

Hey! I strongly oppose merging the peer tagging feature before having strong guarantees this won't have a performance penalty.

Following a suggestion from @achingbrain we could do the tagging / untagging after the sending an rpc object to a peer (e.g. https://github.com/ChainSafe/js-libp2p-gossipsub/blob/master/src/index.ts#L1241) to unblock grafting / pruning operations, this should avoid the performance penalty.

what do you think @dapplion ?

achingbrain · 2023-01-10T18:12:16Z

this should avoid the performance penalty

I think it's important to establish a baseline measurement to ensure we have a basis for comparison of different solutions before we make architectural decisions predicated on performance impact.

@dapplion do you have a specific metric in mind that changes here must not affect? If you do, is there a benchmark that we can use as the baseline measurement? If not, can one be created?

codecov-commenter · 2023-01-19T15:15:30Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (f255ae4) 78.52% compared to head (86ef3d8) 78.76%.

Files	Patch %	Lines
src/index.ts	89.77%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #383      +/-   ##
==========================================
+ Coverage   78.52%   78.76%   +0.24%     
==========================================
  Files          46       46              
  Lines       10868    10992     +124     
  Branches     1058     1077      +19     
==========================================
+ Hits         8534     8658     +124     
  Misses       2334     2334

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dapplion

As argued in libp2p/js-libp2p#369 (comment) I do not believe this feature is understood enough to be a net positive in js-libp2p. Collapsing a multidimensional score of "peer usefulness" into a single number can lead to unintended consequences. Note that Go and Rust gossipsub implementations do not use tagging.

maschad · 2023-01-23T19:27:28Z

As argued in libp2p/js-libp2p#369 (comment) I do not believe this feature is understood enough to be a net positive in js-libp2p. Collapsing a multidimensional score of "peer usefulness" into a single number can lead to unintended consequences. Note that Go and Rust gossipsub implementations do not use tagging.

The go-libp2p-gossipsub implementation uses a tagTracer which applies tags to peer connections based on their behaviour, leveraging the TagPeer methods from the connection manager implementation

Peers are tagged based on the following criteria:

Directly connected peers are tagged with GossipSubConnTagValueDirectPeer (default 1000).
Mesh peers are tagged with a value of GossipSubConnTagValueMeshPeer (default 20).
If a peer is in multiple topic meshes, they'll be tagged for each.
For each message that we receive, we bump a delivery tag for peer that delivered the message first (using the GossipSubConnTagBumpMessageDelivery variable)
The delivery tags have a maximum value, GossipSubConnTagMessageDeliveryCap, and they decay at a rate of GossipSubConnTagDecayAmount / GossipSubConnTagDecayInterval.

So perhaps we need to expand this PR to accommodate those different types of tags and penalties when adjusting a peer-score . What do you think @achingbrain @wemeetagain ?

dapplion · 2023-02-13T02:52:05Z

What happened to the diff? +14,000 -42,000

maschad · 2023-02-13T02:56:34Z

What happened to the diff? +14,000 -42,000

This is just for testing purposes which is why the commit is labelled DNM

maschad · 2023-02-13T18:50:15Z

After deploying this on Feb 7th and observing on our larger cluster I have some observations. In summary

Pros

Improved throughput
More stable peer count
better gossip scores

Cons

Increased heap usage
slightly increased CPU usage

On average feat2 is receiving more messages per second over the last 6 days.

Would be interested to hear your thoughts @dapplion @wemeetagain @tuyennhv

maschad · 2023-02-28T16:29:43Z

Just sending a reminder @dapplion @tuyennhv

dapplion · 2023-03-01T00:56:57Z

Just sending a reminder @dapplion @tuyennhv

What specific input do you need from us?

maschad · 2023-03-01T03:08:55Z

Just sending a reminder @dapplion @tuyennhv

What specific input do you need from us?

I wanted to know if you thought these fluctuations were significant enough to be warranted as a performance penalty otherwise we could go ahead with this PR.

maschad · 2023-10-09T21:21:44Z

Updated @wemeetagain mind you the value is set to 0 not that is removed as referenced in #380 (comment)

wemeetagain · 2023-10-09T21:43:36Z

Updated @wemeetagain mind you the value is set to 0 not that is removed as referenced in #380 (comment)

I don't see the reference to the tag being set to 0 in that comment

maschad · 2023-10-10T19:15:58Z

My bad, I had misinterpreted the comment that says tag values should be between 0-100 to imply that for all cases, I realize in libp2p when untagging we set the metadata to undefined I have done so in c117cf9

maschad · 2023-10-10T19:17:44Z

@wemeetagain the test that actually checks for the tagging is flakey but I am not sure why, could it be that the peerStore is being cleared somewhere else?

src/index.ts

maschad · 2023-10-23T21:57:54Z

I've also updated package-lock.json as npm was reporting critical vulnerabilities.

wemeetagain · 2023-10-23T22:19:31Z

src/index.ts


      // rust-libp2p
      // - peer_score.graft()
      // - Self::control_pool_add()
      // - peer_added_to_mesh()
    })
+
+    if (this.opts?.taggingEnabled ?? false) {
+      Array.from(toAdd).map(async (id) => {


forEach or for-of

probably for-of is better, then no need to create an array

then I would have to make this whole method async and the motivation was to move away from unnecessary promises. i.e. promises would only be created for consumers who enable tagging.

What if peerStore.merge throws?

This should be refactored to a for-of

then I would have to make this whole method async and the motivation was to move away from unnecessary promises. i.e. promises would only be created for consumers who enable tagging.

achingbrain · 2024-01-16T16:46:19Z

libp2p triage update -

@maschad to pick up the final changes:
Error handling to be improved
Then review
Then merge!

wemeetagain · 2024-01-16T18:12:32Z

src/index.ts


      // rust-libp2p
      // - peer_score.graft()
      // - Self::control_pool_add()
      // - peer_added_to_mesh()
    })
+
+    if (this.opts?.taggingEnabled ?? false) {
+      Array.from(toAdd).map(async (id) => {


This should be refactored to a for-of

src/index.ts

wemeetagain · 2024-01-16T18:46:28Z

src/index.ts

@@ -1474,10 +1479,30 @@ export class GossipSub extends TypedEventEmitter<GossipsubEvents> implements Pub
    const now = Date.now()
    let doPX = this.opts.doPX

+    if (this.opts?.taggingEnabled ?? false) {


can you make sure that these peer tagging sections are at the bottom of each code section they're in? Currently, peer tagging is awaited before the functionality of the event that triggers the peer tagging, which blocks core gossipsub functionality (often timing-sensitive) on this feature.

doStuff() { // let all important stuff happen first ... // then do peer tagging if (this.opts?.tagMeshPeers) {...} }

Really this feature can/should be refactored to be a side-effect of grafting / pruning.

export type GraftPeer = {peerId: string, topic: string, direction: Direction} export type PrunePeer = GraftPeer export interface GossipsubEvents extends PubSubEvents { 'gossipsub:heartbeat': CustomEvent 'gossipsub:message': CustomEvent<GossipsubMessage> 'gossipsub:graft': CustomEvent<GraftPeer> 'gossipsub:prune': CustomEvent<PrunePeer> } class Gossipsub ... { ... tagMeshPeer = (evt: CustomEvent<GraftPeer>) => { merge(...).catch(...) } untagMeshPeer = (evt: CustomEvent<PrunePeer>) => { merge(...).catch(...) } start() { ... if (this.opts?.tagMeshPeers) { this.on('gossipsub:graft', this.tagMeshPeer) this.on('gossipsub:prune', this.untagMeshPeer) } } stop() { ... if (this.opts?.tagMeshPeers) { this.off('gossipsub:graft', this.tagMeshPeer) this.off('gossipsub:prune', this.untagMeshPeer) } } handleGraft(graft) { // actually handle the graft // then emit 'gossipsub:graft' } }

I think this can ge done in a follow up PR if necessary considering not only is this an optional feature but it one immediate use case is where it can be used prune a connection via the connection manager in where that connection should not be gossiped to, in the case where it is enabled.

wemeetagain · 2024-02-07T20:54:11Z

@achingbrain can you review this? I'm happy with it.

achingbrain · 2024-02-20T16:42:54Z

src/index.ts

+    this.components.peerStore.merge(peerIdFromString(peerId), {
+      tags: {
+        [topic]: {
+          value: 100


This value could be parameterised?

achingbrain

LGTM, small nit with the hard-coded tag value but it's not essential.

stale

maschad changed the title ~~fix: Tag mesh peers on graft, remove tags on prunehttps://github.com/ChainSafe/js-libp2p-gossipsub/issues/380~~ fix: Tag mesh peers on graft, remove tags on prune Dec 14, 2022

wemeetagain reviewed Dec 14, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

maschad force-pushed the fix/tag-peers-on-graft branch from b82f169 to 09123be Compare December 15, 2022 22:04

maschad added a commit to maschad/js-libp2p-gossipsub that referenced this pull request Dec 23, 2022

fix: tag mesh peers on graft, remove tag on pruning (ChainSafe#383)

cba3b5f

maschad added a commit to maschad/js-libp2p-gossipsub that referenced this pull request Jan 3, 2023

fix: tag mesh peers on graft, remove tag on pruning (ChainSafe#383)

e1abad6

maschad added a commit to maschad/js-libp2p-gossipsub that referenced this pull request Jan 3, 2023

chore: fix vulnerabilities (ChainSafe#383)

eb75c83

maschad force-pushed the fix/tag-peers-on-graft branch from cba3b5f to eb75c83 Compare January 3, 2023 01:38

maschad added a commit to maschad/js-libp2p-gossipsub that referenced this pull request Jan 3, 2023

fix: tag mesh peers on graft, remove tag on pruning (ChainSafe#383)

050642c

maschad added a commit to maschad/js-libp2p-gossipsub that referenced this pull request Jan 3, 2023

chore: fix vulnerabilities (ChainSafe#383)

e41c02c

maschad force-pushed the fix/tag-peers-on-graft branch from eb75c83 to e41c02c Compare January 3, 2023 01:43

maschad mentioned this pull request Jan 12, 2023

Tag relevant peer ChainSafe/lodestar#4997

Merged

maschad marked this pull request as ready for review January 19, 2023 15:11

maschad requested a review from a team as a code owner January 19, 2023 15:11

maschad force-pushed the fix/tag-peers-on-graft branch from 90b3bf6 to 8177270 Compare January 19, 2023 15:29

dapplion suggested changes Jan 20, 2023

View reviewed changes

maschad requested a review from dapplion January 23, 2023 19:27

maschad force-pushed the fix/tag-peers-on-graft branch 3 times, most recently from bbcc209 to 57ab517 Compare February 7, 2023 17:42

test: added test to ensure peer store is cleared of tags

c117cf9

dapplion reviewed Oct 10, 2023

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

test: fix issue with flakey tagging test

bba6997

maschad requested a review from dapplion October 23, 2023 21:51

chore: remove vulnerabilities from pacakge-lock.json

7bcb71a

wemeetagain reviewed Oct 23, 2023

View reviewed changes

maschad added 2 commits January 16, 2024 13:02

Merge branch 'master' into fix/tag-peers-on-graft

2417636

fix: handle error for peerStore merge

fb50dba

maschad requested a review from wemeetagain January 16, 2024 18:09

wemeetagain previously requested changes Jan 16, 2024

View reviewed changes

fix: PR feedback adjustments

ca7d3d3

maschad requested a review from wemeetagain January 29, 2024 21:47

maschad and others added 4 commits January 29, 2024 16:53

refactor: make taggingMesh flag required

3070a88

chore: clean up feature

e7dd1a2

Merge branch 'master' into HEAD

ea9d629

chore: pr review

525a91f

wemeetagain changed the title ~~fix: Tag mesh peers on graft, remove tags on prune~~ feat: graft/prune events and mesh peer tagging Feb 7, 2024

achingbrain reviewed Feb 20, 2024

View reviewed changes

achingbrain approved these changes Feb 20, 2024

View reviewed changes

Merge branch 'master' into fix/tag-peers-on-graft

86ef3d8

wemeetagain merged commit 42b5b92 into ChainSafe:master Feb 27, 2024
9 checks passed

github-actions bot mentioned this pull request Feb 27, 2024

chore(master): release 13.0.0 #492

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: graft/prune events and mesh peer tagging #383

feat: graft/prune events and mesh peer tagging #383

maschad commented Dec 14, 2022 •

edited by wemeetagain

Loading

wemeetagain commented Jan 10, 2023

maschad commented Jan 10, 2023

achingbrain commented Jan 10, 2023

codecov-commenter commented Jan 19, 2023 •

edited

Loading

dapplion left a comment

maschad commented Jan 23, 2023

dapplion commented Feb 13, 2023

maschad commented Feb 13, 2023

maschad commented Feb 13, 2023

maschad commented Feb 28, 2023

dapplion commented Mar 1, 2023

maschad commented Mar 1, 2023

maschad commented Oct 9, 2023

wemeetagain commented Oct 9, 2023

maschad commented Oct 10, 2023

maschad commented Oct 10, 2023

maschad commented Oct 23, 2023

wemeetagain Oct 23, 2023

wemeetagain Oct 23, 2023

maschad Oct 23, 2023

dapplion Oct 24, 2023

wemeetagain Jan 16, 2024

maschad Jan 29, 2024

achingbrain commented Jan 16, 2024 •

edited

Loading

wemeetagain Jan 16, 2024

wemeetagain Jan 16, 2024

maschad Jan 29, 2024

wemeetagain commented Feb 7, 2024

achingbrain Feb 20, 2024

achingbrain left a comment •

edited

Loading

feat: graft/prune events and mesh peer tagging #383

feat: graft/prune events and mesh peer tagging #383

Conversation

maschad commented Dec 14, 2022 • edited by wemeetagain Loading

wemeetagain commented Jan 10, 2023

maschad commented Jan 10, 2023

achingbrain commented Jan 10, 2023

codecov-commenter commented Jan 19, 2023 • edited Loading

Codecov Report

dapplion left a comment

Choose a reason for hiding this comment

maschad commented Jan 23, 2023

dapplion commented Feb 13, 2023

maschad commented Feb 13, 2023

maschad commented Feb 13, 2023

Gossip score (avg / min / max)

Unstable

Feat2

Peer count per gossip score threshold

Unstable

Feat2

Process CPU user seconds

Unstable

Feat2

Process Heap Bytes

Unstable

Feat2

Received messages per second

Unstable

Feat2

maschad commented Feb 28, 2023

dapplion commented Mar 1, 2023

maschad commented Mar 1, 2023

maschad commented Oct 9, 2023

wemeetagain commented Oct 9, 2023

maschad commented Oct 10, 2023

maschad commented Oct 10, 2023

maschad commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achingbrain commented Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wemeetagain commented Feb 7, 2024

Choose a reason for hiding this comment

achingbrain left a comment • edited Loading

Choose a reason for hiding this comment

maschad commented Dec 14, 2022 •

edited by wemeetagain

Loading

codecov-commenter commented Jan 19, 2023 •

edited

Loading

achingbrain commented Jan 16, 2024 •

edited

Loading

achingbrain left a comment •

edited

Loading