Use GetDesireableServiceFlags in seeds, dnsseeds, fixing static seed adding #11512

TheBlueMatt · 2017-10-16T21:45:35Z

4440710 broke inserting entries into addrman from dnsseeds which
did not support service bits, as well as static seeds. Static seeds
were already being filtered by UA for 0.13.1+ (ie NODE_WITNESS), so
simply changing the default service bits to include NODE_WITNESS
(and updating docs appropriately) is sufficient. For DNS Seeds, not
supporting NODE_WITNESS is no longer useful, so instead use
non-filtering seeds as oneshot hosts irrespective of named proxy.

I've set my testnet-seed to also support x9, though because it is simply a static host, it may be useful to leave the support off so that it is used as a oneshot to get addresses from a live node instead. I'm fine with either.

sipa · 2017-10-18T08:53:59Z

src/chainparams.h

@@ -16,6 +16,7 @@

 struct CDNSSeedData {
    std::string host;
+    //! Implies that the dnsseed can filter for at least xGetDesireableSerivceFlags(NODE_NONE)


Typo: xGetDesireableServiceFlags?

x was deliberate (as thats the dns prefix), the e is just because i cant spell desirable.

theuni · 2017-10-18T21:23:01Z

src/net.cpp

@@ -1621,10 +1620,16 @@ void CConnman::ThreadDNSAddressSeed()
        if (HaveNameProxy()) {
            AddOneShot(seed.host);
        } else {
+            if (!seed.supportsServiceBitsFiltering) {


What if we just always tried the desired subdomain, and fell back to the OneShot on the host if the resolve fails? That way seed operators can update for new flags in the future, and we can get rid of the supportsServiceBitsFiltering logic.

Does that not have any annoying interactions with async name lookup changes?

Nah, async resolves will just take a functor specifying what to do with the result. In this case, it'll just do AddOneShot(seed.host) on failure.

That would essentially drop @luke-jr DNS seed. @luke-jr: plans to support filtering by service bits soon?

Using a DNS Seed as a oneshot still provides at least some useful result, and is what we do for Tor nodes anyway, so I'd somewhat prefer to at least do that. Took @theuni's suggestion here.

jonasschnelli · 2017-10-19T18:53:09Z

Needs rebase.

TheBlueMatt · 2017-10-19T21:35:56Z

Rebased.

theuni · 2017-10-19T22:04:54Z

I like this better, concept ACK. The only issue I have with it is that we'll now only create a oneshot to a single resolve result if the subdomain isn't supported, meaning that it's pretty damn important to have subdomain support.

One alternative to this could be to add the notion of oneshot-only peers to addrman: If the source of an address is a special internal value (NET_INTERNAL), assume that it came from a dns seed without filtering. That way, any time addrman chooses that address, it's only usable for oneshots.

I don't think it's necessary to go that far until filtering is sufficiently complicated, though.

TheBlueMatt · 2017-10-19T22:10:06Z

OneShot-only seems like it would uneccessarily delay finding your first peer(s). The fact that oneshot-only makes it pretty important that you find a peer on the first IP you try really needs to be fixed for onlynet=onion nodes anyway, though I agree its relatively important to have seeds that support the service bits set.

theuni · 2017-10-19T22:17:02Z

I wasn't implying that we'd only use oneshot-only, just that after a failed subdomain resolve, we'd do the tld resolve and add all results to addrman as oneshot-only. That way worst-case (no subdomain resolves) you'd get a few dozen oneshot-only peers for bootstrapping. But best-case, all subdomains resolve and we add no oneshots.

Regardless, I don't think that's necessary yet. It would be a good idea, though, to log a subdomain resolve failure, so that it's obvious which ones lack support.

TheBlueMatt · 2017-10-19T22:19:13Z

I'd somewhat rather implement logic that allows you to repeatedly connect to the dnsseed as oneshot using a random result from Lookup, this way (I believe) onlynet=onion hosts will get the same improvements, but, indeed, not in this PR.

TheBlueMatt · 2017-11-06T21:59:22Z

Needs an 0.16 tag since this fixes a bug currently in master.

TheBlueMatt · 2017-11-06T22:25:13Z

Further clarified comment on desirable service flags.

theuni · 2017-11-14T22:04:22Z

utACK c7f5f4ef8990c5abdc1f3f2f76771c46b7e08b07

theuni · 2017-12-20T23:16:12Z

Needs rebase. Also, bump for visibility :)

sipa · 2017-12-20T23:25:04Z

src/net.cpp

        } else {
            std::vector<CNetAddr> vIPs;
            std::vector<CAddress> vAdd;
            ServiceFlags requiredServiceBits = GetDesirableServiceFlags(NODE_NONE);
-            std::string host = GetDNSHost(seed, &requiredServiceBits);
+            std::string host = strprintf("x%x.%s", requiredServiceBits, seed);


Not all DNS seeds support this naming scheme, I think?

That is, in fact, the point. If we fail to look up the host (eg cause the dns seed doesn't support the x$ prefix or they dont support our new required service bits) we fall back to the same thing we do with tor - simply add the generic responses as oneshots.

It seems a bit silly to try to resolve something we know won't resolve (or, even worse, may resolve but have a result with an unintended meaning).

DNS Seeds should generally resolve to something useful with the service bits fine, and I think we should be phasing out DNS seeds which do not, so I'm not too worried about it. Mostly it just simplifies the code somewhat and lets us add new service bits without worrying about keeping the DNS seed constants up to date (and the DNS seeds can add the new service bits after release).

sipa · 2017-12-20T23:26:29Z

src/net.cpp

@@ -1640,6 +1627,10 @@ void CConnman::ThreadDNSAddressSeed()
                    found++;
                }
                addrman.Add(vAdd, resolveSource);
+            } else {
+                // We now just ignore DNS Seeds which do not support service


This branch triggers based on SetInternal returning false, which means host="" - it has nothing to do with service filtering.

I think you reviewed with too little context? Generally -U20 or more is required to get sufficient context in reviews in my experience :p.

Oops, indeed.

sipa · 2017-12-20T23:26:48Z

src/protocol.h

+ * Thus, generally, avoid calling with peerServices == NODE_NONE, unless
+ * state-specific flags must absolutely be avoided. When called with
+ * peerServices == NODE_NONE, the returned desirable service flags are
+ * guaranteed to not change dependant on sate - ie they are suitable for


I love sate.

I am very sate-iated...err, no, that doesnt work either...will fix.

sipa · 2018-01-06T10:49:22Z

Code review ACK 3d097ed3457f9794a6f200522ff18262d44e3d7a

I'm not entirely convinced about the approach. It feels silly to me to do a DNS resolve which we know won't resolve (or, if it happens to be a catch-all for subdomains, we'll even end up adding unfiltered results - possibly with incorrect service flags - to addrman). I'd rather either remove support for unfiltered seeds, or leave the boolean in and for those with a false flag immediately fallback to a oneshot connection.

TheBlueMatt · 2018-01-09T18:20:46Z

@theuni suggested that change, so maybe he should chime in, but I tend to like it, especially because it allows a DNS seed operator to add support for a new service bit after the fact. If they lag too long we can remove them, but its nice to not have the service bit selector tied to a constant in a release.

sipa · 2018-01-09T18:29:32Z

@TheBlueMatt I was mostly concerned about @luke-jr's DNS seed not supporting filtering but still responding, resulting in us storing those results with the wrong flags in addrman. He confirmed on IRC that his seed supports filtering, so that concern goes away.

theuni · 2018-01-10T00:55:03Z

@sipa I think it's reasonable to assume that some seeds will update their seeds after releases have been cut.

That's a good point about getting potential bad responses from the subdomain. I guess we'll want to verify proper filtering before adding new servers. A quick test of the current ones yields no reply for xa.foo.bar.

laanwj · 2018-01-17T11:33:22Z

src/chainparams.cpp

@@ -125,12 +125,12 @@ class CMainParams : public CChainParams {
        assert(genesis.hashMerkleRoot == uint256S("0x4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b"));

        // Note that of those with the service bits flag, most only support a subset of possible options


comment needs updating: there is no 'service bits flag' anymore

Done, in a new commit.

laanwj · 2018-01-17T11:34:49Z

utACK 3d097ed

ryanofsky

utACK 88067afd71b3e98cded3397f366c19131274017a. I'm not super familiar with this code, but the changes all look good to me, and seem to straightforwardly address the bug.

ryanofsky · 2018-01-17T21:43:27Z

contrib/seeds/README.md

@@ -4,7 +4,9 @@ Utility to generate the seeds.txt list that is compiled into the client
 (see [src/chainparamsseeds.h](/src/chainparamsseeds.h) and other utilities in [contrib/seeds](/contrib/seeds)).


In commit "Use GetDesireableServiceFlags in static seeds, document this."

I'm probably just tired, but I found this commit message confusing. Maybe say:

broke inserting entries into addrman from static seeds (as well as dnsseeds which did not support service bits)

instead of:

broke inserting entries into addrman from dnsseeds which did not support service bits, as well as static seeds

since static seeds are really what's being addressed in this commit.

ryanofsky · 2018-01-17T21:58:19Z

src/net.cpp

@@ -1643,6 +1630,10 @@ void CConnman::ThreadDNSAddressSeed()
                    found++;
                }
                addrman.Add(vAdd, resolveSource);
+            } else {
+                // We now just ignore DNS Seeds which do not support service


In commit "Fall back to oneshot for DNS Seeds which don't support filtering."

Perhaps this is obvious with more context, but I'd think "ignoring" is dropping the seed rather than calling AddOneShot. If this said "fall back to one shot" instead of "ignoring" or said how one shot is the same as ignoring, it might be clearer.

laanwj · 2018-01-19T11:20:27Z

src/chainparams.cpp

@@ -124,7 +124,10 @@ class CMainParams : public CChainParams {
        assert(consensus.hashGenesisBlock == uint256S("0x000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f"));
        assert(genesis.hashMerkleRoot == uint256S("0x4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b"));

-        // Note that of those with the service bits flag, most only support a subset of possible options
+        // Note that of those with the service bits flag, most only support a subset of possible options.


You still mention the "service bits flag". This was the boolean argument that you removed.

Ah, I was thinking of the xXXXX prefix as the "service bits flag"...

Ah, yes, that's also a possible interpretation. Thanks for clarifying the text.

4440710 broke inserting entries into addrman from static seeds (as well as dnsseeds which did not support service bits). Static seeds were already being filtered by UA for 0.13.1+ (ie NODE_WITNESS), so simply changing the default service bits to include NODE_WITNESS (and updating docs appropriately) is sufficient. For DNS Seeds, we will later fix by falling back to oneshot if a seed does not support filtering.

This allows us to not have to update the chainparams whenever a DNS Seed changes its filtering support, as well fixes a bug introduced in 4440710 where returned nodes will never be attempted.

TheBlueMatt · 2018-01-19T22:41:44Z

Changed comments and commit messages as requested.

jonasschnelli · 2018-01-21T07:31:55Z

utACK 2b839ab

ryanofsky

utACK 2b839ab. Thanks for the comments! Comment changes only since the last review.

…g static seed adding 2b839ab Update chainparams comment for more info on service bits per dnsseed (Matt Corallo) 62e7642 Fall back to oneshot for DNS Seeds which don't support filtering. (Matt Corallo) 51ae766 Use GetDesireableServiceFlags in static seeds, document this. (Matt Corallo) fb6f6b1 bluematt's testnet-seed now supports x9 (and is just a static list) (Matt Corallo) Pull request description: 4440710 broke inserting entries into addrman from dnsseeds which did not support service bits, as well as static seeds. Static seeds were already being filtered by UA for 0.13.1+ (ie NODE_WITNESS), so simply changing the default service bits to include NODE_WITNESS (and updating docs appropriately) is sufficient. For DNS Seeds, not supporting NODE_WITNESS is no longer useful, so instead use non-filtering seeds as oneshot hosts irrespective of named proxy. I've set my testnet-seed to also support x9, though because it is simply a static host, it may be useful to leave the support off so that it is used as a oneshot to get addresses from a live node instead. I'm fine with either. Tree-SHA512: 3f17d4d2b0b84d876981c962d2b44cb0c8f95f52c56a48c6b35fd882f6d7a40805f320ec452985a1c0b34aebddb1922709156c3ceccd1b9f8363fd7cb537d21d

@sipa

660f5f1 net: don't retry failed oneshot connections forever (Cory Fields) Pull request description: As introduced by (my suggestion, sorry, in) #11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves. Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it. Maybe @sipa can shed a light on what the original intention was. Tree-SHA512: 2dfe35dabfb6354c315cf6f8ae42971765d36575e685662caae7ed8f9dea9472c6fb1fd5e62ec35301550b74b6613a54265e90fca2a6618544f78dacaac4d4fd

…, fixing static seed adding 2b839ab Update chainparams comment for more info on service bits per dnsseed (Matt Corallo) 62e7642 Fall back to oneshot for DNS Seeds which don't support filtering. (Matt Corallo) 51ae766 Use GetDesireableServiceFlags in static seeds, document this. (Matt Corallo) fb6f6b1 bluematt's testnet-seed now supports x9 (and is just a static list) (Matt Corallo) Pull request description: 4440710 broke inserting entries into addrman from dnsseeds which did not support service bits, as well as static seeds. Static seeds were already being filtered by UA for 0.13.1+ (ie NODE_WITNESS), so simply changing the default service bits to include NODE_WITNESS (and updating docs appropriately) is sufficient. For DNS Seeds, not supporting NODE_WITNESS is no longer useful, so instead use non-filtering seeds as oneshot hosts irrespective of named proxy. I've set my testnet-seed to also support x9, though because it is simply a static host, it may be useful to leave the support off so that it is used as a oneshot to get addresses from a live node instead. I'm fine with either. Tree-SHA512: 3f17d4d2b0b84d876981c962d2b44cb0c8f95f52c56a48c6b35fd882f6d7a40805f320ec452985a1c0b34aebddb1922709156c3ceccd1b9f8363fd7cb537d21d

@sipa

660f5f1 net: don't retry failed oneshot connections forever (Cory Fields) Pull request description: As introduced by (my suggestion, sorry, in) bitcoin#11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves. Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it. Maybe @sipa can shed a light on what the original intention was. Tree-SHA512: 2dfe35dabfb6354c315cf6f8ae42971765d36575e685662caae7ed8f9dea9472c6fb1fd5e62ec35301550b74b6613a54265e90fca2a6618544f78dacaac4d4fd

@sipa

660f5f1 net: don't retry failed oneshot connections forever (Cory Fields) Pull request description: As introduced by (my suggestion, sorry, in) bitcoin#11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves. Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it. Maybe @sipa can shed a light on what the original intention was. Tree-SHA512: 2dfe35dabfb6354c315cf6f8ae42971765d36575e685662caae7ed8f9dea9472c6fb1fd5e62ec35301550b74b6613a54265e90fca2a6618544f78dacaac4d4fd

@sipa

660f5f1 net: don't retry failed oneshot connections forever (Cory Fields) Pull request description: As introduced by (my suggestion, sorry, in) bitcoin#11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves. Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it. Maybe @sipa can shed a light on what the original intention was. Tree-SHA512: 2dfe35dabfb6354c315cf6f8ae42971765d36575e685662caae7ed8f9dea9472c6fb1fd5e62ec35301550b74b6613a54265e90fca2a6618544f78dacaac4d4fd fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org>

@sipa

660f5f1 net: don't retry failed oneshot connections forever (Cory Fields) Pull request description: As introduced by (my suggestion, sorry, in) bitcoin#11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves. Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it. Maybe @sipa can shed a light on what the original intention was. Tree-SHA512: 2dfe35dabfb6354c315cf6f8ae42971765d36575e685662caae7ed8f9dea9472c6fb1fd5e62ec35301550b74b6613a54265e90fca2a6618544f78dacaac4d4fd fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org> fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org> fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org>

@sipa

660f5f1 net: don't retry failed oneshot connections forever (Cory Fields) Pull request description: As introduced by (my suggestion, sorry, in) bitcoin#11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves. Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it. Maybe @sipa can shed a light on what the original intention was. Tree-SHA512: 2dfe35dabfb6354c315cf6f8ae42971765d36575e685662caae7ed8f9dea9472c6fb1fd5e62ec35301550b74b6613a54265e90fca2a6618544f78dacaac4d4fd fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org> fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org> fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org>

@sipa

660f5f1 net: don't retry failed oneshot connections forever (Cory Fields) Pull request description: As introduced by (my suggestion, sorry, in) bitcoin#11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves. Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it. Maybe @sipa can shed a light on what the original intention was. Tree-SHA512: 2dfe35dabfb6354c315cf6f8ae42971765d36575e685662caae7ed8f9dea9472c6fb1fd5e62ec35301550b74b6613a54265e90fca2a6618544f78dacaac4d4fd fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org> fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org> fix 12329 backport Signed-off-by: Pasta <pasta@dashboost.org>

fanquake added the P2P label Oct 16, 2017

TheBlueMatt force-pushed the 2017-10-seed-service-bits-cleanups branch 2 times, most recently from 89a0e89 to ddd064d Compare October 17, 2017 13:16

sipa reviewed Oct 18, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-10-seed-service-bits-cleanups branch 2 times, most recently from 5b76a78 to 8fca5ae Compare October 18, 2017 15:40

theuni reviewed Oct 18, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-10-seed-service-bits-cleanups branch 2 times, most recently from 9a8563e to c7d2758 Compare October 19, 2017 21:35

TheBlueMatt force-pushed the 2017-10-seed-service-bits-cleanups branch from c7d2758 to c7f5f4e Compare November 6, 2017 22:24

maflcko added the Needs backport label Nov 7, 2017

maflcko added this to the 0.16.0 milestone Nov 7, 2017

maflcko removed the Needs backport label Nov 9, 2017

sipa reviewed Dec 20, 2017

View reviewed changes

bluematt's testnet-seed now supports x9 (and is just a static list)

fb6f6b1

TheBlueMatt force-pushed the 2017-10-seed-service-bits-cleanups branch from c7f5f4e to 3d097ed Compare December 24, 2017 16:48

TheBlueMatt mentioned this pull request Jan 16, 2018

Net: Bitcoin ONLY attempts to connect to addresses from DNS seeders #12195

Closed

laanwj reviewed Jan 17, 2018

View reviewed changes

ryanofsky reviewed Jan 17, 2018

View reviewed changes

laanwj reviewed Jan 19, 2018

View reviewed changes

TheBlueMatt force-pushed the 2017-10-seed-service-bits-cleanups branch from 88067af to 5226d2b Compare January 19, 2018 22:36

TheBlueMatt added 3 commits January 19, 2018 12:40

Fall back to oneshot for DNS Seeds which don't support filtering.

62e7642

This allows us to not have to update the chainparams whenever a DNS Seed changes its filtering support, as well fixes a bug introduced in 4440710 where returned nodes will never be attempted.

Update chainparams comment for more info on service bits per dnsseed

2b839ab

TheBlueMatt force-pushed the 2017-10-seed-service-bits-cleanups branch from 5226d2b to 2b839ab Compare January 19, 2018 22:41

ryanofsky reviewed Jan 23, 2018

View reviewed changes

laanwj merged commit 2b839ab into bitcoin:master Jan 24, 2018

theuni mentioned this pull request Feb 1, 2018

net: don't retry failed oneshot connections forever #12329

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		@@ -125,12 +125,12 @@ class CMainParams : public CChainParams {
		assert(genesis.hashMerkleRoot == uint256S("0x4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b"));

		// Note that of those with the service bits flag, most only support a subset of possible options

		@@ -4,7 +4,9 @@ Utility to generate the seeds.txt list that is compiled into the client
		(see [src/chainparamsseeds.h](/src/chainparamsseeds.h) and other utilities in [contrib/seeds](/contrib/seeds)).

Use GetDesireableServiceFlags in seeds, dnsseeds, fixing static seed adding #11512

Use GetDesireableServiceFlags in seeds, dnsseeds, fixing static seed adding #11512

Uh oh!

Conversation

TheBlueMatt commented Oct 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Oct 19, 2017

Uh oh!

TheBlueMatt commented Oct 19, 2017

Uh oh!

theuni commented Oct 19, 2017

Uh oh!

TheBlueMatt commented Oct 19, 2017

Uh oh!

theuni commented Oct 19, 2017

Uh oh!

TheBlueMatt commented Oct 19, 2017

Uh oh!

TheBlueMatt commented Nov 6, 2017

Uh oh!

TheBlueMatt commented Nov 6, 2017

Uh oh!

theuni commented Nov 14, 2017

Uh oh!

theuni commented Dec 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Jan 6, 2018

Uh oh!

TheBlueMatt commented Jan 9, 2018

Uh oh!

sipa commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jan 17, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Jan 9, 2018 •

edited

Loading

laanwj Jan 19, 2018 •

edited

Loading