[ALSP] Synchronization Engine spam detection flag support, `config/README.md` updates #4842

gomisha · 2023-10-19T11:53:06Z

Context

This PR:

Defines 3 new start up flags for the Sync Engine to update the probability values that determine how incoming messages will be flagged as spam:
- alsp-sync-engine-batch-request-base-prob
- alsp-sync-engine-range-request-base-prob
- alsp-sync-engine-sync-request-prob
updates config/README.md with more examples, clarifications and broken link fixes

This PR adds flag support to the 3 PRs that added support for spam detection to the Sync Engine:

[ALSP] Synchronization Engine SyncRequest spam detection (Permissionless-related engine level spam detection) #4590
[ALSP] Synchronization Engine RangeRequest spam detection (Permissionless-related engine level spam detection) #4665
[ALSP] Synchronization Engine BatchRequest spam detection (Permissionless-related engine level spam detection) #4704

ref: https://github.com/dapperlabs/flow-go/issues/6812

codecov-commenter · 2023-10-19T12:05:40Z

Codecov Report

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

Comparison is base (cfab011) 55.91% compared to head (561d257) 53.84%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4842      +/-   ##
==========================================
- Coverage   55.91%   53.84%   -2.07%     
==========================================
  Files         945      676     -269     
  Lines       87965    67837   -20128     
==========================================
- Hits        49184    36527   -12657     
+ Misses      35089    28618    -6471     
+ Partials     3692     2692    -1000

Flag	Coverage Δ
unittests	`53.84% <38.09%> (-2.07%)`	⬇️

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

Files	Coverage Δ
engine/common/synchronization/config.go	`57.69% <80.00%> (-20.09%)`	⬇️
cmd/verification_builder.go	`0.00% <0.00%> (ø)`
cmd/execution_builder.go	`0.00% <0.00%> (ø)`

... and 277 files with indirect coverage changes

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

…values

kc1116 · 2023-10-19T19:50:19Z

engine/common/synchronization/config.go

@@ -68,10 +73,16 @@ type SpamDetectionConfig struct {
 }

 func NewSpamDetectionConfig() *SpamDetectionConfig {


Better to return an error and let the parent context throw it so that the node shuts down properly

kc1116 · 2023-10-19T19:53:17Z

network/netconf/config.go

+	SyncEngineBatchRequestBaseProb float32 `mapstructure:"alsp-sync-engine-batch-request-base-prob"`
+
+	// SyncEngineRangeRequestBaseProb is the base probability in [0,1] that's used in creating the final probability of creating a
+	// misbehavior report for a RangeRequest message. This is why the word "base" is used in the name of this field,
+	// since it's not the final probability and there are other factors that determine the final probability.
+	// The reason for this is that we want to increase the probability of creating a misbehavior report for a large range.
+	SyncEngineRangeRequestBaseProb float32 `mapstructure:"alsp-sync-engine-range-request-base-prob"`
+
+	// SyncEngineSyncRequestProb is the probability in [0,1] of creating a misbehavior report for a SyncRequest message.
+	SyncEngineSyncRequestProb float32 `mapstructure:"alsp-sync-engine-sync-request-prob"`


I suggest adding validation tags to make sure these are greater than 0

Suggested change

SyncEngineBatchRequestBaseProb float32 `mapstructure:"alsp-sync-engine-batch-request-base-prob"`

// SyncEngineRangeRequestBaseProb is the base probability in [0,1] that's used in creating the final probability of creating a

// misbehavior report for a RangeRequest message. This is why the word "base" is used in the name of this field,

// since it's not the final probability and there are other factors that determine the final probability.

// The reason for this is that we want to increase the probability of creating a misbehavior report for a large range.

SyncEngineRangeRequestBaseProb float32 `mapstructure:"alsp-sync-engine-range-request-base-prob"`

// SyncEngineSyncRequestProb is the probability in [0,1] of creating a misbehavior report for a SyncRequest message.

SyncEngineSyncRequestProb float32 `mapstructure:"alsp-sync-engine-sync-request-prob"`

SyncEngineBatchRequestBaseProb float32 `validate:"gt=0" mapstructure:"alsp-sync-engine-batch-request-base-prob"`

// SyncEngineRangeRequestBaseProb is the base probability in [0,1] that's used in creating the final probability of creating a

// misbehavior report for a RangeRequest message. This is why the word "base" is used in the name of this field,

// since it's not the final probability and there are other factors that determine the final probability.

// The reason for this is that we want to increase the probability of creating a misbehavior report for a large range.

SyncEngineRangeRequestBaseProb float32 `validate:"gt=0" mapstructure:"alsp-sync-engine-range-request-base-prob"`

// SyncEngineSyncRequestProb is the probability in [0,1] of creating a misbehavior report for a SyncRequest message.

SyncEngineSyncRequestProb float32 `validate:"gt=0" mapstructure:"alsp-sync-engine-sync-request-prob"`

Great idea. I made the validation enforce a range of values between 0 and 1.

26e817f

Also, please consider encapsulating them into a sync engine-specific config type:

// AlspConfig is the config for the Application Layer Spam Prevention (ALSP) protocol. type AlspConfig struct { //... Other Configs SyncEngine SyncEngineAlspConfig } type SyncEngineAlspConfig struct { // SyncEngineBatchRequestBaseProb is the base probability in [0,1] that's used in creating the final probability of creating a // misbehavior report for a BatchRequest message. This is why the word "base" is used in the name of this field, // since it's not the final probability and there are other factors that determine the final probability. // The reason for this is that we want to increase the probability of creating a misbehavior report for a large batch. BatchRequestBaseProb float32 `validate:"range=0,1" mapstructure:"alsp-sync-engine-batch-request-base-prob"` // SyncEngineRangeRequestBaseProb is the base probability in [0,1] that's used in creating the final probability of creating a // misbehavior report for a RangeRequest message. This is why the word "base" is used in the name of this field, // since it's not the final probability and there are other factors that determine the final probability. // The reason for this is that we want to increase the probability of creating a misbehavior report for a large range. RangeRequestBaseProb float32 `validate:"range=0,1" mapstructure:"alsp-sync-engine-range-request-base-prob"` // SyncEngineSyncRequestProb is the probability in [0,1] of creating a misbehavior report for a SyncRequest message. SyncRequestProb float32 `validate:"range=0,1" mapstructure:"alsp-sync-engine-sync-request-prob"` }

Great idea. Now the SyncEngine ALSP config is referenced list this:

batchRequestBaseProb: flowConfig.NetworkConfig.SyncEngine.SyncEngineBatchRequestBaseProb, syncRequestProb: flowConfig.NetworkConfig.SyncEngine.SyncEngineSyncRequestProb, rangeRequestBaseProb: flowConfig.NetworkConfig.SyncEngine.SyncEngineRangeRequestBaseProb,

5a5439d

Shortened field names to this:

batchRequestBaseProb: flowConfig.NetworkConfig.SyncEngine.BatchRequestBaseProb, syncRequestProb: flowConfig.NetworkConfig.SyncEngine.SyncRequestProb, rangeRequestBaseProb: flowConfig.NetworkConfig.SyncEngine.RangeRequestBaseProb,

590e81b

yhassanzadeh13

Except for one encapsulation comment, the rest looks good.

yhassanzadeh13 · 2023-10-20T17:32:55Z

network/netconf/config.go

+	SyncEngineBatchRequestBaseProb float32 `mapstructure:"alsp-sync-engine-batch-request-base-prob"`
+
+	// SyncEngineRangeRequestBaseProb is the base probability in [0,1] that's used in creating the final probability of creating a
+	// misbehavior report for a RangeRequest message. This is why the word "base" is used in the name of this field,
+	// since it's not the final probability and there are other factors that determine the final probability.
+	// The reason for this is that we want to increase the probability of creating a misbehavior report for a large range.
+	SyncEngineRangeRequestBaseProb float32 `mapstructure:"alsp-sync-engine-range-request-base-prob"`
+
+	// SyncEngineSyncRequestProb is the probability in [0,1] of creating a misbehavior report for a SyncRequest message.
+	SyncEngineSyncRequestProb float32 `mapstructure:"alsp-sync-engine-sync-request-prob"`


Also, please consider encapsulating them into a sync engine-specific config type:

// AlspConfig is the config for the Application Layer Spam Prevention (ALSP) protocol. type AlspConfig struct { //... Other Configs SyncEngine SyncEngineAlspConfig } type SyncEngineAlspConfig struct { // SyncEngineBatchRequestBaseProb is the base probability in [0,1] that's used in creating the final probability of creating a // misbehavior report for a BatchRequest message. This is why the word "base" is used in the name of this field, // since it's not the final probability and there are other factors that determine the final probability. // The reason for this is that we want to increase the probability of creating a misbehavior report for a large batch. BatchRequestBaseProb float32 `validate:"range=0,1" mapstructure:"alsp-sync-engine-batch-request-base-prob"` // SyncEngineRangeRequestBaseProb is the base probability in [0,1] that's used in creating the final probability of creating a // misbehavior report for a RangeRequest message. This is why the word "base" is used in the name of this field, // since it's not the final probability and there are other factors that determine the final probability. // The reason for this is that we want to increase the probability of creating a misbehavior report for a large range. RangeRequestBaseProb float32 `validate:"range=0,1" mapstructure:"alsp-sync-engine-range-request-base-prob"` // SyncEngineSyncRequestProb is the probability in [0,1] of creating a misbehavior report for a SyncRequest message. SyncRequestProb float32 `validate:"range=0,1" mapstructure:"alsp-sync-engine-sync-request-prob"` }

…truct

gomisha added 4 commits October 19, 2023 05:57

alsp sync engine flag default values

a1f1d88

alsp sync engine struct fields

b479b7e

alsp sync engine flag definition in const

ecebde7

Merge branch 'master' into misha/6812-alsp-sync-engine-spam-flags

c1024a6

gomisha added 3 commits October 19, 2023 07:52

SpamDetectionConfig reads flags (removed hard codes), update default …

a99d979

…values

lint fix

6027558

Merge branch 'master' into misha/6812-alsp-sync-engine-spam-flags

a69514d

gomisha marked this pull request as ready for review October 19, 2023 19:17

gomisha requested a review from yhassanzadeh13 as a code owner October 19, 2023 19:17

kc1116 reviewed Oct 19, 2023

View reviewed changes

kc1116 approved these changes Oct 19, 2023

View reviewed changes

gomisha added 6 commits October 20, 2023 07:22

validation tags to make sure probability values in range [0,1]

26e817f

more docs for ALSP flags

a040849

NewSpamDetectionConfig() passes up error instead of throwing error

91607b8

Merge branch 'master' into misha/6812-alsp-sync-engine-spam-flags

14cfabd

fix broken link in config README

58c93f6

config README update

5abfe85

gomisha changed the title ~~[ALSP] Synchronization Engine spam detection flag support~~ [ALSP] Synchronization Engine spam detection flag support, config/README.md updates Oct 20, 2023

lint fix

d5f8bf3

gomisha requested review from peterargue, AlexHentschel, durkmurder and jordanschalm as code owners October 20, 2023 15:15

lint fix

175adaf

yhassanzadeh13 approved these changes Oct 20, 2023

View reviewed changes

gomisha added 3 commits October 20, 2023 14:33

encapsulate SyncEngine ALSP config in separate struct

5a5439d

SyncEngineAlspConfig - squashed, fixed valid range values

bf7b982

shorten field names in encapsulated SyncEngine ALSP config separate s…

590e81b

…truct

gomisha added 2 commits October 23, 2023 08:06

config/README.md updated, clarified, fixed broken links

9bf7ec8

README.md update re:encourage config subpackage

9a4769c

gomisha enabled auto-merge October 23, 2023 21:12

Merge branch 'master' into misha/6812-alsp-sync-engine-spam-flags

561d257

gomisha added this pull request to the merge queue Oct 23, 2023

Merged via the queue into master with commit e53460b Oct 23, 2023
36 checks passed

gomisha deleted the misha/6812-alsp-sync-engine-spam-flags branch October 23, 2023 23:25

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ALSP] Synchronization Engine spam detection flag support, `config/README.md` updates #4842

[ALSP] Synchronization Engine spam detection flag support, `config/README.md` updates #4842

gomisha commented Oct 19, 2023 •

edited

Loading

codecov-commenter commented Oct 19, 2023 •

edited

Loading

kc1116 Oct 19, 2023

gomisha Oct 20, 2023

kc1116 Oct 19, 2023

gomisha Oct 20, 2023

yhassanzadeh13 Oct 20, 2023

gomisha Oct 20, 2023 •

edited

Loading

gomisha Oct 21, 2023

yhassanzadeh13 left a comment

yhassanzadeh13 Oct 20, 2023

		@@ -68,10 +73,16 @@ type SpamDetectionConfig struct {
		}

		func NewSpamDetectionConfig() *SpamDetectionConfig {

[ALSP] Synchronization Engine spam detection flag support, config/README.md updates #4842

[ALSP] Synchronization Engine spam detection flag support, config/README.md updates #4842

Conversation

gomisha commented Oct 19, 2023 • edited Loading

Context

codecov-commenter commented Oct 19, 2023 • edited Loading

Codecov Report

kc1116 Oct 19, 2023

Choose a reason for hiding this comment

gomisha Oct 20, 2023

Choose a reason for hiding this comment

kc1116 Oct 19, 2023

Choose a reason for hiding this comment

gomisha Oct 20, 2023

Choose a reason for hiding this comment

yhassanzadeh13 Oct 20, 2023

Choose a reason for hiding this comment

gomisha Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

gomisha Oct 21, 2023

Choose a reason for hiding this comment

yhassanzadeh13 left a comment

Choose a reason for hiding this comment

yhassanzadeh13 Oct 20, 2023

Choose a reason for hiding this comment

[ALSP] Synchronization Engine spam detection flag support, `config/README.md` updates #4842

[ALSP] Synchronization Engine spam detection flag support, `config/README.md` updates #4842

gomisha commented Oct 19, 2023 •

edited

Loading

codecov-commenter commented Oct 19, 2023 •

edited

Loading

gomisha Oct 20, 2023 •

edited

Loading