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

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

Follow up to #34978 to make the per-address IPC connection-limit approach concrete.

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.

It also adds the local listener limit support needed in libmultiprocess and threads the parsed per-address limit through to ListenConnections() so each listener can stop accepting new IPC connections after reaching its local cap and resume accepting after a disconnect.

The draft includes:

parser coverage for -ipcbind max-connections and invalid inputs
functional init coverage for FD reservation logging and init errors
an IPC behavior test verifying that with max_connections=1, a second client only becomes usable after the first disconnects

This is intended to help evaluate the per-address approach downstream against the global-limit direction in #34978.

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 for information on the review process.

Type	Reviewers
Concept ACK	ryanofsky, kevkevinpal, ViniciusCestarii, Sjors

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:

#35084 (ipc: Add nonunix platform support by ryanofsky)
#34978 (init: reserve file descriptors for IPC connections by enirox001)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#29409 (multiprocess: Add capnp wrapper for Chain interface by ryanofsky)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#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)
#10102 (Multiprocess bitcoin 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.

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

ipc: update libmultiprocess subtree c1fa86ab49

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

Thread optional per-address max-connections values through the listener implementation.

Update interfaces::Ipc and ipc::Protocol listen methods to accept an optional connection limit, pass parsed -ipcbind limits from init, and use ListenConnections(..., max_connections) support so each listener stops accepting new IPC connections after reaching its local cap and resumes accepting after a disconnect.

ddd2e84346

ipc: test per-address connection limiting over unix sockets

Add an IPC test covering the local listener limit behavior through ipc::Process and ipc::Protocol over a unix socket.

The new test starts a listener with max_connections=1, verifies the first client is served immediately, checks that a second client remains blocked while the first connection stays open, and then confirms the second client becomes usable after the first disconnects.

15d736f1e0

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