bitcoin-cli: Add -ipcconnect option

ryanofsky commented at 1:55 pm on April 17, 2025: contributor

This implements an idea from sipa in #28722 (comment) to allow bitcoin-cli to connect to the node via IPC instead of TCP, if the ENABLE_IPC cmake option is enabled and the node has been started with -ipcbind.

This feature can be tested with:

0build/bin/bitcoin-node -regtest -ipcbind=unix -debug=ipc
1build/bin/bitcoin-cli -regtest -ipcconnect=unix -getinfo

The -ipconnect parameter can also be omitted, since this change also makes bitcoin-cli prefer IPC over HTTP by default, and falling back to HTTP if an IPC connection can’t be established.

This PR is part of the process separation project.

DrahtBot commented at 1:55 pm on April 17, 2025: 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/32297.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	laanwj, jlest01, yancyribbens, shahsb, sipa
Stale ACK	pinheadmz

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31375 (multiprocess: Add bitcoin wrapper executable by ryanofsky)
#30437 (ipc: add bitcoin-mine test program by ryanofsky)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#29409 (multiprocess: Add capnp wrapper for Chain interface by ryanofsky)
#28690 (build: Introduce internal kernel library by TheCharlatan)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

node.sock’ -> node.sock [trailing single quote]

ryanofsky force-pushed on Apr 17, 2025

DrahtBot added the label CI failed on Apr 17, 2025

laanwj added the label RPC/REST/ZMQ on Apr 17, 2025

in src/bitcoin-cli.cpp:985 in df40f46571 outdated

974+        } else {
975+            throw CConnectionFailed("uri-encode failed");
976+        }
977+    }
978+
979+    std::string username{gArgs.GetArg("-rpcuser", "")};

laanwj commented at 2:34 pm on April 17, 2025:

Passing -rpcconnect and -ipcconnect at the same time should probably result in an error. (at least, the logic of trying IPC first then RPC if that fails is a bit confusing to me, as they’re completely different protocols)

ryanofsky commented at 5:22 pm on April 18, 2025:

re: #32297 (review)

Passing -rpcconnect and -ipcconnect at the same time should probably result in an error. (at least, the logic of trying IPC first then RPC if that fails is a bit confusing to me, as they’re completely different protocols)

Thanks, this makes sense and also makes sense to skip IPC if -rpcconnect is specified. Both these changes are implemented now

laanwj commented at 2:40 pm on April 17, 2025: member

Concept ACK, it’d be useful to have an utility that can talk to the node through IPC and making bitcoin-cli double for that prevents code duplication.

jlest01 commented at 6:28 pm on April 17, 2025: none

Concept ACK, as it can make communication between the CLI and the node simpler and configuration-free.

pinheadmz commented at 6:58 pm on April 17, 2025: member

concept ACK, cool idea and will review. Does this close #5029 (adding unix sockets to the bitcoind rpc api) or does it only apply to bitcoin-node? Or is IPC protocol different from RPC and must be defined in the client?

ryanofsky commented at 7:16 pm on April 17, 2025: contributor

concept ACK, cool idea and will review. Does this close #5029 (adding unix sockets to the bitcoind rpc api) or does it only apply to bitcoin-node? Or is IPC protocol different from RPC and must be defined in the client?

It uses capnproto, so it may or may not achieve goals of #5029 depending on what they are. This PR does make it possible to call RPC methods over a unix socket, but in order to do that you need to use capnproto which is not available everywhere. For example there wouldn’t be an easy way for our python test framework to use the socket directly unless it depended on https://github.com/capnproto/pycapnp. So another solution that uses a another protocol or just uses http over the socket might be preferable to this approach. But the first commit here could be useful for supporting other approaches as well.

ryanofsky force-pushed on Apr 18, 2025

ryanofsky commented at 5:26 pm on April 18, 2025: contributor

Updated df40f46571bc7b8f0a85a437130e99bad5b0de63 -> 538521a37c3788ce2a0ba1bda76844a25cfd3e06 (pr/ipc-cli.2 -> pr/ipc-cli.3, compare) handling -rpcconnect option, adding a test, making various cleanups, adding comments, and fixing CI failures in non-ipc builds Updated 538521a37c3788ce2a0ba1bda76844a25cfd3e06 -> bd29de3896911317fc6dc0010c33b91ebf5b9f16 (pr/ipc-cli.3 -> pr/ipc-cli.4, compare) fixing compile error in intermediate commit, https://github.com/bitcoin/bitcoin/actions/runs/14538988101/job/40793056407?pr=32297, fixing functional test failure when socket path is too long https://cirrus-ci.com/task/6103052337283072, and adding new test to cover “bitcoin-cli was not built with IPC support” error

ryanofsky force-pushed on Apr 18, 2025

ryanofsky marked this as ready for review on Apr 18, 2025

DrahtBot removed the label CI failed on Apr 18, 2025

yancyribbens commented at 12:37 pm on April 19, 2025: contributor

Concept ACK

maflcko commented at 12:11 pm on April 21, 2025: member

Looks like those are real typos that should be fixed:

LLM Linter (✨ experimental)

Possible typos and grammar issues:

The following typos/grammar issues were found in added lines:

1. Extra “unix’” in the –ipcconnect help text
   Replace
   “…node.sock unix’ but fall back to TCP…”
   with
   “…node.sock’ but fall back to TCP…”

2. Double negative in JSONErrorReply comment
   Replace
   “…when a request cannot not be parsed,…”
   with
   “…when a request cannot be parsed,…”

ryanofsky force-pushed on Apr 21, 2025

luke-jr commented at 10:37 pm on April 21, 2025: member

This looks like it uses JSON-RPC inside Capnproto? Why not just use straight Capnproto?

ryanofsky commented at 11:31 pm on April 21, 2025: contributor

This looks like it uses JSON-RPC inside Capnproto? Why not just use straight Capnproto?

Yes it is skipping the HTTP part of the protocol but keeping the JSON part. The capnproto interface this exposes looks like:

0interface Rpc $Proxy.wrap("interfaces::Rpc") {
1    executeRpc [@0](/bitcoin-bitcoin/contributor/0/) (context :Proxy.Context, request :Text, uri :Text, user :Text) -> (result :Text);
2}

where request and response above are JSON text strings with the expected JSON-RPC fields. The schema could be unwrapped further, or the data could be sent in some binary json format depending on what your goal is but that interface was the simplest way I could think of implementing this feature.

shahsb commented at 5:03 am on April 23, 2025: none

Concept ACK

laanwj commented at 12:25 pm on April 24, 2025: member

The schema could be unwrapped further, or the data could be sent in some binary json format depending on what your goal is but that interface was the simplest way I could think of implementing this feature.

Right, Say, representing the entire RPC protocol as cap’n’proto structures directly would be interesting (anologous to how c-lightning has a gRPC version of their RPC), but this would be a huge project for which the first step would be the formal definition of the RPC protocol (including specific-enough types) so that that can be automatically generated.

And this would make it harder to make a cli tool, not easier, because the client needs to know the protocol to convert its command line to suitable calls.

So sending the JSON inside the interface seems like a good, low-impact way to implement this tool.

sipa commented at 2:45 pm on April 30, 2025: member

Concept ACK

in src/httprpc.h:29 in ea98a42640 outdated

22@@ -19,6 +23,9 @@ void InterruptHTTPRPC();
23  */
24 void StopHTTPRPC();
25 
26+/** Execute a single HTTP request, containing one or more JSONRPC requests. */
27+UniValue ExecuteHTTPRPC(const UniValue& valRequest, JSONRPCRequest& jreq, HTTPStatusCode& status);

pinheadmz commented at 5:57 pm on May 1, 2025:

ea98a42640b9ff77a462e55887025ddd1da54727

Might want to indicate status is an “out” param?

ryanofsky commented at 9:51 pm on May 21, 2025:

re: #32297 (review)

Might want to indicate status is an “out” param?

Thanks, expanded comment.

in src/httprpc.cpp:150 in ea98a42640 outdated

158-    // JSONRPC handles only POST
159-    if (req->GetRequestMethod() != HTTPRequest::POST) {
160-        req->WriteReply(HTTP_BAD_METHOD, "JSONRPC server handles only POST requests");
161-        return false;
162-    }
163-    // Check authorization

pinheadmz commented at 6:01 pm on May 1, 2025:

ea98a42640b9ff77a462e55887025ddd1da54727

I notice this ExecuteHTTPRPC() bypasses our only authorization barrier for RPC which is an HTTP header, maybe you deal with this in another commit but that should probably be documented in the ipcbind help text.

You also left behind the jreq.authUser check for RPC whitelist – which I believe only gets filled in by HTTPReq_JSONRPC().

I guess you need to parse the request to get the method before checking the whitelist, but I just wonder if there’s a footgun where a user has a whitelist set up for HTTP and somehow that blocks their IPC usage.

ryanofsky commented at 9:27 pm on May 21, 2025:

re: #32297 (review)

I notice this ExecuteHTTPRPC() bypasses our only authorization barrier for RPC which is an HTTP header, maybe you deal with this in another commit but that should probably be documented in the ipcbind help text.

Makes sense, updated ipcbind help text to mention RPC authentication.

You also left behind the jreq.authUser check for RPC whitelist – which I believe only gets filled in by HTTPReq_JSONRPC().

Yes the username is sent so the bitcoin-cli -rpcuser option works. Currently, the username is only used for applying the RPC method whitelist, but it’s possible to imagine the username being used for other things in the future like logging, or having per-user settings & defautls, or being used for other permissions & whitelists.

I guess you need to parse the request to get the method before checking the whitelist, but I just wonder if there’s a footgun where a user has a whitelist set up for HTTP and somehow that blocks their IPC usage.

I wouldn’t say it’s a necessarily a footgun if the node is configured with a whitelist and the whitelist is then appiled. But of course this could be surprising if unexpected. I think a few different alternatives would be possible here that have different tradeoffs:

Could require username and password or cookie to be specified to call RPCs over the unix socket. I wouldn’t like this approach because it would negate many advantages of using a unix socket: that you can control access based only on file permissions and don’t need to deal with cookie files or hashed passwords or firewall rules. For example I think it is nice that current approach lets you run a node on a remote ssh server and forward the socket over ssh without requiring clients to send a password, and also without exposing the socket to other processes on the server (unlike a TCP socket which is exposed to all local processes).
Could ignore bitcoin-cli -rpcuser option and not send it to the server. This could be ok but be limiting in the future if we wanted to use the username for more things like logging or having per-user defaults and settings.
Could accept username but ignore it when applying the RPC method whitelist. Username could be used for other things in the future, but all RPCs would be available regardless of username, avoiding the footgun you mentioned
(Current approach). Username would be sent with all requests but no passwords or cookies would be required. Other than using file permissions instead of passwords and cookies for authentication, the IPC interface would work the same as the RPC interface, so for example server admins could verify that their whitelist was configured correctly by just specifying usernames and not needing to specify or even know user passwords (if they are hashed).

I think any of these approaches could work but I liked (4) for authenticating with file permissions instead of layering on another authentication mechanism, but after authenticating, respecting the username and treating users the same regardless of how they connect.

in src/httprpc.cpp:298 in ea98a42640 outdated

302+        req->WriteReply(status);
303+    } else {
304+        req->WriteHeader("Content-Type", "application/json");
305+        req->WriteReply(status, reply.write() + "\n");
306+    }
307+    return status < 400;

pinheadmz commented at 6:19 pm on May 1, 2025:

ea98a42640b9ff77a462e55887025ddd1da54727

Do we ever use this return value here on on master? It looks like it just gets swallowed inside HTTPWorkItem

ryanofsky commented at 9:46 pm on May 21, 2025:

re: #32297 (review)

Do we ever use this return value here on on master? It looks like it just gets swallowed inside HTTPWorkItem

Nice catch, dropped the unused return value.

pinheadmz commented at 7:23 pm on May 1, 2025: member

While reviewing this I hit some weird behavior where bitcoin-node doesn’t fully shutdown.

To reproduce:

In terminal 1: build/bin/bitcoin-node -regtest -ipcbind=unix -debug=rpc -debug=http -debug=ipc

Terminal 2: build/bin/bitcoin-cli -regtest waitforblock 0000000000079f8ef3d2c688c244eb7a4570b24c9ed7b4a8c619eb02596f8862 (this will wait forever)

go back to terminal 1 and ctrl-c. I get Shutdown: done in the output but the process does not quit.

Then funny stuff like this, in Terminal 3:

 0--> build/bin/bitcoin-cli -regtest getblockcount
 1error code: -32603
 2error message:
 3Node chainman not found
 4
 5--> build/bin/bitcoin-cli -regtest -getinfo
 6error code: -1
 7error message:
 8Internal bug detected: RPC call "getnetworkinfo" returned incorrect type:
 9{
10    "localservices": "key missing, despite not being optional in doc",
11    "localservicesnames": "key missing, despite not being optional in doc",
12    "localrelay": "key missing, despite not being optional in doc",
13    "timeoffset": "key missing, despite not being optional in doc",
14    "connections": "key missing, despite not being optional in doc",
15    "connections_in": "key missing, despite not being optional in doc",
16    "connections_out": "key missing, despite not being optional in doc",
17    "networkactive": "key missing, despite not being optional in doc",
18    "relayfee": "key missing, despite not being optional in doc",
19    "incrementalfee": "key missing, despite not being optional in doc"
20}
21Bitcoin Core v29.99.0-9ff5bc75659e
22Please report this issue here: https://github.com/bitcoin/bitcoin/issues

I had to use killall -9 bitcoin-node to clean up

ryanofsky commented at 7:30 pm on May 1, 2025: contributor

While reviewing this I hit some weird behavior where bitcoin-node doesn’t fully shutdown.

Thanks for testing! These are all known issues which should be fixed by https://github.com/bitcoin/bitcoin/pull/32345

in src/bitcoin-cli.cpp:986 in 113b46d310 outdated

981+            throw CConnectionFailed("uri-encode failed");
982+        }
983+    }
984+
985+    std::string username{gArgs.GetArg("-rpcuser", "")};
986+    if (auto response{CallIPC(rh, strMethod, args, endpoint, username)}) {

pinheadmz commented at 7:36 pm on May 1, 2025:

113b46d31075c66cecbe4482aed362d50cdec28c

I presume you can ignore rpcwait because the socket won’t exist until the server is running?

What about the try/catch? There’s no “connection failed” case like there is for HTTP?

ryanofsky commented at 9:56 pm on May 21, 2025:

re: #32297 (review)

I presume you can ignore rpcwait because the socket won’t exist until the server is running?

Not really, this is a good catch. I was just ignoring rpcwait option before but updated the code now so it will work for both RPC and IPC. Now the connection failed case is handled the same for both.

in test/functional/interface_ipc_cli.py:38 in 9ff5bc7565 outdated

25+        # Always run IPC node binary
26+        self.binary_paths.bitcoind = self.binary_paths.bitcoin_node
27+
28+        # Work around default CI path exceeding maximum socket path length.
29+        # On Linux sun_path is 108 bytes, on macOS it's only 104. Includes
30+        # null terminator.

pinheadmz commented at 7:59 pm on May 1, 2025:

9ff5bc75659e0314f42c1477fc0c37ac0ad132fa

Hm for some reason I thought it could be as low as 92:

https://github.com/bitcoin/bitcoin/blob/5b8046a6e893b7fad5a93631e6d1e70db31878af/test/functional/feature_proxy.py#L66-L67

ryanofsky commented at 10:10 pm on May 21, 2025:

re: #32297 (review)

Hm for some reason I thought it could be as low as 92:

You are right. I added some code to fall back to 92 if this is not a known platform with a higher limit. This could use 92 everywhere, but since test coverage here is lost when the datadir path length exceeds this value, I think it is good to run the extra checks more places if the platform does support a higher limit.

pinheadmz approved

pinheadmz commented at 8:03 pm on May 1, 2025: member

ACK 9ff5bc75659e0314f42c1477fc0c37ac0ad132fa

Built and ran tests on arm64/macos. Played with the feature locally. Reviewed each commit, some questions below.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 9ff5bc75659e0314f42c1477fc0c37ac0ad132fa
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmgT0ykACgkQ5+KYS2KJ
 7yTogNQ/+NxdemF18NTDuoW6W7fLeDbdbJCbe/9fSse0fhFi0rtTkFpJdXKt+p86T
 8gndlLfTNitJ58BUul5WMbJvotwnydkBoSlsPi1ZT1hWainWuZ7Q6fkYgRV94uuU5
 9arsNYxwOHOlzwu/mbcxvkUgDSWHqLS0zaw1kNfPby6zcF0AajUS/gtAwNObv7h9C
10zMfLjm537So8fbiimSxnF1N4aHchhd63AVm8PUD+X9E1ZRN0xc7IeWfGcUc0W2Zx
11kAxwOKO1kQBBDqHlktlQ8F+cSUjdNXI4JXqb6mCNPXH4WFk669xsBV3Yz8Z/73gt
12lCBk7spyO/TWMdhdZU4NrGRvkgBbn5ZgjRWDIkHP/9JZYfunoBASbsGI1Oiexolz
13t2CTZGDKvdkKD3Zz1aYRKMbeF4uBxY0HA4sPauwt6N9tbtk3Mj2nUBbLQgMO4the
144Lx6IT0ewpBcfbQq+8/7rbuDFvAh+v11oqwUbvckXYuZxkv6epU/3kjyoZOmJ3Xd
15afs6rt3BHRk6HnETLKq8fZgNc6tkmz7rrRPZSYgHw039BStK/MG9SowGxIFtu8Wj
16iYrfZcSVeIeFkag67HDpHnLAaDV3jneZuvocEynypk8R4LHd9TGH+b4KEFj6No1d
17F8Nhx3FyY6AeuGlWKz3lSxEZlqKB/eC4chyxzTWwkHVfIDA8KGU=
18=IN//
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on openpgp.org

DrahtBot requested review from sipa on May 1, 2025

DrahtBot requested review from laanwj on May 1, 2025

DrahtBot requested review from shahsb on May 1, 2025

DrahtBot added the label Needs rebase on May 7, 2025

ryanofsky referenced this in commit 207a607e57 on May 20, 2025

ryanofsky referenced this in commit ba094b6fdd on May 21, 2025

refactor: Add ExecuteHTTPRPC function

Add ExecuteHTTPRPC to provide a way to execute an HTTP request without relying
on HTTPRequest and libevent types.

Behavior is not changing in any way, this is just moving code. This commit may
be easiest to review using git's --color-moved option.

038382f6db

ipc: Expose an RPC interface over the -ipcbind socket

This allows `bitcoin-cli` to connect to the node via IPC instead TCP to execute
RPC methods in an upcoming commit.

84c4e02389

ipc: Add MakeBasicInit function

Add a MakeBasicInit() function so simpler standalone IPC clients like
bitcoin-mine in #30437 and bitcoin-cli in #32297 that only initiate IPC
connections without exposing any IPC interfaces themselves can to avoid needing
to implement their own specialized interfaces::Init subclasses.

3efe67ef95

bitcoin-cli: Add -ipcconnect option

This implements an idea from Pieter Wuille <pieter@wuille.net>
https://github.com/bitcoin/bitcoin/issues/28722#issuecomment-2807026958 to
allow `bitcoin-cli` to connect to the node via IPC instead of TCP, if the
`ENABLE_IPC` cmake option is enabled and the node has been started with
`-ipcbind`.

The feature can be tested with:

build/bin/bitcoin-node -regtest -ipcbind=unix -debug=ipc
build/bin/bitcoin-cli -regtest -ipcconnect=unix -getinfo

The `-ipconnect` parameter can also be omitted, since this change also makes
`bitcoin-cli` prefer IPC over HTTP by default, and falling back to HTTP if an
IPC connection can't be established.

15f8e6b034

ipc: Improve -ipcconnect error checking

When an invalid socket path is passed to -ipcconnect, either because the path
exceeds the maximum socket length, or the path includes a directory component
which is not actually a directory, treat this the same as the same as the
socket refusing connections or not existing, instead of treating it like a more
serious I/O error and throwing a fatal exception.

This is needed to avoid CI errors after the following commit which adds a
functional test and uses -datadir paths exceeding the maximum socket length
when running in CI.

d520093379

test: add interface_ipc_cli.py testing bitcoin-cli -ipcconnect

Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>

6bf727fb88

ryanofsky force-pushed on May 22, 2025

ryanofsky commented at 6:04 pm on May 22, 2025: contributor

Great review! I improved a number of things in response to your comments.

Rebased 9ff5bc75659e0314f42c1477fc0c37ac0ad132fa -> 6bf727fb88009f99dab7c06bff7f8fb391147ac1 (pr/ipc-cli.5 -> pr/ipc-cli.6, compare) due to conflict with #28710, also made a number of improvements to -ipcbind documentation and -rpcwait behavior and other code as suggested, also split commits and cleaned up bitcoin-cli build rule and new tests.

DrahtBot removed the label Needs rebase on May 22, 2025

in src/bitcoin-cli.cpp:114 in 6bf727fb88

110@@ -108,6 +111,7 @@ static void SetupCliArgs(ArgsManager& argsman)
111     argsman.AddArg("-stdin", "Read extra arguments from standard input, one per line until EOF/Ctrl-D (recommended for sensitive information such as passphrases). When combined with -stdinrpcpass, the first line from standard input is used for the RPC password.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
112     argsman.AddArg("-stdinrpcpass", "Read RPC password from standard input as a single line. When combined with -stdin, the first line from standard input is used for the RPC password. When combined with -stdinwalletpassphrase, -stdinrpcpass consumes the first line, and -stdinwalletpassphrase consumes the second.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
113     argsman.AddArg("-stdinwalletpassphrase", "Read wallet passphrase from standard input as a single line. When combined with -stdin, the first line from standard input is used for the wallet passphrase.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
114+    argsman.AddArg("-ipcconnect=<address>", "Connect to bitcoin-node through IPC socket instead of TCP socket to execute requests. Valid <address> values are 'auto' to try to connect to default socket path at <datadir>/node.sock' but fall back to TCP if it is not available, 'unix' to connect to the default socket and fail if it isn't available, or 'unix:<socket path>' to connect to a socket at a nonstandard path. -noipcconnect can be specified to avoid attempting to use IPC at all. Default value: auto", ArgsManager::ALLOW_ANY, OptionsCategory::IPC);

DrahtBot commented at 9:02 am on May 23, 2025:

node.sock' -> node.sock [trailing single quote]

DrahtBot added the label Needs rebase on May 27, 2025

DrahtBot commented at 9:31 pm on May 27, 2025: contributor

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

bitcoin-cli: Add -ipcconnect option #32297

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

LLM Linter (✨ experimental)