[ALSP] Adjust Node Penalty Decay Speed on Repeated Disallow-listing #4485
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…to yahya/6470-alsp-part-5-decay
Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>
yhassanzadeh13
approved these changes
Aug 14, 2023
network/alsp/manager/manager.go
Outdated
Comment on lines
358
to
361
|
|
||
| //// after fully decaying the penalty, update decay for next disallow listing | ||
| //record.UpdateDecay() | ||
|
|
There was a problem hiding this comment.
Please remove commented code if it is not needed.
network/alsp/manager/manager_test.go
Outdated
| p2ptest.RequireConnectedEventually(t, []p2p.LibP2PNode{nodes[honestIndex], nodes[spammerIndex]}, 1*time.Millisecond, 100*time.Millisecond) | ||
|
|
||
| // ensures that the spammer is disallow-listed for the expected amount of time | ||
|
|
|
|
||
| // check the penalty of the spammer node. It should be greater than the disallow-listing threshold. | ||
| require.Greater(t, float64(model.DisallowListingThreshold), record.Penalty) | ||
|
|
| require.Equal(t, float64(expectedDecays[expectedDecay]), record.Decay) | ||
| require.Equal(t, true, record.DisallowListed) | ||
| require.Equal(t, uint64(expectedCutoffCounter), record.CutoffCounter) | ||
|
|
network/alsp/manager/manager_test.go
Outdated
| expectedDecays := []int{1000, 100, 10, 1, 1, 1} // list of expected decay values after each disallow listing | ||
|
|
||
| t.Log("resetting cutoff counter") | ||
| expectedCutoffCounter := 0 |
There was a problem hiding this comment.
Suggested change
| expectedCutoffCounter := 0 | |
| expectedCutoffCounter := uint64(0) |
Please also remove the type casting of uint64(expectedCutoffCounter) and use expectedCutoffCounter instead.
| require.Greater(t, float64(model.DisallowListingThreshold), record.Penalty) | ||
|
|
||
| require.Equal(t, float64(expectedDecays[expectedDecay]), record.Decay) | ||
| require.Equal(t, true, record.DisallowListed) |
There was a problem hiding this comment.
Suggested change
| require.Equal(t, true, record.DisallowListed) | |
| // when a node is disallow-listed, it remains disallow-listed till its penalty decays back to zero. | |
| require.Equal(t, true, record.DisallowListed) |
Comment on lines
+1716
to
+1719
| ////////////////////////////// TEST HELPERS /////////////////////////////////////////////////////////////////////////////// | ||
| // The following functions are helpers for the tests. It wasn't feasible to put them in a helper file in the alspmgr_test | ||
| // package because that would break encapsulation of the ALSP manager and require making some fields exportable. | ||
| // Putting them in alspmgr package would cause a circular import cycle. Therefore, they are put in the internal test package here. |
There was a problem hiding this comment.
Maybe moving these test helpers into a separate file but in the same package is more maintainable, e.g., testHelpers.go.
There was a problem hiding this comment.
Unfortunately, Go doesn't allow that. Only *_test.go files are allowed to be in the alspmgr_test package. If I try and create a new test_helpers.go file with the same alspmgr_test package, the compiler complains: Multiple packages in the directory: alspmgr, alspmgr_test.
network/alsp/manager/manager_test.go
Outdated
| expectedCutoffCounter := 0 | ||
|
|
||
| // keep misbehaving until the spammer is disallow-listed and check that the decay is as expected | ||
| for expectedDecay := range expectedDecays { |
There was a problem hiding this comment.
Suggested change
| for expectedDecay := range expectedDecays { | |
| for expectedDecayIndex := range expectedDecays { |
network/alsp/manager/manager_test.go
Outdated
Comment on lines
395
to
396
| // check the penalty of the spammer node. It should be greater than the disallow-listing threshold. | ||
| require.Greater(t, float64(model.DisallowListingThreshold), record.Penalty) |
There was a problem hiding this comment.
Suggested change
| // check the penalty of the spammer node. It should be greater than the disallow-listing threshold. | |
| require.Greater(t, float64(model.DisallowListingThreshold), record.Penalty) | |
| // check the penalty of the spammer node. It should be below the disallow-listing threshold, i.e., | |
| // spammer penalty should be more negative than the disallow-listing threshold, hence disallow-listed. | |
| require.Less(t, record.Penalty, float64(model.DisallowListingThreshold)) |
network/alsp/manager/manager_test.go
Outdated
Comment on lines
379
to
380
| // check the penalty of the spammer node. It should be greater than the disallow-listing threshold. | ||
| require.Greater(t, float64(model.DisallowListingThreshold), record.Penalty) |
There was a problem hiding this comment.
Suggested change
| // check the penalty of the spammer node. It should be greater than the disallow-listing threshold. | |
| require.Greater(t, float64(model.DisallowListingThreshold), record.Penalty) | |
| // check the penalty of the spammer node. It should be below the disallow-listing threshold, i.e., | |
| // spammer penalty should be more negative than the disallow-listing threshold, hence disallow-listed. | |
| require.Less(t, record.Penalty, float64(model.DisallowListingThreshold)) |
kc1116
reviewed
Aug 16, 2023
kc1116
approved these changes
Aug 16, 2023
Co-authored-by: Khalil Claybon <khalil.claybon@dapperlabs.com>
|
bors merge |
bors bot
added a commit
that referenced
this pull request
Aug 17, 2023
4485: [ALSP] Adjust Node Penalty Decay Speed on Repeated Disallow-listing r=gomisha a=gomisha ref: https://github.com/dapperlabs/flow-go/issues/6766 Creates a `DecayList` of values that are used to decay the Penalty value of the misbehaving node on subsequent disallow listings 10 times slower `[1000, 100, 10, 1]`. This allows us to increase the time a repeatedly misbehaving node is not able to communicate with the victim node. Main test: `TestHandleReportedMisbehavior_And_DisallowListing_RepeatOffender_Integration` - test checks that each time a decay value is updated, the ALSP Manager uses the new decay value (e.g. 1000, 100, 10, 1) - test decays penalty to zero in one heart beat by using custom `WithDecayFunc(fastDecayFunc)` option to avoid having to wait for full decay - checks that the decay value stays at 1 after subsequent misbehaviors after the 4th misbehavior (i.e. after decay decreases from 1000 to 100 to 10 to 1, any subsequent misbehaviors will keep using a decay of 1) - relies on #4524 to allow disabling libp2p backoff mechanism when connecting / disconnecting to a node many times in a row Co-authored-by: Misha <misha.rybalov@dapperlabs.com>
|
Build failed: |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ref: https://github.com/dapperlabs/flow-go/issues/6766
Creates a
DecayListof values that are used to decay the Penalty value of the misbehaving node on subsequent disallow listings 10 times slower[1000, 100, 10, 1]. This allows us to increase the time a repeatedly misbehaving node is not able to communicate with the victim node.Main test:
TestHandleReportedMisbehavior_And_DisallowListing_RepeatOffender_IntegrationWithDecayFunc(fastDecayFunc)option to avoid having to wait for full decay