net: attempts to connect to all resolved addresses when connecting to a node #28834
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. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
CI failed |
sr-gi
force-pushed
the
addnode-tryall
branch
from
6f0affc
to
3bb2cd9
Compare
|
There was an issue with the interaction of the change and using a proxy. It should be fixed now |
sr-gi
force-pushed
the
addnode-tryall
branch
2 times, most recently
from
7029960
to
8efc4c3
Compare
sr-gi
force-pushed
the
addnode-tryall
branch
from
8efc4c3
to
9c1ab79
Compare
There was a problem hiding this comment.
Would be good to adjust the title / commit message to be more general:
The solution doesn't specifically address addnode but changes OpenNetworkConnection / ConnectNode and therefore all callers that use it with pszDest set (-connect, -seednode, test-only -addconnection) are affected by the change.
sr-gi
force-pushed
the
addnode-tryall
branch
from
9c1ab79
to
dc786b5
Compare
sr-gi
changed the title
addnode
Updated both commit message/title and PR title/desciption |
|
Rebased to fix CI |
|
Concept ACK |
sr-gi
force-pushed
the
addnode-tryall
branch
from
494c732
to
ba02108
Compare
sr-gi
force-pushed
the
addnode-tryall
branch
2 times, most recently
from
518019b
to
216efd6
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. |
|
Rebased to address merge conflicts |
|
@vasild @mzumsande @naumenkogs would you mind re-checking this? It feels like it's been ready for a while but it keeps being 1 ack short |
Not really sure what happened here 🤔 but should be fixed now |
… a node Prior to this, when establishing a network connection via CConnman::ConnectNode, if the connection needed address resolution, a single address would be picked at random from the resolved addresses and our node will try to connect to it. However, this would lead to the behavior of ConnectNode being unpredictable when the address was resolved to various ips (e.g. the address resolving to IPv4 and IPv6, but we only support one of them). This patches the aforementioned behavior by going over all resolved IPs until we find one we can connect to or until we exhaust them.
|
re-ACK fd81a37 (just looked at diff, only small logging change) |
|
ACK fd81a37 |
This is a follow-up of #28155 motivated by #28155 (comment)
Rationale
Prior to this, when establishing a network connection via
CConnman::ConnectNode, if the connection needed address resolution, a single address would be picked at random from the resolved addresses and our node would try to connect to it. However, this would lead to the behavior ofConnectNodebeing unpredictable when the address was resolved to various ips (e.g. the address resolving to IPv4 and IPv6, but we only support one of them).This patches the aforementioned behavior by going over all resolved IPs until a valid one is found or until we
exhaust them.