-
Notifications
You must be signed in to change notification settings - Fork 35.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
net: Allow -proxy=[::1] on nodes with IPV6 lo only #30245
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
I like this idea, I'll have a go at implementing it. |
84e63f3
to
d1337d5
Compare
Pushed new approach of calling |
d1337d5
to
a7cbc27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK a7cbc27
eacb559
to
60a753b
Compare
Thanks for your review @vasild, should have addressed all of your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 60a753b
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 60a753b7b38fbe89494f8df66f13a84f28af244b
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmaC23IACgkQ5+KYS2KJ
yTrxpQ/8Cz+gj6bv5wNZ/Nuf3N4uV1I2xScdU8LEjt9RfkppplQaDvExq7Bv3rBt
7eSOeuODMWmy+yjSS69NYcuHIg79mDWHNz7XVFX2uqQ0dpix+CaQPYvGDMfiOYR0
XbXNTcx8tCID+TdvPE1QC4iLQitjwwAtD8c9CNupzEy0r5z5JtyxMjUCWcB68LJa
NvKDjRQtO/+D4I7uv0U0Mq+lDFloRYr6byWP9US6Z/mBeBdlvev/82+jkrhstB3q
tYspNR725QBxh5rLjgxoBGjPmjJfyLBpwCGqGFzLVI8BM1oGaa/YYFLE9NK0WRuk
8xKAbXwIu7CkjF859vlgthzROJPq3FHcK/8T9wZFpv6QvDZbaaN4WwpDjI8zFgqI
BH658s+7yNlrjM6JJL5qHD71h3bztvYp6EpzUD1eTqHTY9OumDPc7av4E2ooDDFv
a3xjokiP1JB4qmjqkXExqwdPQ8WayXAOolMAHCu7+IqvhWjzSFBoovbdpChz/kIX
F3fJXbF1qEM8ybjk0Oroja6jVIeCjdKlDajR3ZUmFevkTTMRy7kDPktlbKXKBFz7
uItf5JZ94aQg9TobJLbKXAkTUeFY57rt1Ucxn0vs7zGJQCLVloxOXNhc13FX8iD8
KoUULcryDATRMNxKWieBKMA7RnEOAiJjEZmj47X73pnuxsIL9kc=
=zVvS
-----END PGP SIGNATURE-----
pinheadmz's public key is on keybase
src/netbase.cpp
Outdated
if (n_err != 0) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
Good find. Left a couple of comments.
src/netbase.cpp
Outdated
if (n_err != 0) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
60a753b
to
017a3f7
Compare
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
f95fb4a
to
e047f4e
Compare
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is really close. Left one more comment.
AI_ADDRCONFIG prevents ::1 from being considered a valid address on hosts that have a IPV6 loopback IP address but no other IPV6 interfaces.
e047f4e
to
23333b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Ready for you to take a second look if you have any time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 23333b7
Reviewed updated code change and tested in linux VM with local ipv6 only.
Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 23333b7ed243071c9b4e4f04c727556d8065acbb
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmaVfcgACgkQ5+KYS2KJ
yTorBw/7BFoooqdMN+Zb000m/qKsUoMhW36vZBdGTuxt3xkWrRHlggm8ym/fQfen
jt9VcWYl6vmZFnU+nUvJbdN+GLTdf+fDT8vdJdYKjKSxmflEIGLRnf5Pktf2E8lG
rycNQqa6rxcMcnHu0p69VWUU2Ay3vXTV3OOZ/UJFMTLo1twLyljxsSwu6JvMSKgM
4qOqFnqxVeJ1fq3Lw7TtjbwvKweDU8iw2B/YE6kXg711u0qdbSHe3MiPiXKl0sRp
RCkFXuO/q0GICUfW+XHqfs4tDikD+5TRhZ4bBRond7KIdYSdizZzgkNdI1elcI9B
/exMlaLD4cJAEeqovhgNpSByI3rJKQMmzUWS2GrzU78Rsk1zg3a0L+XioM7BMrMm
ClZs82CER6VDNApCv1pEZkdF2AsrbCXDT9GmmpIearoTWAKhR2GP9hccUxKOeZB6
qtb5ep5SvyjM+hIj4IBTM6g0i24SvfdP3/9WcuYj5G1Cl7sWVI1tdXJIDP2hMCij
7J9yC9YznQpvx9626yASNpFq/WRcC4Iug7WdTrSlnIBCQDJ9wMt1uzK6Aj/SdDI+
vKE45GqpYag7MPKnNSobDSe83JWssPy8ZNP1W8FmBV1Vd4yRMdKJea4yrmNYDCwC
CiC31ZufOkWBdb4n+rduarigMQxEOQMm/8nqFCKdl7GFaIcUjPQ=
=wUuO
-----END PGP SIGNATURE-----
pinheadmz's public key is on keybase
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
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 bygetaddrinfo
withAI_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
:AI_ADDRCONFIG considered harmful Wiki entry by Fedora
Mozilla discussing slow DNS without AI_ADDRCONFIG and also localhost issues with it