Improve minimum file descriptor accounting and documentation

fanquake commented at 1:55 am on May 8, 2020: member

#16003 has been closed, and I’ve pulled out the unrelated p2p change. However I think it’s worth someone following up and taking a look at our startup file descriptor accounting.

As mentioned in #16003, if bitcoind is started somewhere that the maximum number of open file descriptors is limited, we can run into some nonsensical behaviour. Such as a “negative” number of maximum connections:

0# using bash
1ulimit -n 150
2src/bitcoind
3[init] Using at most -8 automatic connections (150 file descriptors available)

It’s also impossible to shutdown bitcoind from this state as the opencon thread will never exit.

It would be good for someone to look through this code, document assumptions, and fix issues like the above.

You do not need to request permission to start working on this. You are encouraged to comment on the issue if you are planning to work on it. This will help other contributors monitor which issues are actively being addressed and is also an effective way to request assistance if and when you need it.

For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

fanquake added the label good first issue on May 8, 2020

Suraj-Upadhyay commented at 9:55 am on May 8, 2020: none

Hii @fanquake I would like to be assigned to work on this issue as my first contribution to bitcoin-core.

Thanks.

Edit : Started working on this :)

tryphe commented at 6:30 pm on May 18, 2020: contributor

Thanks @MrSquanchee!

I’ve added most of my thoughts on this in the closed issue above. As of now, the FD counts are pretty much hardcoded in, and I think it would be nice to get away from that in a modular way if we want things like a configurable number of inbound/outbound/feeler connections, lots of RPC threads, FDs used by future changes that will have possibly missed modifying the initial FD count, etc.

There’s also the concept of “soft limits” of FDs, which is currently implemented in a way that reduces the possible maximum of 125 inbound connections when we allocate them to some other stuff, but I think this “soft limit” should be applied to the maximum number of FDs available to the process. This would be nice, as most Linux distros give you 1024 FDs, which would allow for much more varied configurations and eliminate the need to reduce the 125 inbound count.

There’s also an assumption that every system will use the same count, which I’m not sure will be true in the future, or is true now (I didn’t really have a large enough test bed of OSes when I opened the issue, which was kind of discouraging). Something that actually measures the number of FDs open by the process might also be helpful, instead of assuming we’ve measured correctly.

With that in mind, something that tells us the number of required, requested, and allocated FDs on startup would be informative. Similar to what I had in the closed issue, which is what causes the bug above (the allocated number of FDs returned gets reduced lower than the required number of FDs). So if we don’t get our requested amount, we can possibly revamp our counts in such a way that we still meet the required amount.

On another note: arbitrarily changing FD counts to critical things is not a solution in my mind, but in support of more robust configs, there’s at least some bigger nodes that need to vary these counts, and probably a sane minimum we can use for most stuff (current values could be a sane minimum and default maximum for some things, ie. feeler connections).

tryphe commented at 7:04 pm on May 18, 2020: contributor

Also, don’t hesitate to pick our brains in #bitcoin-core-dev on freenode or pm me on there! :)

michaelfolkson commented at 7:29 pm on May 18, 2020: contributor

There is no formal assignment process on Bitcoin Core @MrSquanchee so we can’t stop somebody else from working on it if they want to. But it is helpful to know you are working on it.

Suraj-Upadhyay commented at 7:47 pm on May 18, 2020: none

Hii Guys, Thanks for reminding me about this. I have been a bit busy. I would not mind if anyone else would want to work on this Issue. Might start working on bitcoin later :)

troygiorshev commented at 9:23 pm on August 7, 2020: contributor

If anyone is looking for somewhere to get started, I’ve cleaned up the file descriptor code in init.cpp a little here.

NelsonFrancisco commented at 4:00 pm on March 31, 2021: none

If anyone is looking for somewhere to get started, I’ve cleaned up the file descriptor code in init.cpp a little here.

I’ve also taken the liberty to meddle with this: here’s a branch

Made a branch with a “sanitizing” of this code before trying to achieve the issue here. I found the “nMaxConnections” variable value to be really hard to track, so I’ve thrown an error, if -maxconnections is less than 0, and used a temporary variable before finally defining nMaxConnections. So, I believe I changed no behavior besides the error throwing.

I believe also, that “AppInitParameterInteraction” should be divided into several functions. It simply does too much, IMO. Maybe in a separated commit, what do you think?

So, for the problem in itself: is it possible for the nMaxConnections variable to get a negative value? What’s the best approach to solve this? Change the following calculation nFD - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE ?

Tmarlow98 commented at 8:24 pm on May 4, 2021: none

Is anyone working on this? I am very new to open source and would like to start networking :)

luke-jr commented at 2:27 am on March 10, 2022: member

In addition to improving fd planning, I wonder if we should have all our fd allocations check if they exceed our known limitations, and if so, close the new fd, kill a random p2p socket, and try again?

Improve minimum file descriptor accounting and documentation #18911