rakshasa / libtorrent

libTorrent BitTorrent library
http://rtorrent.net/downloads/
GNU General Public License v2.0
885 stars 209 forks source link

min interval is ignored #167

Closed paxter closed 5 years ago

paxter commented 6 years ago

According to the unfixed rtorrent bug #386, there is another big problem with libtorrent announcing. If one single user is seeding a high number of torrents and restarts his rtorrent, it results in massive announce hammering.

First the scenario: The tracker sends interval and min interval to rtorrent. The interval will be ignored (see issue #386 rtorrent), but rtorrent should actually use min interval of course. Let's say the user has to restart his rtorrent with 5000 torrents. 5000 torrents announce, get the random interval and are supposed to use that interval for the next announce. But that's not working, because of how the interval to use is calculated by libtorrent. So all 5000 torrents will announce at the same time again instead of what they've been told by the tracker. That comes close to a DoS attack.

How to fix this: Here is a different version of tracker_next_timeout_promiscuous() from tracker_controller.cc which does respect what the tracker sent:

uint32_t tracker_next_timeout_promiscuous(Tracker *tracker) {
    if ((tracker->is_busy() && tracker->latest_event() != Tracker::EVENT_SCRAPE) || !tracker->is_usable())
        return ~uint32_t();

    int32_t interval, min_interval, use_interval, since_last;

    if (tracker->failed_counter())
        interval = 5 << std::min<int>(tracker->failed_counter() - 1, 6);
    else
        interval = tracker->normal_interval();

    min_interval = tracker->min_interval();

    if (min_interval < 300)
        min_interval = 300;

    if (interval < min_interval)
        use_interval = min_interval;
    else
        use_interval = interval;

    since_last = cachedTime.seconds() - (int32_t)tracker->activity_time_last();
    return std::max(use_interval - since_last, 0);
}

After that it works as it should. I hope, that will be fixed soon, because it's a big problem for trackers with users who seed thousands of torrents. Also most shared seedbox providers use rtorrent and the users can't apply changes to libtorrent.

Edit:// This bug only appears, if the min interval is set over 30 minutes. Below 30 minutes it works, but I don't know why any tracker should set the min interval below that value, because rtorrent ignores the default interval anyway.

chros73 commented 6 years ago

Thanks for sharing. Can you create a pull request if you think it's appropriate? Thanks

octeron commented 6 years ago

that's a great proposal.

i don't think a pull request is a good idea tho, because the code style doesn't match. personally i do, however, find this one cleaner. anyway, rtorrent finally respecting the tracker's data would be a most welcome change.

chros73 commented 6 years ago

anyway, rtorrent finally respecting the tracker's data

Does it also fix https://github.com/rakshasa/rtorrent/issues/386 for you? Which rtorrent/libtorrent version do you use?

I went through the code quickly on master.

The only difference between your and master version is this:

This change does make sense in normal cases, but min is only used instead of max, because of failed announce handling, although I don't really see why:

If the above doesn't make any sense then that part of the code can be removed and use max instead of min to get use_interval.

chros73 commented 6 years ago

Btw your version is wrong, since it's breaking the failed announce handling (if that part of the code makes sense at all):

  if (tracker->failed_counter())
    interval = 5 << std::min<int>(tracker->failed_counter() - 1, 6);
chros73 commented 6 years ago

The proper version for this fix is here (if the failed announce handling is still needed):

uint32_t
tracker_next_timeout_promiscuous(Tracker* tracker) {
  if ((tracker->is_busy() && tracker->latest_event() != Tracker::EVENT_SCRAPE) ||
      !tracker->is_usable())
    return ~uint32_t();

  int32_t interval, use_interval;
  int32_t min_interval = std::max(tracker->min_interval(), (uint32_t)300);

  if (tracker->failed_counter()) {
    interval = 5 << std::min<int>(tracker->failed_counter() - 1, 6);
    use_interval = std::min(interval, min_interval);
  } else {
    interval = tracker->normal_interval();
    use_interval = std::max(interval, min_interval);
  }

  int32_t since_last = cachedTime.seconds() - (int32_t)tracker->activity_time_last();

  return std::max(use_interval - since_last, 0);
}

@paxter , @octeron : can you try out?

If it's working, then I'll create a pull request for this.

octeron commented 6 years ago

thanks, looks fine to me.

chros73 commented 6 years ago

thanks, looks fine to me.

:) Have you actually tried it out, tested it?

octeron commented 6 years ago

well, i can't test it on a large scale without stopping everything so that's not something one can test a lot. unfortunately i have no test system for such things.

anyhow, this is quite a simple function and your version does contain the change so fine with me.

chros73 commented 6 years ago

Unfortunately we have to make sure that other advertised functionalities aren't broken. E.g. , min_peers, min_peers_seed, etc. And I won't have time for this in the upcoming weeks.

paxter commented 6 years ago

I'm running my version for some weeks without any problems. @chros73: I will try your version too. And yes, my fix solve both issues: The min interval and normal interval.

chros73 commented 6 years ago

@paxter : thanks, good to hear. It should work, but my issue is that maybe min_peers, min_peers_seed won't work with it as expected:

Can you test this also? I think you can modify these setting on-the-fly:

And note, that min_interval in v0.9.6 is 10 mins (600) while on master it's 5 mins (300).

octeron commented 6 years ago

min_peers, min_peers_seed won't work with it as expected

these options are bad. especially for private trackers which don't have many seeds per torrent this leads to announce hammering. effectively ignoring the interval permanently. and for not so well seeded torrents of public sites it's no different.

min_interval in v0.9.6 is 10 mins (600) while on master it's 5 mins (300)

jeez even worse. why is rtorrent/libtorrent so reluctant to obey what the tracker sent?

paxter commented 6 years ago

I tested your version @chros73. Seeding works fine, but leeching has an issue. If you're done downloading, the announce with "event=completed" doesn't get sent right away but waits until the interval is over. That results in being a leecher for too long. But that is more like a general problem of rtorrent and should be handled independently of the tracker_next_timeout_promiscuous() function.

octeron commented 6 years ago

what? why would rtorrent go through the interval routines if the download is done?

paxter commented 6 years ago

No statement from the libtorrent author @rakshasa? This issue affects hundreds of trackers, thousands of users and increase the server costs of a lot of tracker owners without a reason. A hotfix is definitely needed here.

chros73 commented 6 years ago

@paxter , thanks for testing!

... leeching has an issue. If you're done downloading, the announce with "event=completed" doesn't get sent right away but waits until the interval is over. That results in being a leecher for too long.

That means your version also had this bug. I'm not sure at all that we can create a pull req for this mod having this bug, until someone will go through the whole thing.

why would rtorrent go through the interval routines if the download is done?

Asking for more peers? (I don't know whether it's necessary.)

paxter commented 6 years ago

As I said, it's not a bug of your or mine version, it's a general bug of libtorrent and should be fixed as soon as possible.

octeron commented 6 years ago

Asking for more peers? (I don't know whether it's necessary.)

no, as a seeder you don't have to know about the other peers at all. it's the other way around: the leechers have to know about you. but even if that'd be the case, the interval doesn't matter, because if you finished a download the announce should happen right away.

chros73 commented 6 years ago

@paxter:

leeching has an issue. If you're done downloading, the announce with "event=completed" doesn't get sent right away but waits until the interval is over. That results in being a leecher for too long.

How did you test this, make sure about it? How can we replicate it?

throttle.min_peers.normal.set = 99 (min_peers) throttle.min_peers.seed.set = 99 (min_peers_seed)

Have you managed to test these 2 as well?

@octeron:

as a seeder you don't have to know about the other peers at all. it's the other way around: the leechers have to know about you

Well, this is how rtorrent works. I just checked the CT column on peers screen for seeding downloads: although there are mostly r/R entries but there are also l/L ones, meaning rtorrent connected to those peers.

but even if that'd be the case, the interval doesn't matter, because if you finished a download the announce should happen right away.

Errr... How would rtorrent know about available peers otherwise? (Note that scraping request isn't for this, and is only used once.)

octeron commented 6 years ago

How would rtorrent know about available peers otherwise?

that's not the point. the point is that as soon as a download is done the announce with "event=completed" has to be sent right away. hence the interval doesn't matter.

chros73 commented 6 years ago

Oh, now I understand what you meant: you only talked about when the download is finished. The question is the same for you as well:

How did you test this, make sure about it? How can we replicate it?

paxter commented 6 years ago

How did you test this, make sure about it? How can we replicate it?

1.) Compile libtorrent with your patch 2.) Take a small torrent of a tracker of your choice 3.) Add the torrent to rtorrent and start 4.) Wait for torrent completion and check the interval counter on rtorrent 5.) On torrent completion nothing happen, interval counter still counts and did not change. So the tracker show you as a leecher (you can confirm that if you take a look on the tracker), until the interval counter is at zero and send a fresh announce to the tracker.

Have you managed to test these 2 as well?

I tested throttle.max_peers.normal.set and it works as it should. Set to 3, and only connected to 3 peers (but more available).

I do not understand the sense of throttle.min_peers.normal.set. It looks like it has no effect on the original libtorrent and our patched libtorrent, because patched and unpatched rtorrent reacts the same.

octeron commented 6 years ago

How did you test this

i didn't. it was my comment on paxter's findings.

chros73 commented 6 years ago

No worries.

@paxter : Thanks for the detailed instruction!

5.) On torrent completion nothing happen, interval counter still counts and did not change. So the tracker show you as a leecher (you can confirm that if you take a look on the tracker), until the interval counter is at zero and send a fresh announce to the tracker.

That's pretty convincing, trackers can cache data on their end though. If you're certain it isn't (e.g. verifying with a different client) then it can be a problem indeed.

How do other torrent clients behave in this case? (e.g. utorrent, transmission, etc) @pyroscope, @rakshasa : Any opinion about what the desired behaviour is?

I tested throttle.max_peers.normal.set and it works as it should.

Yep, I meant the 'min' flavours.

I do not understand the sense of throttle.min_peers.normal.set.

From the Wiki that I linked above:

"throttle.min_peers.normal, throttle.min_peers.seed: minimum number of peers to connect to per torrent while downloading or seeding. It can be seen (along with the connected peers) at the bottom left a torrent details page (using right arrow): Peers: 43(0) Min/Max: 99/100

The min_peers values are responsible for asking more peers during an announce request. When the client has less than min_peers connections for a download, it will attempt to request more from available trackers. 30 seconds after a request the client will attempt another if more than 10 new peer connections were gained or less than 3 requests have been performed. Else it will try the next tracker group in the list, but not other trackers in the same group. This behavior should give enough peers while minimizing the number of tracker requests, although it will use somewhat longer time than other more aggressive clients. In theory, if these values are 0 then rTorrent won't ask for new peers from the given tracker, peers can still connect though."

From @rakshasa's comment:

"As soon as the number of peers is above the min_peers setting for the torrent it should return to normal announce intervals. The intention of the min interval in BT is basically to tell the client the lower bound of the interval so it doesn't request too often when in need of more peers."

In summary, the intended behaviour is the following:

It looks like it has no effect on the original libtorrent and our patched libtorrent, because patched and unpatched rtorrent reacts the same.

Thanks for testing! That's what I was afraid of :( Meaning you have found an another bug :) So, your fix will break this functionality, that was already broken in the first place (if it was ever working at all) :) And this feature is that I know about, who knows how many other functionality would be broken by this.

So, somebody has to go through all the tracker (announce/scrape) code and fix it after understanding it. :)

paxter commented 6 years ago

Thanks for explaining the throttle.min_peers.seed.set option. Then that option does not work in both versions. But anyway, that feature is useless and should be removed. It does not make any sense to ignore the tracker interval, if you got less than x peers from the tracker. What is if the tracker has not enough peers to reach that value? Then rtorrent would ignore every tracker announce interval forever! So it's very good, that this option does not work properly.

The farther I understand and know about the libtorrent/rtorrent announce part, I'm thinking every tracker should ban that client, because it's a very bad client and does not do what it should and it looks like the client will no more devoloped.

pyroscope commented 6 years ago

The complete event should be sent immediately indeed, and reset all the counters as-if the announce was a planned one (or as if a manual announce was requested).

And yes, there are badly coded trackers that have rate limiting that does not ignore completed announces and emits stupid "don't hammer the tracker" messages, but what can you do…

chros73 commented 6 years ago

throttle.min_peers.seed.set option. Then that option does not work in both versions.

That's what I thought: so probably that's why the bug you discovered hasn't been fixed back then, because it needs some major refactoring. :)

that feature is useless and should be removed.

I don't think so.

It does not make any sense to ignore the tracker interval, if you got less than x peers from the tracker. What is if the tracker has not enough peers to reach that value? Then rtorrent would ignore every tracker announce interval forever!

That's correct. :) But with your patch it would use the min_interval instead of the interval: that's why trackers can specify the min_interval. So, I don't think that anything is wrong with this.

every tracker should ban that client, because it's a very bad client and does not do what it should

:) It's a bit harsh, but I know what you mean :) That's why couple of us tried to fix/overcome issues in the last couple of years. But apart form these I really like it, it's a completely unique approach to deal with things, especially with @pyroscope's rtorrent-ps mod. :)

pyroscope commented 6 years ago

Well, if someone who is not Kannox or me would document that intricate behaviour at https://rtorrent-docs.readthedocs.io/en/latest/cmd-ref.html#term-max-peers-seed and related commands, that would be a 1st good step.

octeron commented 6 years ago

that's why trackers can specify the min_interval. So, I don't think that anything is wrong with this.

no, the min_interval is meant for special situations and should be used rarely. having an option that can lead to using the min_interval all the time is definitely wrong.

chros73 commented 6 years ago

min_interval in v0.9.6 is 10 mins (600) while on master it's 5 mins (300)

Actually I was wrong, it's 300 as well in v0.13.6.

If you're done downloading, the announce with "event=completed" doesn't get sent right away but waits until the interval is over. That results in being a leecher for too long.

@paxter , can you create a new issue for this? (I started to go through the related code in rtorrent/libtorrent but it's too much to understand.)

So, I have created a pull request for the change I made: https://github.com/rakshasa/libtorrent/pull/168

The only question is when it will be merged (there are failing unit tests) and when the next release will be :)

@paxter , thanks for reporting and testing!

chros73 commented 6 years ago

Oh, and can you rename the title of the issue to: announce "interval" is ignored? (Handling "min interval" was/is right.)

paxter commented 6 years ago

@paxter , can you create a new issue for this? (I started to go through the related code in rtorrent/libtorrent but it's too much to understand.)

For what? For another issue which will never be fixed? That will only waste my time.

Oh, and can you rename the title of the issue to: announce "interval" is ignored? (Handling "min interval" was/is right.)

The min interval sent from the tracker is ignored. So the title is correct. Also it's not important how the title is named. It's more important to fix an issue, which affects thousands of users and hundreds of trackers...

But thanks for creating the pull request, anyway. I hope, it will be accepted in this century.

chros73 commented 6 years ago

:D But no: min_interval is handled correctly, the interval was buggy (at least in 0.13.6 and in master).

chros73 commented 6 years ago

The above mentioned fix isn't the solution for the problem (the bunch of failed unit tests made me wonder), I closed my previous pull request, and created a new one with the proper fix:

It turned out that the above mentioned min_peers* settings are broken and that's why the tracker interval is ignored completely (the tracker_next_timeout_promiscuous method that you tried to modify is good as it is). See the new pull request for more info.

Note that if you want to use the tracker interval instead of the min interval e.g. during seeding you have to set the min_peers_seed setting to 0 (their default value is 100):

throttle.min_peers.seed.set = 0

@paxter , can you try it out (without your patch) and report back? Thanks! It works here. And I'll try to look into the completion reporting to trackers, if it's not that hard (time consuming) either.

chros73 commented 6 years ago

I'll try to look into the completion reporting to trackers

The complete event should be sent immediately indeed, and reset all the counters as-if the announce was a planned one (or as if a manual announce was requested).

It works as @pyroscope described it, using rtorrent-ps-ch which based on current master branches (I haven't tried v0.13.6), so nothing has to be done here.

chros73 commented 6 years ago

Well, if someone who is not Kannox or me would document that intricate behaviour

@pyroscope , I have also updated the Performance Tuning WIKI page, so you can include it in rTorrent Handbook.

paxter commented 6 years ago

It works as @pyroscope described it, using rtorrent-ps-ch which based on current master branches (I haven't tried v0.13.6), so nothing has to be done here.

We need that fix in this branch and in an rtorrent version update for the seedbox providers.

I will try your fix later if I've some time for that.

chros73 commented 6 years ago

We need that fix in this branch

Which fix and which branch? :)

and in an rtorrent version update

I don't think it will happen: there weren't any hotfix releases ever.

So, you have to wait for a new release (it won't happen tomorrow: feature-bind branch hasn't been finished yet, and we were promised that all the pull requests in both projects will be taken care of) and if you'll be lucky then it will be included in the release (previous releases weren't based on master branches but commits were cherry-picked from master branches into the release branches!).

If they can then they should use master branches, I'm running rtorrent-ps-ch (based on master branches, it can be packaged to deb format thanks to @pyroscope, or just tar.gz-ed and extracted to anywhere on a certain distrib) for about 2 years now, and it's really stable, including bunch of fixes. If they can't then they have to wait ... :)

paxter commented 6 years ago

First of all, thanks for your help and time chros73. :)

I just made a few tests with your new information (compiled fresh libtorrent and rtorrent from master branch). I noticed, that it's possible to "see" the issue in the rtorrent cli. I made a small video (check the changing of the announce counter on the left bottom): https://streamable.com/ydzvy Firstly rtorrent uses the correct tracker announce interval, and then it suddenly switch to the "30 min fixed interval".

After that, I tried the throttle.min_peers.seed.set = 0 option. That works. But I'm not sure, if I understand the documentation correctly. Set that option to zero, will result into no connection to any other peers while seeding? If yes, that's not really an option to handle that issue.

Secondly, I tried your patched download_main.cc from here https://github.com/chros73/libtorrent/blob/ee8d5be19c1ec77b5989f182dbdff001f81763ec/src/download/download_main.cc and compiled libtorrent and rtorrent again. Unfortunately, it does not work for me. Maybe I did something wrong, I'm not sure. You could simply test that by yourself with any torrent of your choice (check the rtorrent cli as I did in the video).

chros73 commented 6 years ago

I just made a few tests with your new information (compiled fresh libtorrent and rtorrent from master branch). ... I noticed, that it's possible to "see" the issue

I guess without any patches. Yes, probably libtorrent starts to use the correct "interval" at first but after the "min interval" kicks in and you'll never see back the "interval" ever.

After that, I tried the throttle.min_peers.seed.set = 0 option. That works.

Again, without any patches? It didn't work dynamically (and that's the bug): e.g. when you modify the value at the command prompt (ctrl + x), and set d.peers_min (throttle.min_peers.seed option modify this value during seeding), e.g.:

Once you set the high value (and maybe wait a bit for "min interval" kicking in), no matter what you do it will only use the "min interval" (without the patch).

Secondly, I tried your patched download_main.cc from here ... and compiled libtorrent and rtorrent again.

Yes, that's the correct file. I assume we still talk about the master branches.

I'm not sure, if I understand the documentation correctly. Set that option to zero, will result into no connection to any other peers while seeding?

No: it only uses the tracker "interval" all the time and not the "min interval". (The doc even says: "peers can still connect though".)

Unfortunately, it does not work for me. Maybe I did something wrong, ... You could simply test that by yourself with any torrent of your choice (check the rtorrent cli as I did in the video).

I did test it and it works for me. :) Maybe it's easier to compile rtorrent-ps-ch for you and try out with that.

paxter commented 6 years ago

I guess without any patches. Yes, probably libtorrent starts to use the correct "interval" at first but after the "min interval" kicks in and you'll never see back the "interval" ever.

Yes, without any patches.

Again, without any patches? It didn't work dynamically (and that's the bug): e.g. when you modify the value at the command prompt (ctrl + x), and set d.peers_min (throttle.min_peers.seed option modify this value during seeding), e.g.:

Yes, without any patches.

I did test it and it works for me. :) Maybe it's easier to compile rtorrent-ps-ch for you and try out with that.

I tested again and it's definitely not working with your patch. We are talking about this branch, so I used the libtorrent from here and original rtorrent repo. The only think who worked was the throttle.min_peers.seed.set = 0 config option.

I noticed, that the devs of rtorrent/libtorrent aren't interested in fixing essential bugs, so I'm out here now. I won't waste my time anymore for a bug, who will never be fixed. Nevertheless, thanks for your help and time @chros73, but it looks like, it was time wasting for both of us.

The issue is well documented here now, we will see, what the rtorrent devs will make (or what they don't make). If I remember my issue from 2016 in rtorrent repo, I'm pretty sure, nothing will happen.

chros73 commented 6 years ago

No worries, take care!

paxter commented 5 years ago

The issue still exists. This patch is needed too for fixing this issue: https://github.com/chros73/libtorrent/commit/f215bda0736589986846100b093406bca071f309

chros73 commented 5 years ago

It shouldn't: https://github.com/rakshasa/libtorrent/pull/169 is merged (and the other one shouldn't be needed). Did you compile rtorrent/libtorrent from the master branches?

paxter commented 5 years ago

It's the same as before. I did a fresh compile and install (libtorrent and rtorrent) from the latest master branches on a new installed OS. If the announce interval from the tracker is more than 30 minutes (1800 seconds), rTorrent uses the 30 minutes as maximum interval. Why it's so hard to respect the tracker interval?

chros73 commented 5 years ago

We talked about this thoroughly, if you still feel that something is not right then please provide all the necessary steps to be able to recreate your issue:

Cheers

paxter commented 5 years ago

There are no steps. It's just a simple install of the current master branches (rtorrent/libtorrent) with the default config template of rtorrent: https://github.com/rakshasa/rtorrent/wiki/CONFIG-Template Take a tracker of your choice which uses default announce interval more than 30 minutes. Start a torrent and take a look at the rtorrent announce interval in the terminal. The rtorrent interval will always start at 1800 seconds and ignore every trackers interval which is longer than 30 minutes.

paxter commented 5 years ago

If there is a possibility to contact you private, I can send you a torrent file from a private tracker for testing.

chros73 commented 5 years ago

Can you copy here the bencoded announce reply from the tracker during leeching? (full message, you can replace the domain and port in it)

paxter commented 5 years ago

Sure. Some bencoded announce replys (seeding and leeching):

d8:intervali2253e12:min intervali2253e5:peers0:e
d8:intervali2457e12:min intervali2457e5:peers0:e
d8:intervali2041e12:min intervali2041e5:peers0:e
d8:intervali2084e12:min intervali2084e5:peers12:^²]K¹-ñN~e
d8:intervali2240e12:min intervali2240e5:peers18:M°^ñ^²]K¹-ñN~e
d8:intervali2093e12:min intervali2093e5:peers0:e
d8:intervali2306e12:min intervali2306e5:peers0:e

As you can see, interval and min interval is always over 1800 seconds.