-proxy=addr:port specifies the proxy for all networks (except I2P). Previously only the Tor proxy could have been specified separately via -onion=addr:port.
Make it possible to specify separately the proxy for IPv4, IPv6, Tor and CJDNS by e.g. -proxy=addr:port=ipv6. Or remove the proxy for a given network, e.g. -proxy=0=cjdns.
Resolves: #24450
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32425.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | pinheadmz, caesrcd, danielabrozzoni |
| Concept ACK | 1440000bytes |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
1229+ {"-zmqpubhashblock", true, false},
1230+ {"-zmqpubhashtx", true, false},
1231+ {"-zmqpubrawblock", true, false},
1232+ {"-zmqpubrawtx", true, false},
1233+ {"-zmqpubsequence", true, false},
1234 }) {
-bind here. I guess before it was omitted because this loop did not support =whatever suffix. Now it does.
ACK 6a9f04688f
I have reviewed the code, unit tests, and functional tests. I built the project and ran a node using the -proxy=0=cjdns parameter to verify the behavior. Confirmed that CJDNS correctly bypasses the general proxy as intended. Manual testing was performed on Arch Linux, and the node successfully connected over CJDNS directly without using the proxy.
551@@ -552,11 +552,26 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
552 argsman.AddArg("-peerblockfilters", strprintf("Serve compact block filters to peers per BIP 157 (default: %u)", DEFAULT_PEERBLOCKFILTERS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
553 argsman.AddArg("-txreconciliation", strprintf("Enable transaction reconciliations per BIP 330 (default: %d)", DEFAULT_TXRECONCILIATION_ENABLE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CONNECTION);
554 argsman.AddArg("-port=<port>", strprintf("Listen for connections on <port> (default: %u, testnet3: %u, testnet4: %u, signet: %u, regtest: %u). Not relevant for I2P (see doc/i2p.md). If set to a value x, the default onion listening port will be set to x+1.", defaultChainParams->GetDefaultPort(), testnetChainParams->GetDefaultPort(), testnet4ChainParams->GetDefaultPort(), signetChainParams->GetDefaultPort(), regtestChainParams->GetDefaultPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::CONNECTION);
555+ const std::string proxy_doc_for_value =
556 #ifdef HAVE_SOCKADDR_UN
557- argsman.AddArg("-proxy=<ip:port|path>", "Connect through SOCKS5 proxy, set -noproxy to disable (default: disabled). May be a local file path prefixed with 'unix:' if the proxy supports it.", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_ELISION, OptionsCategory::CONNECTION);
558+ "ip:port|path";
nit: the port is optional, no?
0 "ip[:<port>]|path";
< and > for the other fields.
1625+ Proxy proxy;
1626+ if (!proxy_str.empty() && proxy_str != "0") {
1627+ if (IsUnixSocketPath(proxy_str)) {
1628+ proxy = Proxy{proxy_str, /*tor_stream_isolation=*/proxyRandomize};
1629+ } else {
1630+ const std::optional<CService> addr{Lookup(proxy_str, 9050, fNameLookup)};
torcontrol.cpp?
If the user sets -proxy=addr:port=tor then -onion= shouldn’t be allowed, no? Otherwise one could set 2 diff proxies for Tor(?).
Also, in Tor’s documentation:
0-proxy=ip:port Set the proxy server. If SOCKS5 is selected (default), this proxy
1 server will be used to try to reach .onion addresses as well.
So, shouldn’t -proxy=0=cjdns be the default if -cjdnsreachable when -proxy=addr:port?
6a9f04688f...2059eaa692: address suggestions
If the user sets
-proxy=addr:port=torthen-onion=shouldn’t be allowed, no? Otherwise one could set 2 diff proxies for Tor(?).
I think that is ok, assuming the expectation is that later options override earlier ones. For example, without this PR, -proxy=x -onion=y is supposed to set the Tor proxy to y, overriding the earlier x. Updated tor.md to mention that.
So, shouldn’t
-proxy=0=cjdnsbe the default if-cjdnsreachablewhen-proxy=addr:port?
Could be, but I do not like these “automatically set x only if y and z are true, otherwise something else”. That would also change the default behavior in a breaking way - for people that use -proxy which does support CJDNS it will stop working. Maybe there are no such users, but we can’t know for sure.
Otherwise one could set 2 diff proxies for Tor(?).
I think that is ok, assuming the expectation is that later options override earlier ones. For example, without this PR,
-proxy=x -onion=yis supposed to set the Tor proxy toy, overriding the earlierx. Updatedtor.mdto mention that.
Ok.
So, shouldn’t
-proxy=0=cjdnsbe the default if-cjdnsreachablewhen-proxy=addr:port?Could be, but I do not like these “automatically set
xonly ifyandzare true, otherwise something else”. That would also change the default behavior in a breaking way - for people that use-proxywhich does support CJDNS it will stop working. Maybe there are no such users, but we can’t know for sure.
Ok, but since the previous statement (-proxy=…) from Tor’s doc combined with:
-onlynet=onion Make automatic outbound connections only to .onion addresses.
Maybe we should give a warning at least (and perhaps suggesting: -proxy=0=cjdns) because that (-proxy=ip[:<port>] -onlynet=onion -cjdnsreachable) would make CJDNS not to work (?).
I think a more long term solution could be to add the possibility of having a separate proxy for CJDNS, as suggested on the issue by other commenters there.
ACK 2059eaa692ff614c41dee50f4bc2d2946e5d42af
I have tested the code.
Maybe we should give a warning at least (and perhaps suggesting:
-proxy=0=cjdns) because that (-proxy=ip[:<port>] -onlynet=onion -cjdnsreachable) would make CJDNS not to work (?).
That would make automatic outbound connections to Tor peers only. Still manual outbound and inbound connections would work and be possible to/from CJDNS peers. Maybe a warning in the lines of “Automatic outbound connections will only be made to Tor peers, if you want to include CJDNS, then use -onlynet=cjdns in addition”. Or maybe not? In either case adding such a warning seems to be out of the scope of this PR whose purpose is to be able to specify proxy per network.
I think a more long term solution could be to add the possibility of having a separate proxy for CJDNS, as suggested on the issue by other commenters there.
That is made possible by this PR: -proxy=addr:port=cjdns.
Concept ACK
Multiple proxies for different networks would be useful. Might need release notes if this eventually gets merged.
75+ -onion they will not route over Tor, so use -proxy if you have privacy
76+ concerns.
77+ ------------------------------------------------------------------------
78+
79+ If -proxy or -onion is specified multiple times, later occurences override
80+ earlier ones and command line overrides the config file.
Since you’re touching this doc, you could also add a note about UNIX sockets which I should’ve added in #27375 ;-)
e.g.
0UNIX domain sockets may be used for proxy connections. Set `-onion` or `-proxy`
1to the local socket path with the prefix `unix:` (e.g. `-onion=unix:/home/me/torsocket`).
1604+ // -proxy sets a proxy for outgoing network traffic, possibly per network.
1605+ // -noproxy, -proxy=0 or -proxy="" can be used to remove the proxy setting, this is the default
1606+ Proxy ipv4_proxy;
1607+ Proxy ipv6_proxy;
1608+ Proxy name_proxy;
1609+ Proxy cjdns_proxy;
https://github.com/bitcoin/bitcoin/pull/32425/commits/4af5b4d4298b4c69cd9131e87fe50e9386def13a
nit: could move Proxy onion_proxy; from L1590 into this group
1625+ Proxy proxy;
1626+ if (!proxy_str.empty() && proxy_str != "0") {
1627+ if (IsUnixSocketPath(proxy_str)) {
1628+ proxy = Proxy{proxy_str, /*tor_stream_isolation=*/proxyRandomize};
1629+ } else {
1630+ const std::optional<CService> addr{Lookup(proxy_str, DEFAULT_TOR_SOCKS_PORT, fNameLookup)};
4af5b4d4298b4c69cd9131e87fe50e9386def13a
The default port here is a little funny since a user could set -proxy=127.0.0.1=cjdns right?
Yes, that is the same in master so no change in behavior by this PR. In master it is using a magic number 9050 whereas here it uses the constant DEFAULT_TOR_SOCKS_PORT.
-proxy=127.0.0.1=cjdns would be interpreted as -proxy=127.0.0.1:9050=cjdns which might be the correct config, or if not then the user could use -proxy=127.0.0.1:1234=cjdns to specify another port.
Personally, I would favor a more dumb implementation that does not do such automations and guessings on behalf of the user. Such an implementation would require the port to always be provided and refuse to start if not.
ACK 2059eaa692ff614c41dee50f4bc2d2946e5d42af
Built and ran all tests on macos/arm64. Reviewed both commits and left a few nits. Played with the feature on mainnet, and connected to all networks:
0 ipv4 ipv6 onion cjdns total block manual
1in 0 0 0 0 0
2out 1 1 2 7 11 0 7
3total 1 1 2 7 11
Even used tor’s ipv4 and unix sockets for separate proxies ;-)
bcd -debug=tor -debug=net -proxy=unix:/Users/matthewzipkin/tor/sock -proxy=127.0.0.1:9050=ipv6 -cjdnsreachable -proxy=0=cjdns
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA256
2
3ACK 2059eaa692ff614c41dee50f4bc2d2946e5d42af
4-----BEGIN PGP SIGNATURE-----
5
6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmgiWz0ACgkQ5+KYS2KJ
7yTp0Ag/+JZCDfs0ILvoCRuuUN74qNh1vlvS9eqie3d32quX4wjYE8ZQC4Ov18xlv
88nOCKzDu369bixp1N6HlQXqa41w0x0KbAshMHWsHwolKr5MumQTfOp6mcnMaD1J4
9zbS1Jbyigs7x+ukeVLW22CH9I64GUGQuizmzuWA2/cR0ehFtkLsKyG/5HCSeQU4H
10/VzzEGcYNeqKKroLKKswqbkcVSrzhz8I1P7ZOEMhYQ6kDTZ5YVKGhbqcb8n2S8nJ
11Tr5GY72gx8WY+LwgLM3nuesus8jYSVKSwleWCGxm2uqKdAkLcBQajBnJy1kgIYO0
12ieAMypz9OF8zYls6VvhuTZseTvMWIM0ai37/1YUS8R3Dk3c9habUhv2GoxEpHmZb
132vcJ133qtVRNj9ZWAy987lmXKz1m3imrH+SGGUdG0etaHjs6qz0JnFxv11/xTzaB
14SBFc8nalkIG/RZqy72i3WskMdOxkywkVx2fzN5RYZdpxSb2krlaOVln3On60H8iW
15Ewnk+MioS0s/1mvp+u31YG5GzRhqO4mCtQMG5IE2W+k58q1gMvq6uQtasgWGu9LX
16ZihaLz2cxeHxCqQFaFP1rL54IB/2MWXWXOBCEp24JQT/LXAfq1sacikd1PzaI/T8
17tVQcYWs1W3ZORKjqMhWaPEpXurz5mstul/zV/BkRcod6E9XgJPw=
18=65/9
19-----END PGP SIGNATURE-----
pinheadmz’s public key is on openpgp.org
`-proxy=addr:port` specifies the proxy for all networks (except I2P).
Previously only the Tor proxy could have been specified separately via
`-onion=addr:port`.
Make it possible to specify separately the proxy for IPv4, IPv6, Tor and
CJDNS by e.g. `-proxy=addr:port=ipv6`. Or remove the proxy for a given
network, e.g. `-proxy=0=cjdns`.
Resolves: https://github.com/bitcoin/bitcoin/issues/24450
Also change the formatting of `tor.md` to have more horizonal space for
the text.
Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
2059eaa692...e98c51fcce: address suggestions
ACK e98c51fcce9ae3f441a416cab32a5c85756c6c64
Trivial changes since last ACK. Built and tested again on mainnet, this time added i2p as well, connected to peers on every network we support. Tried various proxy settings.
The code change is small and mostly just contained in init.cpp. We used to call SetProxy() on every network type with the same proxy endpoint, this new code parses more fine-grained config options and sets separate proxies based on the options.
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA256
2
3ACK e98c51fcce9ae3f441a416cab32a5c85756c6c64
4-----BEGIN PGP SIGNATURE-----
5
6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmgjTV8ACgkQ5+KYS2KJ
7yToKAw/6AgyTgGUOnk+VxgZGTTFufgNES5uvasHJtpkvcNYf9boWGpVQXBxf9ePc
8BR7ly5C1+Tmvv6X36FwLE+mhNbXHIK178lfWFs+CMMeeuEThwR7LapGSbdAZGa+l
9RxXxAx49/Ns5wMnMXPS+7mDp3CKAk1HRm1u1AN5k99RcyXdG5QUqfCb5QctckTUZ
10jS17RuzONdJxW34Xd+p2nfsbKYUBy0YQPfLyxjy4e3KaStmpQTtp0HIKampli4t4
11GhJiD+Yxj3ohJHqMS7so7jFo6nXqYw9k8wNUdaZTySBwMk2vDzRYesoBuXQZqM+j
12xV9KReg1zxbtJTtW4cC38Rr3qRJJm4AKUNQmybs5aDfT07UAe2Ocd+b2lX97Pg4L
13EyFCneylgs9mY+XH6UJpbOMihGWjAzZ63dRGrnMEFwZuNOmSBCSqPgQNjqSyBnU1
14Cwj8liKCzr+mYwgWSA1M57BJGc0ZYeIPOEi8NEBzwejtQN6ljESmtWXh3kgKcXyb
15VE2e3KJvCTTtdjI8r6nk2cNGKDMwgzeJp1DAAMEj2xOT7rI0qYc1tdr4HUGhhl0w
16TiCeCenZhgPyTocW0xVFTzAmvhm+WCHp6Epyucjo0hnK4esw965z2N3B+4tmCfMY
17B6VJFHuUqaerSIlvFzI8QpJsP9UBUUe2VS+XXhBv1VnlNuGewDM=
18=PiAP
19-----END PGP SIGNATURE-----
pinheadmz’s public key is on openpgp.org
Code Review ACK e98c51fcce9ae3f441a416cab32a5c85756c6c64
I have reviewed the code, and it looks good to me.
I run the tests locally and did some very limited manual testing, mimicking the functional test behavior (passing in -proxy=...=network and checking the getnetworkinfo output)
1656+ cjdns_proxy = proxy;
1657+ } else {
1658+ return InitError(strprintf(_("Unrecognized network in -proxy='%s': '%s'"), param_value, net_str));
1659+ }
1660+ }
1661+ if (ipv4_proxy.IsValid()) {
SetProxy itself just returns false if it’s not valid.
SetProxy() an invalid value and rely on it doing the “right thing”. It is not documented what SetProxy() would do with an invalid input, so I prefer to have these checks.
1647+ if (net_str.empty()) { // For all networks.
1648+ ipv4_proxy = ipv6_proxy = name_proxy = cjdns_proxy = onion_proxy = proxy;
1649+ } else if (net_str == "ipv4") {
1650+ ipv4_proxy = name_proxy = proxy;
1651+ } else if (net_str == "ipv6") {
1652+ ipv6_proxy = name_proxy = proxy;
1608+ Proxy onion_proxy;
1609+ Proxy name_proxy;
1610+ Proxy cjdns_proxy;
1611+ for (const std::string& param_value : args.GetArgs("-proxy")) {
1612+ const auto eq_pos{param_value.rfind('=')};
1613+ const std::string proxy_str{param_value.substr(0, eq_pos)}; // e.g. 127.0.0.1:9050=ipv4 -> 127.0.0.1:9050