init: fixes file descriptor accounting

sr-gi commented at 7:47 pm on May 8, 2024: member

The current logic for file descriptor accounting is pretty convoluted and hard to follow. This is partially caused by the lack of documentation plus non-intuitive variable naming (which was more intuitive when fewer things were accounted for, but hasn’t aged well). This has led to this accounting being error-prone and hard to maintain (as shown in the first commit of this PR).

Redefine some of the constants, plus document what are we accounting for so this can be extended more easily

Fixes #18911

DrahtBot commented at 7:47 pm on May 8, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild, TheCharlatan, naumenkogs
Concept ACK	BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

in src/init.cpp:1007 in a44da681f5 outdated

985-    // Trim requested connection counts, to fit into system limitations
986-    // <int> in std::min<int>(...) to work around FreeBSD compilation issue described in #2695
987-    nMaxConnections = std::max(std::min<int>(nMaxConnections, fd_max - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE), 0);
988     if (nFD < MIN_CORE_FILEDESCRIPTORS)
989         return InitError(_("Not enough file descriptors available."));
990-    nMaxConnections = std::min(nFD - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE, nMaxConnections);

sr-gi commented at 7:57 pm on May 8, 2024:

Notice how the latter nMaxConnections trim is redundant once we limit nFD = std::min(FD_SETSIZE, nFD);. Moving things around, plus accounting for nBind (which is currently not being accounted for in master), we get:

0nMaxConnections = std::max(std::min<int>(nMaxConnections, nFD - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE), 0);
1nMaxConnections = std::min(nMaxConnections, nFD - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE,);

TheCharlatan commented at 7:57 pm on May 8, 2024: contributor

Concept ACK

sr-gi commented at 8:03 pm on May 8, 2024: member

I think it is worth mentioning that the current approach is defining the minimum amount of file descriptions required without accounting for a single connection*, this means that if we are at the bare minimum, we will run but won’t be able to connect to any nodes. This is consistent with the current logic in master, but I think it’s not the way it should be.

I’m happy to add another commit addressing this, but I’ve rathered start approaching this sticking to the same assumptions as master.

* This actually accounts for manual connections, which may never happen, but not to any of our outbound. If we happen to not create any manuals we’d have 8 slots (MAX_ADDNODE_CONNECTIONS) for outbounds

sr-gi commented at 8:07 pm on May 8, 2024: member

@vasild you may be interested in this. I decided to fix it when seeing you extending the current logic in #29415

BrandonOdiwuor commented at 3:57 pm on May 11, 2024: contributor

Concept ACK

in src/init.cpp:992 in 927f153845 outdated

 995 #endif
 996+    if (nFD < MIN_CORE_FDS)
 997+        return InitError(strprintf(_("Not enough file descriptors available. %d available, %d required."), nFD, MIN_CORE_FDS));
 998+
 999     // Trim requested connection counts, to fit into system limitations
1000     // <int> in std::min<int>(...) to work around FreeBSD compilation issue described in #2695

fanquake commented at 8:28 am on May 15, 2024:

I think now could also be the time to remove this FreeBSD workaround. It was needed because older versions of FreeBSD, used to ship with an old Clang (3.x). However we now require Clang 15+, and the effected version of FreeBSD 10.x, is long EOL.

sr-gi commented at 1:32 pm on May 15, 2024:

Covered in 55f16f5

laanwj added the label UTXO Db and Indexes on May 16, 2024

laanwj added the label P2P on May 16, 2024

vasild commented at 5:17 am on May 22, 2024: contributor

The current logic for file descriptor accounting is pretty convoluted and hard to follow.

Concept ACK, I stumbled on this recently in https://github.com/bitcoin/bitcoin/pull/29415

in src/init.cpp:997 in 55f16f56f4 outdated

978     nUserMaxConnections = args.GetIntArg("-maxconnections", DEFAULT_MAX_PEER_CONNECTIONS);
979     nMaxConnections = std::max(nUserMaxConnections, 0);
980-
981-    nFD = RaiseFileDescriptorLimit(nMaxConnections + MIN_CORE_FILEDESCRIPTORS + MAX_ADDNODE_CONNECTIONS + nBind + NUM_FDS_MESSAGE_CAPTURE);
982-
983-#ifdef USE_POLL

vasild commented at 10:42 am on May 24, 2024:

In the commit message of a44da681f5 init: fixes fd accounting regarding poll/select:

… file descriptions limits …

s/descriptions/descriptors/

in src/init.cpp:984 in 55f16f56f4 outdated

987+    // Reserve enough FDs to account for the bare minimum, plus any manual connections, plus the bound interfaces
988+    int nMinRequiredFds = MIN_CORE_FDS + MAX_ADDNODE_CONNECTIONS + nBind;
989+
990+    // Try raising the FD limit to what we need (nFD may be smaller than the requested amount if this fails)
991+    nFD = RaiseFileDescriptorLimit(nMaxConnections + nMinRequiredFds);
992+    // If we are using select instead of poll, our actuall limit may be even smaller

vasild commented at 10:51 am on May 24, 2024:

0    // If we are using select instead of poll, our actual limit may be even smaller

in src/init.cpp:992 in 55f16f56f4 outdated

1000-    // <int> in std::min<int>(...) to work around FreeBSD compilation issue described in #2695
1001-    nMaxConnections = std::max(std::min<int>(nMaxConnections, fd_max - nBind - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE), 0);
1002-    if (nFD < MIN_CORE_FILEDESCRIPTORS)
1003-        return InitError(_("Not enough file descriptors available."));
1004-    nMaxConnections = std::min(nFD - MIN_CORE_FILEDESCRIPTORS - MAX_ADDNODE_CONNECTIONS - NUM_FDS_MESSAGE_CAPTURE, nMaxConnections);
1005+    nMaxConnections = std::max(std::min(nMaxConnections, nFD - nMinRequiredFds), 0);

vasild commented at 12:07 pm on May 24, 2024:

An alternative to max(min()) is clamp():

0    nMaxConnections = std::clamp(nFD - nMinRequiredFds, 0, nMaxConnections);

(feel free to ignore if you don’t find it more readable)

sr-gi commented at 2:02 pm on May 31, 2024:

I’ve rolled this back. After some of the refactoring, this looked like:

0std::clamp(available_fds - min_required_fds, 0, nUserMaxConnections);

However, available_fds cannot be smaller than min_required_fds, and nUserMaxConnections cannot be negative. So calling std::min should be enough.

vasild commented at 4:36 pm on June 1, 2024:

Excellent! Some simplifications leading to further simplifications. :heart:

in src/init.cpp:976 in 55f16f56f4 outdated

972+    // Make sure enough file descriptors are available. We need to reserve enough FDs to account for the bare minimum,
973+    // plus all manual connections and all bound interfaces. Any remainder will be available for connection sockets
974+
975+    // Number of bound interfaces (we have at least one)
976     int nBind = std::max(nUserBind, size_t(1));
977+    // Maximum number of connetions with other nodes, this accounts for all types of outbound and inbounds expect for manual

vasild commented at 12:20 pm on May 24, 2024:

0    // Maximum number of connections with other nodes, this accounts for all types of outbounds and inbounds except for manual

in src/init.cpp:978 in 55f16f56f4 outdated

974+
975+    // Number of bound interfaces (we have at least one)
976     int nBind = std::max(nUserBind, size_t(1));
977+    // Maximum number of connetions with other nodes, this accounts for all types of outbound and inbounds expect for manual
978     nUserMaxConnections = args.GetIntArg("-maxconnections", DEFAULT_MAX_PEER_CONNECTIONS);
979     nMaxConnections = std::max(nUserMaxConnections, 0);

vasild commented at 12:38 pm on May 24, 2024:

What about giving an InitError if the provided value is negative instead of silently clamping it to 0? Surely -maxconnections=-50 looks like a typo and probably the intention is not to end up with -maxconnections=0.

sr-gi commented at 3:10 pm on May 24, 2024:

I think that makes more sense

in src/init.cpp:980 in 55f16f56f4 outdated

983-#ifdef USE_POLL
984-    int fd_max = nFD;
985-#else
986-    int fd_max = FD_SETSIZE;
987+    // Reserve enough FDs to account for the bare minimum, plus any manual connections, plus the bound interfaces
988+    int nMinRequiredFds = MIN_CORE_FDS + MAX_ADDNODE_CONNECTIONS + nBind;

vasild commented at 12:45 pm on May 24, 2024:

according to doc/developer-notes.md variables should use snake_case and no need to denote the type in the variable name (n for integer): min_required_fds.

But more importantly, the name nMinRequiredFds is misleading because we accept a smaller number - a bit below we check if (nFD < MIN_CORE_FDS) return error..., so it is not “minimum required”. I think this variable here should be: min_required_fds = MIN_CORE_FDS + nBind, that check should be if (nFD < min_required_fds) return error and then:

0-    nFD = RaiseFileDescriptorLimit(nMaxConnections + nMinRequiredFds);
1+    nFD = RaiseFileDescriptorLimit(min_required_fds + MAX_ADDNODE_CONNECTIONS + nMaxConnections);

and

0-    nMaxConnections = std::max(std::min<int>(nMaxConnections, nFD - nMinRequiredFds), 0);
1+    nMaxConnections = std::max(std::min<int>(nMaxConnections, available_fds - min_required_fds - MAX_ADDNODE_CONNECTIONS), 0);

sr-gi commented at 3:09 pm on May 24, 2024:

I agree the if check should be updated, but I don’t necessarily agree with splitting MAX_ADDNODE_CONNECTIONS from min_required_fds.

There is the outstanding question of whether the minimum required fds to run Core should account for some of the connections. Currently, we are not accounting for any (given the check is performed against MIN_CORE_FDS), but I think we should be accounting for at least, outbound + manuals

vasild commented at 2:08 pm on May 28, 2024:

Ok, no strong opinion whether to include or exclude MAX_ADDNODE_CONNECTIONS in min_required_fds.

I guess better to update the check. It is funny to have code like

0min_required = ...;
1if (available < 10) error

it should really be

0min_required = ...;
1if (available < min_required) error

sr-gi commented at 3:27 pm on May 28, 2024:

I’ve fixed that, plus increased the min_required_fds to also account for automatic outbounds in the last commit. I’m open to discuss about the latter, but I think it’s something we should also account for

vasild commented at 9:11 am on May 29, 2024:

The newly added int auto_outbounds = std::min(MAX_OUTBOUND_FULL_RELAY_CONNECTIONS + MAX_BLOCK_RELAY_ONLY_CONNECTIONS + MAX_FEELER_CONNECTIONS, nUserMaxConnections); seems to be duplicating this logic, but not exactly:

https://github.com/bitcoin/bitcoin/blob/be100cf4c77a3e45750773689e0a396fda39d8a7/src/net.h#L1069-L1073

and I think this is getting complicated again. I would suggest to keep it simple - drop the last commit f569b06a53851f844219cd80cd33676470e3b776 init: fix min required fds check and account for outbounds

and only apply this change to 3982543007 init: improves file descriptors accounting and docs:

0-    if (available_fds < MIN_CORE_FDS)
1+    if (available_fds < min_required_fds)

sr-gi commented at 5:33 pm on May 29, 2024:

It is duplicating part of that, given CConnman::m_max_automatic_outbound is private.

I agree it is adding more complexity, but not accounting for it in the minimum means that could be in a situation where no single outbound connection is accounted for, so the user is basically only connected to manuals.

I acknowledge that’s fairly rare, but the two reasons why we are currently not facing this issue is, first, because the minimum is fairly low, so it would need a system with really limited resources to become an issue, and second, because If we were in that situation it would also need to be that case that the node is defining manual connections, otherwise the 8 reserved slots for manual would be available for automatic.

Despite that, this feels like bad design, so I’d argue accounting for automatic is something we should do (though not necessarily as done in the current approach)

vasild commented at 9:15 am on May 30, 2024:

If the node is connected to 8 manually defined outbound peers and 0 inbounds and 0 automatic outbounds, then it is still operational? No need to prohibit startup ~in this case~ if there is a chance that this might happen?

sr-gi commented at 1:55 pm on May 30, 2024:

Is it though? If the user has not specified that he wants no automatic connections (-maxconnections != 0) I’d argue he’d expect them to happen.

In any case, I think this is such an edge case that it is not worth blocking the PR on it. If you have a strong opinion on this, I’m happy to drop it and squash the rest of the commit to one of the previous ones

vasild commented at 5:01 am on May 31, 2024:

I am somewhat ~0 on erroring in the case of 8manual/0in/0out, but my bigger anxiety with the latest commit is that it makes it confusing again (or too complicated in my judgement). I tried to code a simple way to account for the automatic outbounds but gave up. Also, most naturally, as the “minimum required” name suggests, the check should be if (... < min_required). I find if (... < min_required + something more) confusing.

sr-gi commented at 1:33 pm on May 31, 2024:

I agree with the “making it more confusing”. Given this is an edge case, I’ll remove the outbound count and we can account for it if someone has a better way of doing so, or if someone opens an issue about it.

in src/init.cpp:983 in 55f16f56f4 outdated

986-    int fd_max = FD_SETSIZE;
987+    // Reserve enough FDs to account for the bare minimum, plus any manual connections, plus the bound interfaces
988+    int nMinRequiredFds = MIN_CORE_FDS + MAX_ADDNODE_CONNECTIONS + nBind;
989+
990+    // Try raising the FD limit to what we need (nFD may be smaller than the requested amount if this fails)
991+    nFD = RaiseFileDescriptorLimit(nMaxConnections + nMinRequiredFds);

vasild commented at 1:08 pm on May 24, 2024:

snake_case for variables. I think available or available_fds is a good name:

0    available_fds = RaiseFileDescriptorLimit(nMaxConnections + nMinRequiredFds);

sr-gi commented at 2:56 pm on May 24, 2024:

I kept nFD because it is not locally defined, but defined in the namespace, so at least one other method is using it. If we don’t mind a bigger diff I’m happy to change it

vasild commented at 2:11 pm on May 28, 2024:

ACK to rename nFD (maybe as part of 2f49fa0e761ee67d6eeb424b157689f479da8b2c init: improves file descriptors accounting and docs)

sr-gi commented at 3:25 pm on May 28, 2024:

Updated

vasild commented at 1:14 pm on May 24, 2024: contributor

Approach ACK 55f16f56f4

Might as well squash the last commit 55f16f56f4 init: Remove FreeBSD workaround to [#2695](/bitcoin-bitcoin/2695/) into some of the previous ones that touch that line.

sr-gi force-pushed on May 24, 2024

sr-gi commented at 3:29 pm on May 24, 2024: member

Thanks for the suggestions @vasild, I’ve partially covered them and commented on some of the pending ones

sr-gi force-pushed on May 24, 2024

DrahtBot added the label CI failed on May 24, 2024

DrahtBot commented at 3:36 pm on May 24, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/25385987025

sr-gi force-pushed on May 24, 2024

DrahtBot removed the label CI failed on May 24, 2024

vasild approved

vasild commented at 2:13 pm on May 28, 2024: contributor

ACK fa42bf7ee89999b33d4af0600b0d8da1275e5d5e

This is already an improvement over master. Would be nice to resolve #30065 (review) as well.

DrahtBot requested review from TheCharlatan on May 28, 2024

DrahtBot requested review from BrandonOdiwuor on May 28, 2024

sr-gi force-pushed on May 28, 2024

sr-gi force-pushed on May 29, 2024

sr-gi force-pushed on May 31, 2024

sr-gi commented at 1:59 pm on May 31, 2024: member

ACK fa42bf7

This is already an improvement over master. Would be nice to resolve #30065 (comment) as well.

Addressed all comments now

vasild approved

vasild commented at 4:34 pm on June 1, 2024: contributor

ACK a9e211a5302630635b825655cd984683794aa42c

Note: this is the last but one commit. The last commit 23f843d816 init: fix min required fds check and account for outbounds now contains a single dummy change of removing an empty line. Can/should be dropped altogether. You can push -f a9e211a5302630635b825655cd984683794aa42c into this branch.

Thanks!

sr-gi force-pushed on Jun 3, 2024

sr-gi commented at 2:16 pm on June 3, 2024: member

Nice catch @vasild, I’ve squashed that into one of the previous commits. It should be good now.

vasild approved

vasild commented at 10:27 am on June 20, 2024: contributor

ACK fe76929d4d68a4ca6d27c58c1e243a64fdc70b2a

in src/init.cpp:978 in fe76929d4d outdated

974@@ -975,12 +975,14 @@ bool AppInitParameterInteraction(const ArgsManager& args)
975     int nBind = std::max(nUserBind, size_t(1));
976     // Maximum number of connections with other nodes, this accounts for all types of outbounds and inbounds except for manual
977     nUserMaxConnections = args.GetIntArg("-maxconnections", DEFAULT_MAX_PEER_CONNECTIONS);
978-    nMaxConnections = std::max(nUserMaxConnections, 0);
979+    if (nUserMaxConnections < 0){

TheCharlatan commented at 2:21 pm on August 24, 2024:

Nit (clang-format): Space after closing parenthesis.

in src/init.cpp:150 in 0a7030d312 outdated

147 #else
148-#define MIN_CORE_FILEDESCRIPTORS 150
149+#define MIN_LEVELDB_FDS 150
150 #endif
151 
152+static const int MIN_CORE_FDS = MIN_LEVELDB_FDS + NUM_FDS_MESSAGE_CAPTURE;

TheCharlatan commented at 5:46 pm on August 25, 2024:

nit: static constexpr

TheCharlatan commented at 5:50 pm on August 25, 2024:

Why did you not put MAX_ADDNODE_CONNECTIONS in here?

sr-gi commented at 2:41 pm on September 5, 2024:

It was a matter of definition. The bare minimum does not really account for connections, even though running the node without any connection won’t be too useful.

TheCharlatan commented at 3:04 pm on September 5, 2024:

Oh, that makes sense!

in src/init.cpp:977 in fe76929d4d outdated

973+    // plus all manual connections and all bound interfaces. Any remainder will be available for connection sockets
974+
975+    // Number of bound interfaces (we have at least one)
976     int nBind = std::max(nUserBind, size_t(1));
977+    // Maximum number of connections with other nodes, this accounts for all types of outbounds and inbounds except for manual
978     nUserMaxConnections = args.GetIntArg("-maxconnections", DEFAULT_MAX_PEER_CONNECTIONS);

TheCharlatan commented at 6:26 pm on August 25, 2024:

I think this should be de-globalized and renamed like I’ve done here: https://github.com/TheCharlatan/bitcoin/commit/a2c048322c67436e02a3a9607400f65f10dd6564

The avaialble_fds global variable there are also kind of silly, since it is only there to be logged at a later point in time, but I don’t feel strongly if it should go or not either way.

sr-gi commented at 2:46 pm on September 5, 2024:

Yeah, I agree, I was going to de-globalize most of them but saw that they were used for logging later, so I decided not to overcomplicate the change by addressing things that are not directly related. I’ll do so with nUserMaxConnections given it’s not used for anything else

TheCharlatan commented at 6:31 pm on August 25, 2024: contributor

This looks good, but I think these few, simple things should be addressed.

DrahtBot requested review from TheCharlatan on Aug 25, 2024

DrahtBot added the label CI failed on Aug 31, 2024

DrahtBot removed the label CI failed on Sep 4, 2024

sr-gi force-pushed on Sep 5, 2024

sr-gi commented at 3:02 pm on September 5, 2024: member

Took (most of?) your suggestions @TheCharlatan

init: fixes fd accounting regarding poll/select

We are computing our file descriptors limits based on whether we use
poll or select. However, we are taking that into account only partially
(subtracting from fd_max in one case, but from nFD later on). Moreover,
nBind is also only accounted for partially.

Simplify and fix this logic

29008a7ff4

init: improves file descriptors accounting and docs

The current logic for file descriptor accounting is pretty convoluted and hard
to follow. This is partially caused by the lack of documentation plus non-intuitive
variable naming (which was more intuitive when fewer things were accounted for, but
hasn't aged well). This has led to this accounting being error-prone and hard to maintain
(as shown in he previous commit).

Redefine some of the constants, plus document what are we accounting for so this can be
extended more easily

Remove FreeBSD workaround to #2695

c773649481

init: error out if -maxconnections is negative d4c7c4009d

sr-gi force-pushed on Sep 5, 2024

sr-gi commented at 3:58 pm on September 5, 2024: member

Rebased to make CI green. This was based on a pre-CMake commit

vasild approved

vasild commented at 8:17 am on September 9, 2024: contributor

ACK d4c7c4009da14c84576d43ab4a1241967cfa5ffc

This code gives me a headache :exploding_head: (the old one, the new one - less)

TheCharlatan approved

TheCharlatan commented at 8:30 am on September 9, 2024: contributor

ACK d4c7c4009da14c84576d43ab4a1241967cfa5ffc

sr-gi commented at 2:49 pm on September 9, 2024: member

ACK d4c7c40

This code gives me a headache 🤯 (the old one, the new one - less)

This is literally the reason why I decided to fix this 🫠

https://www.notion.so/srgi/PR-29415-110fd8ede41446fba41e81859bbdf985?pvs=4#521d59ebadd2475bb5630fbad147cbe4

naumenkogs commented at 12:57 pm on September 10, 2024: member

ACK d4c7c4009da14c84576d43ab4a1241967cfa5ffc

The change makes the code much easier to understand, and I couldn’t find any bugs or flaws.

ryanofsky merged this on Sep 10, 2024

ryanofsky closed this on Sep 10, 2024

init: fixes file descriptor accounting #30065

Code Coverage

Reviews

Conflicts