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

Add Better Peer Pruning #8501

Merged
merged 14 commits into from
Feb 24, 2021
Merged

Add Better Peer Pruning #8501

merged 14 commits into from
Feb 24, 2021

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Feb 23, 2021

What type of PR is this?

Feature Improvement

What does this PR do? Why is it needed?

  • Adds in better peer pruning to handle for dynamic inbound limits.
  • Handles more important peers better, instead pruning those are not as critical for a node to
    function better.
  • Adds unit test for additional peer filtering.

Which issues(s) does this PR fix?

N.A

Other notes for review

@nisdas nisdas added Ready For Review Networking P2P related items labels Feb 23, 2021
@nisdas nisdas requested a review from a team as a code owner February 23, 2021 10:44
@codecov
Copy link

codecov bot commented Feb 23, 2021

Codecov Report

Merging #8501 (470e7c6) into develop (4d28d5e) will increase coverage by 0.01%.
The diff coverage is 81.81%.

@@             Coverage Diff             @@
##           develop    #8501      +/-   ##
===========================================
+ Coverage    58.09%   58.11%   +0.01%     
===========================================
  Files          458      458              
  Lines        32823    32863      +40     
===========================================
+ Hits         19070    19099      +29     
- Misses       10859    10872      +13     
+ Partials      2894     2892       -2     

s.aggregatorLock.Lock()
defer s.aggregatorLock.Unlock()
s.subnetsLock.Lock()
defer s.subnetsLock.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be safer if we don't obtain several locks for the same operation, but have 3 inline function calls:

	go func() {
		s.attesterLock.Lock()
		defer s.attesterLock.Unlock()
		s.attester.Purge()
	}() 

Those calls can be blocking/sequential, of course:

// obtain lock1
// clear cache
// release lock1

// obtain lock2..

Whenever function obtains two locks I feel uneasy, worrying about whether there's some part of the code that holds the second lock, and in a race for the first lock of the function (thus, potentially, resulting in a deadlock).

Since this method is to be used for tests only, it is probably an overkill to worry about such things.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, even though its a test only function, should still fix it. Patching it now

farazdagi
farazdagi previously approved these changes Feb 23, 2021
Copy link
Contributor

@farazdagi farazdagi left a comment

Choose a reason for hiding this comment

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

Looks good, approved (if you want to follow the suggestion on multiple locks, will re-approve after the code is updated -- but it is not strictly necessary, so feel free to merge as is).

@prylabs-bulldozer prylabs-bulldozer bot merged commit 0625a69 into develop Feb 24, 2021
@delete-merged-branch delete-merged-branch bot deleted the addBetterPruning branch February 24, 2021 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Networking P2P related items
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants