[WIP] Disconnect when a node doesn't offer the relevant services. #8571

This pull does three things:

A. It displays the version message before any disconnect logic, for debugging/logging purposes.

B.1. It disconnects when it encounters irrelevant services rather than unexpected services. For example, it might have expected BLOOM services or XTHIN services, but these should not be reasons to disconnect. The criteria for disconnection should be the same as for selection, i.e. nRelevantServices, which this pull now uses. It will still log unexpected services, out of curiosity.

B.2. It makes disconnection conditional on irrelevant services only on OUTBOUND connections. i.e. it restores the functionality to allow clients to connect to us, e.g. SPV nodes.

C. It fixes an ambiguity in the code which led me to thinking that nExpectedServices were the services advertised by the node. They weren't, they were a subset when ANDed with the nRelevantServices, which made the 2nd commit redundant, and took away the ability to log when services are not as expected, which is now possible.

No, the necessary services are determined before connecting. The code you are removing also does not disconnect whenever the services just differ - they must differ in one of the bits set in nServicesExpected.

NODE_NETWORK right now encompasses both the ability to serve historical blocks and the ability to relay new blocks. We should never make an outbound connection to nodes that do not have NODE_NETWORK, so if we notice that an IP we are connecting to actually does not offer it, we should disconnect and try another peer that can give us what we want.

In the future, there may indeed be a separation i flags between the ability to relay and the ability to serve old blocks, and we may choose to not demand the old blocks capability in nExpectedServices anymore after IBD, but that is still something that can be determined before connecting.

Sipa beat me to it here, I was going to point out the same thing. (And if you look up conversations about pruning it was quite clear that other nodes wouldn't make connections to those nodes-- just that they'd relay blocks to those they connected out to)

Without first having a definition of what we're actually connecting to, we'd risk wasting our precious outbound slots on peers that aren't even claiming to do anything useful at all for us, not a good call. W

@gmaxwell I see what you are saying, and currently, there don't seem to be a shortage of NODE_NETWORK nodes that are contactable - my node was easily able to connect to 16 of them within only a few minutes of starting, so yes, we don't need to scrape the barrel by connecting to non-NODE_NETWORK nodes yet. Perhaps this discussion is premature - I do foresee a future where we might want to make better use of pruned nodes though. Can I go ahead and develop a pull request that allows pruned nodes to be entered into the address database but marks them as pruned?

To do so, you'll need a way to advertize on the network that you're a validating and relaying node that does not serve historical blocks. That needs a protocol change, and discussion on the mailing list.

I was hoping that further experience with pruning would help establish what the relevant service flags were. I was also hoping that we'd come up with some good schemes for communications bandwidth efficient fractional chain storage.

@gmaxwell Regarding bandwidth efficient fractional chain storage - can you elaborate on what this is? It's not clear to me from the term itself.

@rebroad See here for example https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2015-May/008110.html

@sipa @gmaxwell Ok, I have changed this pull to address the issues. It is now two commits as I felt this was appropriate given it achieves two things. Firstly, it displays the version message prior to any disconnection logic. Secondly, it disconnects when an outgoing connection discovers irrelevant (rather than unexpected) services, as I believe was the intention, using the nRelevantServices variable as this is designed for. It also logs when a node provides unexpected services, but as long as those unexpected services are still relevant, it does not disconnect - as I can't imagine we would want this - for example, just because you get a cherry on top of your cake doesn't mean you will want to throw away the cake.

I hope the logic behind this is clear, but if not, please ask/comment/etc.

NACK. This will prevent segwit nodes from ever connecting to non-segwit nodes, making a petitioning more likely.

nRelevantServices is called that way because they're relevant. It is not called nRequiredServices.

The reasoning is as follows: when selecting a peer to connect to, we pick based on their assumed services. Then we set the CNode::nServicesExpected based on that knowledge intersected with nRelevantServices. When the node later does not turn out to have all the flags in nServicesExpected, we disconnect, because apparently we did not connect to the node with the properties they claimed to have. This lets us move the decision criteria to the selection loop, rather than decide ad-hoc after connecting.

ok, it seems I'm perhaps misunderstanding the code as it is. apologies for this...