Antiflood supernova config#7651
Conversation
| } | ||
|
|
||
| func (qfp *quotaFloodPreventer) getBbaseMaxNumMessagesPerPeer() uint32 { | ||
| if qfp.name == antiflood.OutputIdentifier { |
There was a problem hiding this comment.
there is custom handling here for this case, because some of the variables are not available in this config struct and they are set 0 by default
| initialAntifloodConf := antifloodConfigsHandler.GetCurrentConfig() | ||
|
|
||
| // TODO: this config section have to be loaded with new configration from the start | ||
| cacheConfig := storageFactory.GetCacherFromConfig(initialAntifloodConf.Cache) |
There was a problem hiding this comment.
we might need to update Cacher component to set limit dynamically by round if we want to change it dynamically
| [[Antiflood.ConfigsByRound]] | ||
| Round = 440 |
There was a problem hiding this comment.
will have to update config variables for supernova
| } | ||
|
|
||
| mainThrottler, err := throttler.NewNumGoRoutinesThrottler(args.NumConcurrentResolvingJobs) | ||
| currentConfig := args.AntifloodConfigsHandler.GetCurrentConfig() |
There was a problem hiding this comment.
this config part is not dynamic, it will load only based on the config at boot time
in order to make it dynamic we have to change GoRountinesThrottler from core
| "github.com/multiversx/mx-chain-go/config" | ||
| ) | ||
|
|
||
| func floodPreventerConfigFetcher(confHandler common.AntifloodConfigsHandler, identifier string) config.FloodPreventerConfig { |
There was a problem hiding this comment.
this is a bit hacky, but it's due to the fact that we have 4 flood preventer components and 3 of them use config.FloodPreventerConfig and one is based on another config. Before it was handled statically in factory, but now we have to set the configuration dynamically, and there was need for some custom handling
| @@ -627,71 +621,151 @@ | |||
|
|
|||
| [Antiflood] | |||
There was a problem hiding this comment.
TODO: move antiflood config to a separate file
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/supernova-async-exec #7651 +/- ##
=============================================================
- Coverage 77.59% 77.57% -0.03%
=============================================================
Files 877 879 +2
Lines 121752 121849 +97
=============================================================
+ Hits 94476 94526 +50
- Misses 21008 21032 +24
- Partials 6268 6291 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| antifloodConfigs common.AntifloodConfigsHandler, | ||
| configFetcher antiflood.FloodPreventerConfigFetcher, |
There was a problem hiding this comment.
why not extend AntifloodConfigsHandler, to have new method GetCurrentConfigByType?
There was a problem hiding this comment.
or, instead of [[Antiflood.ConfigsByRound]] -> [Antiflood.ConfigsByRound.FastReacting] have something like [Antiflood] -> [[Antiflood.FastReactingByRound]]
There was a problem hiding this comment.
i wanted to keep the flow as similar as before, not expose those type consts and keep them in that package (at least that's what a thought when i started the refactoring there).
Yes, it might be better to change the config, will analyse more now
There was a problem hiding this comment.
changed config and refactored this part
| } | ||
| if config.Antiflood.Enabled { | ||
| return initP2PAntiFloodComponents(ctx, config, statusHandler, currentPid, processConfigsHandler) | ||
| if antifloodConfigsHandler.IsEnabled() { |
| ctx context.Context, | ||
| antifloodConfigsHandler common.AntifloodConfigsHandler, | ||
| ) (process.P2PAntifloodHandler, error) { | ||
| if antifloodConfigsHandler.IsEnabled() { |
There was a problem hiding this comment.
same here, missing nil check
| [Antiflood.ConfigsByRound.PeerMaxOutput] | ||
| BaseMessagesPerInterval = 150 | ||
| TotalSizePerInterval = 2097152 #2MB/s | ||
| [Antiflood.ConfigsByRound.PeerMaxOutput.PeerMaxInput] |
There was a problem hiding this comment.
yes, in order to make it work with GetConfigByType i had to change it to use the same base config struct (until now 3 flood preventers were using FloodPreventerConfig and one was using directly AntifloodLimitsConfig - which is part of FloodPreventerConfig - with the other variables set to 0 in factory)
There was a problem hiding this comment.
remove the entire file
| ConfigsByRound []AntifloodConfigByRound | ||
| } | ||
|
|
||
| type AntifloodConfigByRound struct { |
Reasoning behind the pull request
Antifloodconfig for supernova with round activationTesting procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?