UNIX sockets support for RPC #9919

pull laanwj wants to merge 4 commits into bitcoin:master from laanwj:2017_03_unix_socket changing 18 files +470 −52
  1. 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}
    
  2. laanwj added the label RPC/REST/ZMQ on Mar 4, 2017
  3. 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.
  4. laanwj force-pushed on Mar 4, 2017
  5. laanwj force-pushed on Mar 4, 2017
  6. gmaxwell commented at 6:44 pm on March 4, 2017: contributor
    Neat.
  7. 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

  8. 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
  9. 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.

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

  11. 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
    
  12. jonasschnelli commented at 8:30 am on March 6, 2017: contributor
    This is great! Concept ACK.
  13. laanwj force-pushed on Mar 6, 2017
  14. 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.
  15. laanwj force-pushed on Mar 6, 2017
  16. laanwj force-pushed on Mar 6, 2017
  17. laanwj force-pushed on Mar 6, 2017
  18. 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?

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

  20. 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.
  21. laanwj force-pushed on Mar 7, 2017
  22. laanwj force-pushed on Mar 9, 2017
  23. laanwj commented at 8:23 am on March 9, 2017: member
    Needed rebase, so rebased and squashed the squasme: commits.
  24. 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?

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

  26. laanwj added this to the milestone 0.15.0 on Mar 10, 2017
  27. laanwj force-pushed on Mar 12, 2017
  28. laanwj force-pushed on Mar 12, 2017
  29. laanwj commented at 12:07 pm on March 12, 2017: member
    Rebased (RPC tests logging changes #9768)
  30. 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
  31. tests: Run RPC tests optionally over UNIX socket 6218b11328
  32. tests: Add environment option to force RPC over TCP for tests f5acc052a8
  33. laanwj force-pushed on Mar 12, 2017
  34. 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
  35. laanwj force-pushed on Mar 12, 2017
  36. 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
  37. 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.

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

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

  40. 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.
  41. 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.

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

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

  44. 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/

  45. ryanofsky commented at 5:15 pm on May 8, 2017: member
    utACK 695e854a0444bb5c81b1422494451f17db0b5f1e, though code is pretty out of date
  46. 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)
  47. 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?
  48. 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.
  49. 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.
  50. 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.
  51. laanwj closed this on Jun 24, 2017

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

  53. MarcoFalke added the label Up for grabs on Oct 30, 2019
  54. MarcoFalke commented at 12:51 pm on October 30, 2019: member
    Marked as “up for grabs”
  55. 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.
  56. 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?
  57. MarcoFalke deleted a comment on May 15, 2021
  58. 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.

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

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

  61. ghazel referenced this in commit 2c12762212 on Mar 10, 2022
  62. DrahtBot locked this on Oct 30, 2022

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: 2024-10-04 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me