Allow IPv6 connections to Bitcoin nodes #4045

ghost · 2020-03-11T04:04:14Z

Currently bisq desktop does not accept IPv6 addresses in the settings for custom nodes or via the --btcNodes command line option.
While the address format is already validated in desktop / GUIUtil, the separation of address and port is handled incorrectly in core / BtcNodes::fromFullAddress.
This results in IPv6 addresses being ignored, regardless of whether they are entered via the GUI or command line.

This change fixes the code in BtcNodes::fromFullAddress to identify an IPv6 address as being surrounded by square brackets.

IPv6 addresses are a sequence of hexadecimal and ':' characters enclosed by square brackets, followed by an optional colon and port number
Onion addresses are a sequence of alphanumeric characters followed by .onion and an optional colon and port number
IPv4 addresses are a sequence of numeric and '.' characters followed by an optional colon and port number

Testing:
the following use case tests were identified and checked:

onion address no port
onion address with port
IPv4 address no port
IPv4 address with port
IPv6 address no port
IPv6 address with port
combination of all the above types
invalid input

ghost · 2020-03-11T13:32:04Z

While initially working ok in clearnet, I found that if 'Use Tor for Bitcoin network' is enabled, IPv6 clearnet addresses are ignored. DnsLookupTor is hard coded to expect 4 bytes in response from the proxy, however with IPv6, 16 bytes are returned. I'm making a subsequent commit here to address this second issue.

wiz · 2020-03-16T15:16:04Z

Can you fix the commit messages so they are capitalized and well documented like

Allow IPv6 connections to Bitcoin nodes (fixes #3990)
Allow IPv6 response (16 byte) from Tor DNS lookup

wiz

NACK

Test case 1 failed:

Set btcnode configuration to node100.wiz.network,node130.wiz.network,node140.wiz.network,node150.wiz.network
Expected) Seeing IPv6 addresses in settings -> network status
Actual) Seeing IPv4 addresses in settings -> network status

Test case 2 failed:

Set btcnode configuration to 2401:b140::42:100,2401:b140::42:130,2401:b140::42:140,2401:b140::42:150
Expected) Connects to IPv6 btcnodes
Actual) Bisq fails to start due to "invalid configuration"

wiz · 2020-03-16T16:06:36Z

I did get it working using the [aaaa::bbbb]:8333 notation, but I believe most users will do the above 2 notations so please fix those. Great work tho.

ghost · 2020-03-16T16:33:26Z

The IP address is obtained from the user parameters by calling DNS lookup in DnsLookupTor.java. So for example node100.wiz.network is passed to DNS, what is returned is either and IPv4 (4 byte buffer) or IPv6 (16 byte buffer). For this example it resolves to 103.99.168.100. Maybe there is a parameter that can be written to the DNS socket telling it that should be IPv6 rather than IPv4? I'll try to find out.

ghost · 2020-03-16T19:11:49Z

DnsLookupTor.java is doing the same SOCKS5 network query as tor-resolve. Namely a CMD_RESOLVE:

buf[0] = b('\u0005');             // version SOCKS5
buf[1] = b('\u00f0');             // CMD_RESOLVE
buf[2] = b('\u0000');             // reserved 
buf[3] = b('\u0003');             // SOCKS5_ATYPE_HOSTNAME
[...]
proxySocket.getOutputStream().write(buf);

Unfortunately:

Only drawback so far: You can't specify the query type (e.g. A, NS, MX, etc.) and also IPv6-only hosts (those with only AAAA records) can't (yet) be resolved at all and cause such a message: [warn] Got SOCKS5 status response '4': host is unreachable

What we would need is the tor DNS equivalent of

nslookup -q=AAAA node100.wiz.network
node100.wiz.network	has AAAA address 2401:b140::42:100

But we get this

tor-resolve node100.wiz.network
103.99.168.100

From reading tor-resolve code, there's no parameter that can be written to the DNS socket telling it that we want IPv6 result rather than IPv4.

I'm open to suggestions.

wiz · 2020-03-17T00:37:24Z

Why you close this PR ? Don't give up so easily lol

I'm open to suggestions.

Why are you using Tor for DNS if Use Tor for Bitcoin network is not checked?

ghost · 2020-03-17T03:18:43Z

Why are you using Tor for DNS if Use Tor for Bitcoin network is not checked?

Bisq is not using Tor when that option is unchecked. However Java is defaulting to IPv4 for the name lookup. Setting the following option with the JavaVM is necessary to make Java default to IPv6 in the case where both address types are available. -Djava.net.preferIPv6Addresses=true

Success!

wiz · 2020-03-17T03:22:52Z

Good work. Perhaps the "prefer IPv6 addresses" should be a setting in the Bisq preferences GUI, and then you can set the JVM options appropriately at startup if the preference is set.

ripcurlx · 2020-03-17T15:20:19Z

Good work. Perhaps the "prefer IPv6 addresses" should be a setting in the Bisq preferences GUI, and then you can set the JVM options appropriately at startup if the preference is set.

~~Could you please add this as suggested by @wiz to the preferences and set it manually (System.setProperty("java.net.preferIPv6Addresses", "true");) if applicable during runtime?~~

That was a lucky timing with your last commit 😉 Could you please adapt the wording of your last commit messages, so it speaks without needing the context of @wiz comment in this PR? Thanks.

ripcurlx · 2020-03-17T15:23:08Z

common/src/main/java/bisq/common/setup/CommonSetup.java

+
+        // #3990 allow IPv6 custom Bitcoin node in settings by hostname
+        System.setProperty("java.net.preferIPv6Addresses", "true");


Is there a drawback when we set this as default for all nodes?

Compared to doing it based on a user preference.

I have not seen any drawback from setting this, even if IPv6 is not available. Here's a document on the subject, which mentions setting the property. And another one which mentions that the setting has to be made very early in app startup.

If you think its too risky we can revert to choosing between IPv4 and IPv6 addresses in BtcNodeConverter where it creates a PeerAddress from hostname.

ripcurlx · 2020-03-18T09:41:14Z

@jmacxx Please sign your commit. Thanks!

wiz

Tested the following in GUI preferences:

node100.wiz.network,node130.wiz.network,node140.wiz.network,node150.wiz.network
node100.wiz.network:8333,node130.wiz.network:8333,node140.wiz.network:8333,node150.wiz.network:8333
2401:b140::42:100,2401:b140::44:130,2401:b140::44:140,2401:b140::44:150
[2401:b140::42:100]:8333,[2401:b140::44:130]:8333,[2401:b140::44:140]:8333,[2401:b140::44:150]:8333

All passed except 2, can you fix it?

wiz

A safer way to implement this would be:

if "Use Tor" is unchecked, enable a new radio button for "Prefer IPv4" or "Prefer IPv6"
if "Prefer IPv6" is selected, then set the JVM option java.net.preferIPv6Addresses=true

Currently bisq desktop does not accept IPv6 addresses in the settings for custom nodes or via the --btcNodes command line option. The separation of address and port is handled incorrectly in core / BtcNodes::fromFullAddress. This results in IPv6 addresses being ignored. Where Tor is enabled for Bitcoin connections, we need to handle the IPv6 address response from Tor DNS lookup. Fixes #3990

wiz

ACK

After discussion, we concluded that "fixing ipv6" and "preferring ipv6" are very separate scopes, and since "preferring ipv6" has potential side effects, we agreed it would be safer to limit the scope of this PR to only "fixing ipv6", which means the expected case for testcases 3 and 4 below is not to connect to IPv6, but to use the JVM default setting, so now all testcases pass. Tested on macOS and Linux using single stack and dual stack configurations.

2401:b140::42:100,2401:b140::44:130,2401:b140::44:140,2401:b140::44:150
[2401:b140::42:100]:8333,[2401:b140::44:130]:8333,[2401:b140::44:140]:8333,[2401:b140::44:150]:8333
node100.wiz.network,node130.wiz.network,node140.wiz.network,node150.wiz.network
node100.wiz.network:8333,node130.wiz.network:8333,node140.wiz.network:8333,node150.wiz.network:8333

ripcurlx

utACK based on #4045 (review)

wiz · 2020-03-20T20:25:57Z

@devinbileck can you please make a note to test both IPv4 single-stack and IPv4/IPv6 dual-stack configurations in your next release testing?

wiz suggested changes Mar 16, 2020

View reviewed changes

ghost closed this Mar 16, 2020

ghost reopened this Mar 17, 2020

ripcurlx reviewed Mar 17, 2020

View reviewed changes

ripcurlx requested a review from wiz March 18, 2020 09:39

wiz suggested changes Mar 18, 2020

View reviewed changes

ripcurlx added the waiting for author label Mar 19, 2020

wiz approved these changes Mar 19, 2020

View reviewed changes

ripcurlx approved these changes Mar 20, 2020

View reviewed changes

ripcurlx merged commit 17bb7b4 into bisq-network:master Mar 20, 2020

ripcurlx added is:priority PR or issue marked with this label is up for compensation and removed waiting for author labels Mar 20, 2020

ripcurlx added this to the v1.3.0 milestone Apr 1, 2020

ghost mentioned this pull request Apr 7, 2020

For Cycle 12 bisq-network/compensation#523

Closed

ripcurlx mentioned this pull request Apr 15, 2020

For Cycle 12 bisq-network/compensation#542

Closed

ghost deleted the fix_btcnode_ipv6 branch May 16, 2020 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow IPv6 connections to Bitcoin nodes #4045

Allow IPv6 connections to Bitcoin nodes #4045

ghost commented Mar 11, 2020

ghost commented Mar 11, 2020

wiz commented Mar 16, 2020 •

edited

Loading

wiz left a comment

wiz commented Mar 16, 2020

ghost commented Mar 16, 2020

ghost commented Mar 16, 2020

wiz commented Mar 17, 2020

ghost commented Mar 17, 2020

wiz commented Mar 17, 2020

ripcurlx commented Mar 17, 2020 •

edited

Loading

ripcurlx Mar 17, 2020

ripcurlx Mar 17, 2020

ghost Mar 17, 2020

ripcurlx commented Mar 18, 2020

wiz left a comment

wiz left a comment

wiz left a comment

ripcurlx left a comment

wiz commented Mar 20, 2020


		// #3990 allow IPv6 custom Bitcoin node in settings by hostname
		System.setProperty("java.net.preferIPv6Addresses", "true");

Allow IPv6 connections to Bitcoin nodes #4045

Allow IPv6 connections to Bitcoin nodes #4045

Conversation

ghost commented Mar 11, 2020

ghost commented Mar 11, 2020

wiz commented Mar 16, 2020 • edited Loading

wiz left a comment

Choose a reason for hiding this comment

wiz commented Mar 16, 2020

ghost commented Mar 16, 2020

ghost commented Mar 16, 2020

wiz commented Mar 17, 2020

ghost commented Mar 17, 2020

wiz commented Mar 17, 2020

ripcurlx commented Mar 17, 2020 • edited Loading

ripcurlx Mar 17, 2020

Choose a reason for hiding this comment

ripcurlx Mar 17, 2020

Choose a reason for hiding this comment

ghost Mar 17, 2020

Choose a reason for hiding this comment

ripcurlx commented Mar 18, 2020

wiz left a comment

Choose a reason for hiding this comment

wiz left a comment

Choose a reason for hiding this comment

wiz left a comment

Choose a reason for hiding this comment

ripcurlx left a comment

Choose a reason for hiding this comment

wiz commented Mar 20, 2020

wiz commented Mar 16, 2020 •

edited

Loading

ripcurlx commented Mar 17, 2020 •

edited

Loading