Repository: bitcoin/bitcoin (language: cpp)

Issue:
Improve minimum file descriptor accounting and documentation
#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:

```bash
# 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](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) before opening your pull request.



Discussion / hints:
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 :) 
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).
Also, don't hesitate to pick our brains in #bitcoin-core-dev on freenode or pm me on there! :)
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.
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 :)
If anyone is looking for somewhere to get started, I've cleaned up the file descriptor code in `init.cpp` a little [here](https://github.com/troygiorshev/bitcoin/commit/d9e1c1e83afc29782e26c1ae020539dc15444f75).
> If anyone is looking for somewhere to get started, I've cleaned up the file descriptor code in `init.cpp` a little [here](https://github.com/troygiorshev/bitcoin/commit/d9e1c1e83afc29782e26c1ae020539dc15444f75).

I've also taken the liberty to meddle with this: here's a [branch](https://github.com/NelsonFrancisco/bitcoin/compare/master...NelsonFrancisco:n2021-31-03-fd)

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` ?
Is anyone working on this? I am very new to open source and would like to start networking :)

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?

Fix patch:
diff --git a/src/init.cpp b/src/init.cpp
index e60feecf108be..3c4b72d3e4ffe 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -147,11 +147,12 @@ static constexpr bool DEFAULT_STOPAFTERBLOCKIMPORT{false};
 // Win32 LevelDB doesn't use filedescriptors, and the ones used for
 // accessing block files don't count towards the fd_set size limit
 // anyway.
-#define MIN_CORE_FILEDESCRIPTORS 0
+#define MIN_LEVELDB_FDS 0
 #else
-#define MIN_CORE_FILEDESCRIPTORS 150
+#define MIN_LEVELDB_FDS 150
 #endif
 
+static constexpr int MIN_CORE_FDS = MIN_LEVELDB_FDS + NUM_FDS_MESSAGE_CAPTURE;
 static const char* DEFAULT_ASMAP_FILENAME="ip_asn.map";
 
 /**
@@ -834,8 +835,7 @@ void InitLogging(const ArgsManager& args)
 namespace { // Variables internal to initialization process only
 
 int nMaxConnections;
-int nUserMaxConnections;
-int nFD;
+int available_fds;
 ServiceFlags nLocalServices = ServiceFlags(NODE_NETWORK_LIMITED | NODE_WITNESS);
 int64_t peer_connect_timeout;
 std::set<BlockFilterType> g_enabled_filter_types;
@@ -987,27 +987,33 @@ bool AppInitParameterInteraction(const ArgsManager& args)
         return InitError(Untranslated("Cannot set -listen=0 together with -listenonion=1"));
     }
 
-    // Make sure enough file descriptors are available
-    int nBind = std::max(nUserBind, size_t(1));
-    nUserMaxConnections = args.GetIntArg("-maxconnections", DEFAULT_MAX_PEER_CONNECTIONS);
-    nMaxConnections = std::max(nUserMaxConnections, 0);
-
-    nFD = RaiseFileDescriptorLimit(nMaxConnections + MIN_CORE_FILEDESCRIPTORS + MAX_ADDNODE_CONNECTIONS + nBind + NUM_FDS_MESSAGE_CAPTURE);
+    // Make sure enough file descriptors are available. We need to reserve enough FDs to account for the bare minimum,
+    // plus all manual connections and all bound interfaces. Any remainder will be available for connection sockets
 
-#ifdef USE_POLL
-    int fd_max = nFD;
-#else
-    int fd_max = FD_SETSIZE;
+    // Number of bound interfaces (we have at least one)
+    int nBind = std::max(nUserBind, size_t(1));
+    // Maximum number of connections with other nodes, this accounts for all types of outbounds and inbounds except for manual
+    int user_max_connection = args.GetIntArg("-maxconnections", DEFAULT_MAX_PEER_CONNECTIONS);
+    if (user_max_connection < 0) {
+        return InitError(Untranslated("-maxconnections must be greater or equal than zero"));
+    }
+    // Reserve enough FDs to account for the bare minimum, plus any manual connections, plus the bound interfaces
+    int min_required_fds = MIN_CORE_FDS + MAX_ADDNODE_CONNECTIONS + nBind;
+
+    // Try raising the FD limit to what we need (available_fds may be smaller than the requested amount if this fails)
+    available_fds = RaiseFileDescriptorLimit(user_max_connection + min_required_fds);
+    // If we are using select instead of poll, our actual limit may be even smaller
+#ifndef USE_POLL
+    available_fds = std::min(FD_SETSIZE, available_fds);
 #endif
+    if (available_fds < min_required_fds)
+        return InitError(strprintf(_("Not enough file descriptors available. %d available, %d required."), available_fds, min_required_fds));
+
     // Trim requested connection counts, to fit into system limitations
-    // <int> in std::min<int>(...) to work around FreeBSD compilation issue described in #2695
-    nMaxConnections = std::max(std::min<int>(nMaxConnections, fd_max - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE), 0);
-    if (nFD < MIN_CORE_FILEDESCRIPTORS)
-        return InitError(_("Not enough file descriptors available."));
-    nMaxConnections = std::min(nFD - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE, nMaxConnections);
+    nMaxConnections = std::min(available_fds - min_required_fds, user_max_connection);
 
-    if (nMaxConnections < nUserMaxConnections)
-        InitWarning(strprintf(_("Reducing -maxconnections from %d to %d, because of system limitations."), nUserMaxConnections, nMaxConnections));
+    if (nMaxConnections < user_max_connection)
+        InitWarning(strprintf(_("Reducing -maxconnections from %d to %d, because of system limitations."), user_max_connection, nMaxConnections));
 
     // ********************************************************* Step 3: parameter-to-internal-flags
     if (auto result{init::SetLoggingCategories(args)}; !result) return InitError(util::ErrorString(result));
@@ -1155,7 +1161,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         return false;
     }
 
-    LogPrintf("Using at most %i automatic connections (%i file descriptors available)\n", nMaxConnections, nFD);
+    LogPrintf("Using at most %i automatic connections (%i file descriptors available)\n", nMaxConnections, available_fds);
 
     // Warn about relative -datadir path.
     if (args.IsArgSet("-datadir") && !args.GetPathArg("-datadir").is_absolute()) {


Test patch: