cli: Display all proxies in -getinfo #22959

pull klementtan wants to merge 1 commits into bitcoin:master from klementtan:getinfo-multiple-proxies changing 2 files +30 −5

klementtan commented at 3:04 AM on September 13, 2021: contributor
Changes: Display all proxies in -getinfo

Motivation:
- Currently -getinfo only return the proxy of the first network in getnetworkinfo.
- This PR will display all unique proxies in getnetworkinfo as suggested in #17314#issue-514543978
  
  List all proxies, at least if they're different from the IPv4 one
Testing:

You can verify this change by starting bitcoind with
```
./src/bitcoind -signet --proxy=127.0.0.1:9050 --i2psam=127.0.0.1:7656
```
Execute -getinfo
```
./src/bitcoin-cli -signet -getinfo
```
DrahtBot added the label Utils/log/libs on Sep 13, 2021
ghost commented at 5:59 AM on September 13, 2021: none

Concept ACK

Since i2p is added in Bitcoin Core which uses a different proxy, it's better if both are shared in -getinfo

-i2psam is used for proxy and incoming both as documented in doc/i2p.md

in src/bitcoin-cli.cpp:1005 in 60502df8ff outdated

1002 | +        unique_proxies.insert(network["proxy"].getValStr());
1003 | +    }
1004 | +
1005 | +    std::string proxies = std::accumulate(unique_proxies.begin(), unique_proxies.end(), std::string{}, [](const std::string& a, const std::string& b) {
1006 | +        return a.empty() ? b : a + ", " + b;
1007 | +    });

theStack commented at 4:02 PM on September 14, 2021:

I think you can use our Join helper from src/util/string.h to achieve the same (this only works on a std::vector though, so you'd need to convert first):

    std::string proxies = Join(unique_proxies_vector, ", ");

klementtan commented at 1:08 AM on September 15, 2021:

Thanks for the suggestion! Updated the PR to use Join instead.

theStack commented at 4:06 PM on September 14, 2021: member

Concept ACK
klementtan force-pushed on Sep 15, 2021
Zero-1729 commented at 1:58 AM on September 15, 2021: contributor

Concept ACK
laanwj commented at 5:18 PM on September 16, 2021: member
Concept ACK. Showing only one proxy is an artifact from the old getinfo RPC call, we can do better now.

Suggestion: maybe add the networks each proxy is used for, example format
```
Proxies: 127.0.0.1:7656 (i2p) 127.0.0.1:9050 (ipv4,ipv6,onion)
```
ghost commented at 5:30 PM on September 16, 2021: none

Suggestion: maybe add the networks each proxy is used for, example format @laanwj Is this something similar to checkboxes we have in GUI which I was trying to remove in https://github.com/bitcoin-core/gui/pull/421?

If yes, this is confusing for me TBH considering the changes and discussions we had for #22834

Also the comments:

127 2021-08-30T11:36:52 <laanwj> vasild: when using a proxy, bitcoind has no idea whether it's connecting to ipv4, ipv6 or something else completely :)

https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-30.html#l-127
klementtan force-pushed on Sep 19, 2021
klementtan commented at 3:27 PM on September 19, 2021: contributor

@laanwj Thanks for the suggestion.

869ad51 to 7d6f0fa: Updated the PR to display the networks each proxy is used for.

in src/bitcoin-cli.cpp:1005 in 7d6f0fa25f outdated

1002 | +    std::vector<std::string> proxies;
1003 | +    for (const auto& [proxy, networks] : proxy_networks) {
1004 | +        proxies.push_back(strprintf("%s (%s)", proxy, Join(networks, ", ")));
1005 | +    }
1006 | +
1007 | +    result_string += strprintf("Proxies: %s\n", proxies.empty() ? "N/A" : Join(proxies, " "));

jonatack commented at 7:28 PM on September 19, 2021:

a few suggestions to pick/choose/ignore

maybe separate the proxies with a comma-space
maybe add a comment, remove unneeded localvars, and keep the lines together to see a glance that it's one section for the proxies
maybe s/proxies.push_back/proxies.emplace_back/ to construct in-place
maybe print "n/a" rather than "N/A", like line 1001 for -netinfo

--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -987,22 +987,19 @@ static void ParseGetInfoResult(UniValue& result)
     result_string += strprintf("Version: %s\n", result["version"].getValStr());
     result_string += strprintf("Time offset (s): %s\n", result["timeoffset"].getValStr());
 
+    // proxies
     std::map<std::string, std::vector<std::string>> proxy_networks;
-
     for (const UniValue& network : result["networks"].getValues()) {
-        std::string proxy = network["proxy"].getValStr();
+        const std::string proxy = network["proxy"].getValStr();
         if (proxy.empty()) continue;
-
-        std::string network_name = network["name"].getValStr();
-        proxy_networks[proxy].push_back(network_name);
+        proxy_networks[proxy].push_back(network["name"].getValStr());
     }
-
     std::vector<std::string> proxies;
     for (const auto& [proxy, networks] : proxy_networks) {
-        proxies.push_back(strprintf("%s (%s)", proxy, Join(networks, ", ")));
+        proxies.emplace_back(strprintf("%s (%s)", proxy, Join(networks, ", ")));
     }
+    result_string += strprintf("Proxies: %s\n", proxies.empty() ? "n/a" : Join(proxies, ", "));
 
-    result_string += strprintf("Proxies: %s\n", proxies.empty() ? "N/A" : Join(proxies, " "));
     result_string += strprintf("Min tx relay fee rate (%s/kvB): %s\n\n", CURRENCY_UNIT, result["relayfee"].getValStr());

klementtan commented at 10:53 AM on September 20, 2021:

Thanks for the suggestion! Updated the PR to adopt the suggestion in 7c3712f

jonatack commented at 7:34 PM on September 19, 2021: member

Tested almost-ACK. The order of proxies is sorted by proxy value; maybe maintain the original order printed by getnetworkinfo "networks".
klementtan force-pushed on Sep 20, 2021
cli: Display all proxies in -getinfo 7c3712fa32
klementtan force-pushed on Sep 20, 2021
klementtan commented at 10:59 AM on September 20, 2021: contributor
7c3712f changes:
- Use , to separate proxies
- s/N/A/n/a
- Proxies are displayed in the same order as getnetworkinfo. Added a new std::vector<std::string> ordered_proxies to store the original ordering of the proxies.
laanwj commented at 1:12 PM on September 20, 2021: member
Thank you! Will test.
- :heavy_check_mark: Node without any proxy configured
```
Proxies: n/a
```
- :heavy_check_mark: Clearnet node which is also connected to onion/i2p:
```
Proxies: 127.0.0.1:9050 (onion), 127.0.0.1:7656 (i2p)
```
- :heavy_check_mark: Generic -proxy=…:
```
Proxies: 127.0.0.1:9050 (ipv4, ipv6, onion)
```
- :heavy_check_mark: Different -proxy=… -onion=…
```
Proxies: 127.0.0.1:3333 (ipv4, ipv6), 127.0.0.1:9050 (onion)
```
Tested ACK 7c3712fa32b79c1c3abc73ef89990c452e22ce2b

127 2021-08-30T11:36:52 vasild: when using a proxy, bitcoind has no idea whether it's connecting to ipv4, ipv6 or something else completely :)

This was specifically about names, so connecting to a DNS seed, for example, or a node passed as argument. For P2P peers it is known what the network is because this information is gossiped along with the address itself. This is what the information here is about.
ghost commented at 2:31 PM on September 20, 2021: none

utACK https://github.com/bitcoin/bitcoin/commit/7c3712fa32b79c1c3abc73ef89990c452e22ce2b
laanwj merged this on Sep 20, 2021
laanwj closed this on Sep 20, 2021
klementtan deleted the branch on Sep 20, 2021
sidhujag referenced this in commit d862d3f796 on Sep 21, 2021
DrahtBot locked this on Oct 30, 2022

Contributors

Labels

Linked (view graph)

#17314 Re-thinking `bitcoin-cli -getinfo`