search

libp2p / jvm-libp2p

a libp2p implementation for the JVM, written in Kotlin 🔥
https://libp2p.io
Apache License 2.0
260 stars 74 forks source link

Change maxPrunePeers and maxPeersPerPruneMessage usage #336

Closed diegomrsantos closed 11 months ago

diegomrsantos commented 11 months ago

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 commented 11 months ago

@Nashatyrev please review if it makes sense.

Nashatyrev commented 11 months ago

Hm, it's actually weird that:

I would ideally left just one param to both

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):

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

@diegomrsantos what do you think on this?

diegomrsantos commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

Additionally, how would you test it?

Nashatyrev commented 11 months ago

Additionally, how would you test it?

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

diegomrsantos commented 11 months ago

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

Nashatyrev commented 11 months ago

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: https://github.com/diegomrsantos/jvm-libp2p/pull/1

diegomrsantos commented 11 months ago

@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?