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

Add support for dnsseeds with option to filter by servicebits #8083

Merged
merged 1 commit into from Jun 8, 2016

Conversation

Projects
None yet
7 participants
@jonasschnelli
Member

jonasschnelli commented May 21, 2016

Opposite part of sipa/bitcoin-seeder#36.
Including new testnet seed that supports filtering.

Required for SW #7910.

@jonasschnelli jonasschnelli added the P2P label May 21, 2016

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd May 23, 2016

Contributor

Concept ACK, but I think it'd be simpler to have this be based on a bitmask rather than service-specific letters.

Contributor

petertodd commented May 23, 2016

Concept ACK, but I think it'd be simpler to have this be based on a bitmask rather than service-specific letters.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 23, 2016

Member

Yes. Agreed. Will change it to a bitmask. Maybe a hex representation of the uint64_t as subdomain.

Member

jonasschnelli commented May 23, 2016

Yes. Agreed. Will change it to a bitmask. Maybe a hex representation of the uint64_t as subdomain.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt May 23, 2016

Contributor

Do we actually connect to DNS Seeds, or is it only for Tor where we connect to them, get a list of addresses, and then immediately disonnect?

Contributor

TheBlueMatt commented May 23, 2016

Do we actually connect to DNS Seeds, or is it only for Tor where we connect to them, get a list of addresses, and then immediately disonnect?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 23, 2016

Member
Member

sipa commented May 23, 2016

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 24, 2016

Member

Updated this PR and sipa/bitcoin-seeder#36 to support non-bitflags-to-string mapping.
Now, node-filtering via dnsseed is possible with a bitmap-filter-hex-string as subdomain.

Example:
dig A 5.testnet-seed.bitcoin.jonasschnelli.ch (filter = 5 == 0101) will return only nodes with NODE_NETWORK and NODE_BLOOM service flags set.
dig A d.testnet-seed.bitcoin.jonasschnelli.ch (filter = d == 1101) will return only nodes with NODE_NETWORK, NODE_BLOOM and NODE_WITNESS service flags set.

A bar main domain query will still result with the default NODE_NETWORK filter:
dig A testnet-seed.bitcoin.jonasschnelli.ch

Member

jonasschnelli commented May 24, 2016

Updated this PR and sipa/bitcoin-seeder#36 to support non-bitflags-to-string mapping.
Now, node-filtering via dnsseed is possible with a bitmap-filter-hex-string as subdomain.

Example:
dig A 5.testnet-seed.bitcoin.jonasschnelli.ch (filter = 5 == 0101) will return only nodes with NODE_NETWORK and NODE_BLOOM service flags set.
dig A d.testnet-seed.bitcoin.jonasschnelli.ch (filter = d == 1101) will return only nodes with NODE_NETWORK, NODE_BLOOM and NODE_WITNESS service flags set.

A bar main domain query will still result with the default NODE_NETWORK filter:
dig A testnet-seed.bitcoin.jonasschnelli.ch

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd May 24, 2016

Contributor

RBF nodes! dig A 4000001.testnet-seed.bitcoin.jonasschnelli.ch Cool!

Something I noticed was you could also query 0000000004000001.testnet-seed.bitcoin.jonasschnelli.ch instead - it'd probably be a good idea to fail the query if there are any leading zeros, to maximally ensure that everyone uses the same queries whenever possible - better for privacy, and a few less bytes on the wire.

Contributor

petertodd commented May 24, 2016

RBF nodes! dig A 4000001.testnet-seed.bitcoin.jonasschnelli.ch Cool!

Something I noticed was you could also query 0000000004000001.testnet-seed.bitcoin.jonasschnelli.ch instead - it'd probably be a good idea to fail the query if there are any leading zeros, to maximally ensure that everyone uses the same queries whenever possible - better for privacy, and a few less bytes on the wire.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 24, 2016

Member

RFC 952 did not permit hostnames to begin with a number. This has since been relaxed but many implementations still manage to screw this up (treating the names that begin with numbers as IP addresses), it might be better to begin the mask with a x, and also require it to be zero extended so that caching isn't degraded by clients using x00000005 vs x5.

Member

gmaxwell commented May 24, 2016

RFC 952 did not permit hostnames to begin with a number. This has since been relaxed but many implementations still manage to screw this up (treating the names that begin with numbers as IP addresses), it might be better to begin the mask with a x, and also require it to be zero extended so that caching isn't degraded by clients using x00000005 vs x5.

@arowser

This comment has been minimized.

Show comment
Hide comment
@arowser

arowser May 25, 2016

Contributor

Can one of the admins verify this patch?

Contributor

arowser commented May 25, 2016

Can one of the admins verify this patch?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 25, 2016

Member

@gmaxwell: Good point.
I just force pushed a change that will require a leading x for the query subdomain.
dig A x5.testnet-seed.bitcoin.jonasschnelli.ch for NODE_BLOOM, etc.

leading zeros will be ignored: dig A x0005.testnet-seed.bitcoin.jonasschnelli.ch also results in a NODE_BLOOM filter.

Member

jonasschnelli commented May 25, 2016

@gmaxwell: Good point.
I just force pushed a change that will require a leading x for the query subdomain.
dig A x5.testnet-seed.bitcoin.jonasschnelli.ch for NODE_BLOOM, etc.

leading zeros will be ignored: dig A x0005.testnet-seed.bitcoin.jonasschnelli.ch also results in a NODE_BLOOM filter.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 25, 2016

Member
Member

sipa commented May 25, 2016

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 25, 2016

Member

@sipa: Good point.
Update the seeder PR to ignore/reject leading zeros.
Rejecting = reply with standard NODE_NETWORK filter (same behavior as before this PR).
https://github.com/sipa/bitcoin-seeder/pull/36/files#diff-118fcbaaba162ba17933c7893247df3aR263

Member

jonasschnelli commented May 25, 2016

@sipa: Good point.
Update the seeder PR to ignore/reject leading zeros.
Rejecting = reply with standard NODE_NETWORK filter (same behavior as before this PR).
https://github.com/sipa/bitcoin-seeder/pull/36/files#diff-118fcbaaba162ba17933c7893247df3aR263

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 25, 2016

Member

How will you pass in the required flags to addrman?

I was imagining something simpler here, where a DNSSeedData struct just has a hostname and a servicebits value. For hosts where it is supported, we use ("x9.seeder.host.tld", 9), for others we use ("seeder.host.tld", 1).

Member

sipa commented May 25, 2016

How will you pass in the required flags to addrman?

I was imagining something simpler here, where a DNSSeedData struct just has a hostname and a servicebits value. For hosts where it is supported, we use ("x9.seeder.host.tld", 9), for others we use ("seeder.host.tld", 1).

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 25, 2016

Member

@sipa: Right. The code is currently pretty much unused.
At the moment, what we want is filter for NODE_NETWORK (which is the default). This results in not using the x.seed filter.

However, this will be required for SW and I though instead of extending the SW PR, try to get this into master because it can be useful anyways.

This also adds a testnet seed (last weekend only one testnet seed was running).

Member

jonasschnelli commented May 25, 2016

@sipa: Right. The code is currently pretty much unused.
At the moment, what we want is filter for NODE_NETWORK (which is the default). This results in not using the x.seed filter.

However, this will be required for SW and I though instead of extending the SW PR, try to get this into master because it can be useful anyways.

This also adds a testnet seed (last weekend only one testnet seed was running).

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 30, 2016

Member

utACK 2d83013

what we want is filter for NODE_NETWORK (which is the default)

I think there's something (for consistency / future-proofness) to be said for always using filtering when the seed supports this, even to just request NODE_NETWORK, instead of relying on a default being the same server-side.

Though this would remove the flexibility for the DNS seed operator to change the default and have everyone use that by default, if that turns out to be necessary - which has been discussed in the context of segwit. So I'm not sure.

Member

laanwj commented May 30, 2016

utACK 2d83013

what we want is filter for NODE_NETWORK (which is the default)

I think there's something (for consistency / future-proofness) to be said for always using filtering when the seed supports this, even to just request NODE_NETWORK, instead of relying on a default being the same server-side.

Though this would remove the flexibility for the DNS seed operator to change the default and have everyone use that by default, if that turns out to be necessary - which has been discussed in the context of segwit. So I'm not sure.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 8, 2016

Member

What is there to be done here?

Member

laanwj commented Jun 8, 2016

What is there to be done here?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 8, 2016

Member

utACK 2d83013

Member

sipa commented Jun 8, 2016

utACK 2d83013

@sipa sipa merged commit 2d83013 into bitcoin:master Jun 8, 2016

1 check passed

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

sipa added a commit that referenced this pull request Jun 8, 2016

Merge #8083: Add support for dnsseeds with option to filter by servic…
…ebits

2d83013 Add support for dnsseeds with option to filter by servicebits (Jonas Schnelli)

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #8083: Add support for dnsseeds with option to filter by servic…
…ebits

2d83013 Add support for dnsseeds with option to filter by servicebits (Jonas Schnelli)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #8083: Add support for dnsseeds with option to filter by servic…
…ebits

2d83013 Add support for dnsseeds with option to filter by servicebits (Jonas Schnelli)

codablock added a commit to codablock/dash that referenced this pull request Dec 22, 2017

Merge #8083: Add support for dnsseeds with option to filter by servic…
…ebits

2d83013 Add support for dnsseeds with option to filter by servicebits (Jonas Schnelli)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment