Replace relevant services logic with a function suite.

TheBlueMatt commented at 3:18 pm on October 5, 2017: member

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 commented at 3:33 pm on October 5, 2017: member

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 commented at 3:50 pm on October 5, 2017: member

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

in src/protocol.h:296 in 4c24f4ccc3 outdated

284+ * service flags which are optionally relevant (eg NODE_NETWORK_LIMITED).
285+ *
286+ * If the peer is missing any of the returned service flags, they aren't
287+ * interesting to us - we probably shouldn't conenct outbound to them.
288+ */
289+static ServiceFlags GetRelevantServiceFlags(ServiceFlags services) {

jimpo commented at 4:08 pm on October 5, 2017:

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

TheBlueMatt commented at 4:27 pm on October 5, 2017:

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

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

or should I document it further?

jimpo commented at 4:50 pm on October 5, 2017:

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:

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

ryanofsky commented at 2:13 pm on October 6, 2017:

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.

TheBlueMatt commented at 3:24 pm on October 6, 2017:

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.

ryanofsky commented at 3:31 pm on October 6, 2017:

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.

TheBlueMatt commented at 4:04 pm on October 6, 2017:

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.

jimpo commented at 4:15 pm on October 5, 2017: contributor

Concept ACK

gmaxwell commented at 5:06 pm on October 5, 2017: contributor

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

fanquake added the label P2P on Oct 5, 2017

TheBlueMatt force-pushed on Oct 5, 2017

TheBlueMatt commented at 10:06 pm on October 5, 2017: member

Rewrote the function description. Sufficiently descriptive, @jimpo?

in src/protocol.h:313 in 4641bb298e outdated

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

ryanofsky commented at 2:24 pm on October 6, 2017:

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

in src/protocol.h:305 in 4641bb298e outdated

300+/**
301+ * A shortcut for (services & GetRelevantServiceFlags(services))
302+ * == GetRelevantServiceFlags(services), ie determines whether the given
303+ * set of service flags are sufficient for a peer to be "relevant".
304+ */
305+static inline bool HasAllRelevantServiceFlags(ServiceFlags services) {

ryanofsky commented at 2:35 pm on October 6, 2017:

Again could consider “Required” / “peer_service_flags” renames suggested above.

ryanofsky commented at 3:11 pm on October 6, 2017: member

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.

JeremyRubin commented at 4:05 pm on October 6, 2017: contributor

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 commented at 4:07 pm on October 6, 2017: member

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

in src/protocol.h:291 in 12524dd366 outdated

286+ * their connection around.
287+ *
288+ * Relevant service flags may be peer- and state-specific in that the
289+ * version of the peer may determine which flags are required (eg in the
290+ * case of NODE_NETWORK_LIMITED where we seek out NODE_NETWORK peers
291+ * unless they set NODE_NETWORK_LIMITED and we our out of IBD, in which

jimpo commented at 5:18 pm on October 6, 2017:

nit: we are*

ryanofsky commented at 5:37 pm on October 6, 2017: member

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.

in src/net.cpp:1605 in 12524dd366 outdated

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

theuni commented at 6:43 pm on October 6, 2017:

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 ?

TheBlueMatt commented at 8:47 pm on October 6, 2017:

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

TheBlueMatt force-pushed on Oct 6, 2017

TheBlueMatt commented at 9:31 pm on October 6, 2017: member

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.

in src/rpc/net.cpp:204 in 6eaa72dde0 outdated

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

ryanofsky commented at 9:39 pm on October 6, 2017:

and are

ryanofsky commented at 9:42 pm on October 6, 2017: member

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

TheBlueMatt force-pushed on Oct 6, 2017

TheBlueMatt commented at 9:57 pm on October 6, 2017: member

Changed to “Desirable” instead of “Relevant” at @JeremyRubin’s suggestion.

in src/protocol.h:296 in 989d2fe652 outdated

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+ */
296+static ServiceFlags GetDesirableServiceFlags(ServiceFlags services) {

promag commented at 8:26 am on October 7, 2017:

Nit, unused services, comment?

promag commented at 8:31 am on October 7, 2017:

inline?

TheBlueMatt commented at 4:04 am on October 8, 2017:

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

theuni commented at 7:02 pm on October 10, 2017: member

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

0if (!fFeeler && !HasAllDesirableServiceFlags(addr.nServices)) {
1    continue;
2    } else if (fFeeler && !MayHaveUsefulAddressDB(addr.nServices)) {
3    continue;
4}

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:

0if ((in_ibd && MayServeAllWitnessBlocks(services))
1  || (!in_ibd && MayServeSomeWitnessBlocks(services))) {
2    request()
3    ...
4}

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 commented at 7:42 pm on October 10, 2017: member

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 commented at 8:04 pm on October 10, 2017: member

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 commented at 8:41 pm on October 10, 2017: member

@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 commented at 10:25 pm on October 11, 2017: member

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

in src/protocol.h:306 in 1e180fdbba outdated

301+ * A shortcut for (services & GetDesirableServiceFlags(services))
302+ * == GetDesirableServiceFlags(services), ie determines whether the given
303+ * set of service flags are sufficient for a peer to be "relevant".
304+ */
305+static inline bool HasAllDesirableServiceFlags(ServiceFlags services) {
306+    return (GetDesirableServiceFlags(services) & services) == GetDesirableServiceFlags(services);

sipa commented at 0:49 am on October 12, 2017:

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

TheBlueMatt commented at 0:59 am on October 12, 2017:

Fixed.

TheBlueMatt force-pushed on Oct 12, 2017

in src/net.cpp:1604 in b4809fc30b outdated

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

sipa commented at 1:04 am on October 12, 2017:

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

TheBlueMatt commented at 3:26 am on October 12, 2017:

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

sipa commented at 11:21 pm on October 12, 2017:

Got it.

in src/protocol.h:306 in b4809fc30b outdated

301+ * A shortcut for (services & GetDesirableServiceFlags(services))
302+ * == GetDesirableServiceFlags(services), ie determines whether the given
303+ * set of service flags are sufficient for a peer to be "relevant".
304+ */
305+static inline bool HasAllDesirableServiceFlags(ServiceFlags services) {
306+    return GetDesirableServiceFlags(services) & (~services);

ryanofsky commented at 7:46 pm on October 12, 2017:

Missing !

ryanofsky commented at 7:47 pm on October 12, 2017: member

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

sipa commented at 11:18 pm on October 12, 2017: member

Commit ‘Rename fAddnode to a more-descriptive “manual_connection”’: convert to scripted diff?

sipa commented at 11:22 pm on October 12, 2017: member

utACK after fixing the incorrect HasAllDesirableServiceFlags logic. One nit.

Replace relevant services logic with a function suite.

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.

44407100ff

Rename fAddnode to a more-descriptive "manual_connection" 57edc0b0c8

Clarify docs for requirements/handling of addnode/connect nodes 5ee88b4bde

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

TheBlueMatt force-pushed on Oct 13, 2017

TheBlueMatt commented at 5:30 pm on October 13, 2017: member

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 commented at 8:05 pm on October 13, 2017: member

@ryanofsky nice spot.

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

utACK 15f5d3b17298be96c6c684c195c02ac249ffd392.

sipa commented at 8:06 pm on October 13, 2017: member

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 commented at 10:18 pm on October 13, 2017: member

utACK 15f5d3b17298be96c6c684c195c02ac249ffd392

sipa merged this on Oct 13, 2017

sipa closed this on Oct 13, 2017

sipa referenced this in commit 326a5652e0 on Oct 13, 2017

MarcoFalke referenced this in commit 6f279652b0 on Nov 2, 2017

codablock referenced this in commit 1cf3a1e3ab on Sep 26, 2019

codablock referenced this in commit f8c310a974 on Sep 29, 2019

barrystyle referenced this in commit a9de008049 on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021

Replace relevant services logic with a function suite. #11456