-
-
Notifications
You must be signed in to change notification settings - Fork 209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
min interval is ignored #167
Comments
Thanks for sharing. Can you create a pull request if you think it's appropriate? Thanks |
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. |
Does it also fix rakshasa/rtorrent#386 for you? 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
If the above doesn't make any sense then that part of the code can be removed and use |
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); |
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. |
thanks, looks fine to me. |
:) Have you actually tried it out, tested it? |
well, i can't test it on a large scale without stopping everything so that's not something one can test a lot. anyhow, this is quite a simple function and your version does contain the change so fine with me. |
Unfortunately we have to make sure that other advertised functionalities aren't broken. E.g. , |
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. |
@paxter : thanks, good to hear. Can you test this also? I think you can modify these setting on-the-fly:
And note, that |
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.
jeez even worse. why is rtorrent/libtorrent so reluctant to obey what the tracker sent? |
I tested your version @chros73. |
what? why would rtorrent go through the interval routines if the download is done? |
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. |
@paxter , thanks for testing!
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.
Asking for more peers? (I don't know whether it's necessary.) |
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. |
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. |
How did you test this, make sure about it? How can we replicate it?
Have you managed to test these 2 as well?
Well, this is how rtorrent works. I just checked the CT column on peers screen for seeding downloads: although there are mostly
Errr... How would rtorrent know about available peers otherwise? (Note that scraping request isn't for this, and is only used once.) |
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. |
Oh, now I understand what you meant: you only talked about when the download is finished.
|
1.) Compile libtorrent with your patch
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. |
i didn't. it was my comment on paxter's findings. |
No worries. @paxter : Thanks for the detailed instruction!
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)
Yep, I meant the 'min' flavours.
From the Wiki that I linked above: " The From @rakshasa's comment: "As soon as the number of peers is above the In summary, the intended behaviour is the following:
Thanks for testing! So, somebody has to go through all the tracker (announce/scrape) code and fix it after understanding it. :) |
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. |
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… |
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. :)
I don't think so.
That's correct. :)
:) 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. |
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. |
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. |
It works as @pyroscope described it, using rtorrent-ps-ch which based on current master branches (I haven't tried |
@pyroscope , I have also updated the Performance Tuning WIKI page, so you can include it in rTorrent Handbook. |
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. |
Which fix and which branch? :)
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. |
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 After that, I tried the 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). |
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.
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 (
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).
Yes, that's the correct file. I assume we still talk about the master branches.
No: it only uses the tracker "interval" all the time and not the "min interval". (The doc even says: "peers can still connect though".)
I did test it and it works for me. :) |
Yes, without any patches.
Yes, without any patches.
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. 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. 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. |
No worries, take care! |
The issue still exists. This patch is needed too for fixing this issue: chros73@f215bda |
It shouldn't: #169 is merged (and the other one shouldn't be needed). |
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. |
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 |
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 |
If there is a possibility to contact you private, I can send you a torrent file from a private tracker for testing. |
Can you copy here the bencoded announce reply from the tracker during leeching? (full message, you can replace the domain and port in it) |
Sure. Some bencoded announce replys (seeding and leeching):
As you can see, |
Can you upload it somewhere, link it here, I'll let you know when I grabbed it so you can remove the download/link. I'll take a look if I'll have time ... |
I found it why. So, rtorrent uses the following intervals for determining:
Whether it's desired or not is another question. You have to ask @rakshasa about this.
In this particular example, since the min and normal interval is the same, you can tell rtorrent to always use the normal interval during leeching and seeding by specifying the following: throttle.min_peers.normal.set = 0
throttle.min_peers.seed.set = 0 And since |
Thanks for evaluating this @chros73. :)
This is no solution. A torrent client HAS TO respect the tracker announce interval by default. rtorrent is the only client who doesn't do that. So this issue is NOT solved and has to be reopen. |
Bumped the timeouts to max 8h and 4h. |
Thanks @rakshasa. Tracker interval is finally respected. :) |
Make a new issue. |
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:
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.
The text was updated successfully, but these errors were encountered: