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

Change maxPrunePeers and maxPeersPerPruneMessage usage #336

Merged
merged 7 commits into from
Oct 9, 2023

Conversation

diegomrsantos
Copy link
Contributor

@diegomrsantos diegomrsantos commented Oct 5, 2023

Seems maxPeersPerPruneMessage should be used to limit the amount of peers allowed in a Prune message received and maxPrunePeers should be used to limit the amount of peers sent.

@diegomrsantos diegomrsantos changed the title Use maxPrunePeers to limit the amount of peers in PX Use maxPrunePeers to limit the amount of peers sent in PX Oct 5, 2023
@diegomrsantos diegomrsantos changed the title Use maxPrunePeers to limit the amount of peers sent in PX Change maxPrunePeers and maxPeersPerPruneMessage usage Oct 5, 2023
@diegomrsantos
Copy link
Contributor Author

@Nashatyrev please review if it makes sense.

@Nashatyrev
Copy link
Collaborator

Nashatyrev commented Oct 6, 2023

Hm, it's actually weird that:

  • we are having 2 params to control that
  • their ambiguous names
  • they used incorrectly (this is what you are fixing)

I would ideally left just one param to both

  • validate the number in inbound list
  • limit the number of outbound

But the potential problem is that nodes based on earlier version and sending the unlimited list would be banned by nodes based on latter jvm-Libp2p versions. In other words I would try to avoid the particular situation when newer Teku nodes would ban older Teku nodes.

Because of that I would propose the following:
1 phase (for this PR):

  • leave 2 parameters however maybe rename them. Maybe like: maxPrunePeersOutbound/maxPrunePeersInbound or whatever
  • leave maxPrunePeersInbound == null (i.e. unbounded) for now for the reason I've mentioned above

2 phase (for the future PR when the first fix is adopted by the majority):

  • leave just a single param maxPrunePeers

@diegomrsantos what do you think on this?

@diegomrsantos
Copy link
Contributor Author

I think it's a good plan.

leave 2 parameters however maybe rename them. Maybe like: maxPrunePeersOutbound/maxPrunePeersInbound or whatever

How about those names maxPeersSentInPruneMsg and maxPeersAcceptedInPruneMsg?

leave maxPrunePeersInbound == null (i.e. unbounded) for now for the reason I've mentioned above

We could also set a default value for maxPeersAcceptedInPruneMsg here and set it to max Integer on Teku in phase 1.

What do you think?

@Nashatyrev
Copy link
Collaborator

How about those names maxPeersSentInPruneMsg and maxPeersAcceptedInPruneMsg?

Sounds good to me 👍

We could also set a default value for maxPeersAcceptedInPruneMsg here and set it to max Integer on Teku in phase 1.

What do you think?

Yeah, great idea!

@diegomrsantos
Copy link
Contributor Author

Additionally, how would you test it?

@Nashatyrev
Copy link
Collaborator

Additionally, how would you test it?

You may want to check GossipV1_1Tests. Would be great if you add a unit test 👍

@diegomrsantos
Copy link
Contributor Author

I added a test for maxPeersSentInPruneMsg, please review it. But not sure how to do it for maxPeersAcceptedInPruneMsg.

@Nashatyrev
Copy link
Collaborator

I added a test for maxPeersSentInPruneMsg, please review it. But not sure how to do it for maxPeersAcceptedInPruneMsg.

Thanks for the test 👍
Here is the one for the other param: diegomrsantos#1

Copy link
Collaborator

@Nashatyrev Nashatyrev left a comment

Choose a reason for hiding this comment

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

LGTM

Just check and maybe merge another test I've suggested

@@ -349,7 +349,7 @@ open class GossipRouter(
}

private fun processPrunePeers(peersList: List<Rpc.PeerInfo>) {
peersList.shuffled(random).take(params.maxPrunePeers)
peersList.shuffled(random).take(params.maxPeersAcceptedInPruneMsg)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: that take() call seems to have no effect (as a message exceeding the limit should be dropped before), but I would probably leave it just to be safe in case of future modifications

@Nashatyrev Nashatyrev merged commit 26efe02 into libp2p:develop Oct 9, 2023
2 checks passed
@diegomrsantos
Copy link
Contributor Author

@Nashatyrev Thanks for merging it. I would like to create a PR to set the new parameters on Teku. How should I proceed about updating the jvm-libp2p version there?

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.

2 participants