UNIX sockets support for RPC #9919

laanwj · 2017-03-04T15:46:17Z

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 (libevent/libevent#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:

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

Client:

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

laanwj · 2017-03-04T16:00:20Z

src/Makefile.am

@@ -377,6 +381,9 @@ bitcoind_LDADD += $(BOOST_LIBS) $(BDB_LIBS) $(SSL_LIBS) $(CRYPTO_LIBS) $(MINIUPN

 # bitcoin-cli binary #
 bitcoin_cli_SOURCES = bitcoin-cli.cpp
+if BUILD_EVUNIX


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

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.

gmaxwell · 2017-03-04T18:44:57Z

Neat.

paveljanik · 2017-03-04T19:46:09Z

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

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

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

laanwj · 2017-03-04T21:05:12Z

@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 · 2017-03-04T21:21:32Z

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 <socket>| 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 <#9919 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnoHrbUK-rDkbYbVKJSmWq4tqSJyg64ks5ridILgaJpZM4MTHvN>.

laanwj · 2017-03-04T21:27:58Z

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 · 2017-03-05T10:43:13Z

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:

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

jonasschnelli · 2017-03-06T08:30:38Z

This is great!
Concept ACK.

laanwj · 2017-03-06T16:54:08Z

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.

jnewbery · 2017-03-06T23:41:16Z

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:

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

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 · 2017-03-07T07:03:02Z

@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 · 2017-03-09T08:23:27Z

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

jonasschnelli · 2017-03-09T08:32:28Z

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 · 2017-03-09T08:39:33Z

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 · 2017-03-12T12:07:19Z

Rebased (RPC tests logging changes #9768)

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.

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.

JeremyRubin · 2017-04-05T16:30:53Z

Oops, I accidentally did my review of this code on the other PR. See here #9979 (review)

ryanofsky

utACK 695e854, though code is pretty out of date

ryanofsky · 2017-05-05T22:18:32Z

src/support/evunix.cpp

+    if (!sockaddr_from_path(&addr, path)) {
+        return -1;
+    }
+    int fd = socket(AF_UNIX, SOCK_STREAM, 0);


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.

ryanofsky · 2017-05-08T15:11:04Z

configure.ac

+    addr.sun_family = AF_UNIX; ]])],
+ [ AC_MSG_RESULT(yes);
+   AC_DEFINE(HAVE_SOCKADDR_UN, 1,[Define this symbol if the sockaddr_un is available]) 
+   AM_CONDITIONAL([BUILD_EVUNIX],[true])


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.

ryanofsky · 2017-05-08T15:21:12Z

src/httpserver.cpp

+
+                // If path is not complete, it is interpreted relative to the data directory.
+                if (!name.is_complete()) {
+                    name = GetDataDir(true) / name;


In commit "rpc: UNIX sockets support"

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

ryanofsky · 2017-05-08T15:48:03Z

src/httpserver.cpp

+            /* Special: will get here for UNIX sockets. As we have no way to indicate that,
+             * just pass localhost IPv6.
+             */
+            address = "::1";


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.

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

ryanofsky · 2017-05-08T16:09:12Z

src/support/evunix.cpp

+{
+    std::string name = path.string();
+    if (name.size() >= sizeof(addr->sun_path)) {
+        /* Name too long */


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.

ryanofsky · 2017-05-08T16:23:01Z

src/support/evunix.cpp

+{
+    struct sockaddr_un peer_unix;
+    socklen_t peer_unix_len = sizeof(peer_unix);
+    if (getpeername(fd, (sockaddr*)&peer_unix, &peer_unix_len) == 0) {


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.

ryanofsky · 2017-05-08T16:31:00Z

qa/rpc-tests/test_framework/conninfo.py

+        '''
+        raise NotImplementedError
+
+class RPCConnectInfoTCP:


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.

ryanofsky · 2017-05-08T16:32:23Z

qa/rpc-tests/test_framework/util.py

    """
    Args:
-        url (str): URL of the RPC server to call
-        node_number (int): the node number (or id) that this calls to
+        urlinfo (RPCConnectInfo): Connection info of the RPC server to call


In commit "tests: Run RPC tests optionally over UNIX socket"

s/urlinfo/conninfo/

jnewbery · 2017-05-09T20:58:49Z

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: http: Make it possible to pass in a pre-existing connection libevent/libevent#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 [tests] Demostration/concept: Add a TestNode class to test_Framework.py #10082 are in. I really dislike having state for nodes floating around in util.py (eg the new conninfo_for() function)

theuni

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

theuni · 2017-06-02T23:26:15Z

src/Makefile.am

@@ -377,6 +381,9 @@ bitcoind_LDADD += $(BOOST_LIBS) $(BDB_LIBS) $(SSL_LIBS) $(CRYPTO_LIBS) $(MINIUPN

 # bitcoin-cli binary #
 bitcoin_cli_SOURCES = bitcoin-cli.cpp
+if BUILD_EVUNIX


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.

theuni · 2017-06-02T23:32:51Z

src/httpserver.cpp

+            /* Special: will get here for UNIX sockets. As we have no way to indicate that,
+             * just pass localhost IPv6.
+             */
+            address = "::1";


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

theuni · 2017-06-02T23:41:17Z

src/support/evunix.cpp

+        close(fd);
+        return -1;
+    }
+    evutil_make_socket_nonblocking(fd);


need to disable SIGPIPE here too?

theuni · 2017-06-02T23:54:45Z

src/httpserver.cpp

+#ifdef HAVE_SOCKADDR_UN
+    //! Clean up UNIX sockets on shutdown
+    for (const auto& path: cleanupUNIXSockets) {
+         evunix_remove_socket(path);


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

laanwj · 2017-06-24T11:31:03Z

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

Sjors · 2018-10-26T04:32:17Z

Concept ACK. c-lightning does this too FWIW.

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

maflcko · 2019-10-30T12:51:13Z

Marked as "up for grabs"

laanwj · 2019-10-30T12:56:47Z

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

vinipsmaker · 2021-05-15T01:12:42Z

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

michaelfolkson · 2021-09-22T15:30:25Z

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 · 2021-09-22T16:18:24Z

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 · 2021-09-22T16:44:07Z

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.

This patch adds functionality to pass a pre-existing connection as a bufferevent to `evhttp_connection_base_bufferevent_new`. When the bufferevent has an existing fd, the evcon starts in state `EVCON_IDLE` so that requests can be made immediately. I am using this in Bitcoin Core for doing HTTP requests over a UNIX socket (bitcoin/bitcoin#9919). It could also be useful in sandboxed environments such as cloudabi that cannot directly make connections but can pass around fds.

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

This was referenced Mar 4, 2017

http: Make it possible to pass in a pre-existing connection libevent/libevent#479

Closed

[RPC] Just an attemt to save some bandwidth #9893

Closed

laanwj commented Mar 4, 2017

View reviewed changes

laanwj force-pushed the 2017_03_unix_socket branch 2 times, most recently from d374f11 to 836d2d5 Compare March 4, 2017 16:16

laanwj force-pushed the 2017_03_unix_socket branch from 3bf02a9 to cd3d513 Compare March 6, 2017 16:52

laanwj force-pushed the 2017_03_unix_socket branch 3 times, most recently from 6f9286c to 971af66 Compare March 6, 2017 20:46

laanwj force-pushed the 2017_03_unix_socket branch 2 times, most recently from 5e20ec3 to 681d73c Compare March 9, 2017 08:23

laanwj added this to the 0.15.0 milestone Mar 10, 2017

laanwj force-pushed the 2017_03_unix_socket branch 2 times, most recently from 8394db2 to 2dd6914 Compare March 12, 2017 12:06

laanwj mentioned this pull request Mar 12, 2017

p2p: Bare minimum to support UNIX sockets #9979

Closed

laanwj added 3 commits March 12, 2017 13:59

rpc: UNIX sockets support

45fad46

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.

tests: Run RPC tests optionally over UNIX socket

6218b11

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

f5acc05

laanwj force-pushed the 2017_03_unix_socket branch from 2dd6914 to 6b2997e Compare March 12, 2017 14:01

laanwj force-pushed the 2017_03_unix_socket branch from 6b2997e to 695e854 Compare March 12, 2017 14:11

laanwj mentioned this pull request Mar 18, 2017

Unix Domain Sockets #5029

Open

ryanofsky reviewed May 8, 2017

View reviewed changes

theuni reviewed Jun 2, 2017

View reviewed changes

laanwj closed this Jun 24, 2017

JustinTArthur mentioned this pull request Aug 6, 2017

Minor optimizations to Bitcoin tx processing kyuupichan/electrumx#214

Merged

laanwj mentioned this pull request Nov 6, 2017

ZMQ: Add decodedtx topic for JSON tx publishing #11614

Closed

Sjors mentioned this pull request Nov 2, 2018

[WIP] 64 bit iOS device support #12557

Closed

laanwj mentioned this pull request Oct 30, 2019

gui: enable RPC server by default #17301

Closed

maflcko added the Up for grabs label Oct 30, 2019

laanwj mentioned this pull request Dec 15, 2019

SSL/TLS support for the HTTP(S) server #17747

Closed

bitcoin deleted a comment May 15, 2021

bitcoin locked and limited conversation to collaborators Oct 30, 2022

UNIX sockets support for RPC #9919

UNIX sockets support for RPC #9919

Conversation

laanwj commented Mar 4, 2017 • edited

Example

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaxwell commented Mar 4, 2017

paveljanik commented Mar 4, 2017 • edited

laanwj commented Mar 4, 2017

TheBlueMatt commented Mar 4, 2017 via email

laanwj commented Mar 4, 2017

laanwj commented Mar 5, 2017 • edited

jonasschnelli commented Mar 6, 2017

laanwj commented Mar 6, 2017

jnewbery commented Mar 6, 2017

laanwj commented Mar 7, 2017 • edited

laanwj commented Mar 7, 2017 • edited

laanwj commented Mar 9, 2017

jonasschnelli commented Mar 9, 2017

laanwj commented Mar 9, 2017 • edited

laanwj commented Mar 12, 2017

JeremyRubin commented Apr 5, 2017

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery commented May 9, 2017

theuni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj commented Jun 24, 2017

Sjors commented Oct 26, 2018

maflcko commented Oct 30, 2019

laanwj commented Oct 30, 2019

vinipsmaker commented May 15, 2021

michaelfolkson commented Sep 22, 2021

ryanofsky commented Sep 22, 2021

JeremyRubin commented Sep 22, 2021

laanwj commented Mar 4, 2017 •

edited

paveljanik commented Mar 4, 2017 •

edited

laanwj commented Mar 5, 2017 •

edited

laanwj commented Mar 7, 2017 •

edited

laanwj commented Mar 7, 2017 •

edited

laanwj commented Mar 9, 2017 •

edited