Replace relevant services logic with a function suite. #11456

TheBlueMatt · 2017-10-05T15:18:23Z

This was mostly written as a way to clean things up so that the NETWORK_LIMITED PR (#10387) can be simplified a ton, but its also a nice standalone cleanup that will also require a bit of review because it tweaks a lot of stuff across net. The new functions are fine in protocol.h right now since they're straight-forward, but after NETWORK_LIMITED will really want to move elsewhere after @theuni moves the nServices-based selection to addrman from connman.

Adds HasAllRelevantServices and GetRelevantServices, which check
for NETWORK|WITNESS.

This changes the following:

Removes nRelevantServices from CConnman, disconnecting it a bit
more from protocol-level logic.
Replaces our sometimes-connect-to-!WITNESS-nodes logic with
simply always requiring WITNESS|NETWORK for outbound non-feeler
connections (feelers still only require NETWORK).
This has the added benefit of removing nServicesExpected from
CNode - instead letting net_processing's VERSION message
handling simply check HasAllRelevantServices.
This implies we believe WITNESS nodes to continue to be a
significant majority of nodes on the network, but also because
we cannot sync properly from !WITNESS nodes, it is strange to
continue using our valuable outbound slots on them.
In order to prevent this change from preventing connection to
-connect= nodes which have !WITNESS, -connect nodes are now
given the "addnode" flag. This also allows outbound connections
to !NODE_NETWORK nodes for -connect nodes (which was already true
of addnodes).
Has the (somewhat unintended) consequence of changing one of the
eviction metrics from the same
sometimes-connect-to-!WITNESS-nodes metric to requiring
HasRelevantServices.

This should make NODE_NETWORK_LIMITED much simpler to implement.

theuni · 2017-10-05T15:33:28Z

In order to prevent this change from preventing connection to
-connect= nodes which have !WITNESS, -connect nodes are now
given the "addnode" flag.

I really disliked that flag anyway, as it's not obvious who should care and why. This change makes sense imo. Mind renaming this to "m_manual_connection", or "!m_from_addrman" (or hopefully something better) ?

TheBlueMatt · 2017-10-05T15:50:23Z

@theuni done in a new commit, that is much more descriptive, indeed.

jimpo

Concept ACK

jimpo · 2017-10-05T16:08:57Z

src/protocol.h

+ * If the peer is missing any of the returned service flags, they aren't
+ * interesting to us - we probably shouldn't conenct outbound to them.
+ */
+static ServiceFlags GetRelevantServiceFlags(ServiceFlags services) {


Can you document what the parameter represents? Hard to tell since it's unused. I assume it's for future compatibility.

Indeed, its for use in #10387. Will it be clearer when the function reads

GetRelevantServiceFlags(services) { if (services & NODE_NETWORK_LIMITED && !in_ibd) return NODE_NETWORK_LIMITED | NODE_WITNESS; return NODE_NETWORK | NODE_WITNESS;

or should I document it further?

Yeah, parameter documentation would be nice because I still find its purpose confusing. Especially since as a parameter of HasAllRelevantServiceFlags it represents both the service flags you want to test and a modifier on what you want to test against. Like, why is the future code not just:

GetRelevantServiceFlags() { if (!in_ibd) return NODE_NETWORK_LIMITED | NODE_WITNESS; else return NODE_NETWORK | NODE_WITNESS;

I like jimpo's suggestion above for getting rid of the service argument, since this code seems more vague and confusing than it needs to be. Other suggestions for making it clearer could be:

Rename "Relevant" to "Required" or "RequiredForOutbound." "Relevant" doesn't suggest what the return value will be used for, while "Required" does.

Rename "services" argument to "peer_service_flags" if it can't be eliminated so it is clearer where the argument value is coming from.

You need the NODE_NETWORK_LIMITED check as a proxy for whether the node is new - eg if the node announces NODE_NETWORK_LIMITED (and you're out of IBD) then you're fine with NODE_NETWORK_LIMITED, but if the node only announces NODE_NETWORK they may be an 0.15.X node and you're perfectly OK with them having only NODE_NETWORK and not NODE_NETWORK_LIMITED.

Oh, ok, I figured that would work because NODE_NETWORK would be a superset of NODE_NETWORK_LIMITED. But if NODE_NETWORK without NODE_NETWORK_LIMITED is a meaningful flag combination then it makes sense that you need the services argument.

In any case, I mostly wanted to suggest the renames here.

NODE_NETWORK without NODE_NETWORK_LIMITED is not meaningful generally, but old nodes will (obviously) be using that, so it is important that we still treat it correctly.

gmaxwell · 2017-10-05T17:06:28Z

I was thinking of opening a PR soon to require WITNESS for outbounds in any case, so ACK on that.

TheBlueMatt · 2017-10-05T22:06:55Z

Rewrote the function description. Sufficiently descriptive, @jimpo?

ryanofsky

utACK 12524dd366306ea59ec8019e076b9a843d55bcf0, though for some reason I really dislike use of word "relevant" here.

It would be good if the -connect help string mentioned the fact that it loosens requirements for peers, since the current -connect help seems to imply that it only restricts connections. Release notes could also mention the change in behavior if you don't think it's too much of an internal detail.

ryanofsky · 2017-10-06T14:13:59Z

src/protocol.h

+ * If the peer is missing any of the returned service flags, they aren't
+ * interesting to us - we probably shouldn't conenct outbound to them.
+ */
+static ServiceFlags GetRelevantServiceFlags(ServiceFlags services) {


I like jimpo's suggestion above for getting rid of the service argument, since this code seems more vague and confusing than it needs to be. Other suggestions for making it clearer could be:

Rename "Relevant" to "Required" or "RequiredForOutbound." "Relevant" doesn't suggest what the return value will be used for, while "Required" does.

Rename "services" argument to "peer_service_flags" if it can't be eliminated so it is clearer where the argument value is coming from.

ryanofsky · 2017-10-06T14:24:31Z

src/protocol.h

+ * Checks if a peer with the given service flags may be capable of having a
+ * robust address-storage DB. Currently an alias for checking NODE_NETWORK.
+ */
+static inline bool MayHaveRelevantAddressDB(ServiceFlags services) {


"Relevant" doesn't seem like the right word here either, because it doesn't imply anything about the addressdb. Maybe "Robust"? "Good"? "Usable"?

ryanofsky · 2017-10-06T14:35:24Z

src/protocol.h

+ * == GetRelevantServiceFlags(services), ie determines whether the given
+ * set of service flags are sufficient for a peer to be "relevant".
+ */
+static inline bool HasAllRelevantServiceFlags(ServiceFlags services) {


Again could consider "Required" / "peer_service_flags" renames suggested above.

JeremyRubin · 2017-10-06T16:05:38Z

Concept Ack and a light CR-Ack.

I agree with @ryanofsky about naming -- I didn't find the new function names you introduced to be particularly elucidating, so maybe you can pick something better.

Also, I don't think we want to introduce new hungarian notation member variables (unless the m_ stands for 'matt_' ;) ).

TheBlueMatt · 2017-10-06T16:07:11Z

@ryanofsky I'm not sure what your comment about -connect is about. The help text still reads correct to me (it still limits you to only that one connection), the only thing it changes is that previously we would disconnect-on-VERSION-message for -connect peers which do not set NODE_NETWORK, now we will keep it connected just the same as -addnode peers. If you think thats a critical change to document I'm happy to do so, but I think thats a rather esoteric case for -connect.

jimpo · 2017-10-06T17:18:18Z

src/protocol.h

+ * Relevant service flags may be peer- and state-specific in that the
+ * version of the peer may determine which flags are required (eg in the
+ * case of NODE_NETWORK_LIMITED where we seek out NODE_NETWORK peers
+ * unless they set NODE_NETWORK_LIMITED and we our out of IBD, in which


nit: we are*

ryanofsky · 2017-10-06T17:37:29Z

Yeah, I wasn't saying the help text is incorrect, I was saying it was incomplete. Help text says -connect does one thing (limit which peers will be connected to) but actually -connect does two things (limit peers, and also loosen requirements for those peers). Feel free to leave it alone if you don't think it's worth mentioning, but it seemed like an omission.

theuni · 2017-10-06T18:43:27Z

src/net.cpp

@@ -1602,7 +1601,7 @@ void CConnman::ThreadDNSAddressSeed()
        LOCK(cs_vNodes);
        int nRelevant = 0;
        for (auto pnode : vNodes) {
-            nRelevant += pnode->fSuccessfullyConnected && ((pnode->nServices & nRelevantServices) == nRelevantServices);


I think services are just acting as a proxy here. Since we enforce service checking for (non-manual) outgoing connections already, and should probably consider incoming nodes as insignificant for the sake of this check, don't we really just want to tally fSuccessfullyConnected && !fInbound && !m_manual_connection ?

Agreed, though I went ahead and added OneShot and Feeler into the mix.

TheBlueMatt · 2017-10-06T21:31:27Z

OK, addressed all the feedback except the desire to rename Relevant to something else. I'm not a fan of Required or RequiredForOutbound, as neither really caputures its full user (its also used in eviction logic). I admit Relevant isnt so descriptive either, but it also doesn't give a false impression (and is documented), so I'm open to other suggestions but not a fan of the current options.

ryanofsky

utACK 41896568e9ba776a61406a86ec565570eaa399e7. Thanks for the updates! Possible alternatives to relevant: Valid, Expected, Usable, Syncable

ryanofsky · 2017-10-06T21:39:59Z

src/rpc/net.cpp

@@ -201,6 +201,8 @@ UniValue addnode(const JSONRPCRequest& request)
            "addnode \"node\" \"add|remove|onetry\"\n"
            "\nAttempts to add or remove a node from the addnode list.\n"
            "Or try a connection to a node once.\n"
+            "Nodes added using addnode (or -connect) are protected from DoS disconnection andare not required to be\n"


TheBlueMatt · 2017-10-06T21:57:55Z

Changed to "Desirable" instead of "Relevant" at @JeremyRubin's suggestion.

promag · 2017-10-07T08:26:57Z

src/protocol.h

+ *
+ * Thus, generally, avoid calling with peerServices == NODE_NONE.
+ */
+static ServiceFlags GetDesirableServiceFlags(ServiceFlags services) {


Nit, unused services, comment?

See #11456 (comment), this function should get much more complicated soon, and hopefully move out of protocol.h at the same time.

theuni · 2017-10-10T19:02:34Z

Concept ACK. Though, I believe that these lines demonstrate why "Desirable" is so awkward:

if (!fFeeler && !HasAllDesirableServiceFlags(addr.nServices)) {
    continue;
    } else if (fFeeler && !MayHaveUsefulAddressDB(addr.nServices)) {
    continue;
}

MayHaveUsefulAddressDB() is explicit in what it's checking, but HasAllDesirableServiceFlags() is not.

I'd be much more comfortable with this (and I think it'd be much more straightforward) if HasAllDesirableServiceFlags() was replaced with two or three different functions, each representing an explicit functionality check. e.g.: MayServeSomeWitnessBlocks(), MayServeAllWitnessBlocks(), etc.

After all, that's what we're really trying to figure out, right?

Then the flag checks aren't dependent on app state, because those checks are done in a straightforward way by the caller instead:

if ((in_ibd && MayServeAllWitnessBlocks(services))
  || (!in_ibd && MayServeSomeWitnessBlocks(services))) {
    request()
    ...
}

That also means that "Desirable" doesn't have to share the same meaning everywhere. The obvious example where that's useful is the eviction logic.

So... basically just a glorified version of the previous binary logic, except that the functions understand cases where multiple flags may be suitable.

TheBlueMatt · 2017-10-10T19:42:22Z

The goal with the naming was to allow the place that owns network logic to have a function which captures whether a peer is interesting to us. Point being you want some function which captures everything net wants to avoid knowing about, even if that's some crazy wrapper for MayServe*WitnessBlocks, that wrapper should still exist outside of CConnman/net. Ideally, eventually, this will be in net_processing.h (ie the place where we know about what stuff we want out of our peers), but the circular deps right now make that a mess. Lacking net_processing being able to take it, protocol.h or some new header seems most reasonable to me.

sipa · 2017-10-10T20:04:19Z

The reason for CNode::nExpected wasn't so much enforcing services, but checking whether we're connecting out to the right node.

Many of our assumptions on partition resistance and privacy are based on the fact that outgoing connections are more under our control. If we're making an outgoing connection, and the node we end up connecting to has different service flags than what we expect - even if they're 'better' - we're likely not connected to who we thought we were. If that's the case, I believe it's better to disconnect and pick another node we want to connect to, rather than accepting wasting an outbound connection slot on an unknown peer.

theuni · 2017-10-10T20:41:40Z

@sipa I think this logic would roughly mimic that of nExpected, as we only attempt connections with preferred flags anyway.

Though disconnecting upon receiving preferred but not expected flags would mean that if a node goes from full -> pruned (or the reverse), it would see a bunch of disconnects for a while. Post bip159, that is.

I think it would also open us up to issues if someone was handing out phony addr's?

sipa · 2017-10-11T22:25:40Z

@theuni My view is that at connection time, we're making a certain determination of what to connect to, with biases based on (among other things) the service flags of all things in addrman. Disconnecting if you see that what you connected to was based on wrong information just gives everything a new chance (including the peer you just connected to). In the end, what node you end up connecting to has the same probability distribution to the one you'd have if you had perfect information (about the nodes you tried) from the start. Using nExpected means all the biasing logic can be encapsulated inside the outgoing-connection-logic, without needing to reason about corrections afterwards.

Talking to @TheBlueMatt about this, I agree that this pretty abstract, as we don't actually want to bias based on service flags anymore (at least for the time being), and even after BIP159, it will likely be a boolean acceptable or not, instead of a complicated bias.

So, I withdraw my objection.

sipa · 2017-10-12T00:49:07Z

src/protocol.h

+ * set of service flags are sufficient for a peer to be "relevant".
+ */
+static inline bool HasAllDesirableServiceFlags(ServiceFlags services) {
+    return (GetDesirableServiceFlags(services) & services) == GetDesirableServiceFlags(services);


Slightly easier expression !(GetDesirableServiceFlags(services) & ~services).

sipa · 2017-10-12T01:04:37Z

src/net.cpp

@@ -1601,7 +1601,7 @@ void CConnman::ThreadDNSAddressSeed()
        LOCK(cs_vNodes);
        int nRelevant = 0;
        for (auto pnode : vNodes) {
-            nRelevant += pnode->fSuccessfullyConnected && HasAllDesirableServiceFlags(pnode->nServices);
+            nRelevant += pnode->fSuccessfullyConnected && !pnode->fFeeler && !pnode->fOneShot && !pnode->m_manual_connection && !pnode->fInbound;


Are you sure you wouldn't want MayHaveUsefulAddressDB at least here?

fSuccessfullyConnected && !special_peer_flag implies the peer is useful (and really want to have as few service-bit-aware pieces of logic in net as possible).

ryanofsky

utACK b4809fc30be4d27cabf1a81bc954014070fa3117 if HasAllDesirableServiceFlags logic is fixed. Changes since last review were the "desirable" rename, "andare" typo fix, and HasAllDesirableServiceFlags bitwise logic simplification.

ryanofsky · 2017-10-12T19:46:11Z

src/protocol.h

+ * set of service flags are sufficient for a peer to be "relevant".
+ */
+static inline bool HasAllDesirableServiceFlags(ServiceFlags services) {
+    return GetDesirableServiceFlags(services) & (~services);


Missing !

sipa · 2017-10-12T23:18:52Z

Commit 'Rename fAddnode to a more-descriptive "manual_connection"': convert to scripted diff?

sipa · 2017-10-12T23:22:52Z

utACK after fixing the incorrect HasAllDesirableServiceFlags logic. One nit.

Adds HasAllRelevantServices and GetRelevantServices, which check for NETWORK|WITNESS. This changes the following: * Removes nRelevantServices from CConnman, disconnecting it a bit more from protocol-level logic. * Replaces our sometimes-connect-to-!WITNESS-nodes logic with simply always requiring WITNESS|NETWORK for outbound non-feeler connections (feelers still only require NETWORK). * This has the added benefit of removing nServicesExpected from CNode - instead letting net_processing's VERSION message handling simply check HasAllRelevantServices. * This implies we believe WITNESS nodes to continue to be a significant majority of nodes on the network, but also because we cannot sync properly from !WITNESS nodes, it is strange to continue using our valuable outbound slots on them. * In order to prevent this change from preventing connection to -connect= nodes which have !WITNESS, -connect nodes are now given the "addnode" flag. This also allows outbound connections to !NODE_NETWORK nodes for -connect nodes (which was already true of addnodes). * Has the (somewhat unintended) consequence of changing one of the eviction metrics from the same sometimes-connect-to-!WITNESS-nodes metric to requiring HasRelevantServices. This should make NODE_NETWORK_LIMITED much simpler to implement.

TheBlueMatt · 2017-10-13T17:30:35Z

Fixed missing !, making the fAnnode -> manul_connection commit scripted isnt trivial given some of them are m_manual_connection but some are manul_connection in function parameters. Its a pretty simple commit anyway so I'll just leave it unless you have strong objections.

theuni · 2017-10-13T20:05:29Z

@ryanofsky nice spot.

I'm still not a big fan of keeping the relevant concept around, but this is an improvement regardless.

utACK 15f5d3b.

sipa · 2017-10-13T20:06:54Z

making the fAnnode -> manul_connection commit scripted isnt trivial given some of them are m_manual_connection but some are manul_connection in function parameters

I missed that. The change looks fine without.

sipa · 2017-10-13T22:18:53Z

utACK 15f5d3b

@theuni

15f5d3b Switch DNSSeed-needed metric to any-automatic-nodes, not services (Matt Corallo) 5ee88b4 Clarify docs for requirements/handling of addnode/connect nodes (Matt Corallo) 57edc0b Rename fAddnode to a more-descriptive "manual_connection" (Matt Corallo) 4440710 Replace relevant services logic with a function suite. (Matt Corallo) Pull request description: This was mostly written as a way to clean things up so that the NETWORK_LIMITED PR (#10387) can be simplified a ton, but its also a nice standalone cleanup that will also require a bit of review because it tweaks a lot of stuff across net. The new functions are fine in protocol.h right now since they're straight-forward, but after NETWORK_LIMITED will really want to move elsewhere after @theuni moves the nServices-based selection to addrman from connman. Adds HasAllRelevantServices and GetRelevantServices, which check for NETWORK|WITNESS. This changes the following: * Removes nRelevantServices from CConnman, disconnecting it a bit more from protocol-level logic. * Replaces our sometimes-connect-to-!WITNESS-nodes logic with simply always requiring WITNESS|NETWORK for outbound non-feeler connections (feelers still only require NETWORK). * This has the added benefit of removing nServicesExpected from CNode - instead letting net_processing's VERSION message handling simply check HasAllRelevantServices. * This implies we believe WITNESS nodes to continue to be a significant majority of nodes on the network, but also because we cannot sync properly from !WITNESS nodes, it is strange to continue using our valuable outbound slots on them. * In order to prevent this change from preventing connection to -connect= nodes which have !WITNESS, -connect nodes are now given the "addnode" flag. This also allows outbound connections to !NODE_NETWORK nodes for -connect nodes (which was already true of addnodes). * Has the (somewhat unintended) consequence of changing one of the eviction metrics from the same sometimes-connect-to-!WITNESS-nodes metric to requiring HasRelevantServices. This should make NODE_NETWORK_LIMITED much simpler to implement. Tree-SHA512: 90606896c86cc5da14c77843b16674a6a012065e7b583d76d1c47a18215358abefcbab44ff4fab3fadcd39aa9a42d4740c6dc8874a58033bdfc8ad3fb5c649fc

Github-Pull: bitcoin#11456 Rebased-From: 57edc0b

@theuni

…suite. 15f5d3b Switch DNSSeed-needed metric to any-automatic-nodes, not services (Matt Corallo) 5ee88b4 Clarify docs for requirements/handling of addnode/connect nodes (Matt Corallo) 57edc0b Rename fAddnode to a more-descriptive "manual_connection" (Matt Corallo) 4440710 Replace relevant services logic with a function suite. (Matt Corallo) Pull request description: This was mostly written as a way to clean things up so that the NETWORK_LIMITED PR (bitcoin#10387) can be simplified a ton, but its also a nice standalone cleanup that will also require a bit of review because it tweaks a lot of stuff across net. The new functions are fine in protocol.h right now since they're straight-forward, but after NETWORK_LIMITED will really want to move elsewhere after @theuni moves the nServices-based selection to addrman from connman. Adds HasAllRelevantServices and GetRelevantServices, which check for NETWORK|WITNESS. This changes the following: * Removes nRelevantServices from CConnman, disconnecting it a bit more from protocol-level logic. * Replaces our sometimes-connect-to-!WITNESS-nodes logic with simply always requiring WITNESS|NETWORK for outbound non-feeler connections (feelers still only require NETWORK). * This has the added benefit of removing nServicesExpected from CNode - instead letting net_processing's VERSION message handling simply check HasAllRelevantServices. * This implies we believe WITNESS nodes to continue to be a significant majority of nodes on the network, but also because we cannot sync properly from !WITNESS nodes, it is strange to continue using our valuable outbound slots on them. * In order to prevent this change from preventing connection to -connect= nodes which have !WITNESS, -connect nodes are now given the "addnode" flag. This also allows outbound connections to !NODE_NETWORK nodes for -connect nodes (which was already true of addnodes). * Has the (somewhat unintended) consequence of changing one of the eviction metrics from the same sometimes-connect-to-!WITNESS-nodes metric to requiring HasRelevantServices. This should make NODE_NETWORK_LIMITED much simpler to implement. Tree-SHA512: 90606896c86cc5da14c77843b16674a6a012065e7b583d76d1c47a18215358abefcbab44ff4fab3fadcd39aa9a42d4740c6dc8874a58033bdfc8ad3fb5c649fc

@theuni

…suite. 15f5d3b Switch DNSSeed-needed metric to any-automatic-nodes, not services (Matt Corallo) 5ee88b4 Clarify docs for requirements/handling of addnode/connect nodes (Matt Corallo) 57edc0b Rename fAddnode to a more-descriptive "manual_connection" (Matt Corallo) 4440710 Replace relevant services logic with a function suite. (Matt Corallo) Pull request description: This was mostly written as a way to clean things up so that the NETWORK_LIMITED PR (bitcoin#10387) can be simplified a ton, but its also a nice standalone cleanup that will also require a bit of review because it tweaks a lot of stuff across net. The new functions are fine in protocol.h right now since they're straight-forward, but after NETWORK_LIMITED will really want to move elsewhere after @theuni moves the nServices-based selection to addrman from connman. Adds HasAllRelevantServices and GetRelevantServices, which check for NETWORK|WITNESS. This changes the following: * Removes nRelevantServices from CConnman, disconnecting it a bit more from protocol-level logic. * Replaces our sometimes-connect-to-!WITNESS-nodes logic with simply always requiring WITNESS|NETWORK for outbound non-feeler connections (feelers still only require NETWORK). * This has the added benefit of removing nServicesExpected from CNode - instead letting net_processing's VERSION message handling simply check HasAllRelevantServices. * This implies we believe WITNESS nodes to continue to be a significant majority of nodes on the network, but also because we cannot sync properly from !WITNESS nodes, it is strange to continue using our valuable outbound slots on them. * In order to prevent this change from preventing connection to -connect= nodes which have !WITNESS, -connect nodes are now given the "addnode" flag. This also allows outbound connections to !NODE_NETWORK nodes for -connect nodes (which was already true of addnodes). * Has the (somewhat unintended) consequence of changing one of the eviction metrics from the same sometimes-connect-to-!WITNESS-nodes metric to requiring HasRelevantServices. This should make NODE_NETWORK_LIMITED much simpler to implement. Tree-SHA512: 90606896c86cc5da14c77843b16674a6a012065e7b583d76d1c47a18215358abefcbab44ff4fab3fadcd39aa9a42d4740c6dc8874a58033bdfc8ad3fb5c649fc

@theuni

…suite. 15f5d3b Switch DNSSeed-needed metric to any-automatic-nodes, not services (Matt Corallo) 5ee88b4 Clarify docs for requirements/handling of addnode/connect nodes (Matt Corallo) 57edc0b Rename fAddnode to a more-descriptive "manual_connection" (Matt Corallo) 4440710 Replace relevant services logic with a function suite. (Matt Corallo) Pull request description: This was mostly written as a way to clean things up so that the NETWORK_LIMITED PR (bitcoin#10387) can be simplified a ton, but its also a nice standalone cleanup that will also require a bit of review because it tweaks a lot of stuff across net. The new functions are fine in protocol.h right now since they're straight-forward, but after NETWORK_LIMITED will really want to move elsewhere after @theuni moves the nServices-based selection to addrman from connman. Adds HasAllRelevantServices and GetRelevantServices, which check for NETWORK|WITNESS. This changes the following: * Removes nRelevantServices from CConnman, disconnecting it a bit more from protocol-level logic. * Replaces our sometimes-connect-to-!WITNESS-nodes logic with simply always requiring WITNESS|NETWORK for outbound non-feeler connections (feelers still only require NETWORK). * This has the added benefit of removing nServicesExpected from CNode - instead letting net_processing's VERSION message handling simply check HasAllRelevantServices. * This implies we believe WITNESS nodes to continue to be a significant majority of nodes on the network, but also because we cannot sync properly from !WITNESS nodes, it is strange to continue using our valuable outbound slots on them. * In order to prevent this change from preventing connection to -connect= nodes which have !WITNESS, -connect nodes are now given the "addnode" flag. This also allows outbound connections to !NODE_NETWORK nodes for -connect nodes (which was already true of addnodes). * Has the (somewhat unintended) consequence of changing one of the eviction metrics from the same sometimes-connect-to-!WITNESS-nodes metric to requiring HasRelevantServices. This should make NODE_NETWORK_LIMITED much simpler to implement. Tree-SHA512: 90606896c86cc5da14c77843b16674a6a012065e7b583d76d1c47a18215358abefcbab44ff4fab3fadcd39aa9a42d4740c6dc8874a58033bdfc8ad3fb5c649fc

jimpo reviewed Oct 5, 2017

View reviewed changes

fanquake added the P2P label Oct 5, 2017

TheBlueMatt force-pushed the 2017-09-service-flags-cleanups branch from 8f50b87 to 12524dd Compare October 5, 2017 22:05

ryanofsky reviewed Oct 6, 2017

View reviewed changes

jimpo reviewed Oct 6, 2017

View reviewed changes

theuni reviewed Oct 6, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-09-service-flags-cleanups branch from 12524dd to 4189656 Compare October 6, 2017 21:29

ryanofsky reviewed Oct 6, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-09-service-flags-cleanups branch from 4189656 to 989d2fe Compare October 6, 2017 21:54

promag reviewed Oct 7, 2017

View reviewed changes

sipa reviewed Oct 12, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-09-service-flags-cleanups branch from 989d2fe to b4809fc Compare October 12, 2017 00:59

sipa reviewed Oct 12, 2017

View reviewed changes

ryanofsky reviewed Oct 12, 2017

View reviewed changes

TheBlueMatt added 4 commits October 13, 2017 13:25

Rename fAddnode to a more-descriptive "manual_connection"

57edc0b

Clarify docs for requirements/handling of addnode/connect nodes

5ee88b4

Switch DNSSeed-needed metric to any-automatic-nodes, not services

15f5d3b

TheBlueMatt force-pushed the 2017-09-service-flags-cleanups branch from b4809fc to 15f5d3b Compare October 13, 2017 17:29

sipa merged commit 15f5d3b into bitcoin:master Oct 13, 2017

jonasschnelli mentioned this pull request Oct 14, 2017

Eventually connect to NODE_NETWORK_LIMITED peers #10387

Merged

morcos mentioned this pull request Nov 2, 2017

0.15: Backports #11592

Merged

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 2, 2017

Rename fAddnode to a more-descriptive "manual_connection"

6f27965

Github-Pull: bitcoin#11456 Rebased-From: 57edc0b

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

Replace relevant services logic with a function suite. #11456

Replace relevant services logic with a function suite. #11456

Uh oh!

Conversation

TheBlueMatt commented Oct 5, 2017

Uh oh!

theuni commented Oct 5, 2017

Uh oh!

TheBlueMatt commented Oct 5, 2017

Uh oh!

jimpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gmaxwell commented Oct 5, 2017

Uh oh!

TheBlueMatt commented Oct 5, 2017

Uh oh!

ryanofsky left a comment

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!

JeremyRubin commented Oct 6, 2017

Uh oh!

TheBlueMatt commented Oct 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Oct 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 6, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 6, 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!

theuni commented Oct 10, 2017

Uh oh!

TheBlueMatt commented Oct 10, 2017

Uh oh!

sipa commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Oct 10, 2017

Uh oh!

sipa commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TheBlueMatt Oct 5, 2017 •

edited

Loading

sipa commented Oct 10, 2017 •

edited

Loading

sipa commented Oct 11, 2017 •

edited

Loading

sipa Oct 12, 2017 •

edited

Loading