Skip to content

net: attempts to connect to all resolved addresses when connecting to a node #28834

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.

Sign up for GitHub

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

Jump to bottom
Merged
achow101 merged 1 commit into from
Apr 25, 2024
Merged

net: attempts to connect to all resolved addresses when connecting to a node #28834

achow101 merged 1 commit into from
Apr 25, 2024

Conversation

@sr-gi
Copy link
Member

@sr-gi sr-gi commented Nov 9, 2023 •
edited by achow101
Loading

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 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 a valid one is found or until we
exhaust them.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 9, 2023 •
edited
Loading

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 vasild, mzumsande, achow101
Concept ACK naumenkogs

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
  • #29231 (logging: Update to new logging API by ajtowns)

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.

@DrahtBot DrahtBot added the P2P label Nov 9, 2023
@DrahtBot DrahtBot mentioned this pull request Nov 9, 2023
Closed
This was referenced Nov 9, 2023
Merged
Merged
@maflcko
Copy link
Member

maflcko commented Nov 13, 2023

CI failed

@sr-gi
Copy link
Member Author

sr-gi commented Nov 15, 2023

There was an issue with the interaction of the change and using a proxy. It should be fixed now

jonatack
jonatack reviewed Nov 15, 2023
View reviewed changes
@sr-gi sr-gi force-pushed the addnode-tryall branch 2 times, most recently from 7029960 to 8efc4c3 Compare November 15, 2023 18:03
mzumsande
mzumsande reviewed Nov 15, 2023
View reviewed changes
Copy link
Contributor

@mzumsande mzumsande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, will review soon.

mzumsande
mzumsande reviewed Dec 19, 2023
View reviewed changes
mzumsande
mzumsande reviewed Dec 20, 2023
View reviewed changes
Copy link
Contributor

@mzumsande mzumsande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 sr-gi changed the title net: Attempts to connect to all resolved addresses on addnode net: attempts to connect to all resolved addresses when connecting to a node Dec 20, 2023
@sr-gi
Copy link
Member Author

sr-gi commented Dec 20, 2023

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.

Updated both commit message/title and PR title/desciption

@DrahtBot DrahtBot mentioned this pull request Jan 17, 2024
Open
@sr-gi
Copy link
Member Author

sr-gi commented Jan 29, 2024

Rebased to fix CI

@naumenkogs
Copy link
Contributor

naumenkogs commented Jan 30, 2024

Concept ACK

@DrahtBot DrahtBot mentioned this pull request Feb 9, 2024
Merged
4 tasks
vasild
vasild reviewed Feb 13, 2024
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK 494c732fabf656764114bfbf824e5aa60c80e2cf, just a naming nit below.

vasild
vasild approved these changes Feb 14, 2024
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK ba021087a2a8d1b5866460a386ea42e0b643c184

vasild
vasild approved these changes Mar 13, 2024
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a75975e1de8062a571afa0a026fc0aee9a6aef56

@sr-gi sr-gi force-pushed the addnode-tryall branch 2 times, most recently from 518019b to 216efd6 Compare March 15, 2024 21:03
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 15, 2024

🚧 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/22724110270

@sr-gi
Copy link
Member Author

sr-gi commented Mar 15, 2024

Rebased to address merge conflicts

@sr-gi
Copy link
Member Author

sr-gi commented Mar 21, 2024

@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

@DrahtBot DrahtBot mentioned this pull request Mar 21, 2024
Closed
mzumsande
mzumsande reviewed Mar 22, 2024
View reviewed changes
Copy link
Contributor

@mzumsande mzumsande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like something went wrong with the rebase, there are two commits with rust code that don't seem related.

@sr-gi
Copy link
Member Author

sr-gi commented Mar 22, 2024

Looks like something went wrong with the rebase, there are two commits with rust code that don't seem related.

Not really sure what happened here 🤔 but should be fixed now

mzumsande
mzumsande reviewed Mar 26, 2024
View reviewed changes
Copy link
Contributor

@mzumsande mzumsande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK ae7afa5547889ccf04c1522555e28e025d14f335

@DrahtBot DrahtBot requested a review from vasild March 26, 2024 16:49
vasild
vasild approved these changes Apr 18, 2024
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK ae7afa5547889ccf04c1522555e28e025d14f335

@sr-gi
net: attempts to connect to all resolved addresses when connecting to…
fd81a37 
… 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.
@sr-gi sr-gi requested review from mzumsande and vasild April 18, 2024 14:39
vasild
vasild approved these changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fd81a37

@mzumsande
Copy link
Contributor

mzumsande commented Apr 22, 2024

re-ACK fd81a37 (just looked at diff, only small logging change)

@achow101
Copy link
Member

achow101 commented Apr 25, 2024

ACK fd81a37

@achow101 achow101 merged commit 2eff198 into bitcoin:master Apr 25, 2024
@bitcoin bitcoin locked and limited conversation to collaborators Apr 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@naumenkogs naumenkogs Awaiting requested review from naumenkogs

@mzumsande mzumsande Awaiting requested review from mzumsande

2 more reviewers

@jonatack jonatack jonatack left review comments

@vasild vasild vasild approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@sr-gi @DrahtBot @maflcko @naumenkogs @mzumsande @achow101 @vasild @jonatack