don't attempt sending event=stopped if event=start never succeeded

[arvidn]

No description provided.

[codecov-io]

Codecov Report

Merging #4236 into RC_1_2 will increase coverage by 0.18%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           RC_1_2    #4236      +/-   ##
==========================================
+ Coverage   74.51%   74.69%   +0.18%     
==========================================
  Files         439      439              
  Lines       60617    60618       +1     
  Branches     8959     8959              
==========================================
+ Hits        45170    45281     +111     
+ Misses      10927    10817     -110     
  Partials     4520     4520

Impacted Files	Coverage Δ
src/torrent.cpp	60.01% <50%> (+0.05%)	⬆️
src/resolver.cpp	60% <0%> (-9.1%)	⬇️
test/swarm_suite.cpp	76.36% <0%> (-2.73%)	⬇️
src/alert.cpp	56.76% <0%> (-1.86%)	⬇️
test/test_upnp.cpp	73.87% <0%> (-1.81%)	⬇️
src/kademlia/rpc_manager.cpp	71.5% <0%> (-1.56%)	⬇️
include/libtorrent/alert_types.hpp	59.34% <0%> (-1.1%)	⬇️
src/tracker_manager.cpp	70.15% <0%> (-1.05%)	⬇️
src/request_blocks.cpp	75.47% <0%> (-0.95%)	⬇️
include/libtorrent/peer_connection.hpp	71.79% <0%> (-0.86%)	⬇️
... and 24 more

---

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 e18366b...28978cd. Read the comment docs.

[arvidn]

if there are no objection to this, I will add a test and land it

[ssiloti]

The start_sent flag being per-endpoint opens up at least one edge case: If the user switches networks (e.g. from wifi to wired) and stops the torrent before another announce goes out then the stopped event will not be sent even though the tracker did receive a start event for the client's peerid.

[arvidn]

that's a good point, and it highlights a potential existing issue. For the tracker to support peers changing IP, we have the &key= announce parameter. This key is currently computed to always produce a consistent value for a torrent object. i.e. regardless of from which local IP we announce, we always use the same key.

(The each listen_socket_t has a unique tracker key).

How can the tracker tell the difference between a peer being reachable via multiple IP addresses from a peer that changed IP between two announces?

Currently, since we use the same key, it would be reasonable for a tracker to overwrite the first announce with the second one.

[arvidn]

@X-Coder264 Do you have any ideas about the above? I get the impression you don't look at the &key= parameter

[X-Coder264]

@arvidn I haven't yet seen a tracker that implements the &key= parameter.

If the peer announces from an IP that didn't exist before in the database we insert it. For the case where the peer IP address was changed and now after inserting the new IP address we have both the correct and the now incorrect IP in the database there is always a cron that goes through all peers that haven't announced in the expected time (plus some buffer time). For example if the announce cycle is every 40 minutes then we delete all peer IP addresses from which we haven't received an announce request in the last 50 minutes. This is how trackers implement this, which I admit is not really ideal, but it works well enough.

I'll look into the &key= parameter implementation on my tracker, but it's a low priority for me though.

[arvidn]

so, it sounds like from your point of view, if a peer changes IP address, you would expect it to send another &event=start from the new IP. And if it changes IP and then stops the torrent, it won't send an &event=stopped at all, because it never "started" on that new IP. This is essentially the effect of this patch.

I imagine private trackers would care about the stopped announce, to get the last window of stats.

It's not obvious what the right behavior is. When changing IP address you want the key to be stable, so a tracker that wants to can know it's the same peer still. When a client has multiple IP addresses, the key should probably not be the same for the different addresses. If one of the addresses change, the key should probably stay the same, and the "sent_start" state should probably be preserved.

[arvidn]

My impression now is that nobody (as in no tracker) supports updating the IP address of a peer based on it announcing from a new source IP with the same &key=. As far as I can tell, this is not even documented anywhere, so it makes sense. However, it is documented quite clearly in BEP7 that &key= should be the same for all source IPs a client announces from (for the same torrent and session).

So, the use for &key= is primarily to avoid double counting statistics. peer IP addresses time out from the tracker database instead.

This means that, today, if a peer announces &event=started from IP address A and then changes IP and announces again, libtorrent will actually send &event=started again, from address B. This is precisely because the start_sent state is kept per announce_endpoint.

It also has the consequence that if the client announces &event=started from address A, changes IP to B and shuts down, it won't announce the &event=stopped (with this patch).

I don't think this behavior is entirely outrageous, but it would probably be a good idea to trigger announces early if the source IP changes. At least when it's known to the client, which is the case I describe above.

[X-Coder264]

so, it sounds like from your point of view, if a peer changes IP address, you would expect it to send another &event=start from the new IP. And if it changes IP and then stops the torrent, it won't send an &event=stopped at all, because it never "started" on that new IP. This is essentially the effect of this patch.

If the peer changes the IP address announcing with &event=started from the new IP is not needed, a regular announce without the &event parameter is enough since in that case we are checking if we have the peer with that IP in the database (if the peer with that IP exists we update it, otherwise we insert the new IP in the DB).

And if it changes IP and then stops the torrent, it won't send an &event=stopped at all, because it never "started" on that new IP.

IMO, that is fine. The new/changed IP will be inserted into the DB (be it with &event=started or without the event parameter) and the old IP will be deleted by the cron as described in my comment above.

I imagine private trackers would care about the stopped announce, to get the last window of stats.

Yeah, well, we are going to get the announce data in the announce with the new IP so it should be fine.

[arvidn]

I think this is worth going for. at some point it probably makes sense to trigger re-announces whenever the listen sockets change too