[Networking] Implements a silence period for GossipSub peer scoring. #5084

kc1116 · 2023-11-29T23:04:15Z

This PR addresses an issue observed in the mainnet24 spork, where nodes wrongly penalize peers for misbehavior during startup. The problem arises when nodes, not fully subscribed to all channels at startup, misinterpret messages from unsubscribed channels as malicious activity. This leads to unnecessary logs, inaccurate metrics, and the risk of early network fragmentation.

To solve this, the PR introduces a configurable silence duration (gossipsub-scoring-registry-startup-silence-duration). During this period:

Gossipsub queries for a peer's application-specific score return a default score of 0, with no penalties.
Invalid control message notifications are ignored, preventing the creation of spam records and penalties. This ensures nodes aren't penalized immediately after the silence period.

ref: #4979

…coring-registry-startup-silence-period

codecov-commenter · 2023-12-01T23:39:50Z

Codecov Report

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

Comparison is base (f040031) 56.29% compared to head (bb1d6a3) 56.29%.

Files	Patch %	Lines
network/p2p/test/fixtures.go	0.00%	12 Missing ⚠️
network/netconf/flags.go	25.00%	3 Missing ⚠️
network/p2p/scoring/registry.go	88.88%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5084      +/-   ##
==========================================
- Coverage   56.29%   56.29%   -0.01%     
==========================================
  Files         994      994              
  Lines       94963    95003      +40     
==========================================
+ Hits        53463    53485      +22     
- Misses      37526    37540      +14     
- Partials     3974     3978       +4

Flag	Coverage Δ
unittests	`56.29% <58.13%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

peterargue · 2023-12-11T18:35:41Z

config/default-config.yml

+  # returned for all nodes will be 0, and any invalid control message notifications
+  # will be ignored. This configuration allows nodes to stabilize and initialize before
+  # applying penalties or processing invalid control message notifications.
+  gossipsub-scoring-registry-startup-silence-duration: 20m


how did you come up with this default, and the min 10m? This seems too long for some node types and potentially too short for others (ENs). rather than a timer, is it possible to trigger a signal when startup is complete?

@kc1116 maybe we can use this node.Ready channel instead of a silence period.

flow-go/cmd/node.go

Line 98 in cd7acd6

case <-node.Ready():

b8a2fd2 The silence period default duration has been changed to 1 hour. Here is the follow up issue to create a component that can notify other components when it is ready

yhassanzadeh13

Overall looks good 🚀, please consider applying the comments prior merging.

yhassanzadeh13 · 2023-12-11T18:12:18Z

network/p2p/scoring/registry.go

+func (r *GossipSubAppSpecificScoreRegistry) Start(parent irrecoverable.SignalerContext) {
+	if !r.silencePeriodStartTime.IsZero() {
+		parent.Throw(fmt.Errorf("gossipsub scoring registry started more than once"))
+	}
+	r.silencePeriodStartTime = time.Now()
+	r.Component.Start(parent)
+}


Please consider adding this as a worker in NewGossipSubAppSpecificScoreRegistry, overwriting the Start seems unnecessary.

}).AddWorker(func(ctx irrecoverable.SignalerContext, ready component.ReadyFunc) { if !reg.silencePeriodStartTime.IsZero() { ctx.Throw(fmt.Errorf("gossipsub scoring registry started more than once")) } reg.silencePeriodStartTime = time.Now() })

network/p2p/scoring/registry.go

yhassanzadeh13 · 2023-12-11T22:02:20Z

network/p2p/scoring/registry.go

+// afterSilencePeriod returns true if registry silence period is over, false otherwise.
+func (r *GossipSubAppSpecificScoreRegistry) afterSilencePeriod() bool {
+	return time.Since(r.silencePeriodStartTime) > r.silencePeriodDuration
+}
+


The silence period is a short startup interval. Once the silence period elapses, comparing the time on each invocation of the afterSilencePeriod is unnecessary and may apply a computation overhead. Especially, this method is called frequently on each incoming message to the node (or each invalid control message notification). Maybe introducing an atomic boolean variable acts more efficiently:

In the below, we assume silencePeriodElpased is an atomic boolean that is initialized to false.

// afterSilencePeriod returns true if registry silence period is over, false otherwise. func (r *GossipSubAppSpecificScoreRegistry) afterSilencePeriod() bool { if !r.silencePeriodElpased.Load() { if time.Since(r.silencePeriodStartTime) > r.silencePeriodDuration { r.silencePeriodElpased.Store(true) return true } return false } return true }

network/p2p/scoring/registry_test.go

yhassanzadeh13 · 2023-12-11T22:07:08Z

network/p2p/scoring/registry_test.go

@@ -387,8 +421,13 @@ func TestPersistingInvalidSubscriptionPenalty(t *testing.T) {
 			}
 		}))

+	ctx, cancel := context.WithCancel(context.Background())
+	signalerCtx := irrecoverable.NewMockSignalerContext(t, ctx)
+	reg.Start(signalerCtx)


For all these tests, we should first ensure that the registry has started and is in Ready mode.

yhassanzadeh13 · 2023-12-11T22:08:34Z

network/p2p/scoring/registry_test.go

+	silencedNotificationLogs := atomic.NewInt32(0)
+	hook := zerolog.HookFunc(func(e *zerolog.Event, level zerolog.Level, message string) {
+		if level == zerolog.DebugLevel {
+			if message == "ignoring invalid control message notification for peer during silence period" {


Please consider casting this message as a constant that is shared between the code and test logic. In this way changing the log wording doesn't break the test.

Co-authored-by: Yahya Hassanzadeh, Ph.D. <[email protected]>

… github.com:onflow/flow-go into khalil/4979-scoring-registry-startup-silence-period

…ce-period

kc1116 added 5 commits November 27, 2023 18:11

Merge branch 'master' of github.com:onflow/flow-go

192aa74

Merge branch 'master' of github.com:onflow/flow-go

f20ba06

add gossipsub-scoring-registry-startup-silence-period config

49150ac

update scoring registry to not penalize nodes during silence period

7550f65

add registry test that ensures silence period behaves as expected

135f027

kc1116 requested a review from yhassanzadeh13 as a code owner November 29, 2023 23:04

kc1116 requested a review from peterargue November 29, 2023 23:05

kc1116 added 2 commits November 29, 2023 18:08

Update registry.go

e214946

Merge branch 'master' of github.com:onflow/flow-go into khalil/4979-s…

271d744

…coring-registry-startup-silence-period

yhassanzadeh13 changed the title ~~Khalil/4979 LibP2P node startup gossipsub silence period.~~ [Networking] Implements a silence period for GossipSub peer scoring. Dec 11, 2023

peterargue reviewed Dec 11, 2023

View reviewed changes

yhassanzadeh13 approved these changes Dec 11, 2023

View reviewed changes

kc1116 and others added 11 commits January 1, 2024 22:18

Update network/p2p/scoring/registry.go

ef26bbd

Co-authored-by: Yahya Hassanzadeh, Ph.D. <[email protected]>

Update network/p2p/scoring/registry.go

793b2fc

Co-authored-by: Yahya Hassanzadeh, Ph.D. <[email protected]>

Update network/p2p/scoring/registry_test.go

ee84e4c

Co-authored-by: Yahya Hassanzadeh, Ph.D. <[email protected]>

move silce startup to worker

763754e

Merge branch 'khalil/4979-scoring-registry-startup-silence-period' of…

79e0149

… github.com:onflow/flow-go into khalil/4979-scoring-registry-startup-silence-period

store silence period time elapsed in atomic bool

080e024

ensure registry is ready

af54b59

use const message for silence log

6d9f1d2

add long silence period

b8a2fd2

Merge branch 'master' into khalil/4979-scoring-registry-startup-silen…

b00521c

…ce-period

update registry tests

3e45ea0

kc1116 requested a review from peterargue January 9, 2024 16:52

kc1116 added 4 commits January 17, 2024 13:39

Merge branch 'master' into khalil/4979-scoring-registry-startup-silen…

6d2d169

…ce-period

update flags

042274f

fix lint

52db024

Update flags.go

bb1d6a3

peterargue approved these changes Jan 17, 2024

View reviewed changes

kc1116 added this pull request to the merge queue Jan 18, 2024

Merged via the queue into master with commit aeb29e8 Jan 18, 2024
50 of 51 checks passed

kc1116 deleted the khalil/4979-scoring-registry-startup-silence-period branch January 18, 2024 03:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Networking] Implements a silence period for GossipSub peer scoring. #5084

[Networking] Implements a silence period for GossipSub peer scoring. #5084

kc1116 commented Nov 29, 2023 •

edited

Loading

codecov-commenter commented Dec 1, 2023 •

edited

Loading

peterargue Dec 11, 2023

yhassanzadeh13 Dec 11, 2023

kc1116 Jan 3, 2024 •

edited

Loading

yhassanzadeh13 left a comment

yhassanzadeh13 Dec 11, 2023

kc1116 Jan 2, 2024

yhassanzadeh13 Dec 11, 2023

kc1116 Jan 2, 2024

yhassanzadeh13 Dec 11, 2023

kc1116 Jan 2, 2024

yhassanzadeh13 Dec 11, 2023

kc1116 Jan 2, 2024

[Networking] Implements a silence period for GossipSub peer scoring. #5084

[Networking] Implements a silence period for GossipSub peer scoring. #5084

Conversation

kc1116 commented Nov 29, 2023 • edited Loading

codecov-commenter commented Dec 1, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kc1116 Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

yhassanzadeh13 left a 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

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

kc1116 commented Nov 29, 2023 •

edited

Loading

codecov-commenter commented Dec 1, 2023 •

edited

Loading

kc1116 Jan 3, 2024 •

edited

Loading