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

1898 slashing violations consumer alsp integration test #4549

Merged
merged 11 commits into from
Jul 19, 2023

Conversation

kc1116
Copy link
Contributor

@kc1116 kc1116 commented Jul 10, 2023

This PR adds an E2E functional test for the ALSP slashing violations consumer integration. The test ensures that the slashing violation consumer is reporting misbehaviors for slashable offenses as expected.

  • It sets up a victim node, honest node and spammer node for each slashing violation
  • The victim node reports enough slashing violations for each slashing violation type such that the corresponding spammer node is disallow-listed.
  • Ensures all misbehaving nodes have been disallow-listed from the victim node.

@kc1116 kc1116 requested a review from gomisha July 10, 2023 17:42
@kc1116 kc1116 requested a review from yhassanzadeh13 as a code owner July 10, 2023 17:42
@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2023

Codecov Report

Merging #4549 (1c55cf1) into master (2528190) will decrease coverage by 1.81%.
The diff coverage is 47.45%.

@@            Coverage Diff             @@
##           master    #4549      +/-   ##
==========================================
- Coverage   56.25%   54.45%   -1.81%     
==========================================
  Files         653      914     +261     
  Lines       64699    85282   +20583     
==========================================
+ Hits        36396    46437   +10041     
- Misses      25362    35255    +9893     
- Partials     2941     3590     +649     
Flag Coverage Δ
unittests 54.45% <47.45%> (-1.81%) ⬇️

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

Impacted Files Coverage Δ
cmd/scaffold.go 14.83% <0.00%> (+0.06%) ⬆️
engine/access/rpc/backend/backend_transactions.go 49.01% <0.00%> (+2.86%) ⬆️
model/flow/identifierList.go 22.36% <0.00%> (+0.84%) ⬆️
model/verification/chunkDataPackRequest.go 43.75% <0.00%> (-6.25%) ⬇️
module/metrics/herocache.go 0.00% <0.00%> (ø)
module/metrics/network.go 0.00% <0.00%> (ø)
module/metrics/noop.go 0.00% <0.00%> (ø)
network/p2p/inspector/internal/cache/cache.go 64.70% <ø> (ø)
network/p2p/middleware/middleware.go 1.71% <0.00%> (ø)
network/p2p/p2pbuilder/libp2pNodeBuilder.go 0.00% <0.00%> (ø)
... and 33 more

... and 243 files with indirect coverage changes

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.

Overall, looks excellent 🚀 , I added a few cosmetic comments.

network/alsp/manager/manager_test.go Outdated Show resolved Hide resolved
network/alsp/manager/manager_test.go Outdated Show resolved Hide resolved
network/alsp/manager/manager_test.go Outdated Show resolved Hide resolved
network/alsp/manager/manager_test.go Outdated Show resolved Hide resolved
kc1116 and others added 5 commits July 12, 2023 18:37
Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>
Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>
Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>
Co-authored-by: Yahya Hassanzadeh, Ph.D. <yhassanzadeh@ieee.org>
@kc1116 kc1116 requested a review from peterargue July 17, 2023 16:46
}()
}
}
unittest.RequireReturnsBefore(t, violationsWg.Wait, 100*time.Millisecond, "slashing violations not reported in time")
Copy link
Contributor

Choose a reason for hiding this comment

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

do the calls to report each violation need to be done async? This could introduce flakiness due to timeouts, while reporting the callbacks serially most likely wouldn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done async so that we can ensure all async components under the hood are working as expected (queue, locks, workers etc). This will not cause flakiness as there are no network operations happening, it's just building up a queue of violations that will eventually be processed.

@kc1116 kc1116 merged commit 9682381 into master Jul 19, 2023
@kc1116 kc1116 deleted the 1898-slashing-violations-consumer-alsp-integration-test branch July 19, 2023 00:09
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