p2p: Bare minimum to support UNIX sockets #9979

pull laanwj wants to merge 6 commits into bitcoin:master from laanwj:2017_03_unix_socket_p2p changing 25 files +639 −65
  1. laanwj commented at 12:42 pm on March 12, 2017: member

    Two commit on top of #9919 (first adds the implementation, second adds a test). Make it possible to listen and connect to UNIX sockets for P2P. There are two main use cases for this:

    • Testing without possibility of port collisions
    • TOR hidden service listening with less risk of privacy leaks by not opening any internet ports

    Usage: specify :unix:<path> for bind, whitebind, connect, addnode.

  2. laanwj added the label P2P on Mar 12, 2017
  3. 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
  4. tests: Run RPC tests optionally over UNIX socket 6218b11328
  5. tests: Add environment option to force RPC over TCP for tests f5acc052a8
  6. 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
  7. p2p: Bare minimum to support UNIX sockets
    Make it possible to listen and connect to UNIX sockets for P2P.
    There are two main use cases for this:
    
    - Testing without possibility of port collisions
    - TOR hidden service listening with less risk of privacy leaks by not
      opening any internet ports
    
    Usage: specify `:unix:<path>` for `bind`, `whitebind`, `connect`,
    `addnode`.
    a9b732f86d
  8. p2p: Add test for P2P UNIX socket connection 9abf3c9da2
  9. laanwj force-pushed on Mar 12, 2017
  10. in src/httpserver.cpp:340 in 45fad466d1 outdated
    336@@ -331,6 +337,8 @@ static bool HTTPBindAddresses(struct evhttp* http)
    337     } else if (mapMultiArgs.count("-rpcbind")) { // Specific bind address
    338         const std::vector<std::string>& vbind = mapMultiArgs.at("-rpcbind");
    339         for (std::vector<std::string>::const_iterator i = vbind.begin(); i != vbind.end(); ++i) {
    340+            if (boost::starts_with(*i, RPC_ADDR_PREFIX_UNIX)) // Skip UNIX sockets here
    


    JeremyRubin commented at 2:47 pm on April 5, 2017:

    an easy way to replicate this without a new boost dep is:

    0if (!i->rfind(RPC_ADD_PREFIX_UNIX, 0))
    

    laanwj commented at 5:37 pm on April 5, 2017:
    I like boost::starts_with. It’s python idom (and many other languages) so easy to read. If we want to do without that, I think we should simply implement our own in terms of what you write.
  11. in src/bitcoin-cli.cpp:203 in 45fad466d1 outdated
    197@@ -195,8 +198,35 @@ UniValue CallRPC(const std::string& strMethod, const UniValue& params)
    198     // Obtain event base
    199     raii_event_base base = obtain_event_base();
    200 
    201-    // Synchronously look up hostname
    202-    raii_evhttp_connection evcon = obtain_evhttp_connection_base(base.get(), host, port);
    203+    raii_evhttp_connection evcon;
    204+    if (boost::starts_with(host, RPC_ADDR_PREFIX_UNIX)) {
    205+#if defined(HAVE_SOCKADDR_UN) && defined(LIBEVENT_EXPERIMENTAL)
    


    JeremyRubin commented at 2:53 pm on April 5, 2017:

    Might be better to have the ifdef define two different functions HandleUnixSocket.

    I’m always nervous with compiler directives inside functions, unless the code switched is ~1 line. Large ifdefs are in a couple places in this PR.


    laanwj commented at 5:41 pm on April 5, 2017:
    Indeed, factoring this out to a function makes sense
  12. in src/httpserver.cpp:552 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) {
    


    JeremyRubin commented at 3:18 pm on April 5, 2017:

    slight preference for something with ownership guarantees of

    0std::vector<std::unique_ptr<void>> cleanupUNIXSockets;
    1// must be non null value
    2cleanupUNIXSockets.emplace_back(1, [=](void*) { evunix_remove_socket(path); });
    3cleanupUNIXSockets.clear();
    

    so that sockets aren’t double removed.

    Maybe wrap the evunix stuff in a class and just keep a vector of those around?


    laanwj commented at 5:39 pm on April 5, 2017:
    Ok, makes sense
  13. in src/httpserver.cpp:671 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,
    


    JeremyRubin commented at 3:31 pm on April 5, 2017:

    recommend comment language (took me a minute to understand your comment because of ambiguity on here and that):

    We will get an address of localhost either via a unix socket or via a localhost peer. There is no sane way to return an address for a socket, so we always return localhost’s IPv6.

  14. JeremyRubin commented at 4:19 pm on April 5, 2017: contributor
    Not sure I’m familiar enough with libevent to give more than a concept ack. I read through it & it looks great. Concept Ack.
  15. in src/net.cpp:2113 in a9b732f86d outdated
    2108+    LogPrintf("P2P bound to %s\n", path.string());
    2109+
    2110+    // Listen for incoming connections
    2111+    if (listen(hListenSocket, SOMAXCONN) == SOCKET_ERROR)
    2112+    {
    2113+        strError = strprintf(_("Error: Listening for incoming connections failed (listen returned error %s)"), NetworkErrorString(WSAGetLastError()));
    


    ryanofsky commented at 6:03 pm on May 8, 2017:

    In commit “p2p: Bare minimum to support UNIX sockets”

    Any reason this error is translated while others are not?

  16. in src/net.cpp:2108 in a9b732f86d outdated
    2103+        strError = "Unable to bind to UNIX socket " + path.string();
    2104+        LogPrintf("%s\n", strError);
    2105+        return false;
    2106+    }
    2107+    SOCKET hListenSocket = (SOCKET)fd;
    2108+    LogPrintf("P2P bound to %s\n", path.string());
    


    ryanofsky commented at 6:04 pm on May 8, 2017:

    In commit “p2p: Bare minimum to support UNIX sockets”

    Maybe mention path is unix socket path

  17. ryanofsky commented at 6:11 pm on May 8, 2017: member

    utACK 9abf3c9da2c9e8e90b813ee379c7b8ff16a8f550, though many files are out of date.

    I’m wondering why this is labeled “bare minimum to support UNIX sockets” and when support seems pretty complete. Are there other p2p features over unix sockets that you’d want to support?

  18. laanwj commented at 2:03 pm on June 1, 2017: member

    Thanks for reviewing @JeremyRubin @jnewbery

    though many files are out of date.

    I somewhat dread having to rebase this, but hope to do so in the near future.

    I’m wondering why this is labeled “bare minimum to support UNIX sockets” and when support seems pretty complete. Are there other p2p features over unix sockets that you’d want to support?

    There’s no support in RPC calls yet, for example. Unix sockets can only be passed on the command line.

  19. jnewbery commented at 2:29 pm on June 1, 2017: member

    I somewhat dread having to rebase this, but hope to do so in the near future.

    Yes - rebase looks like it’s going to be a big job :(

    Let me know if I can help in any way - I’m definitely interested in getting Unix socket support for RPC.

  20. ryanofsky commented at 8:49 pm on June 1, 2017: member

    I somewhat dread having to rebase this, but hope to do so in the near future.

    I didn’t try rebasing, but the merge with master doesn’t look bad at all. The only C++ code conflicts are mapMultiArgs -> gArgs replacements. There is a huge scary looking conflict in the test framework util.py, but it just comes from the initialize_chain function moving to another file, and the changes there are minor.

  21. 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.
  22. laanwj closed this on Jun 24, 2017

  23. DrahtBot locked this on Feb 15, 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