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

Added WithConnLimiter option for resource manager #2872

Closed
wants to merge 2 commits into from

Conversation

sstanculeanu
Copy link
Contributor

Considering the case where multiple hosts are running on the same ip, it would be very hard to decide what would be the proper limit in terms of using one. Although the default options, with ConnCount set to math.MaxInt would be fix this problem, the internal array of maps would still consume a lot of memory.

WithConnLimiter option would be a great option in order to allow clients their own custom implementation of the connLimiter. In our case, simply use an empty limiter would be the best option for the moment.

Also added a tiny change request on the readme, in order to completely align with our new naming.

@MarcoPolo
Copy link
Collaborator

Considering the case where multiple hosts are running on the same ip, it would be very hard to decide what would be the proper limit in terms of using one.

You probably want some sort of bound here. Otherwise you make sybil like attacks much much easier. The conn limiter isn't perfect but it does up the ante by requiring some set of different IP addresses. I don't mean to tell you how to design your system, but you likely do want some sort of protection around this in production. Feel free to email me directly and we can chat more.

with ConnCount set to math.MaxInt would be fix this problem, the internal array of maps would still consume a lot of memory.

This is wrong. You can use the existing options and not consume any extra memory. Here's an example that basically turns off the conn limiter: https://gist.github.com/MarcoPolo/1b0fd80c25695ddf6cf7e6601fef855c. But, again, you probably don't want that in a production environment.

The options aren't that complicated. You have two knobs (each for ipv4 and ipv6):

  1. The network prefix limit. This lets you specify limits for a custom network (like localhost or some private network prefix).
  2. The general subnet limit. This is the fallback if you haven't defined a limit for the subnet. It allows N conns per subnet.

By setting the general subnet limit to a prefix length of 0 (every ip address will fall into this subnet), and max count you then store only a single prefix and int.

Closing this PR as you don't need it. If you'd like to open another PR with just the naming changes, I'm happy to review and merge.

@MarcoPolo MarcoPolo closed this Jul 15, 2024
@iulianpascalau
Copy link

Hey @MarcoPolo, thanks for the explanations. How can we be sure that more than 8 hosts on the same machine will be able to connect to the network if some of our validators decide to run them? Secondly, are those limits documented somewhere?
I believe these changes are quite dangerous for our protocol and that is why I have encouraged @sstanculeanu to add this option function to completely disable this connection management at this low level, at least for now until we can instrument better testing (having 3200 hosts spread around the globe is not an easy task).
We have a connection management system at a higher level, we call it network sharding that allows ~75 nodes to connect to a host based on the kademlia "distance" between the peer IDs and some other information.
Without this PR we are kind of forced to use the old version of the resource manager.

@MarcoPolo
Copy link
Collaborator

are those limits documented somewhere?

They are documented in the Go Doc comments: WithLimitPerSubnet and WithNetworkPrefixLimit. They were called out on the release notes for v0.34 and v0.35 as well.

We have a connection management system at a higher level, we call it network sharding that allows ~75 nodes to connect to a host based on the kademlia "distance" between the peer IDs and some other information.

Are you protecting yourself at lower levels as well? The resource manager tries to protect memory as soon as possible. Without doing something at the beginning you open yourself up to certain attacks. Maybe you're using the connection gater?

Without this PR we are kind of forced to use the old version of the resource manager.

What logic do you require that you need a configurable conn limiter? If you're just disabling it to get the old behavior you can do that with the gist I shared.

How can we be sure that more than 8 hosts on the same machine will be able to connect to the network if some of our validators decide to run them?

You can use the WithLimitPerSubnet option and set the conn count to something higher. Although the default is already 8, you may still hit the limit if each host is doing multiple concurrent dials. Setting it to something between 32-64 is probably okay for your use case. As an example, maybe something like this would work?

WithLimitPerSubnet(
[]ConnLimitPerSubnet{{PrefixLength: 32, ConnCount: 32}}, // IPv4. Every ip4 address gets 32 concurrent connections.
[]ConnLimitPerSubnet{ // IPv6
   {
   	ConnCount:    32,
   	PrefixLength: 56,
   },
   {
   	ConnCount:    8 * 32,
   	PrefixLength: 48,
   },
})

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.

3 participants