[Networking] GossipSub iWant Flooding Mitigation #4574

kc1116 · 2023-07-19T21:40:45Z

This PR adds iWant flooding detection to the control message RPC validation inspector. iWant validation is performed by taking a sample of all message ID's included in all the iWant control messages of an RPC and performing validation on the sample. An iWant control message ID must not be duplicated and must have been sent in a previous iHave control message by the receiver of the iWant message. This PR simplifies control message validation by replacing Hard and Safety thresholds with a single configuration value MaxSampleSize. Validation will be done on samples up to MaxSampleSize for all control message types, and any control messages not included in that sample will be dropped and resent with a subsequent RPC. Validation for the other control message types will be updated with this new strategy.

yhassanzadeh13

Overall, the trajectory of the development looks good, I made a few comments for improvement.

network/p2p/inspector/validation/control_message_validation_inspector.go

yhassanzadeh13 · 2023-07-19T23:44:25Z

network/p2p/inspector/validation/control_message_validation_inspector.go

+	err := c.sampleCtrlMessages(uint(len(iWants)), sampleSize, swap)
+	if err != nil {
+		return fmt.Errorf("failed to sample iwant messages: %w", err)
+	}


This looks fine and is a fail-safe approach, i.e., we don't want to continue the execution if we cannot sample a control message properly. But please note that whatever error a worker processFunc returns will result in the node to crash, hence we may need to be more cautious on what to return as error and what to log.

network/p2p/inspector/validation/control_message_validation_inspector.go

…want-flooding-detection

…spector.go Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>

- update builders - update unit test

- log fatal error if any error encountered while sampling

- remove nested for loop

codecov-commenter · 2023-08-16T21:44:27Z

Codecov Report

Merging #4574 (37cfa4d) into master (a8f28b9) will decrease coverage by 2.85%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4574      +/-   ##
==========================================
- Coverage   54.71%   51.87%   -2.85%     
==========================================
  Files         917      635     -282     
  Lines       85633    59471   -26162     
==========================================
- Hits        46858    30848   -16010     
+ Misses      35183    26163    -9020     
+ Partials     3592     2460    -1132

Flag	Coverage Δ
unittests	`51.87% <ø> (-2.85%)`	⬇️

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

Files Changed	Coverage Δ
module/component/component.go	`89.92% <ø> (ø)`

... and 287 files with indirect coverage changes

insecure/corruptlibp2p/fixtures.go

network/p2p/inspector/validation/control_message_validation_inspector.go

yhassanzadeh13 · 2023-08-18T19:57:35Z

network/p2p/inspector/validation/control_message_validation_inspector.go

+	swap := func(i, j uint) {
+		iHaves[i], iHaves[j] = iHaves[j], iHaves[i]
+	}
+
 	activeClusterIDS := c.tracker.GetActiveClusterIds()
 	count := c.getCtrlMsgCount(validationConfig.ControlMsg, controlMessage)


I have observed that the current implementation of getCtrlMsgCount does not meet our specific requirements. Instead of returning the total count of control message types within the RPC, it should provide the total number of message-ids for a specific message type. Take the iHave message as an example; an RPC may contain several iHave messages, each with a distinct set of message-ids.

This situation creates a problem, particularly in the evaluation of count > validationConfig.HardThreshold. In this context, count is representative of the total number of iHave messages, each containing a distinct message-id count, whereas validationConfig.HardThreshold denotes the total number of iHave message-ids. Consequently, the condition within the if statement is often incorrectly assessed as false in many cases. This inconsistency must be addressed to align the logic with our intended functionality.

I kindly request that you refactor this function to accurately reflect the message-id count for the given message type, aligning with the discussion found at this GitHub comment. Thank you.

This is covered in the follow up PR that adapts this strategy to the other control messages #4642

network/p2p/inspector/validation/errors.go

network/p2p/inspector/validation/errors_test.go

network/p2p/inspector/validation/utils.go

network/p2p/tracer/internal/rpc_sent_tracker.go

network/p2p/inspector/validation/control_message_validation_inspector.go

…spector.go Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>

Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>

…low/flow-go into khalil/6472-iwant-flooding-detection

gomisha · 2023-08-24T09:01:47Z

insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go

+func meshTracerFixture(flowConfig *config.FlowConfig, idProvider module.IdentityProvider) *tracer.GossipSubMeshTracer {
+	meshTracerCfg := &tracer.GossipSubMeshTracerConfig{
+		Logger:                             unittest.Logger(),
+		Metrics:                            metrics.NewNoopCollector(),
+		IDProvider:                         idProvider,
+		LoggerInterval:                     time.Second,
+		HeroCacheMetricsFactory:            metrics.NewNoopHeroCacheMetricsFactory(),
+		RpcSentTrackerCacheSize:            flowConfig.NetworkConfig.GossipSubConfig.RPCSentTrackerCacheSize,
+		RpcSentTrackerWorkerQueueCacheSize: flowConfig.NetworkConfig.GossipSubConfig.RPCSentTrackerQueueCacheSize,
+		RpcSentTrackerNumOfWorkers:         flowConfig.NetworkConfig.GossipSubConfig.RpcSentTrackerNumOfWorkers,
+	}
+	return tracer.NewGossipSubMeshTracer(meshTracerCfg)
+}


should this test helper be moved to insecure/integration/functional/test/gossipsub/rpc_inspector/utils.go?

network/p2p/inspector/validation/control_message_validation_inspector.go

gomisha

Looks good. Please address comments and the failing unit test before merging.

Co-authored-by: Misha <misha.rybalov@dapperlabs.com>

…low/flow-go into khalil/6472-iwant-flooding-detection

…spector.go Co-authored-by: Misha <misha.rybalov@dapperlabs.com>

add iWant control message validation

0093a47

yhassanzadeh13 reviewed Jul 20, 2023

View reviewed changes

kc1116 and others added 8 commits July 25, 2023 09:27

Update component.go

41ef40b

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

5cf8b9d

…want-flooding-detection

Update network/p2p/inspector/validation/control_message_validation_in…

322aee5

…spector.go Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>

add rpc control tracking to control message validation inspector

03a4423

- update builders - update unit test

distribute invalid control message notification for iwant errs

85f63c6

- log fatal error if any error encountered while sampling

sample all message IDs

3e99842

- remove nested for loop

add duplicate messageID and cache miss threshold tests

2e6c88a

Delete export_report.json

b745ea3

kc1116 marked this pull request as ready for review August 16, 2023 21:36

kc1116 requested review from gomisha and peterargue as code owners August 16, 2023 21:36

Merge branch 'master' into khalil/6472-iwant-flooding-detection

acf9486

kc1116 mentioned this pull request Aug 16, 2023

[Access] Add caching tx results #4598

Merged

yhassanzadeh13 changed the title ~~Khalil/6472 iwant flooding detection~~ [Networking] iWant Flooding Mitigation Aug 18, 2023

yhassanzadeh13 changed the title ~~[Networking] iWant Flooding Mitigation~~ [Networking] GossipSub iWant Flooding Mitigation Aug 18, 2023

kc1116 requested a review from yhassanzadeh13 August 18, 2023 19:13

yhassanzadeh13 reviewed Aug 18, 2023

View reviewed changes

network/p2p/inspector/validation/control_message_validation_inspector.go Outdated Show resolved Hide resolved

kc1116 and others added 9 commits August 22, 2023 10:46

update WithIWant godoc and var names

3ff2eaf

allow a threshold of duplicate message ids

eccb7a1

update threshold comparisons

38d5caf

Update network/p2p/inspector/validation/control_message_validation_in…

704a20d

…spector.go Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>

Update network/p2p/inspector/validation/errors.go

26a7159

Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>

Update network/p2p/inspector/validation/errors.go

c815a12

Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>

Merge branch 'khalil/6472-iwant-flooding-detection' of github.com:onf…

9c09799

…low/flow-go into khalil/6472-iwant-flooding-detection

add round trip tests for new errors

d9c7970

add happy path tests for util dup str tracker

bb87e90

gomisha reviewed Aug 24, 2023

View reviewed changes

network/p2p/inspector/validation/control_message_validation_inspector.go Outdated Show resolved Hide resolved

gomisha approved these changes Aug 24, 2023

View reviewed changes

kc1116 and others added 25 commits August 24, 2023 12:33

add trace level log to improve debugging

848a708

add debug log with cache miss and duplicate counts

efdeb97

fix flag

c9e1dab

add logging.KeyNetworkingSecurity to all error logs

fb521fc

Update config/default-config.yml

ba7406e

Co-authored-by: Misha <misha.rybalov@dapperlabs.com>

Update insecure/corruptlibp2p/fixtures.go

6d226ad

Co-authored-by: Misha <misha.rybalov@dapperlabs.com>

rename inspectDisseminatedNotif -> inspectDisseminatedNotifyFunc

acb84a0

move utils to utils file

4090cb0

Update validation_inspector_test.go

45e77ff

move utils to utils file

a6d9eaa

Merge branch 'khalil/6472-iwant-flooding-detection' of github.com:onf…

130fc6b

…low/flow-go into khalil/6472-iwant-flooding-detection

Update network/p2p/inspector/validation/control_message_validation_in…

e249547

…spector.go Co-authored-by: Misha <misha.rybalov@dapperlabs.com>

Merge branch 'master' into khalil/6472-iwant-flooding-detection

1f71d13

fix test

eb6434d

Merge branch 'master' into khalil/6472-iwant-flooding-detection

e530edc

remove old test

897cb98

improve geometric test flakiness

96c1d4f

improve echoengine test flakiness

2e3a710

add 500 millisecond sleep before lauching second batch of goroutines

a865809

quarantine flaky tests

88ab754

Update decay_test.go

8150c52

Update decay_test.go

367b94c

Update echoengine_test.go

37cfa4d

Update validation_inspector_test.go

7c1d963

Update validation_inspector_test.go

a4fcdb5

gomisha merged commit 5f9c8a0 into master Aug 29, 2023
35 of 36 checks passed

gomisha deleted the khalil/6472-iwant-flooding-detection branch August 29, 2023 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Networking] GossipSub iWant Flooding Mitigation #4574

[Networking] GossipSub iWant Flooding Mitigation #4574

kc1116 commented Jul 19, 2023 •

edited

Loading

yhassanzadeh13 left a comment

yhassanzadeh13 Jul 19, 2023

codecov-commenter commented Aug 16, 2023 •

edited

Loading

yhassanzadeh13 Aug 18, 2023

kc1116 Aug 22, 2023

gomisha Aug 24, 2023

kc1116 Aug 24, 2023

gomisha left a comment

[Networking] GossipSub iWant Flooding Mitigation #4574

[Networking] GossipSub iWant Flooding Mitigation #4574

Conversation

kc1116 commented Jul 19, 2023 • edited Loading

yhassanzadeh13 left a comment

Choose a reason for hiding this comment

yhassanzadeh13 Jul 19, 2023

Choose a reason for hiding this comment

codecov-commenter commented Aug 16, 2023 • edited Loading

Codecov Report

yhassanzadeh13 Aug 18, 2023

Choose a reason for hiding this comment

kc1116 Aug 22, 2023

Choose a reason for hiding this comment

gomisha Aug 24, 2023

Choose a reason for hiding this comment

kc1116 Aug 24, 2023

Choose a reason for hiding this comment

gomisha left a comment

Choose a reason for hiding this comment

kc1116 commented Jul 19, 2023 •

edited

Loading

codecov-commenter commented Aug 16, 2023 •

edited

Loading