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

1. 
   TheBlueMatt commented at 9:45 pm on October 16, 2017: member

   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.

2. fanquake added the label P2P on Oct 16, 2017
3. TheBlueMatt force-pushed on Oct 17, 2017
4. TheBlueMatt force-pushed on Oct 17, 2017
5. in src/chainparams.h:19 in ddd064dcbd outdated

   15@@ -16,6 +16,7 @@
   16 
   17 struct CDNSSeedData {
   18     std::string host;
   19+    //! Implies that the dnsseed can filter for at least xGetDesireableSerivceFlags(NODE_NONE)
   

   ---

   ---

   

   sipa commented at 8:53 am on October 18, 2017:

   Typo: xGetDesireableServiceFlags?

   ---

   

   TheBlueMatt commented at 3:30 pm on October 18, 2017:

   x was deliberate (as thats the dns prefix), the e is just because i cant spell desirable.
6. TheBlueMatt force-pushed on Oct 18, 2017
7. TheBlueMatt force-pushed on Oct 18, 2017
8. in src/net.cpp:1623 in 8fca5ae7ad outdated

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

   ---

   ---

   

   theuni commented at 9:23 pm on October 18, 2017:

   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.

   ---

   

   TheBlueMatt commented at 0:48 am on October 19, 2017:

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

   ---

   

   theuni commented at 3:39 am on October 19, 2017:

   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.

   ---

   

   jonasschnelli commented at 6:52 pm on October 19, 2017:

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

   ---

   

   TheBlueMatt commented at 9:34 pm on October 19, 2017:

   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.
9. 
   jonasschnelli commented at 6:53 pm on October 19, 2017: contributor

   Needs rebase.
10. TheBlueMatt force-pushed on Oct 19, 2017
11. TheBlueMatt force-pushed on Oct 19, 2017
12. 
    TheBlueMatt commented at 9:35 pm on October 19, 2017: member

    Rebased.
13. 
    theuni commented at 10:04 pm on October 19, 2017: member

    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.

14. 
    TheBlueMatt commented at 10:10 pm on October 19, 2017: member

    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.
15. 
    theuni commented at 10:17 pm on October 19, 2017: member

    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.

16. 
    TheBlueMatt commented at 10:19 pm on October 19, 2017: member

    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.
17. 
    TheBlueMatt commented at 9:59 pm on November 6, 2017: member

    Needs an 0.16 tag since this fixes a bug currently in master.
18. TheBlueMatt force-pushed on Nov 6, 2017
19. 
    TheBlueMatt commented at 10:25 pm on November 6, 2017: member

    Further clarified comment on desirable service flags.
20. MarcoFalke added the label Needs backport on Nov 7, 2017
21. MarcoFalke added this to the milestone 0.16.0 on Nov 7, 2017
22. MarcoFalke removed the label Needs backport on Nov 9, 2017
23. 
    theuni commented at 10:04 pm on November 14, 2017: member

    utACK c7f5f4ef8990c5abdc1f3f2f76771c46b7e08b07
24. 
    theuni commented at 11:16 pm on December 20, 2017: member

    Needs rebase. Also, bump for visibility :)
25. in src/net.cpp:1617 in c7f5f4ef89 outdated

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

    ---

    ---

    

    sipa commented at 11:25 pm on December 20, 2017:

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

    ---

    

    TheBlueMatt commented at 5:03 am on December 23, 2017:

    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.

    ---

    

    sipa commented at 2:25 pm on December 25, 2017:

    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).

    ---

    

    TheBlueMatt commented at 4:58 pm on December 26, 2017:

    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).
26. in src/net.cpp:1634 in c7f5f4ef89 outdated

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

    ---

    ---

    

    sipa commented at 11:26 pm on December 20, 2017:

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

    ---

    

    TheBlueMatt commented at 5:04 am on December 23, 2017:

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

    ---

    

    sipa commented at 10:42 am on January 6, 2018:

    Oops, indeed.
27. in src/protocol.h:297 in c7f5f4ef89 outdated

    290@@ -291,7 +291,15 @@ enum ServiceFlags : uint64_t {
    291  * unless they set NODE_NETWORK_LIMITED and we are out of IBD, in which
    292  * case NODE_NETWORK_LIMITED suffices).
    293  *
    294- * Thus, generally, avoid calling with peerServices == NODE_NONE.
    295+ * Thus, generally, avoid calling with peerServices == NODE_NONE, unless
    296+ * state-specific flags must absolutely be avoided. When called with
    297+ * peerServices == NODE_NONE, the returned desirable service flags are
    298+ * guaranteed to not change dependant on sate - ie they are suitable for
    

    ---

    ---

    

    sipa commented at 11:26 pm on December 20, 2017:

    I love sate.

    ---

    

    TheBlueMatt commented at 5:04 am on December 23, 2017:

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

    ---

    

    TheBlueMatt commented at 4:48 pm on December 24, 2017:

    Fixed.
28. bluematt's testnet-seed now supports x9 (and is just a static list) fb6f6b1519
29. TheBlueMatt force-pushed on Dec 24, 2017
30. 
    sipa commented at 10:49 am on January 6, 2018: member

    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.

31. 
    TheBlueMatt commented at 6:20 pm on January 9, 2018: member

    @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.
32. 
    sipa commented at 6:29 pm on January 9, 2018: member

    @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.
33. 
    theuni commented at 0:55 am on January 10, 2018: member

    @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.

34. in src/chainparams.cpp:127 in 3d097ed345 outdated

    124@@ -125,12 +125,12 @@ class CMainParams : public CChainParams {
    125         assert(genesis.hashMerkleRoot == uint256S("0x4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b"));
    126 
    127         // Note that of those with the service bits flag, most only support a subset of possible options
    

    ---

    ---

    

    laanwj commented at 11:33 am on January 17, 2018:

    comment needs updating: there is no ‘service bits flag’ anymore

    ---

    

    TheBlueMatt commented at 9:19 pm on January 17, 2018:

    Done, in a new commit.
35. 
    laanwj commented at 11:34 am on January 17, 2018: member

    utACK 3d097ed
36. in contrib/seeds/README.md:4 in f791eb1063 outdated

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

    ---

    ---

    

    ryanofsky commented at 9:43 pm on January 17, 2018:

    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.

37. in src/net.cpp:1634 in 3d097ed345 outdated

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

    ---

    ---

    

    ryanofsky commented at 9:58 pm on January 17, 2018:

    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.

38. 
    ryanofsky commented at 10:03 pm on January 17, 2018: member

    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.
39. in src/chainparams.cpp:127 in 88067afd71 outdated

    123@@ -124,7 +124,10 @@ class CMainParams : public CChainParams {
    124         assert(consensus.hashGenesisBlock == uint256S("0x000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f"));
    125         assert(genesis.hashMerkleRoot == uint256S("0x4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b"));
    126 
    127-        // Note that of those with the service bits flag, most only support a subset of possible options
    128+        // Note that of those with the service bits flag, most only support a subset of possible options.
    

    ---

    ---

    

    laanwj commented at 11:20 am on January 19, 2018:

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

    ---

    

    TheBlueMatt commented at 10:32 pm on January 19, 2018:

    Ah, I was thinking of the xXXXX prefix as the “service bits flag”…

    ---

    

    laanwj commented at 12:02 pm on January 24, 2018:

    Ah, yes, that’s also a possible interpretation. Thanks for clarifying the text.
40. TheBlueMatt force-pushed on Jan 19, 2018
41. Use GetDesireableServiceFlags in static seeds, document this.

    44407100f 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.

    51ae7660b8
42. Fall back to oneshot for DNS Seeds which don't 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 44407100f where returned nodes will never be
    attempted.

    62e764219b
43. Update chainparams comment for more info on service bits per dnsseed 2b839abd3e
44. 
    TheBlueMatt commented at 10:41 pm on January 19, 2018: member

    Changed comments and commit messages as requested.
45. TheBlueMatt force-pushed on Jan 19, 2018
46. 
    jonasschnelli commented at 7:31 am on January 21, 2018: contributor

    utACK 2b839abd3e78e28748bbef0708fac74b442e2e36
47. 
    ryanofsky commented at 8:59 pm on January 23, 2018: member

    utACK 2b839abd3e78e28748bbef0708fac74b442e2e36. Thanks for the comments! Comment changes only since the last review.
48. laanwj merged this on Jan 24, 2018
49. laanwj closed this on Jan 24, 2018

    ---

50. laanwj referenced this in commit 6e89de5ba7 on Jan 24, 2018
51. laanwj referenced this in commit aa360e76a7 on Feb 2, 2018
52. PastaPastaPasta referenced this in commit 94e99441c5 on Jan 26, 2020
53. PastaPastaPasta referenced this in commit 7ebd9698eb on Jan 26, 2020
54. PastaPastaPasta referenced this in commit 815672467f on Jan 26, 2020
55. PastaPastaPasta referenced this in commit 88cb85752c on Jan 26, 2020
56. PastaPastaPasta referenced this in commit 539d71e671 on Jan 27, 2020
57. PastaPastaPasta referenced this in commit 03576217e5 on Jan 27, 2020
58. ckti referenced this in commit 6939fabbec on Mar 28, 2021
59. ckti referenced this in commit a6968bb6e5 on Mar 28, 2021
60. gades referenced this in commit 9e36050932 on Jun 25, 2021
61. MarcoFalke locked this on Sep 8, 2021

Contributors
TheBlueMatt sipa theuni jonasschnelli laanwj ryanofsky

Labels
P2P

Milestone
0.16.0