Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Khalil/6743 Slashing Violations Consumer ALSP misbehavior report integration #4512

Merged
merged 20 commits into from
Jul 7, 2023

Conversation

kc1116
Copy link
Contributor

@kc1116 kc1116 commented Jun 23, 2023

This PR integrates the ALSP misbehavior reporter into the slashing violations consumer. The slashing violations consumer will now not only log a potentially slashable offense but also report the misbehavior using the misbehavior reporter. Currently, all slashing violations use the default score penalty and all slashable offenses have now been moved to the ALSP package and included with the list of misbehaviors. This PR also upgrades all authorization and topic validator tests that utilize the slashing violations consumer, now these tests incorporate a mock misbehavior report consumer and ensure a misbehavior report is disseminated or not as expected.

  • The SlashingViolationConsumer component has been effectively integrated with the ALSP.
  • Upon detection of an offense, the violating peer is reported to the ALSP.
  • Appropriate tests are in place to ensure that the violating peers are penalized and potentially evicted from the network.
  • Documentation is updated to reflect the changes and new functionality.

@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2023

Codecov Report

Merging #4512 (8a0bef7) into master (2528190) will decrease coverage by 4.31%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #4512      +/-   ##
==========================================
- Coverage   56.25%   51.94%   -4.31%     
==========================================
  Files         653      636      -17     
  Lines       64699    59330    -5369     
==========================================
- Hits        36396    30819    -5577     
- Misses      25362    26055     +693     
+ Partials     2941     2456     -485     
Flag Coverage Δ
unittests 51.94% <0.00%> (-4.31%) ⬇️

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

Impacted Files Coverage Δ
cmd/scaffold.go 14.78% <0.00%> (+0.02%) ⬆️
module/metrics/network.go 0.00% <0.00%> (ø)
module/metrics/noop.go 0.00% <0.00%> (ø)
utils/unittest/unittest.go 7.48% <0.00%> (-0.12%) ⬇️

... and 344 files with indirect coverage changes

@kc1116 kc1116 requested a review from gomisha June 23, 2023 16:52
cmd/node_builder.go Outdated Show resolved Hide resolved
cmd/access/node_builder/access_node_builder.go Outdated Show resolved Hide resolved
network/p2p/middleware/middleware.go Outdated Show resolved Hide resolved
network/p2p/middleware/middleware.go Outdated Show resolved Hide resolved
network/slashing/consumer.go Outdated Show resolved Hide resolved
network/slashing/consumer.go Outdated Show resolved Hide resolved
network/slashing/violations_consumer.go Outdated Show resolved Hide resolved
network/slashing/consumer.go Outdated Show resolved Hide resolved
network/validator/authorized_sender_validator.go Outdated Show resolved Hide resolved
@kc1116 kc1116 requested a review from yhassanzadeh13 June 27, 2023 22:19
module/metrics/network.go Outdated Show resolved Hide resolved
if err != nil {
c.log.Fatal().
Err(err).
Bool(logging.KeySuspicious, true).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't really suspicious since any error is from a misconfigured argument passed in.

looking at the code, this isn't actually possible here as the code is written, but it could be in the future. Can you add a comment about how this failure would occur?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@yhassanzadeh13 yhassanzadeh13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

cmd/scaffold.go Outdated
@@ -502,6 +499,7 @@ func (fnb *FlowNodeBuilder) InitFlowNetworkWithConduitFactory(
}

fnb.Network = net
fnb.MisbehaviorReportConsumer = net
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fnb.MisbehaviorReportConsumer = net

Please remove fnb.MisbehaviorReportConsumer as it is unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -224,6 +224,9 @@ type NodeConfig struct {
// UnicastRateLimiterDistributor notifies consumers when a peer's unicast message is rate limited.
UnicastRateLimiterDistributor p2p.UnicastRateLimiterDistributor

// MisbehaviorReportConsumer consumers used to disseminate misbehavior reports to the ALSP misbehavior report manager.
MisbehaviorReportConsumer network.MisbehaviorReportConsumer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
MisbehaviorReportConsumer network.MisbehaviorReportConsumer

Please eliminate this field; given the recent modifications, it is now obsolete. Moreover, it is imperative to avoid exposing internal attributes of networking components through the builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kc1116 kc1116 merged commit 70a431d into master Jul 7, 2023
@kc1116 kc1116 deleted the khalil/6743-slashing-violations-consumer-alsp branch July 7, 2023 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants