ipc: support per-address max-connections options on -ipcbind #35037

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

The branch extends -ipcbind to accept ,max-connections=<n> options, for example -ipcbind=unix:,max-connections=8 or -ipcbind=unix:/custom/path,max-connections=8, instead of introducing a separate global -ipcmaxconnections option.

This keeps the connection limit attached to each IPC listener instead of introducing a global -ipcmaxconnections option (https://github.com/bitcoin/bitcoin/pull/34978). A global option would not allow different limits for different -ipcbind addresses, and the inline option format can be extended in the future with additional options.

DrahtBot commented at 1:44 PM on April 9, 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/35037.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	kevkevinpal, ViniciusCestarii, Sjors, xyzconstant
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)
#35167 (Convert check-deps.sh to python by ajtowns)
#34978 (init: reserve file descriptors for IPC connections by enirox001)
#28690 (build: Introduce internal kernel library by sedited)

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.

enirox001 renamed this:
~~04 26 ipcbind max connections draft~~
ipc: support per-address max-connections options on -ipcbind
on Apr 9, 2026

enirox001 force-pushed on Apr 9, 2026

DrahtBot added the label CI failed on Apr 9, 2026

enirox001 force-pushed on Apr 9, 2026

ryanofsky commented at 2:40 PM on April 9, 2026: contributor

Concept ACK. Looks like suggestions from #35036 (comment) apply here too (sorry, saw that PR first in my inbox).

IMO this is a better alternative to adding a new -ipcmaxconnections option like #34978, and I'd prefer this approach but both approaches seem reasonable. Main reason I prefer this approach is IPC is a pretty generic feature, so it seems useful to be able to set different limits for different purposes (like GUIs vs mining clients).

enirox001 force-pushed on Apr 9, 2026

enirox001 commented at 3:50 PM on April 9, 2026: contributor

Thanks for the review @ryanofsky

I pushed updates based on your review here as well

the bind-address type is now shared
parsing moved behind the interfaces::Ipc boundary
the parser handles trailing socket options
listenAddress() now takes the bind-address struct directly

in test/functional/interface_ipc_init.py:29 in aadc51fb5f outdated

  24 | +        super().setup_nodes()
  25 | +        self.nodes[0].args = [arg for arg in self.nodes[0].args if not arg.startswith("-ipcbind=")]
  26 | +        self.ipc_tmpdir = tempfile.TemporaryDirectory(prefix="btc-ipc-init-")
  27 | +        self.ipcbind_path = Path(self.ipc_tmpdir.name) / "ipcinit.sock"
  28 | +
  29 | +    def test_ipcbind_max_connections(self):

ViniciusCestarii commented at 5:21 PM on April 9, 2026:

Same suggestion I made on #34978. It would be good to have a functional test case where max-connections is not specified, to verify the default value is actually applied correctly

enirox001 commented at 11:49 AM on April 20, 2026:

Thanks for the suggestion, applied in the latest push

kevkevinpal commented at 3:43 PM on April 13, 2026: contributor

Concept ACK

I agree with Ryanofsky. I think this approach seems more reasonable than #34978, even though both can work and both make sense this seems cleaner. I think it makes sense to go with this approach instead of adding the -ipcmaxconnections option.

fanquake commented at 1:02 PM on April 14, 2026: member

cc @Sjors

enirox001 force-pushed on Apr 20, 2026

enirox001 force-pushed on Apr 21, 2026

enirox001 commented at 9:07 AM on April 21, 2026: contributor

Had a peer review with @Eunovo a few days ago, and I pushed a few follow-up changes to make the branch a bit easier to review:

Updated ParseBindAddress() to return util::Result, and adjusted the callers/tests accordingly.
Extended parser coverage to include 0, large valid values, and negative/overflow cases.
Cleaned up the commit structure so the branch is split into a subtree update, per-address integration, and tests.
Fixed the IPC limit test by correcting the async lifetime issue that could trigger disconnect errors.

The remaining subtree lint failure is expected for this draft, since the branch carries the libmultiprocess subtree update ahead of the upstream merge.

enirox001 force-pushed on May 12, 2026

DrahtBot added the label Needs rebase on May 22, 2026

enirox001 marked this as ready for review on Jun 25, 2026

enirox001 force-pushed on Jun 26, 2026

enirox001 commented at 11:17 AM on June 26, 2026: contributor

Rebased against master to resolve some merge conflicts. This PR should now be ready for review

DrahtBot removed the label Needs rebase on Jun 26, 2026

in src/ipc/libmultiprocess/include/mp/proxy-io.h:870 in b194acc106

 871 | +    auto* ptr = state->listener.get();
 872 |      loop.m_task_set->add(ptr->accept().then(
 873 | -        [&loop, &init, listener = kj::mv(listener)](kj::Own<kj::AsyncIoStream>&& stream) mutable {
 874 | -            _Serve<InitInterface>(loop, kj::mv(stream), init);
 875 | -            _Listen<InitInterface>(loop, kj::mv(listener), init);
 876 | +        [&loop, &init, state](kj::Own<kj::AsyncIoStream>&& stream) mutable {

ViniciusCestarii commented at 2:28 PM on June 26, 2026:

In Squashed 'src/ipc/libmultiprocess/' changes from 3edbe8f67c..8511c68f8 b194acc106b50af2a97e5475845b46f31a5d91f3

nit: this mutable keyword is no longer necessary

        [&loop, &init, state](kj::Own<kj::AsyncIoStream>&& stream) {

enirox001 commented at 2:08 PM on June 29, 2026:

Thanks, updated this in the upstream libmultiprocess PR https://github.com/bitcoin-core/libmultiprocess/pull/269 and squashed into this PR.

in src/interfaces/ipc.h:109 in ddfc34b65a

 104 | +                if (value.empty()) {
 105 | +                    return util::Error{Untranslated("Missing value for max-connections option")};
 106 | +                }
 107 | +                int64_t parsed_limit{-1};
 108 | +                const auto [last, ec] = std::from_chars(value.data(), value.data() + value.size(), parsed_limit);
 109 | +                if (ec != std::errc{} || last != value.data() + value.size() || parsed_limit < 0) {

ViniciusCestarii commented at 5:32 PM on June 26, 2026:

In ipc: add per-address max-connections parsing and support for -ipcbind ddfc34b65ab8c38e33eac05794246dab614a7f01

Shouldn't the lower bound check be parsed_limit < 1? I can't see why someone would use -ipcbind with max-connections=0 to bind something that doesn't accept any connections.

I believe it would be worth to add an upper bound check too because large values flow into the int FD reservation and overflow it: -ipcbind=unix::max-connections=200000000000 aborts the node with Assertion 'min_fd >= 0' failed.

Also it'd be worth splitting the range check from the parse check so out-of-range values like -1 and 0 get a specific message ("max-connections must be at least 1") and too big values for an int get ("max-connections must be at most [upper bound]") instead of a generic "Invalid max-connections value".

enirox001 commented at 2:11 PM on June 29, 2026:

Yes, looking at this, it isn't proper that the lower bound is 0 and the upper bound is not checked.

Changed this to 1 for the lower bound and added an upper bound check.

Also, split the range check; this is not only good for the message specificity, but also makes it more readable

Thanks

in src/ipc/test/ipc_tests.cpp:65 in ddfc34b65a outdated

  60 | +    check_bind("unix:path.sock:max-connections=8,unknown=1", "", 0, "Unknown socket option 'unknown'");
  61 | +    check_bind("unix:max-connections=8", "", 0, "Missing unix socket path before socket options; use unix::<options> for the default path");
  62 | +    check_bind("unix::max-connections=-1", "", 0, "Invalid max-connections value '-1'");
  63 | +    check_bind("unix::max-connections=-9223372036854775808", "", 0, "Invalid max-connections value '-9223372036854775808'");
  64 | +    check_bind("unix::max-connections=9223372036854775808", "", 0, "Invalid max-connections value '9223372036854775808'");
  65 | +    check_bind("unix::max-connections=", "", 0, "Missing value for max-connections option");

ViniciusCestarii commented at 7:28 PM on June 26, 2026:

In ipc: add per-address max-connections parsing and support for -ipcbind ddfc34b65ab8c38e33eac05794246dab614a7f01

There could be a test for the error "Empty socket option":

check_bind("unix::max-connections=8,", "", 0, "Empty socket option");

enirox001 commented at 2:11 PM on June 29, 2026:

Done, added

in test/functional/interface_ipc_init.py:42 in 5d66053ff0

  37 | +            self.restart_node(0, extra_args=[f"-ipcbind=unix:{self.ipcbind_path}"])
  38 | +
  39 | +        ipcbind_arg = f"-ipcbind=unix:{self.ipcbind_path}:max-connections={{}}"
  40 | +
  41 | +        with node.assert_debug_log([
  42 | +            "file descriptors available",

ViniciusCestarii commented at 7:43 PM on June 26, 2026:

In ipc: test per-address connection limiting over unix sockets 5d66053ff08f3d64a81f7bb99462a9a5ce475ada

nit: checking for "file descriptors available" seems unnecessary.

enirox001 commented at 2:12 PM on June 29, 2026:

Yes, this is unecessary, i think i added this while testing and forgot to remove it.

Removed. Thanks

ViniciusCestarii commented at 7:49 PM on June 26, 2026: contributor

Concept ACK, this is a nice feature. Left a few comments

enirox001 force-pushed on Jun 29, 2026

enirox001 commented at 2:35 PM on June 29, 2026: contributor

Thanks for the review @ViniciusCestarii

Made the changes suggested in the latest push

Sjors commented at 9:09 AM on June 30, 2026: member

Concept ACK.

I would be could to explain in the PR description that the alternative, -ipcmaxconnections, won't let us set the limit per binding. That itself doesn't seem very useful now, but it might be when we expand IPC functionality. The -ipcbind=<address>:max-connections=8 option can later be expanded to include a whitelist of interfaces / methods (like -rpcwhitelist), unix group name and chmod flags.

It's bit confusing that the PR description describes this as a "Follow to #34978", even though it doesn't build on it.

The subtree update is done incorrectly, which is presumably why the linter fails. I also prefer to do these in separate pull requests, unless changes are exclusively related to the change here, or it's obvious from the PR title that it involves such an update - neither is the case here.

ddd2e84346b10877c713bb7d383391c1ce2d860c could be split between introducing a default limit per connection and then the second commit that makes it configurable.

ParseBindAddress is rather large. The name implies it's reusable for p2p / rpc bindings , but this PR doesn't do that, and I don't think there's much demand for it. It could still be justified based on the ability to expand it. I don't think the implementation needs to be in the header. Maybe add node/ipc_args.{h,cpp}.

The new default DEFAULT_MAX_CONNECTIONS limit (and how to override it) needs a release note.

Sjors commented at 9:29 AM on June 30, 2026: member

Also it looks like the subtree is based on https://github.com/bitcoin-core/libmultiprocess/pull/269, which is still open. So this PR should be draft.

DrahtBot marked this as a draft on Jun 30, 2026

enirox001 commented at 11:52 AM on July 2, 2026: contributor

Thanks for the review @Sjors

I agree with your point regarding the subtree update. I'm moving this to draft pending the merge of the standalone subtree PR. Once that lands, I will rebase this and apply all of your feedback

DrahtBot added the label Needs rebase on Jul 7, 2026

fanquake referenced this in commit b6becf3534 on Jul 10, 2026

Kino1994 referenced this in commit 75e5bbe4c5 on Jul 10, 2026

enirox001 force-pushed on Jul 11, 2026

DrahtBot removed the label Needs rebase on Jul 11, 2026

enirox001 force-pushed on Jul 11, 2026

enirox001 commented at 7:58 PM on July 11, 2026: contributor

Subtree PR adding the max connections PR has been merged and added to core https://github.com/bitcoin/bitcoin/pull/35684

enirox001 commented at 8:03 PM on July 11, 2026: contributor

Concept ACK.

I would be could to explain in the PR description that the alternative, -ipcmaxconnections, won't let us set the limit per binding. That itself doesn't seem very useful now, but it might be when we expand IPC functionality. The -ipcbind=<address>:max-connections=8 option can later be expanded to include a whitelist of interfaces / methods (like -rpcwhitelist), unix group name and chmod flags.

It's bit confusing that the PR description describes this as a "Follow to #34978", even though it doesn't build on it.

Updated the PR description to mention the alternative approach as why this approach was taken instead. Also removed mention of the followup as it does not build on it

The subtree update is done incorrectly, which is presumably why the linter fails. I also prefer to do these in separate pull requests, unless changes are exclusively related to the change here, or it's obvious from the PR title that it involves such an update - neither is the case here.

ddd2e84 could be split between introducing a default limit per connection and then the second commit that makes it configurable.

Split this to introduce the default limit per connection and another commit that makes it configurable

ParseBindAddress is rather large. The name implies it's reusable for p2p / rpc bindings , but this PR doesn't do that, and I don't think there's much demand for it. It could still be justified based on the ability to expand it. I don't think the implementation needs to be in the header. Maybe add node/ipc_args.{h,cpp}.

The name did imply more applications than what it was intended to be used for. I have renamed it to ParseIpcBindAddress and moved into into the new node/ipc_args.{h,cpp} files

The new default DEFAULT_MAX_CONNECTIONS limit (and how to override it) needs a release note.

Added the release notes in the latest commit

enirox001 commented at 8:04 PM on July 11, 2026: contributor

Once again moving this out of draft. Should be ready for review now. Thanks

enirox001 marked this as ready for review on Jul 11, 2026

DrahtBot removed the label CI failed on Jul 11, 2026

in src/init.cpp:1071 in 3b9075a0b6

1066 |          return InitError(Untranslated("-maxconnections must be greater or equal than zero"));
1067 |      }
1068 | +    // Reserve one listening socket per -ipcbind address plus its accepted
1069 | +    // connection slots. Unlike P2P, IPC has no default listener.
1070 | +    const size_t ipc_bind{args.GetArgs("-ipcbind").size()};
1071 | +    if (ipc_bind > 0 && interfaces::Ipc::DEFAULT_MAX_CONNECTIONS > (MAX_IPC_FDS - ipc_bind) / ipc_bind) {

ryanofsky commented at 9:06 PM on July 15, 2026:

In commit "ipc: add default connection limit for ipcbind listeners" (3b9075a0b67fbf4c6bd4e87579200252b7323565)

I think this would be more readable if the comparison was simplified to if (ipc_max_connections > MAX_IPC_FDS - ipc_bind). Also would consider renaming ipc_bind to ipc_addresses.

enirox001 commented at 11:54 AM on July 23, 2026:

Done. I renamed ipc_bind to ipc_addresses and simplified the default limit comparison to use the aggregate connection count in this commit. This changes a bit in the subsequent commit, but this change is nice to have here as well

in src/init.cpp:751 in b5c72a4c42

 747 | @@ -747,7 +748,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
 748 |      argsman.AddArg("-rpcworkqueue=<n>", strprintf("Set the maximum depth of the work queue to service RPC calls (default: %d)", DEFAULT_HTTP_WORKQUEUE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC);
 749 |      argsman.AddArg("-server", "Accept command line and JSON-RPC commands", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
 750 |      if (can_listen_ipc) {
 751 | -        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. Each configured listener reserves " + ToString(interfaces::Ipc::DEFAULT_MAX_CONNECTIONS) + " accepted connections. 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);
 752 | +        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. Append socat-style socket options like \":max-connections=<n>\" to set per-address listener options, for example \"unix::max-connections=8\" or \"unix:/custom/path:max-connections=8\". If no max-connections option is specified, " + ToString(interfaces::Ipc::DEFAULT_MAX_CONNECTIONS) + " accepted connections will be reserved per listener. 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);

ryanofsky commented at 9:42 PM on July 15, 2026:

In commit "ipc: support per-address max-connections on ipcbind" (b5c72a4c4288c17188f34a689a6fbe578b523768)

I think it would be good to change the colon to a comma here (:max-connections=<n> -> ,max-connections=<n>) because this should work better with windows paths which include colons (C:\Bitcoin\node.sock). This would also be more similar to socat syntax.

enirox001 commented at 11:54 AM on July 23, 2026:

Addressed. socket options now use a comma separator, updated the PR description as well

in src/interfaces/ipc.h:77 in 3b9075a0b6

  71 | @@ -68,8 +72,9 @@ class Ipc
  72 |      virtual std::unique_ptr<Init> connectAddress(std::string& address) = 0;
  73 |  
  74 |      //! Listen on a socket address exposing this process's init interface to
  75 | -    //! clients. Throws an exception if there was an error.
  76 | -    virtual void listenAddress(std::string& address) = 0;
  77 | +    //! clients, accepting at most max_connections simultaneous client
  78 | +    //! connections. Throws an exception if there was an error.
  79 | +    virtual void listenAddress(std::string& address, size_t max_connections) = 0;

ryanofsky commented at 9:54 PM on July 15, 2026:

In commit "ipc: add default connection limit for ipcbind listeners" (3b9075a0b67fbf4c6bd4e87579200252b7323565)

I think it would be nice if this took an const node::IpcBindAddress& so this interface could be future proof if new options are added.

enirox001 commented at 11:54 AM on July 23, 2026:

Done, listenAddress() now takes a const ipc::ListenAddress&,

in src/node/ipc_args.h:15 in b5c72a4c42

  10 | +#include <cstddef>
  11 | +#include <string>
  12 | +
  13 | +namespace node {
  14 | +
  15 | +struct IpcBindAddress {

ryanofsky commented at 9:59 PM on July 15, 2026:

In commit "ipc: support per-address max-connections on ipcbind" (b5c72a4c4288c17188f34a689a6fbe578b523768)

I think it probably makes more sense for this to live inside the IPC library than the node library. Would suggest renaming node::IpcBindAddress to ipc::ListenAddress and defining it inside an src/ipc/types.h file for external consumption similar to src/node/types.h and src/wallet/types.h.

Similarly, would suggest changing node::ParseIpcBindAddress to an interfaces::Ipc::parseListenAddress method.

Would also suggest moving interfaces::Ipc::DEFAULT_MAX_CONNECTIONS to ipc::DEFAULT_MAX_CONNECTIONS in src/ipc/types.h using it to initialize the struct field size_t max_connections{DEFAULT_MAX_CONNECTIONS}

These changes would make it easier to add more options in the future without needing to change the interfaces::Ipc method signatures, and I think would organize code better.

enirox001 commented at 11:54 AM on July 23, 2026:

Adddressed the suggestion here. Thanks

I tried a few locations for the parser implementation. Defining the static method in src/ipc/interfaces.cpp caused undefined references linker errors

I also tried making parsing a virtual method implemented by the concrete IPC class. That resolved the linker errors but required additional plumbing in the interface and init.cpp which seemed too intrusive.

Placing it in the general common/interfaces.cpp implementation linked correctly but felt unrelated to that file’s responsibilities.

I ultimately kept the implementation in a specific src/ipc/listen.cpp and compiled that file into bitcoin_common, which is available to the init consumers.

I’m open to changing the target or implementation location if there is a cleaner way to keep it in ipc/interfaces.cpp without introducing the linker or interface plumbing issues above

ryanofsky commented at 9:16 PM on July 28, 2026:

re: #35037 (review)

Thanks, the approach taken seems ok. My suggestion was to make the Ipc::parseListenAddress implementation live in the bitcoin_ipc library in a virtual method. This would add a small amount of extra plumbing as you mentioned, but I think not very much, and this would match the way other functions of the IPC library (spawnProcess, connectAddress etc) are exposed to bitcoin core.

The reason for making these functions virtual is to keep bitcoind free of IPC dependencies, and allow IPC code to access libmultiprocess and capnproto functions which are not allowed to be accessed directly by the rest of bitcoin core code.

The approach taken in 65214d870d3b4e27889ffc05920291904e637f99 is reasonable, but has a a few drawbacks compared to the suggested approach. parseListenAddress code is linked into bitcoin_common library instead of bitcoin_ipc so it can't call other IPC functions or libmultiprocess or capnproto code. Also parseListenAddress is linked into bitcoind even though it can never be called by it.

Current code is fine. Just wanted to explain rationale behind the suggestion.

ryanofsky commented at 10:09 PM on July 15, 2026: contributor

Code review c6d2cd8d0c64ad9723f02d05e3924374a5b76314. I plan to review more but this looks like a nicely implemented change. I left a few suggestions below, none are too important or blocking, except I do really think the : options separator should be replaced by , to avoid problems with windows paths

in src/ipc/test/ipc_test.cpp:246 in c6d2cd8d0c

 241 | +BOOST_AUTO_TEST_CASE(ipc_socket_limit_test)
 242 | +{
 243 | +    IpcSocketMaxConnectionsTest(m_args.GetDataDirNet());
 244 | +}
 245 | +
 246 | +BOOST_AUTO_TEST_SUITE_END()

xyzconstant commented at 7:55 PM on July 22, 2026:

I tried to run C++ tests and missed this suite because it's registered under a new name (ipc_test) different from the existing one (ipc_tests). Suggest calling IpcSocketMaxConnectionsTest from the existing ipc_tests case in ipc_tests.cpp instead of adding a near-identically named suite, e.g.:

diff --git a/src/ipc/test/ipc_test.cpp b/src/ipc/test/ipc_test.cpp
index b1e49df092..88f27231c2 100644
--- a/src/ipc/test/ipc_test.cpp
+++ b/src/ipc/test/ipc_test.cpp
@@ -235,12 +235,3 @@ void IpcSocketMaxConnectionsTest(const fs::path& datadir)
     BOOST_CHECK_EQUAL(addr2, address);
     BOOST_CHECK_EQUAL(echo2, "echo test 2");
 }
-
-BOOST_FIXTURE_TEST_SUITE(ipc_test, BasicTestingSetup)
-
-BOOST_AUTO_TEST_CASE(ipc_socket_limit_test)
-{
-    IpcSocketMaxConnectionsTest(m_args.GetDataDirNet());
-}
-
-BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/ipc/test/ipc_tests.cpp b/src/ipc/test/ipc_tests.cpp
index 45f5f0a28c..3d194d8a7b 100644
--- a/src/ipc/test/ipc_tests.cpp
+++ b/src/ipc/test/ipc_tests.cpp
@@ -17,6 +17,7 @@ BOOST_AUTO_TEST_CASE(ipc_tests)
     IpcPipeTest();
     IpcSocketPairTest();
     IpcSocketTest(m_args.GetDataDirNet());
+    IpcSocketMaxConnectionsTest(m_args.GetDataDirNet());
 }

This also makes the <test/util/setup_common.h> include above unnecessary.

enirox001 commented at 12:34 PM on July 23, 2026:

Done thanks. Removed the ipc_test so that IpcSocketMaxConnectionsTest() is now called from the existing ipc_tests test case,

in src/init.cpp:1081 in c6d2cd8d0c

1076 | +            return InitError(Untranslated(strprintf("Invalid -ipcbind address '%s': %s", configured_address, util::ErrorString(bind).original)));
1077 | +        }
1078 | +        // Each configured IPC bind contributes one listening socket and max_connections accepted sockets.
1079 | +        // Check the aggregate before adding this entry so many individually-valid -ipcbind values cannot
1080 | +        // overflow min_required_fds below.
1081 | +        if (bind->max_connections > MAX_IPC_FDS - ipc_bind - ipc_max_connections - 1) {

xyzconstant commented at 8:39 PM on July 22, 2026:

According to Fable 5, this overflow check can be defeated in two ways:

1. size_t underflow lets the aggregate check pass. When ipc_bind + ipc_max_connections lands exactly on MAX_IPC_FDS, the right side becomes 0 - 1 → SIZE_MAX, so any further entry is accepted and the static_cast<int> below wraps negative:

./build/bin/bitcoin-node -regtest -ipcbind=unix:/tmp/a.sock:max-connections=2147483646 -ipcbind=unix:/tmp/b.sock:max-connections=2147483646
...
2026-07-22T20:29:26Z Reserving -2 file descriptors for IPC (2 listening sockets, -4 accepted connections)

The node starts up with min_required_fds reduced by 2 instead of failing with "Too many IPC file descriptors requested".

2. Even a single entry that passes the check crashes at startup. Keeping the IPC total ≤ int max is not sufficient, because min_required_fds adds MIN_CORE_FDS + MAX_ADDNODE_CONNECTIONS + nBind on top, so the addition on L1101 overflows int (UB) and dies on an assert instead of a clean InitError:

./build/bin/bitcoin-node -regtest -ipcbind=unix:/tmp/a.sock:max-connections=2147483646
util/fs_helpers.cpp:161 int RaiseFileDescriptorLimit(int): Assertion `min_fd >= 0' failed.

Both would be solved by making MAX_IPC_FDS conservatively small instead of int max — even something like 1 << 20 is far beyond any realistic deployment and leaves all the downstream int arithmetic trivially safe — together with rearranging the check so the subtraction can't go below zero:

-        if (bind->max_connections > MAX_IPC_FDS - ipc_bind - ipc_max_connections - 1) {
+        if (bind->max_connections + 1 > MAX_IPC_FDS - ipc_bind - ipc_max_connections) {

(max_connections is parse-capped at 2147483647, so the + 1 cannot overflow.)

I verified the diff locally and:

Repro 1 now fails with "Too many IPC file descriptors requested" and the tests pass.
Repro 2 however still hits the assertion. It needs a smaller MAX_IPC_FDS, and the parse-level cap in ParseIpcBindAddress may want to match the chosen bound so users get the clearer "max-connections must be at most N" error at parse time.

enirox001 commented at 12:34 PM on July 23, 2026:

Good catch. I agree that using int max here was not very safe because min_required_fds adds other FD reservations afterward.

I took your suggestion and capped IPC connections at 1 << 20, applied the same limit during parsing, and changed the aggregate check to avoid unsigned underflow. I also added boundary and multi-address regression tests

in test/functional/interface_ipc_init.py:58 in c6d2cd8d0c outdated

  53 | +            extra_args=[ipcbind_arg.format(2147483648)],
  54 | +            expected_msg=f"Error: Invalid -ipcbind address 'unix:{self.ipcbind_path}:max-connections=2147483648': max-connections must be at most 2147483647",
  55 | +        )
  56 | +
  57 | +    def run_test(self):
  58 | +        self.test_ipcbind_max_connections()

xyzconstant commented at 9:24 PM on July 22, 2026:

In commit 2b3c6ddd6cbfbc140eb92f8db6142001ec969d59: "ipc: test per-address connection limiting over unix sockets"

It would be great to cover it in functional tests as well:

diff --git a/test/functional/interface_ipc_init.py b/test/functional/interface_ipc_init.py
index a2ef23b6aa..551bb43abe 100755
--- a/test/functional/interface_ipc_init.py
+++ b/test/functional/interface_ipc_init.py
@@ -4,7 +4,9 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test IPC initialization behavior."""
 
+import socket
 import tempfile
+import time
 from pathlib import Path
 
 from test_framework.test_framework import BitcoinTestFramework
@@ -54,8 +56,35 @@ class IPCInitTest(BitcoinTestFramework):
             expected_msg=f"Error: Invalid -ipcbind address 'unix:{self.ipcbind_path}:max-connections=2147483648': max-connections must be at most 2147483647",
         )
 
+    def test_ipcbind_connection_limiting(self):
+        self.log.info("Test -ipcbind max-connections limiting over the socket")
+        node = self.nodes[0]
+        self.restart_node(0, extra_args=[f"-ipcbind=unix:{self.ipcbind_path}:max-connections=1", "-debug=ipc"])
+
+        def ipc_client():
+            client = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+            client.connect(str(self.ipcbind_path))
+            return client
+
+        # First client takes the only one (max-connections=1) slot.
+        with node.assert_debug_log(["IPC server: socket connected."], timeout=5):
+            first = ipc_client()
+
+        # Second client tries to connect but should not be accepted while the first one occupies the slot.
+        with node.assert_debug_log([], unexpected_msgs=["IPC server: socket connected."]):
+            second = ipc_client()
+            time.sleep(0.5)  # give some chance
+
+        # The second client must be accepted right after freeing up the slot.
+        with node.assert_debug_log(["IPC server: socket disconnected.", "IPC server: socket connected."], timeout=5):
+            first.close()
+
+        with node.assert_debug_log(["IPC server: socket disconnected."], timeout=5):
+            second.close()
+
     def run_test(self):
         self.test_ipcbind_max_connections()
+        self.test_ipcbind_connection_limiting()
 
 
 if __name__ == '__main__':

Plus, having a per-address case with two addresses, each with its own limits, works well because that's what the commit mentions.

enirox001 commented at 12:34 PM on July 23, 2026:

Done. I used your suggested functional test with slight modifications to cover two listeners, verifying that each listener enforces its limit independently and accepts its waiting client after a slot is freed.

in src/init.cpp:1089 in c6d2cd8d0c

1084 | +        ++ipc_bind;
1085 | +        ipc_max_connections += bind->max_connections;
1086 | +    }
1087 | +
1088 | +    if (ipc_bind > 0) {
1089 | +        LogInfo("Reserving %d file descriptors for IPC (%d listening sockets, %d accepted connections)",

xyzconstant commented at 11:27 PM on July 22, 2026:

"X accepted connections" reads like already accepted at startup when it actually means capacity.

Maybe Reserving %d file descriptors for IPC (%d listening sockets, %d connection slots) reads more precisely?

enirox001 commented at 12:34 PM on July 23, 2026:

Done, changed the log and help text to use "connection slots"

in src/init.cpp:1110 in c6d2cd8d0c

xyzconstant commented at 1:01 AM on July 23, 2026:

With the updates introduced by this PR, it's now very easy to trigger this error, say, by setting a large max-connections value:

$ ./build/bin/bitcoin-node -regtest -ipcbind=unix::max-connections=1000
Error: Not enough file descriptors available. 1024 available, 1161 required.

This message doesn’t indicate that the user's max-connections setting is the root of the problem, and understanding the connection between these numbers requires some knowledge of how internal fd accounting works. It might be helpful to explicitly mention the IPC contribution in the message or log the line "Reserving N file descriptors for IPC" before the error occurs, so users have better insight into what’s happening.

enirox001 commented at 12:34 PM on July 23, 2026:

Done thanks. Made the preceding IPC log clearer by reporting listening sockets and connection slots.

xyzconstant commented at 2:39 AM on July 23, 2026: contributor

Concept ACK. Limiting IPC connections per listener makes sense.

Tested manually on macOS and Linux by exercising the limit over the socket with raw and real clients, checking per-address behavior with multiple listeners, and running the new unit and functional tests.

Left a few comments and suggestions. I'm planning to review the code more deeply, but I can confirm the core feature works as expected.

xyzconstant commented at 2:55 AM on July 23, 2026: contributor

With the updates in this PR, a single -ipcbind value can silently reduce the automatic P2P connection slots to zero. For example,

$ ./build/bin/bitcoin -m node -signet -ipcbind=unix::max-connections=863
Warning: Reducing -maxconnections from 125 to 0, because of system limitations.
...
2026-07-23T00:22:52Z 64 addresses found from DNS seeds

Here, the IPC reservation is deducted from the fd budget before automatic P2P connections are allocated. If max-connections is set close to the platform's fd cap, there may be no room left for automatic P2P connections (the exact limit varies by platform). The node completes startup and even fetches addresses from DNS seeds, but never makes or accepts peer connections. On mainnet, this means it would stop following the chain after that single warning log at startup.

This (warning-only) behavior when maxconnections hits 0 is a pre-existing issue, so I don't think it's a blocker for this PR. A follow-up could make it an InitError when the user didn't explicitly set -maxconnections=0.

enirox001 force-pushed on Jul 23, 2026

DrahtBot added the label CI failed on Jul 23, 2026

DrahtBot commented at 10:44 AM on July 23, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/30000116348/job/89183040191 LLM reason (✨ experimental): CI failed because lint-include-guards.py detected a missing/incorrect include guard in src/ipc/types.h (double-inclusion risk).

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

DrahtBot removed the label CI failed on Jul 23, 2026

DrahtBot added the label IPC on Jul 23, 2026

enirox001 commented at 12:39 PM on July 23, 2026: contributor

With the updates in this PR, a single -ipcbind value can silently reduce the automatic P2P connection slots to zero. For example,
$ ./build/bin/bitcoin -m node -signet -ipcbind=unix::max-connections=863
Warning: Reducing -maxconnections from 125 to 0, because of system limitations.
...
2026-07-23T00:22:52Z 64 addresses found from DNS seeds
Here, the IPC reservation is deducted from the fd budget before automatic P2P connections are allocated. If max-connections is set close to the platform's fd cap, there may be no room left for automatic P2P connections (the exact limit varies by platform). The node completes startup and even fetches addresses from DNS seeds, but never makes or accepts peer connections. On mainnet, this means it would stop following the chain after that single warning log at startup.

This (warning-only) behavior when maxconnections hits 0 is a pre-existing issue, so I don't think it's a blocker for this PR. A follow-up could make it an InitError when the user didn't explicitly set -maxconnections=0.

I agree with this, and it is worth fixing, but it seems broader than this PR because the warning-only behavior is pre existing and can result from non ipc fd pressure too. I left it unchanged here, btu this would be good for a followup

enirox001 commented at 12:43 PM on July 23, 2026: contributor

Thanks for the reviews, @ryanofsky and @xyzconstant. I’ve addressed the suggestions and updated the PR description accordingly.

enirox001 commented at 6:33 PM on July 23, 2026: contributor

except I do really think the : options separator should be replaced by , to avoid problems with windows paths

Changed the option separator to ,. Microsoft documents Windows AF_UNIX pathname sockets as using Win32 filesystem paths (https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/), which may contain a drive-letter colon such as C:.

Looking more closely, I don’t think an ordinary Windows path would have broken the previous parser. It used rfind(':'), followed by this check:

const std::string_view options_view{address_view.substr(option_pos + 1)};
if (options_view.empty() || options_view.find_first_of("=,") == std::string_view::npos) {
    return IpcBindAddress{std::move(address), DEFAULT_MAX_CONNECTIONS};
}

For unix:C:\bitcoin\node.sock, it would find the C: colon but then preserve the complete address because the remaining text contains no option syntax. For unix:C:\bitcoin\node.sock:max-connections=8, rfind(':') would select the final colon before max-connections.

So I agree the colon syntax was ambiguous and fragile, especially if more options are added. Still, I don’t think it would have broken ordinary Windows drive-letter paths with the existing parser.

enirox001 force-pushed on Jul 23, 2026

in src/init.cpp:1082 in 21378ce132

1089 | +            return InitError(Untranslated(strprintf("Invalid -ipcbind address '%s': %s", configured_address, util::ErrorString(listen_address).original)));
1090 | +        }
1091 | +        // Check the aggregate before adding this listener so multiple
1092 | +        // individually valid -ipcbind values cannot exceed the IPC FD cap.
1093 | +        const size_t ipc_fds{ipc_addresses + ipc_max_connections};
1094 | +        if (listen_address->max_connections + 1 > MAX_IPC_FDS - ipc_fds) {

ryanofsky commented at 8:48 PM on July 28, 2026:

In commit "ipc: support per-address max-connections on ipcbind" (21378ce132a94c49d9eb62a0aa14c5b98f533458)

Seems a little unexpected to subtract. Also seems like this could be simplified by adding to the totals earlier like:

ipc_addresses += 1;
ipc_max_connections += listen_address->max_connections;
if (ipc_addressess + ipc_max_connections > MAX_IPC_FDS) { InitError(...); }

enirox001 commented at 3:30 PM on July 31, 2026:

Updated as suggested. The listener and connection totals are now incremented before checking the aggregate. I also updated the comment to reflect that the change

ryanofsky approved

ryanofsky commented at 9:40 PM on July 28, 2026: contributor

Code review ACK 65214d870d3b4e27889ffc05920291904e637f99. Looks great! This fixes a gap in descriptor accounting, and adds a helpful connection limiting feature, and is nicely implemented. I didn't review the tests very closely and plan to look more at them, but all of the non-test changes look good.

DrahtBot requested review from ViniciusCestarii on Jul 28, 2026

DrahtBot requested review from Sjors on Jul 28, 2026

DrahtBot requested review from xyzconstant on Jul 28, 2026

ryanofsky commented at 9:42 PM on July 28, 2026: contributor

re: #35037 (comment)

Good point the previous requirement for options to contain = characters would have prevented the previous implementation from having problems with windows drive letters in practice. Still I think it's nice that you switched to commas to match socat syntax more, and allow a little more flexibility in the future.

DrahtBot added the label Needs rebase on Jul 29, 2026

ipc: add default connection limit for ipcbind listeners

Reserve file descriptors for each configured -ipcbind listener and its
default connection slots during initialization. Cap IPC reservations
conservatively so downstream int-based file descriptor accounting
cannot overflow.

Introduce ipc::ListenAddress to keep a listener and its connection limit
together. Pass it through interfaces::Ipc and ipc::Protocol so each IPC
listener stops accepting new clients after reaching the cap and resumes
after a disconnect.

446ae5ddbd

enirox001 force-pushed on Jul 31, 2026

DrahtBot added the label CI failed on Jul 31, 2026

DrahtBot commented at 3:42 PM on July 31, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/30643104939/job/91197705923 LLM reason (✨ experimental): CI failed due to a lint-includes error: src/ipc/capnp/protocol.cpp uses quote-style #include "ipc/util.h" instead of bracket-style #include <ipc/util.h>.

<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 3:46 PM on July 31, 2026: contributor

Resolved some rebase conflicts after #35084 got merged

DrahtBot removed the label Needs rebase on Jul 31, 2026

DrahtBot removed the label CI failed on Jul 31, 2026

ipc: support per-address max-connections on ipcbind

Parse optional max-connections socket options from -ipcbind values
through interfaces::Ipc, returning ipc::ListenAddress values that keep
each listener address and connection limit together.

Use comma delimited socket options to avoid ambiguity with paths
containing colons, and validate each configured limit before adding it
to IPC file descriptor reservations.

776368e8e4

enirox001 force-pushed on Aug 3, 2026

ipc: test per-address connection limiting over unix sockets

Add IPC coverage for local listener capacity over unix sockets.

Verify a listener at capacity resumes accepting after a disconnect, check separate
listeners enforce their limits independently, and cover parser and aggregate file
descriptor limit boundaries.

ccd27feb6e

doc: add ipcbind max-connections release note 04593a1656

enirox001 force-pushed on Aug 3, 2026

DrahtBot added the label CI failed on Aug 3, 2026

DrahtBot commented at 8:38 AM on August 3, 2026: contributor

🚧 At least one of the CI tasks failed. Task macOS native: https://github.com/bitcoin/bitcoin/actions/runs/30797261731/job/91633554807 LLM reason (✨ experimental): CI failed because the ipc_tests ctest test failed (1/372 tests), with 3 failures detected in the “Bitcoin Core Test Suite”.

<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 8:55 AM on August 3, 2026: contributor

Updated the parser to only split the unix socket address when it finds the recognized ,max-connections= option. Previously, it split on any comma, which broke valid socket paths containing commas.

I also added tests to make sure commas are preserved in paths while max-connections is still parsed correctly.

DrahtBot removed the label CI failed on Aug 3, 2026