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

Improve minimum file descriptor accounting and documentation #18911

Open
fanquake opened this issue May 8, 2020 · 9 comments · May be fixed by #30065
Open

Improve minimum file descriptor accounting and documentation #18911

fanquake opened this issue May 8, 2020 · 9 comments · May be fixed by #30065

Comments

@fanquake
Copy link
Member

fanquake commented May 8, 2020

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

# using bash
ulimit -n 150
src/bitcoind
[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.

@Suraj-Upadhyay
Copy link

Suraj-Upadhyay commented May 8, 2020

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
Copy link
Contributor

tryphe commented May 18, 2020

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
Copy link
Contributor

tryphe commented May 18, 2020

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

@michaelfolkson
Copy link
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
Copy link

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
Copy link
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
Copy link

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
Copy link

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

@luke-jr
Copy link
Member

luke-jr commented Mar 10, 2022

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants