net: support unix domain sockets for -proxy and -onion#27375
net: support unix domain sockets for -proxy and -onion#27375achow101 merged 12 commits intobitcoin:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
73d2c56 to
7785e4d
Compare
willcl-ark
left a comment
There was a problem hiding this comment.
Concept ACK
With some light testing it seems to work pretty nicely:
will@ubuntu in ~/src/bitcoin on tor-unix-domain-socket [$?] via 🐍 3.7.16 took 6s
₿ sudo exa -al /proc/(pidof bitcoind)/fd
lrwx------ 64 will 31 Mar 09:21 0 -> /dev/null
lrwx------ 64 will 31 Mar 09:21 1 -> /dev/null
lrwx------ 64 will 31 Mar 09:21 2 -> /dev/null
lr-x------ 64 will 31 Mar 09:21 3 -> pipe:[47084978]
l-wx------ 64 will 31 Mar 09:21 4 -> pipe:[47084978]
lrwx------ 64 will 31 Mar 09:21 5 -> /home/will/.bitcoin/signet/.lock
l-wx------ 64 will 31 Mar 09:21 6 -> /home/will/.bitcoin/signet/debug.log
lrwx------ 64 will 31 Mar 09:21 7 -> anon_inode:[eventpoll]
lr-x------ 64 will 31 Mar 09:21 8 -> pipe:[47101090]
l-wx------ 64 will 31 Mar 09:21 9 -> pipe:[47101090]
lrwx------ 64 will 31 Mar 09:21 10 -> anon_inode:[eventfd]
lrwx------@ 64 will 31 Mar 09:21 11 -> socket:[47101093]
lrwx------@ 64 will 31 Mar 09:21 12 -> socket:[47101095]
lrwx------ 64 will 31 Mar 09:21 13 -> /home/will/.bitcoin/signet/blocks/index/LOCK
l-wx------ 64 will 31 Mar 09:21 14 -> /home/will/.bitcoin/signet/blocks/index/000387.log
l-wx------ 64 will 31 Mar 09:21 15 -> /home/will/.bitcoin/signet/blocks/index/MANIFEST-000385
lrwx------ 64 will 31 Mar 09:21 16 -> /home/will/.bitcoin/signet/chainstate/LOCK
l-wx------ 64 will 31 Mar 09:21 17 -> /home/will/.bitcoin/signet/chainstate/002594.log
l-wx------ 64 will 31 Mar 09:21 18 -> /home/will/.bitcoin/signet/chainstate/MANIFEST-002593
lrwx------ 64 will 31 Mar 09:21 19 -> /home/will/.bitcoin/signet/wallets/legacy/.walletlock
l-wx------ 64 will 31 Mar 09:21 20 -> /home/will/.bitcoin/signet/wallets/legacy/db.log
lrwx------ 64 will 31 Mar 09:21 21 -> /home/will/.bitcoin/signet/wallets/legacy/wallet.dat
lrwx------ 64 will 31 Mar 09:21 22 -> /home/will/.bitcoin/signet/wallets/legacy/database/log.0000000001
lrwx------ 64 will 31 Mar 09:21 23 -> /home/will/.bitcoin/signet/wallets/test-descriptor/wallet.dat
lrwx------ 64 will 31 Mar 09:21 24 -> /home/will/.bitcoin/signet/wallets/test-descriptor/wallet.dat-journal
lrwx------ 64 will 31 Mar 09:21 25 -> anon_inode:[eventpoll]
lr-x------ 64 will 31 Mar 09:21 26 -> pipe:[47084995]
l-wx------ 64 will 31 Mar 09:21 27 -> pipe:[47084995]
lrwx------ 64 will 31 Mar 09:21 28 -> anon_inode:[eventfd]
lrwx------@ 64 will 31 Mar 09:21 29 -> socket:[47099059]
lrwx------@ 64 will 31 Mar 09:21 30 -> socket:[47084996]
lrwx------@ 64 will 31 Mar 09:21 31 -> socket:[47084997]
lrwx------@ 64 will 31 Mar 09:21 32 -> socket:[47099065]
lrwx------@ 64 will 31 Mar 09:21 33 -> socket:[47084998]
lrwx------@ 64 will 31 Mar 09:21 34 -> socket:[47100197]
lrwx------@ 64 will 31 Mar 09:21 35 -> socket:[47099299]
lrwx------@ 64 will 31 Mar 09:21 36 -> socket:[47099533]
lrwx------@ 64 will 31 Mar 09:22 37 -> socket:[47099588]
lrwx------@ 64 will 31 Mar 09:22 38 -> socket:[47085043]
lrwx------@ 64 will 31 Mar 09:22 39 -> socket:[47085047]With new socket connections being opened for each connection.
As I mentioned in one of my comments, I am curious why you didn't go for the -onion case too, to reduce the scope of the change?
Enabling -onion in the PortErrorMessage check loop did appear to also work nicely (although i did not check whether there were any other side-effects).
The info shown by -netinfo 4 looked correct in both cases.
7785e4d to
4128840
Compare
There was a problem hiding this comment.
Thanks for the feedback @willcl-ark. force-push to 2436b00f2231c52e6bee6d7f79690c5e709cd6f9 should address your notes. I'm still expecting some CI failures from platforms that don't support unix sockets, then I'll add configure options like wumpus did here
261ce7e to
2436b00
Compare
0d19a39 to
3436ddb
Compare
3436ddb to
502db51
Compare
0267cfe to
44dd4cb
Compare
vasild
left a comment
There was a problem hiding this comment.
Concept ACK, this would be nice to have.
I do not like that the current approach expands enum Network and CNetAddr with the UNIX type, making it first-class-citizen network that has to be handled all over the networking code, whereas it is only needed to connect to the proxy. It is not like this PR adds support for connecting to other Bitcoin nodes over UNIX sockets (using the P2P protocol). This creates meaningless situations like having a CService on a UNIX socket (it is meaningless because CService has port, whereas UNIX sockets don't have port). Similarly CSubNet on a UNIX socket is meaningless.
I will play a bit with this to see if it would be possible to constrain the logic in the Proxy class and hide from the rest of the code whether the proxy is at addr:port or unix:/path.
44dd4cb to
3ea142e
Compare
|
Thanks so much for the great review @vasild I think I addressed all your feedback up to the adding UNIX to CNetAddr. I understand your concerns there and we can chat about it, I'll look at alternatives as well. What I did do in this last push was mostly separate unix sockets from regular sockets, and adding |
3ea142e to
7008e8f
Compare
7008e8f to
2c19dd7
Compare
|
Rebased on master to fix conflicts and ( 🤞 ) pass CI. |
I agree that shouldn't be called "manual". re-ACK 567cec9 |
vasild
left a comment
There was a problem hiding this comment.
ACK 567cec9
I agree it is better to keep the hardcoded /*manual=*/true because in this case that is about connecting to the proxy itself, not to the bitcoin peer. In this case it is better to log with higher severity.
In the future maybe it would be better to rename the manual_connection argument of ConnectToSocket() to log_severity or error_severity or is_imporant_please_log_errorswithhigherseverity.
|
re ACK for 567cec9. Pulled, built, ran all unit and functional tests (all passed). Re-ran the test actions (quote below) from #27375 (review) Overall, similar results were observed. For the second action (i.e. running with For both actions, UNIX socket traffic was sniffed and contained bitcoin protocol traffic.
|
|
ACK 567cec9 |
|
Seems like there's a silent merge conflict somewhere, getting a functional test failure when merged with master: |
| #if defined(HAVE_CONFIG_H) | ||
| #include <config/bitcoin-config.h> | ||
| #endif |
There was a problem hiding this comment.
@achow101 Its very mysterious, when I rebase this branch on master (git rebase master) these lines disappear. I can't figure out why!
However when I do interactive git rebase master -i and stop on this commit with an edit command, DO NOTHING, and just git rebase --continue... the lines remain intact. I am force-pushing this interactive rebase branch now. 😵💫
There was a problem hiding this comment.
when I rebase this branch on master (git rebase master) these lines disappear. I can't figure out why!
What are the exact steps to reproduce?
I did
git checkout 567cec9a05e1261e955535f734826b12341684b6
git rebase bitcoin-core/master
git range-diff bitcoin-core/master 6df896fc37f5b2023b891d5300345e2a4352e521
And got an empty diff (only the commit ids are different)
There was a problem hiding this comment.
Maybe you two are using a different git --version (with a bug)?
I re-ran the CI and it didn't fail. Are you sure this is a silent conflict, because if it was, the CI should be detecting it on a re-run? |
|
Oh! The test command I use doesn't rerun autogen.sh every time, I think that's what caused the test failure. @pinheadmz You can reset the branch back to 567cec9 and this can be merged. Sorry about the confusion. |
Ugh and when I was trying to reproduce I ALSO didn't rerun autogen or config 🤦 wow THANKS @maflcko for the sanity check |
wen cmake? |
|
Is the expectation here that we should see a lot more |
Yes but that log message can be removed or set to debug in a follow-up. With I have a few unix sockets follow ups planned: RPC & bitcoin-cli (maybe speed up the tests!) and ZMQ. I'll clean up this log message in those |
|
Ok, it needs to be cleaned up because it's just spammy/pointless. i.e: 2024-03-13T13:26:00Z Cannot connect to socket for 151.213.177.128:8333
2024-03-13T13:26:24Z Cannot connect to socket for 75.88.65.59:8333
2024-03-13T13:26:48Z Cannot connect to socket for 76.114.61.112:8333
2024-03-13T13:28:55Z Cannot connect to socket for [2600:8805:5c26:eb00:61b0:c6a7:451:1b39]:8333
2024-03-13T13:30:45Z New block-relay-only v1 peer connected: version: 70016, blocks=834513, peer=39
2024-03-13T13:32:01Z Cannot connect to socket for [2a04:6ec0:20d:d050:819a:ceb8:4416:2bc0]:8333
2024-03-13T13:32:14Z New block-relay-only v1 peer connected: version: 70016, blocks=834513, peer=40
2024-03-13T13:34:47Z New block-relay-only v1 peer connected: version: 70016, blocks=834513, peer=41
2024-03-13T13:34:58Z Cannot connect to socket for 122.206.190.60:8333
2024-03-13T13:36:23Z New block-relay-only v1 peer connected: version: 70016, blocks=834513, peer=42
2024-03-13T13:37:38Z Cannot connect to socket for 138.199.60.26:8333
2024-03-13T13:38:34Z Cannot connect to socket for 120.236.171.239:8333
2024-03-13T13:39:00Z Cannot connect to socket for 35.72.149.104:8333
2024-03-13T13:41:22Z Cannot connect to socket for 176.199.211.63:8333
2024-03-13T13:41:25Z Cannot connect to socket for [2804:7f0:7c80:196f:b0b2:1642:e4a9:b794]:8333
2024-03-13T13:41:37Z Cannot connect to socket for [2a00:b700::3:379]:8333
2024-03-13T13:42:55Z Cannot connect to socket for [2001:871:24b:ab3e:d1f2:1f7e:b950:692f]:8333
2024-03-13T13:43:01Z New block-relay-only v1 peer connected: version: 70016, blocks=834513, peer=43
2024-03-13T13:45:30Z Cannot connect to socket for 121.237.138.96:8333
2024-03-13T13:46:56Z Cannot connect to socket for 110.153.86.87:8333
2024-03-13T13:48:32Z Saw new header hash=0000000000000000000071923ef5a18dc17d47717fae386296dfe8902414ea71 height=834514
2024-03-13T13:48:32Z Saw new cmpctblock header hash=0000000000000000000071923ef5a18dc17d47717fae386296dfe8902414ea71 peer=16
2024-03-13T13:48:32Z UpdateTip: new best=0000000000000000000071923ef5a18dc17d47717fae386296dfe8902414ea71 height=834514 version=0x2fffe000 log2_work=94.792163 tx=976064192 date='2024-03-13T13:48:27Z' progress=1.000000 cache=232.4MiB(1767736txo)
2024-03-13T13:50:28Z Cannot connect to socket for 58.219.96.208:8333
2024-03-13T13:50:41Z Cannot connect to socket for 189.19.183.194:8333
2024-03-13T13:52:56Z New block-relay-only v1 peer connected: version: 70016, blocks=834514, peer=45
2024-03-13T13:53:12Z Cannot connect to socket for [2600:381:6a90:cfa5:b0a4:5c43:fdb8:9298]:8333
2024-03-13T13:56:25Z Cannot connect to socket for [2003:d7:df24:a900:53a3:8916:d525:9958]:8333
2024-03-13T14:00:04Z Cannot connect to socket for 97.96.100.48:8333
2024-03-13T14:03:56Z Cannot connect to socket for 195.181.167.197:8333
2024-03-13T14:04:02Z Saw new header hash=00000000000000000001ee20083bb8df96e256bb46dd856c48f686b692dbc29b height=834515
2024-03-13T14:04:02Z Saw new cmpctblock header hash=00000000000000000001ee20083bb8df96e256bb46dd856c48f686b692dbc29b peer=16
2024-03-13T14:04:02Z UpdateTip: new best=00000000000000000001ee20083bb8df96e256bb46dd856c48f686b692dbc29b height=834515 version=0x294c8000 log2_work=94.792178 tx=976067423 date='2024-03-13T14:03:37Z' progress=1.000000 cache=234.9MiB(1785815txo)
2024-03-13T14:04:31Z Cannot connect to socket for [2606:6080:1001:30:5f94:f242:c829:1017]:8333
2024-03-13T14:04:32Z Saw new header hash=00000000000000000001cee842398455cae402ab13037a450fe4829db7efd2e2 height=834516
2024-03-13T14:04:32Z Saw new cmpctblock header hash=00000000000000000001cee842398455cae402ab13037a450fe4829db7efd2e2 peer=16
2024-03-13T14:04:33Z UpdateTip: new best=00000000000000000001cee842398455cae402ab13037a450fe4829db7efd2e2 height=834516 version=0x27fd4000 log2_work=94.792192 tx=976069773 date='2024-03-13T14:04:13Z' progress=1.000000 cache=236.4MiB(1799558txo)
2024-03-13T14:04:59Z Cannot connect to socket for 178.70.102.186:8333
2024-03-13T14:05:43Z Cannot connect to socket for [2409:8a55:6ad:7101:49d4:4737:ccad:17c]:8333
2024-03-13T14:06:21Z Saw new header hash=000000000000000000030739bcd83bfdfc6a03a1a58e21e4c61d6b59ac047569 height=834517
2024-03-13T14:06:21Z Saw new cmpctblock header hash=000000000000000000030739bcd83bfdfc6a03a1a58e21e4c61d6b59ac047569 peer=16
2024-03-13T14:06:21Z UpdateTip: new best=000000000000000000030739bcd83bfdfc6a03a1a58e21e4c61d6b59ac047569 height=834517 version=0x2cbea000 log2_work=94.792206 tx=976071288 date='2024-03-13T14:05:33Z' progress=1.000000 cache=236.4MiB(1802022txo)
2024-03-13T14:06:45Z Cannot connect to socket for 86.168.50.224:8333
2024-03-13T14:08:29Z Cannot connect to socket for 194.163.131.91:39388
2024-03-13T14:11:12Z Cannot connect to socket for [2603:6080:7a0e:59ff:9da7:4932:3d76:58ec]:8333
2024-03-13T14:11:41Z Cannot connect to socket for 108.214.8.13:60501
2024-03-13T14:12:06Z Cannot connect to socket for 84.178.138.51:8333 |
Closes #27252
UNIX domain sockets are a mechanism for inter-process communication that are faster than local TCP ports (because there is no need for TCP overhead) and potentially more secure because access is managed by the filesystem instead of serving an open port on the system.
There has been work on unix domain sockets before but for now I just wanted to start on this single use-case which is enabling unix sockets from the client side, specifically connecting to a local Tor proxy (Tor can listen on unix sockets and even enforces strict curent-user-only access permission before binding) configured by
-onion=or-proxy=I copied the prefix
unix:usage from Tor. With this patch built locally you can test with your own filesystem path (example):tor --SocksPort unix:/Users/matthewzipkin/torsocket/xbitcoind -proxy=unix:/Users/matthewzipkin/torsocket/xPrep work for this feature includes:
sockaddrandSockto accommodateAF_UNIXwithout disturbingCServiceProxyclass to represent either aCServiceor a UNIX socket (by its file path)Future work: