UNIX sockets support for RPC

laanwj commented at 3:46 pm on March 4, 2017: member

Add functionality for RPC over UNIX sockets, on platforms that support UNIX sockets. This is specified with :unix:/path/to/socket to -rpcbind and -rpcconnect. By default the socket path is relative to the data directory.

For the server side this works as-is.

For the client side (bitcoin-cli) this requires a small patch to libevent to be able to pass in a file descriptor of an existing connection (https://github.com/libevent/libevent/pull/479).

Even without the client change this could be useful though. For example the Python RPC tests could connect through UNIX socket to avoid port collisions ~~(TODO: make a Python example)~~. This has been implemented.

Also the overhead of using UNIX sockets should be lower than using local TCP (see https://stackoverflow.com/questions/14973942/performance-tcp-loopback-connection-vs-unix-domain-socket).

Example

Server:

0src/bitcoind -printtoconsole -datadir=/store/tmp/testbtc -connect=0 -debug=rpc -debug=http -rpcbind=:unix:socket
1...
22017-03-05 10:29:31 Binding RPC on UNIX socket /store/tmp/testbtc/socket
3...

Client:

0src/bitcoin-cli -rpcconnect=":unix:socket" -datadir=/store/tmp/testbtc getinfo
1{
2  "version": 149900,
3  ...
4}

laanwj added the label RPC/REST/ZMQ on Mar 4, 2017

in src/Makefile.am: in 2f6a34c40c outdated

380@@ -377,6 +381,9 @@ bitcoind_LDADD += $(BOOST_LIBS) $(BDB_LIBS) $(SSL_LIBS) $(CRYPTO_LIBS) $(MINIUPN
381 
382 # bitcoin-cli binary #
383 bitcoin_cli_SOURCES = bitcoin-cli.cpp
384+if BUILD_EVUNIX

laanwj commented at 4:00 pm on March 4, 2017:

@theuni This is pretty ugly, but I don’t quite know how to do this otherwise. My first intuition was to add the file to libbitcoin_util_a_SOURCES, however these can’t depend on libevent as that is shared between all executables. There is no library that is shared between just bitcoind and bitcoin-cli and adding one just for this seems overkill.

theuni commented at 11:26 pm on June 2, 2017:

I think this is OK. I think we’ll end up with a few wrappers for libevent, so moving those to their own internal helper lib will make sense.

laanwj force-pushed on Mar 4, 2017

gmaxwell commented at 6:44 pm on March 4, 2017: contributor

Neat.

paveljanik commented at 7:46 pm on March 4, 2017: contributor

This does not compile with libevent2-2.0.22 which is OK with the current master.

0bitcoin-cli.cpp:214:13: error: use of undeclared identifier `evhttp_connection_base_bufferevent_new'
1            evhttp_connection_base_bufferevent_new(base.get(), __null, bev, "", 0)
2            ^
31 error generated.

https://github.com/libevent/libevent/blob/e7ff4ef2b4fc950a765008c18e74281cdb5e7668/whatsnew-2.1.txt#L422

laanwj commented at 9:05 pm on March 4, 2017: member

@paveljanik Yes, that needs a version #ifdef, that call doesn’t exist in older versions of libevent, and on newer ones (until my patch lands) it ignores the current connection and tries to open a new one so that trick doesn’t work. I’d recommend testing with my patched libevent. Alternatively comment out the bitcoin-cli change and test connecting with something else:

nc -U <socket> works, for example
curl also has a --unix-socket parameter, but haven’t tried it yet

TheBlueMatt commented at 9:21 pm on March 4, 2017: member

For merge we should probably just not have the client version, then?

Still, this is cool - lots of better auth controls can be applied to sockets than IPs :)

On 03/04/17 21:05, Wladimir J. van der Laan wrote:

@paveljanik https://github.com/paveljanik Yes, that needs a version |#ifdef|, that call doesn’t exist in older versions of libevent, and on newer ones (until my patch lands) it ignores the current connection and tries to open a new one so that trick doesn’t work. I’d recommend testing with my patched libevent. Alternatively comment out the bitcoin-cli change and test connecting with something else:

|nc -U | works, for example

curl also has a |–unix-socket| parameter, but haven’t tried it yet

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9919#issuecomment-284180780, or mute the thread https://github.com/notifications/unsubscribe-auth/AAnoHrbUK-rDkbYbVKJSmWq4tqSJyg64ks5ridILgaJpZM4MTHvN.

laanwj commented at 9:27 pm on March 4, 2017: member

For merge we should probably just not have the client version, then?

Would prefer putting it behind #ifdef EXPERIMENTAL_LIBEVENT or such, with a comment that it can be replaced with the appropriate version comparison when it lands in a version. That way, people that compile their own stuff can use it already.

laanwj commented at 10:43 am on March 5, 2017: member

Still, this is cool - lots of better auth controls can be applied to sockets than IPs :)

Indeed. Default permissions of the socket will be 700 due to our umask setting (can be overridden with -sysperms), which seems okay:

0srwx------  1 user user          0 Mar  5 11:40 socket

jonasschnelli commented at 8:30 am on March 6, 2017: contributor

This is great! Concept ACK.

laanwj force-pushed on Mar 6, 2017

laanwj commented at 4:54 pm on March 6, 2017: member

Added a commit that makes the tests’ RPC run over UNIX sockets on platforms that support it, except for a few tests that make explicit assumptions about running over TCP/IP.

laanwj force-pushed on Mar 6, 2017

jnewbery commented at 11:41 pm on March 6, 2017: member

I’ve been trying to test this locally by running the qa tests. Something that I do quite often is to run the tests with –noshutdown, wait for the test to fail, and use bitcoin-cli to interactively run RPCs on the test nodes:

bitcoin.conf -conf=<test temp directory>/node<x>/bitcoin.conf

With this PR, I’m not able to do that anymore because the conf file doesn’t contain the username/password, and even when I add them back in, I get the error:

 0/tmp/user/1000/test_6o70u2b/28404/node0
 1→ ~/bitcoin/src/bitcoin-cli -rpcconnect=":unix:rpc_socket" -datadir=/tmp/user/1000/test_6o70u2b/28404/node0 help
 2[warn] evhttp_connection_connect_: connection to "" failed: No such file or directory
 3error: couldn't connect to server: unknown (code -1)
 4(make sure server is running and you are connecting to the correct RPC port)
 5/tmp/user/1000/test_6o70u2b/28404/node0
 6→ ll
 7total 16
 8drwxrwxr-x 3 ubuntu ubuntu 4096 Mar  6 23:31 ./
 9drwxrwxr-x 4 ubuntu ubuntu 4096 Mar  6 23:11 ../
10-rw-rw-r-- 1 ubuntu ubuntu   79 Mar  6 23:20 bitcoin.conf
11drwx------ 5 ubuntu ubuntu 4096 Mar  6 23:26 regtest/
12srwx------ 1 ubuntu ubuntu    0 Mar  6 23:11 rpc_socket=

Do you have a better way of connecting to a test node during qa testing?

laanwj commented at 5:59 am on March 7, 2017: member

With this PR, I’m not able to do that anymore because the conf file doesn’t contain the username/password, and even when I add them back in, I get the error:

Right, I could change it to add the options (including -rpcconnect) to the bitcoin.conf.

And even when I add them back in, I get the error:

The above should work, though. That’s exactly how I’ve been testing it. The connection failure is strange. Did you patch libevent so that bitcoin-cli is able to connect to UNIX sockets? (you did add LIBEVENT_EXPERIMENTAL otherwise you’d be getting a RPC was asked to connect to a UNIX socket. However, the version of libevent that this utility was compiled with has no support for that. error when specifying :unix:)

Do you have a better way of connecting to a test node during qa testing?

I’ll add an environment variable to force running the RPC tests over TCP.

laanwj commented at 7:03 am on March 7, 2017: member

@jnewbery Okay, added two commits:

add BITCOIN_TEST_RPC_TCP environment variable to force tests to use TCP instead of UNIX sockets for RPC
write connection info to bitcoin.conf instead of providing it on the command line. -conf should just work, also for UNIX sockets, if your client supports it.

laanwj force-pushed on Mar 7, 2017

laanwj force-pushed on Mar 9, 2017

laanwj commented at 8:23 am on March 9, 2017: member

Needed rebase, so rebased and squashed the squasme: commits.

jonasschnelli commented at 8:32 am on March 9, 2017: contributor

F.I.Y: I wanted to test this via binaries built in gitian. I could start the daemon (wanted to test in conjunction with Q): ./bitcoin-0.14.99/bin/bitcoin-qt -debug=rpc -debug=http -rpcbind=nix:socket --regtest

However the CLI responded with: error: RPC was asked to connect to a UNIX socket. However, the version of libevent that this utility was compiled with has no support for that.

Maybe we should bump libevent in the dependencies directory?

laanwj commented at 8:39 am on March 9, 2017: member

However the CLI responded with:

Yes, that’s expected. bitcoin-cli support needs this patch: https://github.com/laanwj/libevent/commit/403b793771341460b262edb1abc31fae267174bd.patch as well as defining -DLIBEVENT_EXPERIMENTAL

Without, you should still be able to use the socket with other clients, for example from Python or with curl. The server-side support is not dependent on that patch.

This could be done as part of the depends system, but rather not in this PR as it is a wholly separate discussion.

laanwj added this to the milestone 0.15.0 on Mar 10, 2017

laanwj force-pushed on Mar 12, 2017

laanwj commented at 12:07 pm on March 12, 2017: member

Rebased (RPC tests logging changes #9768)

rpc: UNIX sockets support

Add functionality for RPC over UNIX sockets, on platforms that support
UNIX sockets. This is specified with `:unix:/path/to/socket` to
`-rpcbind` and `-rpcconnect`. By default the socket path is relative to the
data directory.

45fad466d1

tests: Run RPC tests optionally over UNIX socket 6218b11328

tests: Add environment option to force RPC over TCP for tests f5acc052a8

laanwj force-pushed on Mar 12, 2017

rpc: More robust localhost-UNIX detection

evhttp has no direct support for UNIX sockets thus
is not able to recognize UNIX peers. Add custom code in
HTTPRequest::GetPeer and evunix to recognize these connections
as coming from localhost.

The previous solution did not work on some libevent versions.

695e854a04

laanwj force-pushed on Mar 12, 2017

JeremyRubin commented at 4:30 pm on April 5, 2017: contributor

Oops, I accidentally did my review of this code on the other PR. See here #9979#pullrequestreview-31071186

in src/support/evunix.cpp:44 in 45fad466d1 outdated

39+{
40+    struct sockaddr_un addr;
41+    if (!sockaddr_from_path(&addr, path)) {
42+        return -1;
43+    }
44+    int fd = socket(AF_UNIX, SOCK_STREAM, 0);

ryanofsky commented at 10:18 pm on May 5, 2017:

In commit “rpc: UNIX sockets support”

Maybe add a sock_from_path function to dedup code between evunix_bind_fd & evunix_connect_fd a little more.

in configure.ac:612 in 45fad466d1 outdated

607+  #include <sys/un.h>]],
608+ [[ struct sockaddr_un addr;
609+    addr.sun_family = AF_UNIX; ]])],
610+ [ AC_MSG_RESULT(yes);
611+   AC_DEFINE(HAVE_SOCKADDR_UN, 1,[Define this symbol if the sockaddr_un is available]) 
612+   AM_CONDITIONAL([BUILD_EVUNIX],[true])

ryanofsky commented at 3:11 pm on May 8, 2017:

In commit “rpc: UNIX sockets support”

Naming of BUILD_EVUNIX seems a little unusual. Would expect something more like ENABLE_UNIX_SOCKETS (to be similar to ENABLE_WALLET, ENABLE_QT, ENABLE_ZMQ, etc).

Maybe more to the point though, since BUILD_EVUNIX is only used to control building of a single source file, maybe consider not defining it as build variable at all and just using #if HAVE_SOCKADDR_UN in the c++ source. I think this makes sense since you are already using HAVE_SOCKADDR_UN to avoid calling evunix functions, so you might as well use the same variable to avoid defining those functions.

in src/httpserver.cpp:376 in 45fad466d1 outdated

371+                int fd;
372+                evhttp_bound_socket *bind_handle = 0;
373+
374+                // If path is not complete, it is interpreted relative to the data directory.
375+                if (!name.is_complete()) {
376+                    name = GetDataDir(true) / name;

ryanofsky commented at 3:21 pm on May 8, 2017:

In commit “rpc: UNIX sockets support”

Maybe consider deduping the path manipulation code with the same code in bitcoin-cli

in src/httpserver.cpp:674 in 45fad466d1 outdated

666@@ -619,6 +667,12 @@ CService HTTPRequest::GetPeer()
667         const char* address = "";
668         uint16_t port = 0;
669         evhttp_connection_get_peer(con, (char**)&address, &port);
670+        if (!strcmp(address, "localhost")) {
671+            /* Special: will get here for UNIX sockets. As we have no way to indicate that,
672+             * just pass localhost IPv6.
673+             */
674+            address = "::1";

ryanofsky commented at 3:48 pm on May 8, 2017:

In commit “rpc: UNIX sockets support”

Seems like this instead of emulating an NET_IPV6 address, this should have it’s own network type in netaddress.h similar to NET_TOR.

theuni commented at 11:32 pm on June 2, 2017:

+1. This seems like another good use-case for NET_INTERNAL from #10446 (or something similar). As a bonus, that would allow us to use associative containers for unix socket addresses, since each path would have its own hash.

in src/support/evunix.cpp:17 in 45fad466d1 outdated

12+/** Helper function to initialize a sockaddr_un from a path */
13+static bool sockaddr_from_path(struct sockaddr_un *addr, const boost::filesystem::path &path)
14+{
15+    std::string name = path.string();
16+    if (name.size() >= sizeof(addr->sun_path)) {
17+        /* Name too long */

ryanofsky commented at 4:09 pm on May 8, 2017:

In commit “rpc: UNIX sockets support”

Would it be possible to throw an exception here? Since the path comes from the user’s configuration it seems like it would be better to provide a specific error about the path than a generic connect or bind error.

in src/support/evunix.cpp:91 in 695e854a04

83@@ -84,6 +84,18 @@ int evunix_connect_fd(const boost::filesystem::path &path)
84     return fd;
85 }
86 
87+bool evunix_is_conn_from_unix_fd(int fd)
88+{
89+    struct sockaddr_un peer_unix;
90+    socklen_t peer_unix_len = sizeof(peer_unix);
91+    if (getpeername(fd, (sockaddr*)&peer_unix, &peer_unix_len) == 0) {

ryanofsky commented at 4:23 pm on May 8, 2017:

In commit “rpc: More robust localhost-UNIX detection”

Would it be better to use getsockname than getpeername for this? Seems like maybe that would work even on a socket that isn’t connected.

in qa/rpc-tests/test_framework/conninfo.py:31 in 6218b11328 outdated

26+        '''
27+        Return arguments for setting up bitcoind.
28+        '''
29+        raise NotImplementedError
30+
31+class RPCConnectInfoTCP:

ryanofsky commented at 4:31 pm on May 8, 2017:

In commit “tests: Run RPC tests optionally over UNIX socket”

Probably should inherit from RPCConnectInfo. Same with RPCConnectInfoUNIX class below. Doesn’t really affect anything, but might make generated documentation / help messages a little better.

in qa/rpc-tests/test_framework/util.py:77 in 6218b11328 outdated

75+def get_rpc_proxy(conninfo, timeout=None):
76     """
77     Args:
78-        url (str): URL of the RPC server to call
79-        node_number (int): the node number (or id) that this calls to
80+        urlinfo (RPCConnectInfo): Connection info of the RPC server to call

ryanofsky commented at 4:32 pm on May 8, 2017:

In commit “tests: Run RPC tests optionally over UNIX socket”

s/urlinfo/conninfo/

ryanofsky commented at 5:15 pm on May 8, 2017: member

utACK 695e854a0444bb5c81b1422494451f17db0b5f1e, though code is pretty out of date

jnewbery commented at 8:58 pm on May 9, 2017: member

I’ve had a quick read through the changes to the functional tests, but I’ll defer doing a full review until this is rebased. A few general comments:

I’d prefer a command line argument over and environment variable for BITCOIN_TEST_RPC_TCP, partly because adding a command line argument means the help text will be updated and the option will be better documented.
there’s a response to your libevent PR here: https://github.com/libevent/libevent/pull/479. It’d be good to the functionality merged into libevent so we can avoid the LIBEVENT_EXPERIMENTAL stuff
I also think it would be better to make the changes to the functional test framework after TestNode changes in #10082 are in. I really dislike having state for nodes floating around in util.py (eg the new conninfo_for() function)

in src/support/evunix.cpp:83 in 45fad466d1 outdated

78+    }
79+    if (connect(fd, (struct sockaddr*)&addr, sizeof(addr)) < 0) {
80+        close(fd);
81+        return -1;
82+    }
83+    evutil_make_socket_nonblocking(fd);

theuni commented at 11:41 pm on June 2, 2017:

need to disable SIGPIPE here too?

in src/httpserver.cpp:553 in 45fad466d1 outdated

546@@ -505,6 +547,12 @@ void StopHTTPServer()
547         event_base_free(eventBase);
548         eventBase = 0;
549     }
550+#ifdef HAVE_SOCKADDR_UN
551+    //! Clean up UNIX sockets on shutdown
552+    for (const auto& path: cleanupUNIXSockets) {
553+         evunix_remove_socket(path);

theuni commented at 11:54 pm on June 2, 2017:

should probably .clear() in case this gets called twice.

theuni commented at 11:55 pm on June 2, 2017: member

Concept ACK. I added a few comments, and @ryanofsky beat me to several others.

laanwj commented at 11:31 am on June 24, 2017: member

Closing this for now. I’ll probably pick it up again for 0.16.

laanwj closed this on Jun 24, 2017

Sjors commented at 4:32 am on October 26, 2018: member

Concept ACK. c-lightning does this too FWIW.

This was briefly discussed during the meeting today. Tag as Up for Grabs?

MarcoFalke added the label Up for grabs on Oct 30, 2019

MarcoFalke commented at 12:51 pm on October 30, 2019: member

Marked as “up for grabs”

laanwj commented at 12:56 pm on October 30, 2019: member

The server side should be easy to pick up. As for the client side I have opened a libevent issue to track this here: https://github.com/libevent/libevent/issues/907 Unfortunately there is very little interest from their side for this.

vinipsmaker commented at 1:12 am on May 15, 2021: none

Does it have to be HTTP? Would it be easier to implement if it was pure (line delimited) JSON-RPC over UNIX domain sockets?

MarcoFalke deleted a comment on May 15, 2021

michaelfolkson commented at 3:30 pm on September 22, 2021: contributor

Does it have to be HTTP? Would it be easier to implement if it was pure (line delimited) JSON-RPC over UNIX domain sockets?

This was discussed today on IRC. In summary, this is what c-lightning does for its UNIX socket support and what Core could potentially do in future for UNIX socket support as an alternative approach to getting a PR merged in libevent. Any non HTTP based UNIX socket support would need to be in addition to the existing HTTP based JSON-RPC rather than as a replacement.

ryanofsky commented at 4:18 pm on September 22, 2021: member

i don’t see any problem personally with adding support for unix sockets rpc server / client that uses plain json-rpc lines, if someone wants to work on that

This seems ok, but without URLs, you would have to decide how to handle wallet endpoints. Maybe create different sockets for different wallets like <datadir>/wallets/<walletname>/rpc.sock, or use one socket and accept an extra wallet RPC param.

JeremyRubin commented at 4:44 pm on September 22, 2021: contributor

Any non HTTP based UNIX socket support would need to be in addition to the existing HTTP based JSON-RPC rather than as a replacement.

contestable, i think it would be great to deprecate then remove http support & leave it up to external proxy services.

ghazel referenced this in commit 2c12762212 on Mar 10, 2022

DrahtBot locked this on Oct 30, 2022

UNIX sockets support for RPC #9919

Example