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: Split DNS resolving functionality out of net structures #7868

Merged
merged 4 commits into from Apr 21, 2016

Conversation

Projects
None yet
6 participants
@theuni
Member

theuni commented Apr 13, 2016

First PR of many in the p2p refactor.

This brings CNetAddr/CSubNet/CService one step closer to being dumb storage structures. By forcing addresses to be resolved elsewhere, the implementation details are free to change. In particular, this is necessary for making the resolves fully async, which is necessary in a model in which the entire connection process is asynchronous.

The DNS seed TODO could be fixed more properly with something like this: theuni@792b0f5 (an ipv6 range would probably make more sense, though), but I'll leave that for another PR.

@dcousens

View changes

Show outdated Hide outdated src/netbase.cpp
@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Apr 13, 2016

Contributor

concept ACK, light utACK 825de38

Contributor

dcousens commented Apr 13, 2016

concept ACK, light utACK 825de38

@jonasschnelli jonasschnelli added the P2P label Apr 13, 2016

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 13, 2016

Member

Concept ACK.

Member

jonasschnelli commented Apr 13, 2016

Concept ACK.

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Apr 13, 2016

Member

meta-comment: Is there an issue open with all the various things to be done for p2p refactor, for a higher-level view? I'd like to review but my knowledge of p2p part of Core is weak. I think a higher-level view may help me review.

Member

instagibbs commented Apr 13, 2016

meta-comment: Is there an issue open with all the various things to be done for p2p refactor, for a higher-level view? I'd like to review but my knowledge of p2p part of Core is weak. I think a higher-level view may help me review.

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Apr 14, 2016

Member

@instagibbs good point. I have another RFC PR coming up today, that will be a good place to discuss general scope/goals. I'll begin writing something up.

Member

theuni commented Apr 14, 2016

@instagibbs good point. I have another RFC PR coming up today, that will be a good place to discuss general scope/goals. I'll begin writing something up.

Show outdated Hide outdated src/init.cpp
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 14, 2016

Member

Concept ACK; utACK apart from one comment

Member

sipa commented Apr 14, 2016

Concept ACK; utACK apart from one comment

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 15, 2016

Member

utACK, good to see DNS lookup being decoupled from connecting.

Member

laanwj commented Apr 15, 2016

utACK, good to see DNS lookup being decoupled from connecting.

net: require lookup functions to specify all arguments
To make it clear where DNS resolves are happening
@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Apr 18, 2016

Member

Updated for @sipa's comment.

@instagibbs I didn't manage to get it ready last week as hoped, but a PR that looks similar to https://github.com/theuni/bitcoin/tree/net-refactor12 plus a lengthy explanation is hopefully coming up in the next day or two. That work aims to segregate the p2p functionality from the rest of the code so that it can be replaced in chunks without interfering with other subsystems.

Member

theuni commented Apr 18, 2016

Updated for @sipa's comment.

@instagibbs I didn't manage to get it ready last week as hoped, but a PR that looks similar to https://github.com/theuni/bitcoin/tree/net-refactor12 plus a lengthy explanation is hopefully coming up in the next day or two. That work aims to segregate the p2p functionality from the rest of the code so that it can be replaced in chunks without interfering with other subsystems.

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Apr 18, 2016

Member

concept ACK, lightly tACK

Member

instagibbs commented Apr 18, 2016

concept ACK, lightly tACK

@sipa

View changes

Show outdated Hide outdated src/net.cpp

theuni added some commits Apr 13, 2016

net: manually resolve dns seed sources
Note: Some seeds aren't actually returning an IP for their name entries, so
they're being added to addrman with a source of [::].

This commit shouldn't change that behavior, for better or worse.
net: resolve outside of storage structures
Rather than allowing CNetAddr/CService/CSubNet to launch DNS queries, require
that addresses are already resolved.

This greatly simplifies async resolve logic, and makes it harder to
accidentally leak DNS queries.
net: disable resolving from storage structures
CNetAddr/CService/CSubnet can no longer resolve DNS.
@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Apr 20, 2016

Member

Addressed @sipa's concern above (I hope). For reference, the following mainnet seeds don't currently resolve their names properly (for me, anyway), so they end up grouped together with sources of [::]:

xf2.org
bluematt.me

I've confirmed that seed results are added to addrman that way before and after this change. I'll work on fixing that in a follow-up pull-request.

Member

theuni commented Apr 20, 2016

Addressed @sipa's concern above (I hope). For reference, the following mainnet seeds don't currently resolve their names properly (for me, anyway), so they end up grouped together with sources of [::]:

xf2.org
bluematt.me

I've confirmed that seed results are added to addrman that way before and after this change. I'll work on fixing that in a follow-up pull-request.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 21, 2016

Member

utACK d39f5b4

Meta question: I am right to expect that there will be follow-ups that split the Lookup functions in resolving ones and non-resolving ones, and make the CNetAddr constructors only depend in the non-resolving ones?

Member

sipa commented Apr 21, 2016

utACK d39f5b4

Meta question: I am right to expect that there will be follow-ups that split the Lookup functions in resolving ones and non-resolving ones, and make the CNetAddr constructors only depend in the non-resolving ones?

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Apr 21, 2016

Member

@sipa yep, exactly.

Member

theuni commented Apr 21, 2016

@sipa yep, exactly.

@sipa sipa merged commit d39f5b4 into bitcoin:master Apr 21, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request Apr 21, 2016

Merge #7868: net: Split DNS resolving functionality out of net struct…
…ures

d39f5b4 net: disable resolving from storage structures (Cory Fields)
3675699 net: resolve outside of storage structures (Cory Fields)
a98cd1f net: manually resolve dns seed sources (Cory Fields)
e9fc71e net: require lookup functions to specify all arguments (Cory Fields)

@kyuupichan kyuupichan referenced this pull request Apr 9, 2017

Merged

Net backports #435

kyuupichan referenced this pull request in kyuupichan/BitcoinUnlimited Apr 29, 2017

Merge #7868: net: Split DNS resolving functionality out of net struct…
…ures

d39f5b4 net: disable resolving from storage structures (Cory Fields)
3675699 net: resolve outside of storage structures (Cory Fields)
a98cd1f net: manually resolve dns seed sources (Cory Fields)
e9fc71e net: require lookup functions to specify all arguments (Cory Fields)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment