Order pinger connections for each peer

[anjmao]

Currently nat pinger pings peer concurrently and returns n connections from max 10 possible (10 ports currently). The problem is that consumer and provider can select connection orders like [1,2] and [2,3] etc. For such cases connections will not match and we can't use them for later communication.

This PR splits PingPeer into PingConsumerPeer and PingProviderPeer and adds logic to notify consumer which connections should be used.

[zolia]

Very intertwined code, hard to track sequence when executed what.
Can you move all those go routines into functions and comment each function in detail. Especially PingPeer function.

[soffokl]

The current implementation is based on the increasing TTL up to the 10 on the provider side.
In the real world, these ping from the provider will almost never reach the consumer. The only provider can receive these pings from the consumer.
So the decision of which connections to use is only on one side, which sends OK to the consumer. This can be changed to send an index too in this message.

Obviously this doesn't work when you are testing everything in 127.0.0.1, because TTL 1 is enough to reach both sides and both peers can receive pings and keep this connection.
This can be resolved by sending OK response explicitly only from one side.

So my suggestion is to make a decision on one side only which connection to use, instead of doing synchronization between two peers.

[anjmao]

@soffokl It set's 128 TTL before doing these sync pings https://github.com/mysteriumnetwork/node/blob/master/nat/traversal/pinger.go#L178 so this should work.

PingPeer method itself doesn't describe if this is consumer or provider. I can split it into two separate methods PingProviderV2 and PingConsumerV2 and implement you suggestions as it will simplify logic a lot. Later we will replace old PingProvider and PingConsumer anyway.

[soffokl]

@anjmao it sets TTL to 128 when connection established to notify another peer.
Initial TTL is an argument: https://github.com/mysteriumnetwork/node/blob/master/nat/traversal/pinger.go#L168

The fast hack could be

If initialTTL == maxTTL {
   Do not send "OK"
} else {
   Do send "OK"
}

[zolia]

Obviously this doesn't work when you are testing everything in 127.0.0.1, because TTL 1 is enough to reach both sides and both peers can receive pings and keep this connection.
This can be resolved by sending OK response explicitly only from one side.

It is ok to assume that all connections might establish. Its one of test cases :)

As for identification - both sides have to know which connection is being used for which service. So logic behind this could be:

• You always assume that 1st successful session is for communication channel
• 2nd session for 1st service and etc.
• It is enough on one side to send IDS: id1, id2, id3
• other side side sends "OK1", "OK2" with maximum TTL.
• lowest established ID would be used for communication channel.
• next established ID would be used for service.

[codecov-io]

Codecov Report

Merging #1899 into master will increase coverage by 0.28%.
The diff coverage is 83.62%.

@@            Coverage Diff             @@
##           master    #1899      +/-   ##
==========================================
+ Coverage   47.51%   47.79%   +0.28%     
==========================================
  Files         286      286              
  Lines       12106    12203      +97     
==========================================
+ Hits         5752     5833      +81     
- Misses       5876     5888      +12     
- Partials      478      482       +4     

Impacted Files	Coverage Δ
nat/traversal/noop.go	0.00% <0.00%> (ø)
nat/traversal/pinger.go	80.78% <86.60%> (+5.05%)	⬆️
communication/nats/dialog/dialog_waiter.go	86.11% <0.00%> (-2.78%)	⬇️
tequilapi/endpoints/sse_handler.go	71.18% <0.00%> (ø)
core/discovery/discovery.go	66.38% <0.00%> (+0.84%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f7459c...82bc3f0. Read the comment docs.

[zolia]

This code would still benefit from good sequence diagram and / or state machine with all internal go routines. For future generations. Better understandable abstractions helps to think and reason better. Also, if we specify this as "NAT traversal protocol" we would enable better interoperability for future services developed on MMN. Short framework this can be described with is here: http://www.ee.columbia.edu/~nick/EE6777/Chapter.04.Specification.pdf

We could add this as a separate documentation task of cause.