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

Feat fix peers rating handler #4800

Merged
merged 50 commits into from Mar 28, 2023
Merged

Conversation

sstanculeanu
Copy link
Contributor

@sstanculeanu sstanculeanu commented Dec 13, 2022

Reasoning behind the pull request

  • missing call to DecreaseRating + found a better approach

Proposed changes

  • added multiple fixes to peers rating handler + integration tests

Testing procedure

  • integration tests + system test scenario:
    • start a testnet from test_peersRatingHandler branch which has some extra logs(printed each 10 seconds)
    • start a node connected to this testnet using test_peersRatingHandler_malicious branch(this one should not respond to requests after nonce 1000, ~epoch 5-6). let's call it malicious node.
    • get the pid of this malicious node
    • check the logs of the nodes from testnet for testing- Ratings cachers. Following this line should be displayed full list of peers and their ratings, we should see ratings trending to 100 before nonce 1000(including for the malicious node)
    • check the new route for ratings: /node/connected-peers-ratings which should provide same results with the log after testing- Connected peers ratings:
    • after nonce 1000, we should start seeing the rating of malicious node decreasing, perhaps going to the bad rated list.

Note: during testing, you will see log errors testing- forced to not respond to requests anymore. They are the forced errors on malicious node. This confirms that the node does what is should do.

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

  • was the PR targeted to the correct branch?
  • if this is a larger feature that probably needs more than one PR, is there a feat branch created?
  • if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

sstanculeanu and others added 12 commits December 2, 2022 18:53
…decrease_call

Add missing call to DecreaseRating on peersRatingHandler
… on peersRatingHandler and added integration test for peersRatingHandler
…nto fix_peers_rating_handler

# Conflicts:
#	dataRetriever/resolvers/topicResolverSender/topicResolverSender_test.go
Fix missing call to decrease rating into peers rating handler
…nto peers_rating_handler_integration_test

# Conflicts:
#	go.mod
#	go.sum
#	integrationTests/testProcessorNode.go
#	node/metrics/metrics.go
@sstanculeanu sstanculeanu added type:bug Something isn't working low prio not critical - to be solved with low prio area:processing impact:low interested:protocol complexity:medium labels Dec 13, 2022
@sstanculeanu sstanculeanu self-assigned this Dec 13, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2022

Codecov Report

❗ No coverage uploaded for pull request base (rc/v1.6.0@e272723). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head a44b682 differs from pull request most recent head 79146d3. Consider uploading reports for the commit 79146d3 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@             Coverage Diff              @@
##             rc/v1.6.0    #4800   +/-   ##
============================================
  Coverage             ?   70.87%           
============================================
  Files                ?      675           
  Lines                ?    87654           
  Branches             ?        0           
============================================
  Hits                 ?    62129           
  Misses               ?    20854           
  Partials             ?     4671           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

sstanculeanu and others added 10 commits December 13, 2022 16:27
…gration_test

Peers rating handler integration test
…at_fix_peers_rating_2022.12.29

# Conflicts:
#	go.mod
#	go.sum
…ers_rating_2022.12.29

Merge rc into feat fix peers rating 2022.12.29
…at_fix_peers_rating_handler_2022.01.25

# Conflicts:
#	go.mod
#	go.sum
…_rating_handler_2022.01.25

Merge rc into feat fix peers rating handler 2022.01.25
…_feat_fix_peers_rating_handler_2023.02.14

# Conflicts:
#	go.mod
#	go.sum
sstanculeanu and others added 24 commits March 3, 2023 09:39
…ers_rating_handler_2023.03.06

Merge rc160 into feat fix peers rating handler 2023.03.06
…o_feat_fix_peers_rating_handler_2022.03.06_2

# Conflicts:
#	go.mod
#	go.sum
…eers_rating_handler_2022.03.06_2

Merge rc 160 into feat fix peers rating handler 2022.03.06 2
Update mx-chain-p2p-go with latest peers rating handler fixes
…into merge_rc160_into_feat_fix_peers_rating_handler_27.03.2023

# Conflicts:
#	consensus/spos/bls/subroundEndRound_test.go
#	errors/errors.go
#	go.mod
#	go.sum
…ers_rating_handler_27.03.2023

Merge rc160 into feat fix peers rating handler 27.03.2023
@sstanculeanu sstanculeanu marked this pull request as ready for review March 27, 2023 14:32
Copy link
Collaborator

@gabi-vuls gabi-vuls left a comment

Choose a reason for hiding this comment

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

System test passed
@@ Log scanner @@

fix-peers-rating-handler

================================================================================

  • Known Warnings 0
  • New Warnings 0
  • Known Errors 0
  • New Errors 1
  • Panics 0
    ================================================================================
  • block hash does not match 11279
  • wrong nonce in block 4035
  • miniblocks does not match 0
  • num miniblocks does not match 0
  • miniblock hash does not match 0
  • block bodies does not match 0
  • receipts hash missmatch 0
    ================================================================================
  • No jailed nodes on the thestnet
    ================================================================================

@sstanculeanu sstanculeanu merged commit 1375708 into rc/v1.6.0 Mar 28, 2023
4 checks passed
@sstanculeanu sstanculeanu deleted the feat/fix-peers-rating-handler branch March 28, 2023 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:processing complexity:medium impact:low interested:protocol low prio not critical - to be solved with low prio type:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants