lsalzman / enet

ENet reliable UDP networking library
MIT License
2.75k stars 670 forks source link

Increasing ENET_PROTOCOL_MAXIMUM_PEER_ID beyond 4096 #41

Open Sempran opened 9 years ago

Sempran commented 9 years ago

The maximum value ENet can take for ENET_PROTOCOL_MAXIMUM_PEER_ID appears to be the default of 0xFFF, enforcing a maximum concurrent value of 4096 users, despite this field being a UINT 16 with a theoretical maximum of 65536. This is probably related to the fact the field is reused to store some header flags, ENET_PROTOCOL_HEADER_FLAG_SENT_TIME on bit 16 and ENET_PROTOCOL_HEADER_FLAG_COMPRESSED on bit 15. But this still leaves us with 14 bits, although for some reason it doesn't work with 2^14 bytes either, only with 2^12, so I'm thinking it also has anything to do with ENET_PROTOCOL_HEADER_SESSION_SHIFT which is set to 12.

A little guidance would be appreciated about how to go about increasing the limit, I could tinker myself and figure it out but I'm scared of introducing any potential security exploits.

Once it's done, is there any possibility a patch that uses a 32-bit peer id could be incorporated? There could be a define MASS_PEERS_MODE that allows you to have more peers.

Nowadays a single server can easily take tens of thousands of clients, so I think this limitation is a big problem.

pisto commented 9 years ago

All the bits are used, you can't just change the enum value. Increasing it breaks protocol compatibility with vanilla enet.

Sempran commented 9 years ago

Yeah, I know, what I want to do is change the field from 16 bits to 32 bits. The impact is minimal nowadays and the limitation of 4096 doesn't make much sense in a modern server which can take 10,000 users easily.

pavlog commented 9 years ago

Let me add my situation (online cards game). We have the same situation at project start. But after brain storm we decided use multiple server instances (on one hardware box) vs single server with big amount of players (each server instance limited to 800 players simultaneously, yes can be increased to 4096).

Our Pros:

Our Cons:

pisto commented 9 years ago

why do you need enet in the first place for a card game?

pavlog commented 9 years ago

Online card game, up to 6 players per table, dedicated server instance responsible for the: 1) Matchmaking for Connected players (sends to clients list of available rooms) 2) Game sessions (rooms).

rocaltair commented 9 years ago

@Sempran Have you got any good idea? I got the same issue.

pisto commented 9 years ago

@rocaltair besides making room for bits, you need to take care of this nasty pointer arithmetic, https://github.com/lsalzman/enet/blob/master/protocol.c#L994 . I don't know if that's all it takes though, never tried.

rocaltair commented 9 years ago

@pisto you're right,thanks.

Sempran commented 9 years ago

I talked to pisto in IRC, he gave me some pointers about some looping that occurs when processing outgoing packages that would make large user numbers non-scalable and pointed me to his fork of enet where he solves this.

https://github.com/pisto/spaghettimod/commits/master/enet

(This doesn't increase the field to 32 bits though, the limitation remains.)

It seems a few of us in here are also affected by this limitation, I wouldn't mind working on it, lifting the limitation and then backporting pisto's fixes, if lsalzman was willing to add such a patch into official enet. Having a private fork is not ideal since updating enet versions would be a bit cumbersome and it would mean there are less eyes looking at the code to look for bugs.

By switching to a 32 bit field, we'd get 28 bits instead of 12 bits, if I'm not mistaken, which is plenty for anybody.

If there is no interest adding this to the official enet I will probably go with having many instances of enet in different ports, just like pavlog does, I don't want to complicate my project maintenance by having private forks of open source libraries if I can avoid it.

@rocaltair, did you succeed at increasing the limit?

Sempran commented 9 years ago

I was able to make it work, now I can theoretically have up to 268435456 peers up from 4096, I was missing some bitwise arithmetic when the outgoing packet is created.

I could wrap all the changes in a #define MASS_PEER_MODE then people could turn this on when they don't mind the extra 2 bytes per package, although ideally I think enet should just switch, those 2 bytes are worth not having a hard ceiling, but that's a matter of each one's particular usage scenario.

The question now is whether lsalzman is interested in merging this into the enet official trunk. Otherwise maybe I can setup a MassEnet repository, but it would be a pain to be backporting enet updates, I think it would be better for everybody to have it in the main enet branch.

EDIT: By the way, even though I've increased the limit my solution still isn't scalable, there are instances where enet loops all the clients and this needs to be avoided in a mass-peer situation.

pisto commented 9 years ago

@Sempran In your situation I would actually consider reimplementing enet in some language more viable than C. It's very difficult to work with the code as it is now, because lsalzman coded it targeting stone age compilers (all variables declared at the beginning of the function and other funny things), which is fine for a low level library such as this. A new implementation could also embed concurrency from the beginning, which would make it scalable just like regular tcp/udp sockets.

Sempran commented 9 years ago

I'm really not interested in thread concurrency if that's what you mean, it's not the same than scalability. I think enet is fine the way it is, its simplicity of design is a good thing. Or were you talking about high-player concurrency? The fact that is in C is also not bad if your project is in C++, I actually like that. The only problem with C is that you open yourself to security exploits but that is true of anything you do that is low level.

Enet provides a reliable UDP stream and I'm not aware of a better solution if that's what you want. In any case I'm kind of committed to it (and I don't regret it.)

Realistically each instance is not gonna go over a thousand peers, and I don't think eNet would perform badly in such an scenario, do you? I'd love to hear your opinion seems you seem to have experience on the matter. Also, would you agree that the only major obstacle to scalability is the for() loop in enet_protocol_send_outgoing_commands()? That probably could be fixed with a few hash tables.

But for me, it's still better to have a soft limit (where performance starts to degrade a bit, and where you have the option to solve your problem with a hardware upgrade if it's cheaper than development) than a hard limit where you start dropping clients and you are forced to act immediately and create new subsystems to deal with user balancing.

pisto commented 9 years ago

It's been a while now, but if I remember correctly just increasing the number of available peers at the time of host creation causes enet to loop over all of them in various places (and most of the time, just realize that the peer is disconnected). The overhead was noticeable in the cpu usage (sorry, can't remember numbers, say 5-10% cpu?), and definitely evidenced by perf.

For concurrency I was thinking of processing the enet packets, just like the kernel normally processes multiple tcp streams on all the cores. It may be useful or not, it depends on the number of actually connected peers (I would guess somewhere over ten thousands), contrary to the almost trivially solvable "bug" that enet loops over disconnected peers too. If you look for scalability on this matter, you should definitely go multithreaded, with multiple UDP sockets bound to the same port and connected to the remote client ip:port, poll() them, use recvmmsg and sendmmsg (at least on linux). I have implemented per-peer sockets in my repo, all the rest is complicate to insert in the code without substantial changes.

Sempran commented 9 years ago

contrary to the almost trivially solvable "bug" that enet loops over disconnected peers too.

I assume those disconnected slots are filled with new clients, right? So that issue in particular is fine. On the other hand if new clients are still added to the end of the list then it's a bit of a problem.

Regarding concurrency, I was thinking of a per-process solution with different ports rather than a per-thread solution but your suggestion is also interesting, thanks.

pisto commented 9 years ago

@Sempran if I remember correctly there's no counter in vanilla enet that keeps track of how many peers are connected, so it loops on all the available peers. Also I implemented peerID randomization for security purposes, hence I need to keep a list of connected and unconnected peers.

By the way, I lately realized I need to implement more things with enet, and I'm quite fed up with the difficulty of hacking into vanilla new features, so I might be as well the one starting a C++ compatible library, in mid-term.

Sempran commented 9 years ago

@pisto thanks for the info. Could you elaborate what are the security issues you are solving with peerID randomization? Is it an impersonation attack from a malicious user from the same IP?

rocaltair commented 9 years ago

@Sempran So you succeed to change outgoingPeerID from enet_uint16 to enet_uint32 ? Could you share the code? Thank you ~

rpgplayerrobin commented 8 years ago

I am very interested in this. Did Sempran succeed in increasing the maximum peer limit? I can't find a branch or anything, is it possible for me to somehow get the code that Sempran made?

BeRo1985 commented 7 years ago

It's very simple to do it. It requires only few code changes for raising ENET_PROTOCOL_MAXIMUM_PEER_ID from 0xFFF to 0xFFFF (which should be then enough in my opinion), but it breaks the protocol compatibly to vanilla upstream ENet, due to small changes in the _ENetProtocolHeader structure.

You can find my code changes at https://github.com/BeRo1985/enet/commit/75344d13bf4b94b784c7a1b5c9e42487a6320e1a and https://github.com/BeRo1985/enet/commit/28ef9e213114a5376e4198d8d4d4cd938fb7e9a4 which is just more or less a single-feature backport from my Pascal port of enet at https://github.com/BeRo1985/pasenet/commit/e06b3a95b6fbac5ce5dec2abb33edf5a611593ed and https://github.com/BeRo1985/pasenet/commit/a9f5de376f57b16769689f5fa6bb873dd5cfaca5 and https://github.com/BeRo1985/pasenet/commit/a211451f97de762522d3224ae081e9eada701db9 .