net: Allow -proxy=[::1] on nodes with IPV6 lo only #30245

m3dwards · 2024-06-07T12:53:49Z

This is similar to (but does not fix) #13155 which I believe is the same issue but in libevent.

The issue is on a host that has IPV6 enabled but only a loopback IP address -proxy=[::1] will fail as [::1] is not considered valid by getaddrinfo with AI_ADDRCONFIG flag. I think the loopback interface should be considered valid and we have a functional test that will try to test this: feature_proxy.py.

To replicate the issue, run feature_proxy.py inside a docker container that has IPV6 loopback ::1 address without specifically giving that container an external IPV6 address. This should be the default with recent versions of docker. IPV6 on loopback interface was enabled in docker engine 26 and later (https://docs.docker.com/engine/release-notes/26.0/#bug-fixes-and-enhancements-2).

AI_ADDRCONFIG was introduced to prevent slow DNS lookups on systems that were IPV4 only.

References:

Man section on AI_ADDRCONFIG:

If hints.ai_flags includes the AI_ADDRCONFIG flag, then IPv4 addresses are returned in the list pointed to by res only if the local system has at least one IPv4 address configured, and  IPv6  addresses
       are  returned only if the local system has at least one IPv6 address configured.  The loopback address is not considered for this case as valid as a configured address.  This flag is useful on, for ex‐
       ample, IPv4-only systems, to ensure that getaddrinfo() does not return IPv6 socket addresses that would always fail in connect(2) or bind(2).

AI_ADDRCONFIG considered harmful Wiki entry by Fedora

Mozilla discussing slow DNS without AI_ADDRCONFIG and also localhost issues with it

DrahtBot · 2024-06-07T12:53:51Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	tdb3, pinheadmz, achow101
Stale ACK	vasild

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

vasild

Concept ACK on allowing usage of [::1] on nodes with IPv6 lo only.

A workaround is to use -dns=0 (which will avoid AI_ADDRCONFIG but will make it impossible to use hostnames).

I can reproduce the problem on Linux. FreeBSD treats the loopback address as valid, so this problem is non-existent on FreeBSD.

I think there is a merit in this for our use cases of getaddrinfo() / Lookup*():

IPv4 addresses are returned in the list pointed to by res only if the local system has at least one IPv4 address configured ... (and same for IPv6)

But not this:

The loopback address is not considered for this case as valid as a configured address. This flag is useful on, for example, IPv4-only systems, to ensure that getaddrinfo() does not return IPv6 socket addresses that would always fail in connect(2) or bind(2).

This is even bogus to some extent - if the system has only [::1] configured and we want to connect(2) or bind(2) to [::1] that will work. So the "always" is too strong / untrue.

Now, can we have the AI_ADDRCONFIG behavior but get it to consider loopback addresses as valid? For example, after running getaddrinfo() with AI_ADDRCONFIG run it again without AI_ADDRCONFIG and append any loopback addresses from the second run to the results?

src/netbase.cpp

m3dwards · 2024-06-14T16:39:51Z

Now, can we have the AI_ADDRCONFIG behavior but get it to consider loopback addresses as valid? For example, after running getaddrinfo() with AI_ADDRCONFIG run it again without AI_ADDRCONFIG and append any loopback addresses from the second run to the results?

I like this idea, I'll have a go at implementing it.

src/netbase.cpp

m3dwards · 2024-06-18T10:28:39Z

Pushed new approach of calling getaddrinfo twice. First pass is unchanged from original behaviour, second pass adds local IP addresses should they have not been found on the first pass.

vasild

Approach ACK a7cbc27

src/netbase.cpp

m3dwards · 2024-06-21T16:10:03Z

Thanks for your review @vasild, should have addressed all of your comments.

vasild

ACK 60a753b

Thank you!

pinheadmz

ACK 60a753b

Tested in an ubuntu docker container with local ipv6 but not external ipv6:

# ip address | grep inet 
    inet 127.0.0.1/8 scope host lo
    inet6 ::1/128 scope host 
    inet 172.17.0.2/16 brd 172.17.255.255 scope global eth0

Confirmed the feature_proxy test fails without the patch. Added extra logging to confirm the error:

getaddrinfo: ::1 error: Address family for hostname not supported

With the patch, getaddrinfo is called a second time with flags=0 and the call succeeds.

A few questions below.

Show Signature

pinheadmz's public key is on keybase

src/netbase.cpp

pinheadmz · 2024-07-01T16:35:09Z

src/netbase.cpp

+        if (n_err != 0) {
+            return;


You could check specifically for EAI_FAMILY and then only recheck without AI_ADDRCONFIG in that condition. I think the code you have now will always check everything twice, but ignore duplicates?

This comment from @pinheadmz about checking for the specific error we care about makes sense to me as well. It might make for a significantly smaller code change and prevent extraneous calls to getaddrinfo.

I looked into catching specifically EAI_ADDRFAMILY errors but this only applies on GNU Libc and is not POSIX. I checked windows and they have different errors. Rather than try to catalogue every possible system and libc implementation, the code now retries on any error (assuming you first tried with AI_ADDRCONFIG in the first place).

tdb3

Concept ACK
Good find. Left a couple of comments.

tdb3 · 2024-07-02T02:19:11Z

src/netbase.cpp

+        if (n_err != 0) {
+            return;


This comment from @pinheadmz about checking for the specific error we care about makes sense to me as well. It might make for a significantly smaller code change and prevent extraneous calls to getaddrinfo.

src/netbase.cpp

DrahtBot · 2024-07-03T15:44:48Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

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

_{Debug: https://github.com/bitcoin/bitcoin/runs/27000088118}

m3dwards · 2024-07-09T08:52:12Z

@pinheadmz @tdb3 I should have addressed all of your comments.

@vasild the change since your review is rather than always check twice and discard duplicates, we now only check on error. I chose not to look for the specific error as it was not POSIX and was different on different platforms.

tdb3

I think this is really close. Left one more comment.

src/netbase.cpp

AI_ADDRCONFIG prevents ::1 from being considered a valid address on hosts that have a IPV6 loopback IP address but no other IPV6 interfaces.

tdb3

ACK 23333b7

Tested with a similar approach to #30245 (review)

Created an LXC container with an external and loopback IPv4 address, and with a loopback IPv6 address.

root@test30245:~/bitcoin# ip a | grep inet
    inet 127.0.0.1/8 scope host lo
    inet6 ::1/128 scope host noprefixroute 
    inet 192.168.10.81/24 brd 192.168.10.255 scope global eth0

Inserted some extra log statments:

2024-07-15T12:06:09Z =============First try
2024-07-15T12:06:09Z =============n_err != 0
2024-07-15T12:06:09Z =============Trying a second time
2024-07-15T12:06:09Z =============Second time worked!

Appeared to work well.

Also synced approx 8000 blocks on signet with a tor proxy set up to listen on [::1]:9050 (-proxy=[::1] with bitcoind).

m3dwards · 2024-07-15T18:26:10Z

@pinheadmz @vasild

Ready for you to take a second look if you have any time.

pinheadmz

ACK 23333b7

Reviewed updated code change and tested in linux VM with local ipv6 only.

Show Signature

pinheadmz's public key is on keybase

pinheadmz · 2024-07-15T19:44:37Z

src/netbase.cpp

+        if ((ai_hint.ai_flags & AI_ADDRCONFIG) == AI_ADDRCONFIG) {
+            // AI_ADDRCONFIG on some systems may exclude loopback-only addresses
+            // If first lookup failed we perform a second lookup without AI_ADDRCONFIG
+            ai_hint.ai_flags = (ai_hint.ai_flags & ~AI_ADDRCONFIG);


If you need to retouch, you might consider using a local addrinfo variable for the second call since it serves a specific purpose only inside this if condition. It doesn't matter because it's not like it gets used again later, just something I noticed.

Don't have strong feeling either way but there is an argument that doing it with one variable means any changes to flags passed to the first call get automatically added to the second call which is I think better default behaviour.

achow101 · 2024-07-18T21:41:31Z

ACK 23333b7

AI_ADDRCONFIG prevents ::1 from being considered a valid address on hosts that have a IPV6 loopback IP address but no other IPV6 interfaces. Github-Pull: bitcoin#30245 Rebased-From: 23333b7

23333b7 net: Allow DNS lookups on nodes with IPV6 lo only (Max Edwards) Pull request description: This is similar to (but does not fix) bitcoin#13155 which I believe is the same issue but in libevent. The issue is on a host that has IPV6 enabled but only a loopback IP address `-proxy=[::1]` will fail as `[::1]` is not considered valid by `getaddrinfo` with `AI_ADDRCONFIG` flag. I think the loopback interface should be considered valid and we have a functional test that will try to test this: `feature_proxy.py`. To replicate the issue, run `feature_proxy.py` inside a docker container that has IPV6 loopback ::1 address without specifically giving that container an external IPV6 address. This should be the default with recent versions of docker. IPV6 on loopback interface was enabled in docker engine 26 and later ([https://docs.docker.com/engine/release-notes/26.0/#bug-fixes-and-enhancements-2](https://docs.docker.com/engine/release-notes/26.0/#bug-fixes-and-enhancements-2)). `AI_ADDRCONFIG` was introduced to prevent slow DNS lookups on systems that were IPV4 only. References: Man section on `AI_ADDRCONFIG`: ``` If hints.ai_flags includes the AI_ADDRCONFIG flag, then IPv4 addresses are returned in the list pointed to by res only if the local system has at least one IPv4 address configured, and IPv6 addresses are returned only if the local system has at least one IPv6 address configured. The loopback address is not considered for this case as valid as a configured address. This flag is useful on, for ex‐ ample, IPv4-only systems, to ensure that getaddrinfo() does not return IPv6 socket addresses that would always fail in connect(2) or bind(2). ``` [AI_ADDRCONFIG considered harmful Wiki entry by Fedora](https://fedoraproject.org/wiki/QA/Networking/NameResolution/ADDRCONFIG) [Mozilla discussing slow DNS without AI_ADDRCONFIG and also localhost issues with it](https://bugzilla.mozilla.org/show_bug.cgi?id=467497) ACKs for top commit: achow101: ACK 23333b7 tdb3: ACK 23333b7 pinheadmz: ACK 23333b7 Tree-SHA512: 5ecd8c72d1e1c28e3ebff07346381d74eaddef98dca830f6d3dbf098380562fa68847d053c0d84cc8ed19a45148ceb5fb244e4820cf63dccb10ab3db53175020

b654479 Merge bitcoin#30705: test: Avoid intermittent block download timeout in p2p_ibd_stalling (merge-script) 745a819 Merge bitcoin#30690: devtools, utxo-snapshot: Fix block height out of range in script (Ava Chow) 01b570e Merge bitcoin#29999: guix: fix suggested fake date for openssl-1.1.1l (Ava Chow) 432f352 Merge bitcoin#30580: doc: Add note about distro's `g++-mingw-w64-x86-64-posix` version (merge-script) 1bd090e Merge bitcoin#30597: doc: Drop no longer needed workaround for WSL (merge-script) 8a12237 Merge bitcoin#30630: doc: Update ccache website link (merge-script) f66547f Merge bitcoin#30588: depends: fix ZMQ CMake getcachesize check (merge-script) ddaec96 Merge bitcoin#30565: depends: Fix `zeromq` build on OpenBSD (merge-script) e4e5605 Merge bitcoin#30552: test: fix constructor of msg_tx (merge-script) df3c239 Merge bitcoin#26950: cleanse: switch to SecureZeroMemory for Windows cross-compile (merge-script) 57945ce Merge bitcoin#30506: depends: Cleanup postprocess commands after switching to CMake (merge-script) e016ffa Merge bitcoin#29878: depends: build expat with CMake (merge-script) 62dcd43 Merge bitcoin#29880: depends: build FreeType with CMake (merge-script) 745addf Merge bitcoin#30245: net: Allow -proxy=[::1] on nodes with IPV6 lo only (Ava Chow) 4e144be Merge bitcoin-core/gui#795: Keep focus on "Hide" while ModalOverlay is visible (Hennadii Stepanov) 69c04b2 Merge bitcoin#30372: util: Use SteadyClock in RandAddSeedPerfmon (merge-script) ebed8af Merge bitcoin#30336: depends: update doc in Qt pwd patch (merge-script) 9793fb1 Merge bitcoin#30340: test: Added coverage to Block not found error using gettxoutsetinfo (Ava Chow) 479cb8b Merge bitcoin#30312: contrib: add R(UN)PATH check to ELF symbol-check (merge-script) ca83773 Merge bitcoin#30283: upnp: fix build with miniupnpc 2.2.8 (merge-script) 63e139d Merge bitcoin#30185: guix: show `*_FLAGS` variables in pre-build output (merge-script) 3be0d3e Merge bitcoin#30097: crypto: disable asan for sha256_sse4 with clang and -O0 (merge-script) 3070c3e Merge bitcoin#30078: depends: set AR & RANLIB for CMake (merge-script) Pull request description: ## Issue being fixed or feature implemented Trivial backports ## What was done? ## How Has This Been Tested? built locally ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK b654479 kwvg: utACK b654479 Tree-SHA512: 10b5af4e92c83fa9d6764b20bf066bba8e4c600402966fd5c1d6dad07b0549d8a42151a33f21e2f8263336c12a810a6f3fc2828d90bc98153e09c165d9e5b043

DrahtBot added the P2P label Jun 7, 2024

This was referenced Jun 10, 2024

ci: move ASan job to GitHub Actions from Cirrus CI #30193

Merged

RPC wont bind without an IP address on a non-localhost interface #13155

Open

vasild reviewed Jun 14, 2024

View reviewed changes

src/netbase.cpp Outdated Show resolved Hide resolved

m3dwards force-pushed the allow-dns-ipv6-lo-only branch from 84e63f3 to d1337d5 Compare June 18, 2024 09:06

m3dwards commented Jun 18, 2024

View reviewed changes

src/netbase.cpp Show resolved Hide resolved

m3dwards force-pushed the allow-dns-ipv6-lo-only branch from d1337d5 to a7cbc27 Compare June 18, 2024 18:21

DrahtBot added CI failed and removed CI failed labels Jun 18, 2024

vasild reviewed Jun 21, 2024

View reviewed changes

src/netbase.cpp Outdated Show resolved Hide resolved

src/netbase.cpp Outdated Show resolved Hide resolved

src/netbase.cpp Outdated Show resolved Hide resolved

m3dwards force-pushed the allow-dns-ipv6-lo-only branch 2 times, most recently from eacb559 to 60a753b Compare June 21, 2024 16:08

vasild approved these changes Jun 25, 2024

View reviewed changes

pinheadmz approved these changes Jul 1, 2024

View reviewed changes

tdb3 reviewed Jul 2, 2024

View reviewed changes

m3dwards force-pushed the allow-dns-ipv6-lo-only branch from 60a753b to 017a3f7 Compare July 3, 2024 14:34

DrahtBot added the CI failed label Jul 3, 2024

m3dwards force-pushed the allow-dns-ipv6-lo-only branch 2 times, most recently from f95fb4a to e047f4e Compare July 4, 2024 11:37

DrahtBot removed the CI failed label Jul 4, 2024

tdb3 reviewed Jul 9, 2024

View reviewed changes

src/netbase.cpp Outdated Show resolved Hide resolved

net: Allow DNS lookups on nodes with IPV6 lo only

23333b7

AI_ADDRCONFIG prevents ::1 from being considered a valid address on hosts that have a IPV6 loopback IP address but no other IPV6 interfaces.

m3dwards force-pushed the allow-dns-ipv6-lo-only branch from e047f4e to 23333b7 Compare July 15, 2024 10:08

tdb3 approved these changes Jul 15, 2024

View reviewed changes

DrahtBot requested review from pinheadmz and vasild July 15, 2024 16:16

pinheadmz approved these changes Jul 15, 2024

View reviewed changes

achow101 merged commit ec74f45 into bitcoin:master Jul 18, 2024

bitcoin locked and limited conversation to collaborators Jul 18, 2025

net: Allow -proxy=[::1] on nodes with IPV6 lo only #30245

net: Allow -proxy=[::1] on nodes with IPV6 lo only #30245

Uh oh!

Conversation

m3dwards commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

vasild left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

m3dwards commented Jun 14, 2024

Uh oh!

Uh oh!

m3dwards commented Jun 18, 2024

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

m3dwards commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pinheadmz Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

tdb3 Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m3dwards Jul 4, 2024

Choose a reason for hiding this comment

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

tdb3 Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Jul 3, 2024

Uh oh!

m3dwards commented Jul 9, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

m3dwards commented Jul 15, 2024

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

m3dwards Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 18, 2024

Uh oh!

m3dwards commented Jun 7, 2024 •

edited

Loading

DrahtBot commented Jun 7, 2024 •

edited

Loading

vasild left a comment •

edited

Loading

m3dwards commented Jun 21, 2024 •

edited

Loading

tdb3 Jul 2, 2024 •

edited

Loading

tdb3 Jul 2, 2024 •

edited

Loading