Fix display issues for IPv6 proxy setup in Options Dialog (UI only, no functionality impact)

pablomartin4btc commented at 6:31 pm on September 15, 2024: contributor

Currently, setting up a proxy (whether SOCKS5 or Tor) with an IPv6 address works correctly via the command line or configuration file in both bitcoind and bitcoin-qt (also from the UI the ipv6 address gets saved properly in settings.json). However, the UI does not reflect this properly, which can create confusion. Since some ISPs and VPNs still experience issues with IPv6, users may mistakenly think there is a problem with Bitcoin Core, when in fact the proxy setup is functioning as expected.

So this PR ensures that the proxy IP is displayed correctly in the UI when using an IPv6 address.

No functionality impact; changes only affect UI display.

Before:
After:

(Ubuntu 22.04)

Start ssh service on localhost.

ssh -D [::1]:1080 -f -C -q -N localhost

Check that the service is up and running.

0ps aux | grep ssh
1pepe    2860289  0.0  0.0  20456  5576 ?        Ss   06:59   0:00 ssh -D [::1]:1080 -f -C -q -N localhost

Check with bitcoind if it works correctly.

bitcoind -onlynet=ipv6 -proxy=[::1]:1080

Check for established connections.

0netstat -natl |grep 1080
1tcp6       0      0 ::1:1080                :::*                    LISTEN     
2tcp6       0      0 ::1:47610               ::1:1080                ESTABLISHED
3tcp6       0      0 ::1:1080                ::1:47610               ESTABLISHED
4tcp6       0      0 ::1:1080                ::1:47606               TIME_WAIT

0[
1   {
2     "id": 0,
3     "addr": "[2a01:4f9:4a:2a07::2]:8333",
4     "addrbind": "[::1]:47638",
5     "network": "ipv6",
6...

Stop bitcoind and run bitcoin-qt adding the corresponding configuration in settings.json.
```
0{
1   "onlynet": "ipv6",
2   "proxy": "[::1]:1080",
3}
```
Open the Peers window to check available connections or run getpeerinfo on the rpc-console window.
Same can be done for Tor setting up tor service (I’ll add instructions later) and configuring on its default port 9050 and forcing "onlynet": "onion" to verify easily the net traffic.

Thanks jarolrod and vasild for your help on validating ipv6 was not broken.

DrahtBot commented at 6:31 pm on September 15, 2024: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild, promag, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

pablomartin4btc renamed this:
~~Fix both ipv6 proxy setup and reachable networks display issues in Options Dialog (UI only, no functionality impact)~~
Fix both display issues for IPv6 proxy setup and reachable networks in Options Dialog (UI only, no functionality impact)
on Sep 15, 2024

DrahtBot added the label CI failed on Sep 15, 2024

DrahtBot commented at 8:46 pm on September 15, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin-core/gui/runs/30169388972

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

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

pablomartin4btc force-pushed on Sep 15, 2024

DrahtBot removed the label CI failed on Sep 16, 2024

in src/qt/optionsdialog.cpp:469 in ba58007588 outdated

465@@ -466,13 +466,13 @@ void OptionsDialog::updateDefaultProxyNets()
466     bool has_proxy;
467 
468     has_proxy = model->node().getProxy(NET_IPV4, proxy);
469-    ui->proxyReachIPv4->setChecked(has_proxy && proxy.ToString() == proxyIpText);
470+    ui->proxyReachIPv4->setChecked((has_proxy && proxy.ToString() == proxyIpText) && g_reachable_nets.Contains(NET_IPV4));

vasild commented at 9:28 am on September 17, 2024:

I think that we are not supposed to access the global state from here, @hebasto?

Also, this change is mixing the “reachable nets” with the “which network is accessed via which proxy” which are two different things. Maybe drop this change altogether?

Since we are here, I have been thinking that those text descriptions are confusing: “Used for reaching peers via [ ] IPv4 [ ] IPv6 [ ] Tor” “Use separte SOCKS5 proxy to reach peers via Tor onion services”. It is not that we reach peers via IPv4. We reach IPv4 peers via the proxy. IPv4 is the type of the peer in this context. Thus, I think this warrants a reword (here or in another PR):

“Used for reaching peers via [ ] IPv4 [ ] IPv6 [ ] Tor” -> “Used for reaching peers of type: [ ] IPv4/IPv6/CJDNS [ ] Tor”. (combine IPv4, IPv6 and CJDNS peers in one checkbox because we always have a single proxy for all those 3 networks, i.e. can’t split it to have proxy1 for IPv4 and proxy2 for IPv6).

“Use separte SOCKS5 proxy to reach peers via Tor onion services” -> “Use separte SOCKS5 proxy to reach Tor peers”.

pablomartin4btc commented at 10:41 am on September 17, 2024:

Also, this change is mixing the “reachable nets” with the “which network is accessed via which proxy” which are two different things. Maybe drop this change altogether?

I get it, that’s correct, but also when -onlynet is specified, g_reachable_nets contains only the nets that are specified by each -onlynet option (ie so in this case -onlynet=ipv6, ipv4 peers won’t be reached by the proxy):

https://github.com/bitcoin-core/gui/blob/225718eda89d65a7041c33e1994d3bf7bd71bbdd/src/init.cpp#L1391-L1399

It is not that we reach peers via IPv4. We reach IPv4 peers via the proxy.

I agree, happy to do it here or on a follow-up to get this merged quicker.

“Used for reaching peers of type: [ ] IPv4/IPv6/CJDNS [ ] Tor”. (combine IPv4, IPv6 and CJDNS peers in one checkbox because we always have a single proxy for all those 3 networks, i.e. can’t split it to have proxy1 for IPv4 and proxy2 for IPv6).

Also when a separate SOCKS5 for Tor is set, the “[ ] Tor” should disappear from the proxy above.

pablomartin4btc commented at 9:45 am on September 18, 2024:

I’ll drop the changes on src/qt/optionsdialog.cpp as suggested by @vasild, since the approach is incorrect.

vasild commented at 10:16 am on September 17, 2024: contributor

The changes to src/qt/optionsmodel.cpp look ok to me.

pablomartin4btc force-pushed on Sep 18, 2024

pablomartin4btc renamed this:
~~Fix both display issues for IPv6 proxy setup and reachable networks in Options Dialog (UI only, no functionality impact)~~
Fix display issues for IPv6 proxy setup in Options Dialog (UI only, no functionality impact)
on Sep 18, 2024

in src/qt/optionsmodel.cpp:325 in a7d8de94ad outdated

324-    QStringList ip_port = GUIUtil::SplitSkipEmptyParts(proxy, ":");
325-    if (ip_port.size() == 2) {
326-        return {true, ip_port.at(0), ip_port.at(1)};
327+    uint16_t port{0};
328+    std::string hostname;
329+    if (!SplitHostPort(proxy.toStdString(), port, hostname) || port != 0) {

vasild commented at 7:12 am on September 19, 2024:

SplitHostPort() returns “true if port-portion is absent or within its allowed range”. Here we want the port to be present and with its allowed range. So if we check that SplitHostPort() is true, that is not sufficient. For our usecase SplitHostPort() must be true and the port must be present. How to check whether the port is present? That would be port != 0. So that condition should be:

0    if (SplitHostPort(proxy.toStdString(), port, hostname) && port != 0) {
1        // valid and port is present and within the valid range

If there is a bug in SplitHostPort() that should be fixed separately.

pablomartin4btc commented at 9:40 am on September 19, 2024:

After playing with the function and the tests (netbase_tests - TestSplitHost - & net_tests - addr.IsValid()), what you propose looks more correct/ understandable to me as the use cases we were looking at were “::1:1080” (which passed to SplitHostPort() returns valid/ true and port 0) vs “[::1]:1080” (returning true and port 1080).

If there is a bug in SplitHostPort() that should be fixed separately.

I think it’s clear to me now that if you have multiple “:” it’s because it’s an ipv6 address and in that case you need to use the brackets to separate it from the port as [ipv6]:port otherwise the split will be ignored and the function will return the whole thing as the hostname (check other cases in TestSplitHost). Perhaps we can add these 2 cases in the tests so there’s no confusion in the future (but there are similar cases with different values).

0BOOST_CHECK(TestSplitHost("::1:1080", "::1:1080", 0, true));
1BOOST_CHECK(TestSplitHost("[::1]:1080", "::1", 1080, true));

vasild commented at 7:13 am on September 19, 2024: contributor

Approach ACK a7d8de94adc3e94ea33d698c307a87ab775c3d20

pablomartin4btc force-pushed on Sep 19, 2024

gui: Fix proxy details display in Options Dialog

- Ensured that the proxy IP is displayed correctly in the UI when using an IPv6 address.

No functionality impact; changes only affect UI display.

fee4cba484

pablomartin4btc force-pushed on Sep 19, 2024

pablomartin4btc commented at 9:53 am on September 19, 2024: contributor

Updates:

Addressed @vasild’s feedback.

vasild approved

vasild commented at 6:45 am on September 20, 2024: contributor

ACK fee4cba48472239f7a426db62f45c4b6af8e5ef2

promag commented at 11:17 pm on October 4, 2024: contributor

Code review ACK fee4cba48472239f7a426db62f45c4b6af8e5ef2.

hebasto approved

hebasto commented at 6:26 pm on October 6, 2024: member

ACK fee4cba48472239f7a426db62f45c4b6af8e5ef2, I have reviewed the code and it looks OK.

hebasto merged this on Oct 6, 2024

hebasto closed this on Oct 6, 2024

Fix display issues for IPv6 proxy setup in Options Dialog (UI only, no functionality impact) #836

Reviews