init: reserve file descriptors for IPC connections #34978

enirox001 commented at 8:34 AM on April 1, 2026: contributor

When -ipcbind is used, the node opens one listening socket FD per bound address and accepts concurrent IPC connections. Neither was previously accounted for in min_required_fds, meaning IPC-heavy workloads could silently exhaust available file descriptors.

This PR adds -ipcmaxconnections (default: 16, mirroring -rpcworkqueue) so operators can control how many FDs are reserved for accepted IPC connections. It also adds ipc_bind to account for the listening socket FDs opened per -ipcbind address, which were previously unaccounted for. A warning is emitted when -ipcmaxconnections is set without -ipcbind.

This reserves file descriptors at startup. Upstream PR enforcing the connection limit https://github.com/bitcoin-core/libmultiprocess/pull/269

Note: there is also a draft alternative downstream approach in bitcoin/bitcoin#35037, using the local per-listener limit API proposed in bitcoin-core/libmultiprocess#269 to support per-address max-connections= options on -ipcbind.

So the two concrete directions under discussion are:

the global -ipcmaxconnections reservation approach in this PR
the per-address -ipcbind ... max-connections= approach in #35037

Suggested by Sjors in #32297 (comment).

DrahtBot commented at 8:35 AM on April 1, 2026: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34978.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	Sjors, kevkevinpal, w0xlt, sedited
Stale ACK	ryanofsky

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35730 (http: limit connected HTTPRemoteClients by pinheadmz)
#35037 (ipc: support per-address max-connections options on -ipcbind by enirox001)
#17783 (common: Disallow calling IsArgSet() on ALLOW_LIST options by ryanofsky)
#17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/init.cpp:724 in 90ad4dc5e6

 720 | @@ -720,6 +721,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
 721 |      argsman.AddArg("-server", "Accept command line and JSON-RPC commands", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
 722 |      if (can_listen_ipc) {
 723 |          argsman.AddArg("-ipcbind=<address>", "Bind to Unix socket address and listen for incoming connections. Valid address values are \"unix\" to listen on the default path, <datadir>/node.sock, or \"unix:/custom/path\" to specify a custom path. Can be specified multiple times to listen on multiple paths. Default behavior is not to listen on any path. If relative paths are specified, they are interpreted relative to the network data directory. If paths include any parent directory components and the parent directories do not exist, they will be created. Enabling this gives local processes that can access the socket unauthenticated RPC access, so it's important to choose a path with secure permissions if customizing this.", ArgsManager::ALLOW_ANY, OptionsCategory::IPC);
 724 | +        argsman.AddArg("-ipcmaxconnections=<n>", "Reserve file descriptors for up to <n> concurrent IPC socket connections (default: " + ToString(DEFAULT_IPC_MAX_CONNECTIONS) + ")", ArgsManager::ALLOW_ANY, OptionsCategory::IPC);

Sjors commented at 8:51 AM on April 1, 2026:

Maybe make call this -unixmaxconnections so it's clear that this covers both the Capnp IPC and the RPC-via-IPC use case (and maybe Tor over unix?).

enirox001 commented at 3:18 PM on April 1, 2026:

Done, renamed to -unixmaxconnections.

in src/init.cpp:1036 in 90ad4dc5e6

1030 | @@ -1029,16 +1031,34 @@ bool AppInitParameterInteraction(const ArgsManager& args)
1031 |  
1032 |      // Number of bound interfaces (we have at least one)
1033 |      int nBind = std::max(nUserBind, size_t(1));
1034 | +
1035 | +    // IPC listening socket FDs — one per -ipcbind address, no default listener
1036 | +    int nIpcBind = static_cast<int>(args.GetArgs("-ipcbind").size());

Sjors commented at 8:51 AM on April 1, 2026:

New code can use modern variable name style.

enirox001 commented at 3:19 PM on April 1, 2026:

Done, renamed to ipc_bind.

Sjors commented at 8:54 AM on April 1, 2026: member

Concept ACK

This does not enforce a hard connection limit at the accept loop

I think it should, because the error for running out file descriptors is presumably confusing, and it will happen at unpredictable moments depending on how many RPC, P2P and IPC connections are active.

It would also be good to cover the number of reserved file descriptors that's printed in the log. If you do that in a test commit before the actual change, that nicely demonstrates that this PR actually increases it.

DrahtBot added the label CI failed on Apr 1, 2026

enirox001 commented at 2:06 PM on April 1, 2026: contributor

I think it should, because the error for running out file descriptors is presumably confusing, and it will happen at unpredictable moments depending on how many RPC, P2P and IPC connections are active.

Regarding the hard connection limit at the accept loop, implementing that would require changes to mp::ListenConnections in libmultiprocess. Would you expect that to be done in this PR, or as a follow-up in libmultiprocess?

Sjors commented at 2:23 PM on April 1, 2026: member

that would require changes to mp::ListenConnections in libmultiprocess

Good question. libmultiprocess should probably give us a hook to (optionally) control the maximum number of connections. It might be good to draft a PR for that on bitcoin-core/libmultiprocess, or at least open an issue.

It doesn't have to a pre-requisite for this PR, it could indeed be a followup.

enirox001 force-pushed on Apr 1, 2026

in src/init.cpp:167 in b1abf1a5e1

 163 | @@ -164,6 +164,7 @@ static constexpr bool DEFAULT_STOPAFTERBLOCKIMPORT{false};
 164 |  #endif
 165 |  
 166 |  static constexpr int MIN_CORE_FDS = MIN_LEVELDB_FDS + NUM_FDS_MESSAGE_CAPTURE;
 167 | +static constexpr int DEFAULT_IPC_MAX_CONNECTIONS{16};

kevkevinpal commented at 3:04 PM on April 1, 2026:

I would update this to reflect the arg name DEFAULT_UNIX_MAX_CONNECTIONS

enirox001 commented at 3:19 PM on April 1, 2026:

Done, thanks

in src/init.cpp:1048 in b1abf1a5e1

1043 | +
1044 | +    // We only need to reserve these FDs if the user is actually binding an IPC socket
1045 | +    int user_ipc_max_connections = 0;
1046 | +
1047 | +    if (args.IsArgSet("-ipcbind")) {
1048 | +        user_ipc_max_connections = args.GetIntArg("-unixmaxconnections", DEFAULT_IPC_MAX_CONNECTIONS);

kevkevinpal commented at 3:05 PM on April 1, 2026:

same here to update this to reflect the arg name user_unix_max_connections

enirox001 commented at 3:20 PM on April 1, 2026:

Done, made this update

kevkevinpal commented at 3:07 PM on April 1, 2026: contributor

Concept ACK b1abf1a

I would also update the description to reflect the usage of -unixmaxconnections

enirox001 force-pushed on Apr 1, 2026

enirox001 commented at 3:27 PM on April 1, 2026: contributor

Thanks for the reviews @Sjors @kevkevinpal.

I have addressed the comments in the recent push, also updated the PR description to match the changes made.

w0xlt commented at 11:45 PM on April 1, 2026: contributor

Concept ACK

enirox001 force-pushed on Apr 2, 2026

enirox001 force-pushed on Apr 3, 2026

enirox001 commented at 12:20 PM on April 3, 2026: contributor

C.I failure is unrelated

in src/init.cpp:1050 in 37e195d99e outdated

1045 | +    int user_unix_max_connections = 0;
1046 | +
1047 | +    if (args.IsArgSet("-ipcbind")) {
1048 | +        user_unix_max_connections = args.GetIntArg("-unixmaxconnections", DEFAULT_UNIX_MAX_CONNECTIONS);
1049 | +        if (user_unix_max_connections < 0) {
1050 | +            return InitError(Untranslated("-unixmaxconnections must be greater than or equal to zero"));

kevkevinpal commented at 12:35 PM on April 3, 2026:

Can we test this in the functional test?

enirox001 commented at 12:57 PM on April 3, 2026:

Good suggeestion, added this in the recent push. Thanks

in src/init.cpp:1047 in 1c0a5e38de outdated

1042 |      }
1043 | +
1044 | +    // We only need to reserve these FDs if the user is actually binding an IPC socket
1045 | +    int user_unix_max_connections = 0;
1046 | +
1047 | +    if (args.IsArgSet("-ipcbind")) {

ryanofsky commented at 12:48 PM on April 3, 2026:

In commit "init: Reserve file descriptors for IPC connections" (fa57007dfde3a532f76709b8ff0f38e52f7412aa)

This line should be changed to if (ipc_bind > 0) so it will work correctly when -noipcbind is specified. (In general IsArgSet should be avoided because it works poorly with negation).

The other check on line 1055 will have similar problems. Would suggest something more like:

auto ipc_bind = args.GetArgs("-ipcbind").size();
auto unix_max_connections = args.GetIntArg("-unixmaxconnections", 0);

if (unix_max_connections < 0) {
    return InitError(Untranslated("-unixmaxconnections must be greater than or equal to zero"));
} else if (ipc_bind > 0) {
    if (!IsArgSet("-unixmaxconnections")) unix_max_connections = DEFAULT_UNIX_MAX_CONNECTIONS;
    LogInfo("Reserving %d file descriptors for IPC (%d listening sockets, %d accepted connections)",
                    ipc_bind + user_unix_max_connections , ipc_bind, user_unix_max_connections);
} else if (unix_max_connections > 0) {
    LogWarning("-unixmaxconnections is %s but -ipcbind is not enabled; option will have no effect.\n", unix_max_connections);
    unix_max_connections = 0;
}

enirox001 commented at 5:30 PM on April 3, 2026:

Good catch.

Updated to check ipc_bind > 0 instead of IsArgSet("-ipcbind") so it handles -noipcbind correctly, and restructured the logic as suggested.

enirox001 force-pushed on Apr 3, 2026

in src/init.cpp:724 in 1c0a5e38de

 720 | @@ -720,6 +721,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
 721 |      argsman.AddArg("-server", "Accept command line and JSON-RPC commands", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
 722 |      if (can_listen_ipc) {
 723 |          argsman.AddArg("-ipcbind=<address>", "Bind to Unix socket address and listen for incoming connections. Valid address values are \"unix\" to listen on the default path, <datadir>/node.sock, or \"unix:/custom/path\" to specify a custom path. Can be specified multiple times to listen on multiple paths. Default behavior is not to listen on any path. If relative paths are specified, they are interpreted relative to the network data directory. If paths include any parent directory components and the parent directories do not exist, they will be created. Enabling this gives local processes that can access the socket unauthenticated RPC access, so it's important to choose a path with secure permissions if customizing this.", ArgsManager::ALLOW_ANY, OptionsCategory::IPC);
 724 | +        argsman.AddArg("-unixmaxconnections=<n>", "Reserve file descriptors for up to <n> concurrent IPC socket connections (default: " + ToString(DEFAULT_UNIX_MAX_CONNECTIONS) + ")", ArgsManager::ALLOW_ANY, OptionsCategory::IPC);

ryanofsky commented at 1:17 PM on April 3, 2026:

In commit "init: Reserve file descriptors for IPC connections" (fa57007dfde3a532f76709b8ff0f38e52f7412aa)

I think it would be better to call this -ipcmaxconnections to more clearly connect this to other IPC parameters like -ipcbind and -ipcconnect.

enirox001 commented at 5:48 PM on April 3, 2026:

Renamed to -ipcmaxconnections to keep it consistent with -ipcbind and -ipcconnect.

ryanofsky approved

ryanofsky commented at 1:32 PM on April 3, 2026: contributor

Code review ac1e51db8639e194967454ee0a4da0e8dd7949a0. This looks pretty good, but it looks like there is a problem with negation (see below), and also I made a suggestion about renaming the new option.

Ideally, I think I would want to implement the max connection limit by extending the -ipcbind option instead of introducing a new option. I'd probably extend the -ipcbind syntax to support socat-style options like -ipcbind=unix:/path/to/socket:max-connections=8 when listening on a custom path or -ipcbind=unix::max-connections=10 when listening on the default path. This way different paths (or different transports if we add tcp support) could have different connection limits.

But I can see that a separate option is probably easiest to implement, and it might be useful to users to have an overall cap on the number of connections, so current approach makes sense and might be best for now.

DrahtBot removed the label CI failed on Apr 3, 2026

enirox001 force-pushed on Apr 3, 2026

enirox001 commented at 5:40 PM on April 3, 2026: contributor

Thanks for the reviews so far @ryanofsky @kevkevinpal @Sjors

Ideally, I think I would want to implement the max connection limit by extending the -ipcbind option instead of introducing a new option. I'd probably extend the -ipcbind syntax to support socat-style options like -ipcbind=unix:/path/to/socket:max-connections=8 when listening on a custom path or -ipcbind=unix::max-connections=10 when listening on the default path. This way different paths (or different transports if we add tcp support) could have different connection limits.

The socat-style syntax for per-path limits is an interesting idea. Happy to follow up with that approach in a separate PR if there's appetite for it

Changes made in this commit are:

Renamed to -ipcmaxconnections to keep it consistent with -ipcbind and -ipcconnect. @Sjors had suggested, unixmaxconnections to be more transport-specific, but -ipcmaxconnections seems clearer given the current codebase only has Unix socket IPC.
Fixed the IsArgSet negation bug by restructuring the logic to check ipc_bind > 0 instead, so -noipcbind is handled correctly:
Also extended the functional test to assert that passing -ipcmaxconnections=-1 raises the expected init error.

in test/functional/interface_ipc_cli.py:38 in dc0583bb8b

  30 | @@ -31,16 +31,27 @@ def test_cli(self, args, error=None):
  31 |          if error is None:
  32 |              assert_equal(result.stdout, '[\n  "foo"\n]\n')
  33 |          else:
  34 | -            assert result.stdout.startswith(error), f"Output didn't start with the expected error {error!r}:\n{result.stdout}"
  35 | +            assert_equal(result.stdout, error)
  36 |          assert_equal(result.stderr, None)
  37 |          assert_equal(result.returncode, 0 if error is None else 1)
  38 |  
  39 | +    def test_ipcmaxconnections(self):

ryanofsky commented at 3:14 PM on April 6, 2026:

In commit "test: assert file descriptors available is logged on -ipcbind startup" (dc0583bb8b78ba11c824dc5cb628ab7835f88aee)

I think this file is probably not the right place for these checks, because this file is trying to test bitcoin-cil. Would suggest moving this to interface_ipc.py or adding a new test like interface_ipc_init.py

enirox001 commented at 8:39 AM on April 8, 2026:

Good point, this didn’t really belong in interface_ipc_cli.py. I moved the startup logging coverage into a dedicated interface_ipc_init.py test so the tests are more focused on their intended use

in test/functional/interface_ipc_cli.py:43 in bf037b7476

  36 | @@ -37,10 +37,27 @@ def test_cli(self, args, error=None):
  37 |  
  38 |      def test_ipcmaxconnections(self):
  39 |          self.log.info("Test -ipcmaxconnections")
  40 | +        node = self.nodes[0]
  41 |  
  42 | -        # Verify FD availability is logged at startup with -ipcbind.
  43 | -        with self.nodes[0].assert_debug_log(["file descriptors available"]):
  44 | -            self.restart_node(0)

ryanofsky commented at 3:18 PM on April 6, 2026:

In commit "test: extend -ipcmaxconnections coverage to assert FD reservation logging" (bf037b74762220afab8d506c56f7d801b5e6da7e)

Is there a reason for deleting this test? Maybe it should just be extended to check for the new log string?

enirox001 commented at 8:41 AM on April 8, 2026:

Agreed, I kept the original "file descriptors available" check and extended it in interface_ipc_init.py so the same startup also asserts the new IPC FD reservation log line.

in test/functional/interface_ipc_cli.py:60 in bf037b7476

  58 | +        )
  59 | +        self.start_node(0)
  60 | +
  61 | +        # Verify FD reservation is explicitly logged with correct values
  62 | +        with node.assert_debug_log(["Reserving 9 file descriptors for IPC (1 listening sockets, 8 accepted connections)"]):
  63 | +            self.restart_node(0, extra_args=["-ipcmaxconnections=8"])

ryanofsky commented at 3:22 PM on April 6, 2026:

In commit "test: extend -ipcmaxconnections coverage to assert FD reservation logging" (bf037b74762220afab8d506c56f7d801b5e6da7e)

It would seem good to just make this the first test and only start the node with -ipcmaxconnections=8 one time, instead of twice.

enirox001 commented at 8:41 AM on April 8, 2026:

Changed that so the first startup uses -ipcmaxconnections=8, and that single restart now checks both the existing startup log and the new reservation log.

ryanofsky approved

ryanofsky commented at 4:29 PM on April 6, 2026: contributor

Code review ACK bf037b74762220afab8d506c56f7d801b5e6da7e. Strictly speaking I think this PR is an improvement over the status quo, because right now no file descriptors are reserved for IPC and this does reserve some.

But I have the same concerns as Sjors #34978#pullrequestreview-4042683109 about how usable the new command line option will be because it only reserves file descriptors for IPC, without limiting how many descriptors IPC uses.

I was also initially concerned that a global limit on number of incoming connections could be more awkward to implement than local limits per listening address, because implementing local limits would just seem to require adding a max_connections parameter to mp::ListenConnections and using a local counter to stop calling ConnectionReceiver::accept when the limit is reached, while a global limit would require adding more state to the EventLoop class. But probably overall complexity of both approaches is about the same.

I'd still prefer local limits, and accepting socat-style max-connections=<n> options in -ipcbind over introducing a new command line option and global limit, but that's not a strong preference.

It would also be nice to see a draft PR with a some complete implementation of connection limiting because it would be a natural followup and could help compare the different approaches.

DrahtBot requested review from Sjors on Apr 6, 2026

DrahtBot requested review from kevkevinpal on Apr 6, 2026

enirox001 force-pushed on Apr 8, 2026

test: assert file descriptors available is logged on -ipcbind startup

Before adding IPC FD reservation, verify that the existing "file
descriptors available" log line fires when the node starts with
-ipcbind=unix. This establishes a baseline; the following commits
rename this helper to test_ipcmaxconnections and extend it to assert
the new reservation log line.

f7e40ee984

enirox001 force-pushed on Apr 8, 2026

DrahtBot added the label CI failed on Apr 8, 2026

DrahtBot removed the label CI failed on Apr 8, 2026

enirox001 commented at 10:05 AM on April 8, 2026: contributor

Thanks everyone for the review so far.

I addressed the test-related comments by moving the startup logging coverage out of interface_ipc_cli.py into a dedicated interface_ipc_init.py test. The original "file descriptors available" check is still there and is now extended to also assert the new IPC FD reservation log line, with the -ipcmaxconnections=8 case using a single restart for both checks.

I also opened an upstream draft PR to prototype local per-listener connection limiting, which would be the natural follow-up for enforcing a real IPC connection cap instead of only reserving FDs.

ryanofsky approved

ryanofsky commented at 2:55 PM on April 8, 2026: contributor

Code review ACK bf037b74762220afab8d506c56f7d801b5e6da7e. Thanks for the test updates!

One caveat: after reviewing https://github.com/bitcoin-core/libmultiprocess/pull/269 I'm less sure that we will want to add a global -ipcmaxconnections option if we can add per-address limits instead (with -ipcbind max-connections options or something similar).

Either approach should work but should probably pick one before merging any of these PRs.

enirox001 commented at 1:51 PM on April 9, 2026: contributor

I opened a separate draft PR here #35037 to make the per-address approach concrete downstream using the local listener limit API from bitcoin-core/libmultiprocess#269.

That draft uses -ipcbind ... max-connections=<n> instead of a global -ipcmaxconnections option so the two approaches can be compared more directly.

ViniciusCestarii commented at 3:01 PM on April 9, 2026: contributor

There doesn’t seem to be a test covering the default behavior when -ipcmaxconnections is not set.

It might be useful to add a case asserting the expected default (16 connections) at test/functional/interface_ipc_init.py:

        with node.assert_debug_log([
            "file descriptors available",
            "Reserving 17 file descriptors for IPC (1 listening sockets, 16 accepted connections)",
        ]):
            self.restart_node(0)
        assert_equal(node.getblockcount(), 0)

in src/init.cpp:1042 in 365be355a5 outdated

1037 |      if (user_max_connection < 0) {
1038 |          return InitError(Untranslated("-maxconnections must be greater or equal than zero"));
1039 |      }
1040 | +
1041 | +    // IPC listening socket FDs — one per -ipcbind address, no default listener
1042 | +    auto ipc_bind = args.GetArgs("-ipcbind").size();

sedited commented at 10:28 AM on April 21, 2026:

The comment for nBind says "Number of bound interfaces". I think ipc_bind arguably counts towards that too. How about moving that line above the definition of max_private and adding ipc_bind to it?

<details> <summary>Click to expand diff</summary>

diff --git a/src/init.cpp b/src/init.cpp
index 962b6c9105..86f0efdf6d 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1032,3 +1031,0 @@ bool AppInitParameterInteraction(const ArgsManager& args)
-    // Number of bound interfaces (we have at least one)
-    int nBind = std::max(nUserBind, size_t(1));
-
@@ -1057,0 +1055,3 @@ bool AppInitParameterInteraction(const ArgsManager& args)
+    // Number of bound interfaces (we have at least one)
+    int binds = std::max(nUserBind + ipc_bind, size_t(1));
+
@@ -1062 +1062 @@ bool AppInitParameterInteraction(const ArgsManager& args)
-    int min_required_fds = MIN_CORE_FDS + MAX_ADDNODE_CONNECTIONS + nBind + ipc_bind + ipc_max_connections;
+    int min_required_fds = MIN_CORE_FDS + MAX_ADDNODE_CONNECTIONS + binds + ipc_max_connections;

</details>

enirox001 commented at 1:17 PM on May 4, 2026:

Moving the calculation down and grouping the bound interfaces makes sense, and I applied that in the latest push.

While doing that, I noticed the previous nBind = std::max(nUserBind, size_t(1)) logic had a small pre-existing inaccuracy: it reserves one P2P listener FD even when the node is started with -listen=0.

Also, using std::max(nUserBind + ipc_bind, size_t(1)) directly would undercount in the case where there are no -bind args and one -ipcbind, because the default P2P listener and the IPC listener would together open two listening sockets, but that expression would only reserve one.

So I split the accounting as such

int p2p_binds = args.GetBoolArg("-listen",
DEFAULT_LISTEN) ? std::max(nUserBind, size_t(1)) :
0;
int binds = p2p_binds + ipc_bind;

This keeps the grouping you suggested, fixes the old -listen=0 behavior, and accounts for IPC listener without under-reserving.

sedited commented at 8:44 PM on May 2, 2026: contributor

Concept ACK

The per-bind accounting seems fine too, but isn't that a bit overkill? Are there really scenarios where different binds or transports would command a completely different number of descriptors?

enirox001 renamed this:
~~init: Reserve file descriptors for IPC connections~~
init: reserve file descriptors for IPC connections
on May 4, 2026

enirox001 force-pushed on May 4, 2026

init: Reserve file descriptors for IPC connections

When -ipcbind is used, the node opens one listening socket FD per
bound address and accepts concurrent IPC connections. Neither was
previously accounted for in min_required_fds, meaning IPC-heavy
workloads could silently exhaust available file descriptors at
unpredictable moments.

Add -ipcmaxconnections (default: 16, mirroring -rpcworkqueue) so
operators can control how many FDs are reserved for accepted IPC
connections. Add ipc_bind to account for the listening socket FDs
opened per -ipcbind address, which were previously unaccounted for.
Emit a warning when -ipcmaxconnections is set without -ipcbind. Log
the total IPC FD reservation at startup so operators can verify it.

Suggested by Sjors in #32297 (comment).

2f731bdee7

test: extend -ipcmaxconnections coverage to assert FD reservation logging

Add -ipcmaxconnections startup checks and assert the new IPC FD
reservation log line fires with correct values. With -ipcmaxconnections=8,
the node logs "Reserving 9 file descriptors for IPC (1 listening sockets,
8 accepted connections)", demonstrating that this PR increases the reserved
FD count.

c29e28c704

enirox001 force-pushed on May 4, 2026

enirox001 commented at 1:17 PM on May 4, 2026: contributor

The per-bind accounting seems fine too, but isn't that a bit overkill? Are there really scenarios where different binds or transports would command a completely different number of descriptors?

I don't think the strongest argument for per-address limits is that current transports need wildly different FD budgets. It's more that accepted connections are attached to individual listeners, so per-listener limits are a more natural fit if we later enforce limits in the accept path.

For this PR though, since the goal is only startup FD reservation, the global accounting is simpler and probably sufficient.

DrahtBot added the label CI failed on May 4, 2026

DrahtBot commented at 1:18 PM on May 4, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task macOS-cross to arm64: https://github.com/bitcoin/bitcoin/actions/runs/25321017192/job/74229816753</sub> <sub>LLM reason (✨ experimental): CI failed due to a C++ build error: init.cpp references an undeclared identifier ipc_bind, causing clang++ to error out and stop compilation.</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

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

</details>

enirox001 commented at 2:07 PM on May 4, 2026: contributor

C.I failure is unrelated https://github.com/bitcoin/bitcoin/issues/35199

DrahtBot removed the label CI failed on May 5, 2026

in src/init.cpp:1056 in 2f731bdee7

1053 | +        LogWarning("-ipcmaxconnections is %d but -ipcbind is not enabled; option will have no effect.\n", ipc_max_connections);
1054 | +        ipc_max_connections = 0;
1055 | +    }
1056 | +
1057 | +    // Number of bound interfaces.
1058 | +    int p2p_binds = args.GetBoolArg("-listen", DEFAULT_LISTEN) ? std::max(nUserBind, size_t(1)) : 0;

ryanofsky commented at 3:26 PM on May 18, 2026:

In commit "init: Reserve file descriptors for IPC connections" (2f731bdee79e711fa78409593aeb64aeb8b1759c)

It seems like this is no longer adding std::max(nUserBind, size_t(1)) to number of required fd's if -listen=0 is used, which seems like a potentially good change, but not related to IPC, and it doesn't seem to be mentioned in the commit description.

If this change will be included I'd suggest making it in a separate commit, or maybe even a separate PR so reviewers more familiar with p2p can ensure it is correct.

ryanofsky approved

ryanofsky commented at 4:13 PM on May 18, 2026: contributor

Code review c29e28c7044f793cfea553dc64825604cc24d65a. Would suggest changing this PR from ready to draft state, because while the accounting logic and test here seem useful, I don't think it would be good to add a new -ipcmaxconnections option that's global and independent from actual IPC interfaces and endpoints.

The related PRs https://github.com/bitcoin-core/libmultiprocess/pull/269 and #35037 seem promising and this PR could build on them using them to control ipc_max_connections. (I'm planning to review them soon.) This PR could also hardcode a small value for ipc_max_connections now and allow it to be configurable later. Then it could be merged independently of the other PRs.

re: #34978#pullrequestreview-4146944718

The per-bind accounting seems fine too, but isn't that a bit overkill? Are there really scenarios where different binds or transports would command a completely different number of descriptors?

It's more straightforward to implement a per-address connnection limit than a global connection limit because each address requires a separate ListenConnections call and ListenConnection calls do not know about each other. A per-address limit also provides an easy-to-understand UX because it doesn't require any new command line options to be added or any command line interactions to be explained. It lets listening options be specified directly along with the listening address.

enirox001 marked this as a draft on May 18, 2026

enirox001 commented at 4:22 PM on May 18, 2026: contributor

Marking this PR to draft in favor of #35037

Sjors commented at 12:49 PM on June 25, 2026: member

I'm confused, that PR is also draft and says:

This is a draft follow-up to https://github.com/bitcoin/bitcoin/pull/34978

enirox001 commented at 12:59 PM on June 25, 2026: contributor

I'm confused, that PR is also draft and says:

This is a draft follow-up to #34978

I had mentioned this in #35037 when I opened the PR because this PR was Open for Review at the time.

I suppose I should move that PR to Ready for Review since this has been moved to draft.

DrahtBot added the label Needs rebase on Aug 24, 2026

DrahtBot commented at 9:22 AM on August 24, 2026: contributor

🐙 This pull request conflicts with the target branch and needs rebase.