ZMQ: Support UNIX domain sockets

pinheadmz commented at 8:08 pm on May 16, 2023: member

This is a follow-up to #27375, allowing ZMQ notifications to be published to a UNIX domain socket.

Fortunately, libzmq handles unix sockets already, all we really have to do to support it is allow the format in the actual option.

libzmq uses the prefix ipc:// as opposed to unix: which is used by Tor and now also by bitcoind so we need to switch that internally.

As far as I can tell, LND supports ipc:// and unix:// (notice the double slashes).

With this patch, LND can connect to bitcoind using unix sockets:

Example:

bitcoin.conf:

0zmqpubrawblock=unix:/tmp/zmqsb
1zmqpubrawtx=unix:/tmp/zmqst

lnd.conf:

0bitcoind.zmqpubrawblock=ipc:///tmp/zmqsb
1bitcoind.zmqpubrawtx=ipc:///tmp/zmqst

DrahtBot commented at 8:08 pm on May 16, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	laanwj, guggero, tdb3, achow101
Concept ACK	TheCharlatan

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:

#29432 (Stratum v2 Template Provider (take 3) by Sjors)

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.

DrahtBot added the label RPC/REST/ZMQ on May 16, 2023

TheCharlatan commented at 9:07 pm on May 16, 2023: contributor

Concept ACK

DrahtBot added the label CI failed on May 16, 2023

pinheadmz force-pushed on May 17, 2023

DrahtBot removed the label CI failed on May 17, 2023

DrahtBot added the label Needs rebase on May 30, 2023

pinheadmz commented at 8:02 pm on May 30, 2023: member

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

Relax bot! sheesh. I’m gonna wait until #27375 gets approved

in src/init.cpp:1325 in 77019cd6ec outdated

1308             if (!SplitHostPort(socket_addr, port_out, host_out)) {
1309 #if HAVE_SOCKADDR_UN
1310-                // Allow unix domain sockets for -proxy and -onion e.g. unix:/some/file/path
1311-                if ((port_option != "-proxy" && port_option != "-onion") || socket_addr.find(ADDR_PREFIX_UNIX) != 0) {
1312-                    return InitError(InvalidPortErrMsg(port_option, socket_addr));
1313+                // Allow unix domain sockets for some options e.g. unix:/some/file/path

luke-jr commented at 9:14 pm on July 27, 2023:

Prefer we just allow it for everything and error somewhere else

in src/netaddress.h:38 in 77019cd6ec outdated

32@@ -33,8 +33,9 @@
33  */
34 static constexpr int ADDRV2_FORMAT = 0x20000000;
35 
36-/** Prefix for unix domain socket addresses (which are local filesystem paths) */
37-const std::string ADDR_PREFIX_UNIX = "unix:";
38+/** Prefixes for unix domain socket addresses (which are local filesystem paths) */
39+const std::string ADDR_PREFIX_UNIX = "unix:"; // example unix:/root/path/to/file
40+const std::string ADDR_PREFIX_IPC = "ipc://"; // example icp:///root/path/to/file

luke-jr commented at 9:16 pm on July 27, 2023:

ipc*

pinheadmz commented at 1:55 pm on October 25, 2023: member

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Waiting for https://github.com/bitcoin/bitcoin/pull/27375

achow101 referenced this in commit 0ed2c130e7 on Mar 13, 2024

pinheadmz force-pushed on Mar 13, 2024

DrahtBot removed the label Needs rebase on Mar 13, 2024

pinheadmz force-pushed on Mar 13, 2024

DrahtBot commented at 4:44 pm on March 13, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/22614485115

DrahtBot added the label CI failed on Mar 13, 2024

pinheadmz marked this as ready for review on Mar 13, 2024

in src/init.cpp:1316 in 7b4c7bdb35 outdated

1336+        {"-whitebind",              false},
1337+        {"-zmqpubhashblock",        true},
1338+        {"-zmqpubhashtx",           true},
1339+        {"-zmqpubrawblock",         true},
1340+        {"-zmqpubrawtx",            true},
1341+        {"-zmqpubsequence",         true}

pinheadmz commented at 4:57 pm on March 13, 2024:

unix socket support for i2psam and rpcbind will be implemented in future work, so this little matrix is anticipating that

DrahtBot removed the label CI failed on Mar 13, 2024

in src/init.cpp:1326 in 7b4c7bdb35 outdated

1350 #if HAVE_SOCKADDR_UN
1351-                // Allow unix domain sockets for -proxy and -onion e.g. unix:/some/file/path
1352-                if ((port_option != "-proxy" && port_option != "-onion") || socket_addr.find(ADDR_PREFIX_UNIX) != 0) {
1353-                    return InitError(InvalidPortErrMsg(port_option, socket_addr));
1354+                // Allow unix domain sockets for some options e.g. unix:/some/file/path
1355+                if (!unix || socket_addr.find(ADDR_PREFIX_UNIX) != 0) {

tdb3 commented at 4:19 am on March 15, 2024:

If adjusted to:

 0diff --git a/src/init.cpp b/src/init.cpp
 1index a592e7d763..f9bc33d57f 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -1336,7 +1336,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 5             if (!SplitHostPort(socket_addr, port_out, host_out)) {
 6 #if HAVE_SOCKADDR_UN
 7                 // Allow unix domain sockets for some options e.g. unix:/some/file/path
 8-                if (!unix || socket_addr.find(ADDR_PREFIX_UNIX) != 0) {
 9+                if (!unix || (socket_addr.find(ADDR_PREFIX_UNIX) != 0 && socket_addr.find(ADDR_PREFIX_IPC) != 0)) {
10                     return InitError(InvalidPortErrMsg(arg, socket_addr));
11                 }
12 #else

it seems like support for both forms (unix: and ipc://) might be achievable.

If I’m not missing something, https://github.com/bitcoin/bitcoin/blob/7b4c7bdb35eda125781d14535a69dc6b9898069d/src/zmq/zmqnotificationinterface.cpp#L63 would allow ipc:// to pass through (while also normalizing unix: to ipc://).

Was there a reason/rationale to use only unix: rather than both forms? I would guess that folks familiar with ZMQ would be used to using ipc://, so including support for that form may be more straightforward for them.

I’m ok with either approach (supporting only unix: or supporting both unix: and ipc://).

pinheadmz commented at 9:34 pm on March 15, 2024:

Heh yeah actually the branch used to be exactly like this but after #27375 got revised I simplified it. I’m open to discuss options, we could support any combination of unix: unix:// ipc: ipc:// or use a regex to cover them all. The motivation for keeping it simple isn’t super strong but goes like this:

Tor and libzmq only support one prefix each, and not even the same one. Meanwhile LND supports another set of prefixes.
We would also need to update IsUnixSocketPath() to be more flexible
We’d also need a new function GetUnixPathFromArg() or something to replace lines like this that only expect one prefix:

https://github.com/bitcoin/bitcoin/blob/015ac13dcc964a31ef06dfdb565f88f901607f0e/src/netbase.cpp#L649

tdb3 commented at 1:49 pm on March 16, 2024:

Ah, thanks for the explanation. TLDR: After some thought, my vote is support only the unix: prefix.

Some thoughts for posterity. Seems like the following are (generally) the case:

Supporting a straightforward and uniform way to indicate usage of UNIX domain sockets makes sense (and can be less confusing to the user). Currently Core supports unix domain sockets with -proxy and -onion (thanks to PR 27375!), and the goal is to add it for more areas, so whatever prefix is chosen should be reasonable for each of the options that can use it.
- In the long run, support for UNIX domain sockets would be added for ZMQ (this PR), with I2P SAM proxy, on Windows, and in the GUI. I’m not seeing use of a single prefix unix: being an issue for any of these applications (even in Windows, where the socket type is still called AF_UNIX AFAIK).
- Users typically experience confusion when there are many possible ways to accomplish the goal (e.g. “Wait, which prefix should I use?”)
- There would be a ripple effect for guides written in the future (e.g. some tutorials/guides would state to use one prefix, and some would state to use another). This could add confusion for users.
Some prefixes make more sense for some options than others. unix: seems to be the most straightforward and universally applicable. ipc:// could possibly make sense for ZMQ options since ZMQ uses the prefix, but it wouldn’t make much sense for -proxy with Tor, so may even be a little confusing to support it there.
Supporting multiple indicators (e.g. unix:, ipc://, etc.) simultaneously means more code and more complexity to maintain. The added value for users accustomed to using ipc:// with non-Bitcoin ZMQ application seems minimal compared to the effort for code maintenance. These users could simply start using unix: for Bitcoin application.
More code and more complexity typically means more bugs, vulns, more review burden, etc. If there was a massive value gain it might be worth the effort, but doesn’t like that’s the case here.

laanwj commented at 12:12 pm on April 16, 2024:

fwiw, i prefer having only a single consistent syntax for unix sockets throughout the RPC settings. Outside of zmq i’ve never heard of a ipc:// scheme. It’s better to just document this.

tdb3 commented at 4:30 am on March 15, 2024: contributor

ACK 7b4c7bdb35eda125781d14535a69dc6b9898069d Nice addition to the suite of unix domain socket support (e.g. PR #27375, etc.)!

Performed code review. Built and ran all functional tests (all passed, including interface_zmq.py)

Since bitcoind uses the unix: form, the unused form (ipc:///tmp/mysocket) was tested and bitcoind startup errors out (as expected). Inline nit discusses this.

Another malformed format (e.g. notvalid:///tmp/sock) was also tested and errors out (as expected).

It was too tempting not to try LND with ZMQ unix domain sockets! Ran LND 0.17.4 and bitcoind (this PR branch) on signet, configured as follows:

bitcoin.conf:

0...
1zmqpubrawblock=unix:/tmp/zmqpubrawblock
2zmqpubrawtx=unix:/tmp/zmqpubrawtx
3...

(also ran bitcoind with ipc:///tmp/zmqpubrawblock and ipc:///tmp/zmqpubrawtx as described in the nit comment and that was successful as well)

lnd.conf:

0...
1bitcoind.zmqpubrawblock=ipc:///tmp/zmqpubrawblock
2bitcoind.zmqpubrawtx=ipc:///tmp/zmqpubrawtx
3...

Using https://github.com/mechpen/sockdump, unix socket traffic was sniffed on each socket file. ZMQ traffic was observed (e.g. rawblock, rawtx), so seems to be working well! Great work!

DrahtBot requested review from TheCharlatan on Mar 15, 2024

Sjors commented at 9:52 am on March 19, 2024: member

@wiz you may find this interesting, since Mempool also uses ZMQ

laanwj requested review from laanwj on Apr 13, 2024

in src/netbase.h:32 in 7b4c7bdb35 outdated

26@@ -27,8 +27,9 @@ static const int DEFAULT_CONNECT_TIMEOUT = 5000;
27 //! -dns default
28 static const int DEFAULT_NAME_LOOKUP = true;
29 
30-/** Prefix for unix domain socket addresses (which are local filesystem paths) */
31-const std::string ADDR_PREFIX_UNIX = "unix:";
32+/** Prefixes for unix domain socket addresses (which are local filesystem paths) */
33+const std::string ADDR_PREFIX_UNIX = "unix:"; // used by bitcoind, example "unix:/root/path/to/file"
34+const std::string ADDR_PREFIX_IPC = "ipc://"; // used by libzmq, example "ipc:///root/path/to/file"

laanwj commented at 12:24 pm on April 16, 2024:

Let’s move this prefix to some zmq specific header or implementation file, no need for it to be in netbase.h.

in src/zmq/zmqnotificationinterface.cpp:63 in 7b4c7bdb35 outdated

57@@ -57,7 +58,10 @@ std::unique_ptr<CZMQNotificationInterface> CZMQNotificationInterface::Create(std
58     {
59         std::string arg("-zmq" + entry.first);
60         const auto& factory = entry.second;
61-        for (const std::string& address : gArgs.GetArgs(arg)) {
62+        for (std::string& address : gArgs.GetArgs(arg)) {
63+            // libzmq uses prefix "ipc://" for UNIX domain sockets
64+            ReplaceAll(address, ADDR_PREFIX_UNIX, ADDR_PREFIX_IPC);

laanwj commented at 12:26 pm on April 16, 2024:

Is ReplaceAll the right function to use here? Sure, it’s unlikely for it to appear multiple times, but from a principle of least surprise angle, it’d make sense to only replace the prefix.

pinheadmz force-pushed on Apr 16, 2024

pinheadmz commented at 5:51 pm on April 16, 2024: member

Rebasing on master since one commit was merged separately in #29649

zmq: accept unix domain socket address for notifier c87b0a0ff4

test: cover unix sockets in zmq interface 791dea204e

doc: release notes for PR 27679 21d0e6c7b7

pinheadmz force-pushed on Apr 16, 2024

pinheadmz commented at 6:16 pm on April 16, 2024: member

Thanks so much for your review @laanwj addressed your comments

laanwj commented at 11:05 am on April 17, 2024: member

Looks good to me now Code review ACK 21d0e6c7b7c7af7f6e54a45829b4fbfba6923b86

DrahtBot requested review from tdb3 on Apr 17, 2024

tdb3 commented at 11:16 am on April 17, 2024: contributor

crACK for 21d0e6c7b7c7af7f6e54a45829b4fbfba6923b86. Changes lgtm. Will follow up with some testing within the next few days as time allows.

guggero approved

guggero commented at 2:28 pm on April 18, 2024: contributor

Tested and code review ACK 21d0e6c7b7c7

Thanks a lot for the fix!

tdb3 commented at 1:24 am on April 19, 2024: contributor

crACK for 21d0e6c. Changes lgtm. Will follow up with some testing within the next few days as time allows.

re ACK for 21d0e6c7b7c7af7f6e54a45829b4fbfba6923b86

Retested the following:

using “ipc:///tmp/mysocket” form in bitcoin.conf causes error and quit (as expected, since it’s not unix:/)
using “notvalid:///tmp/sock” in bitcoin.conf causes error and quit (as expected, since it’s not unix:/)
Running with LND 0.17.4 on signet with UNIX sockets for ZMQ (bitcoin.conf and lnd.conf as described above in #27679#pullrequestreview-1938090332)

p.s. Looks like the invalid port error message is duplicated.

02024-04-19T01:35:55Z init message: Loading banlist…
12024-04-19T01:35:55Z SetNetworkActive: true
22024-04-19T01:35:55Z [error] Invalid port specified in -zmqpubrawtx: 'notvalid:///tmp/sock'
3Error: Invalid port specified in -zmqpubrawtx: 'notvalid:///tmp/sock'
42024-04-19T01:35:55Z Shutdown: In progress...
52024-04-19T01:35:55Z scheduler thread exit
62024-04-19T01:35:55Z Flushed fee estimates to fee_estimates.dat.
72024-04-19T01:35:55Z Shutdown: done
8dev@bdev01:~/bitcoin$

achow101 commented at 3:11 pm on April 22, 2024: member

ACK 21d0e6c7b7c7af7f6e54a45829b4fbfba6923b86

achow101 merged this on Apr 22, 2024

achow101 closed this on Apr 22, 2024

luke-jr referenced this in commit dabf896ea8 on Apr 24, 2024

luke-jr referenced this in commit 99a5703b2b on Apr 24, 2024

in src/init.cpp:1319 in 21d0e6c7b7

1327+        {"-zmqpubrawtx",            true},
1328+        {"-zmqpubsequence",         true}
1329     }) {
1330-        for (const std::string& socket_addr : args.GetArgs(port_option)) {
1331+        const std::string arg{port_option.first};
1332+        const bool unix{port_option.second};

fanquake commented at 7:35 am on April 26, 2024:

FYI https://github.com/bitcoin/bitcoin/runs/24257306970:

0init.cpp: In function ‘bool AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)’:
1init.cpp:1319:20: error: unused variable ‘unix’ [-Werror=unused-variable]
2 1319 |         const bool unix{port_option.second};
3      |                    ^~~~

maflcko commented at 8:18 am on April 26, 2024:

Fixed in #29968

hebasto commented at 9:41 am on April 26, 2024:

No warnings for me locally using

0x86_64-w64-mingw32-g++-posix (GCC) 13-posix

fanquake commented at 9:43 am on April 26, 2024:

No warnings for me locally using

Did you turn on the relevant warning flag

hebasto commented at 9:46 am on April 26, 2024:

No warnings for me locally using

Did you turn on the relevant warning flag

My bad! :facepalm:

ZMQ: Support UNIX domain sockets #27679

Code Coverage

Reviews

Conflicts