bitcoin-cli: Add -ipcconnect option #32297

pull ryanofsky wants to merge 3 commits into bitcoin:master from ryanofsky:pr/ipc-cli changing 24 files +407 −86
  1. 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.

  2. 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.
    ea98a42640
  3. 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.
    113b46d310
  4. 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
    ACK pinheadmz
    Concept ACK laanwj, jlest01, yancyribbens, shahsb, sipa

    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:

    • #32061 (Replace libevent with our own HTTP and socket-handling implementation by pinheadmz)
    • #31375 (multiprocess: Add bitcoin wrapper executable by ryanofsky)
    • #30437 (multiprocess: 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)
    • #28710 (Remove the legacy wallet and BDB dependency by achow101)

    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:

    default socket path at /node.sock’ → default socket path at /node.sock

  5. ryanofsky force-pushed on Apr 17, 2025
  6. DrahtBot added the label CI failed on Apr 17, 2025
  7. laanwj added the label RPC/REST/ZMQ on Apr 17, 2025
  8. 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

  9. 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.
  10. 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.
  11. 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?
  12. 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.

  13. test: add interface_ipc_cli.py testing bitcoin-cli -ipcconnect 
    Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
    9ff5bc7565
  14. ryanofsky force-pushed on Apr 18, 2025
  15. 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
  16. ryanofsky force-pushed on Apr 18, 2025
  17. ryanofsky marked this as ready for review on Apr 18, 2025
  18. DrahtBot removed the label CI failed on Apr 18, 2025
  19. yancyribbens commented at 12:37 pm on April 19, 2025: contributor
    Concept ACK
  20. 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,…”
  21. ryanofsky force-pushed on Apr 21, 2025
  22. 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?
  23. 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.

  24. shahsb commented at 5:03 am on April 23, 2025: none
    Concept ACK
  25. 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.

  26. sipa commented at 2:45 pm on April 30, 2025: member
    Concept ACK
  27. in src/httprpc.h:27 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?

  28. in src/httprpc.cpp:163 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.

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

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

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

  32. 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?

  33. in test/functional/interface_ipc_cli.py:30 in 9ff5bc7565 
    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

  34. pinheadmz approved
  35. 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

  36. DrahtBot requested review from sipa on May 1, 2025
  37. DrahtBot requested review from laanwj on May 1, 2025
  38. DrahtBot requested review from shahsb on May 1, 2025
  39. DrahtBot commented at 2:19 pm on May 7, 2025: contributor

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

  40. DrahtBot added the label Needs rebase on May 7, 2025


ryanofsky DrahtBot laanwj jlest01 pinheadmz yancyribbens maflcko luke-jr shahsb sipa


laanwj sipa shahsb

Labels
RPC/REST/ZMQ Needs rebase

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-05-09 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me