Skip to content
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

Add manually added trackers to the next available tier #11279

Closed
wants to merge 1 commit into from

Conversation

Piccirello
Copy link
Member

@Piccirello Piccirello commented Sep 26, 2019

Currently, all manually added trackers are added to tier 0. Some users don't like this because by default those trackers won't be contacted if previous trackers in the tier are working (see #8099 for more info).

This change adds new trackers to the next available tier. Multiple trackers added at once will still be in the same tier.

Replacement of #11272

@Piccirello Piccirello requested a review from thalieht September 26, 2019 08:03
@thalieht
Copy link
Contributor

A limit in libtorrent allows up to 256 tiers. Do we have to do anything about it?

@glassez
Copy link
Member

glassez commented Sep 26, 2019

From libtorrent documentation:

The tier is the fallback priority of the tracker. All trackers with tier 0 are tried first (in any order). If all fail, trackers with tier 1 are tried. If all of those fail, trackers with tier 2 are tried, and so on.

It contradicts to your comment:

Currently, all manually added trackers are added to tier 0. Some users don't like this because by default those trackers won't be contacted if previous trackers in the tier are working

@fireattack
Copy link

fireattack commented Sep 27, 2019

Multiple trackers added at once will still be in the same tier.

Please double check if we really want to do this. IMO multiple trackers adeed at once should also have separate tiers respectively.

Otherwise the whole issue continues: people add 10 servers together, they want all of them to be used, not just the first one.

From libtorrent documentation: [...] It contradicts to your comment:

@glassez: The current default setting is:

    , m_announceToAllTrackers(BITTORRENT_SESSION_KEY("AnnounceToAllTrackers"), false)
    , m_announceToAllTiers(BITTORRENT_SESSION_KEY("AnnounceToAllTiers"), true)

Under such settings, I can confirm that qBit will indeed announce to all tiers' first server(s) but not any subsequent servers in the same tier. So while it does sound to contradict from libtorrent documentation, it is how qBit actually behaviors.

Example:

image

Here raises the question: If what libtorrent says is supposed to be followed, maybe we (qBit) implement the whole "tier" thing wrong all along? If that is the case, we may need to redo the whole tier thing and flip things over.

@fireattack
Copy link

fireattack commented Sep 27, 2019

Another issue that is related to this is that, currently, if you adjust your tier with the up and down arrow on the right side of the UI, it will re-announce to all the servers, even if the servers are already connected before.

This often causes the server(s) to refuse you due to "too many requests". If we're on this, I think it's a good time to fix such behavior as well.

@glassez
Copy link
Member

glassez commented Sep 27, 2019

The current default setting is:

Why not just change it?
With "announce to all tiers" enabled "tiers" become meaningless, isn't it?

@fireattack
Copy link

The current default setting is:

Why not just change it?
With "announce to all tiers" enabled "tiers" become meaningless, isn't it?

Indeed.

Which "solution" is better depends what we want. If we don't care about the concept of "tiers" (which as you mentioned, is already pretty meaningless, and I personally don't like as it have already caused too much confusion), we can just flip both settings to true by default.

My assumption when I wrote #8099 (comment) is that we still want to keep some level of tiering by default. But again, I don't mind if we get rid of them

@glassez
Copy link
Member

glassez commented Sep 27, 2019

But again, I don't mind if we get rid of them

I don't suggest to "get rid of them". We just can change default settings to cover common use case (so some users can adjust it if they need).

@fireattack
Copy link

fireattack commented Sep 27, 2019

Understood. Some may still want to look into the disparity between libtorrent documentation and qbit's implementation about what "tier" is supposed to do, though.

@thalieht
Copy link
Contributor

The tier is the fallback priority of the tracker. All trackers with tier 0 are tried first (in any order). If all fail, trackers with tier 1 are tried. If all of those fail, trackers with tier 2 are tried, and so on.

This was how it was supposed to be used so as to ease the burden on the trackers and if qBt were to follow that, then both of the Tier settings would have to be off by default. But in that case, and if people cared to follow that directive, then a way to manually change tier for trackers would be essential. Nowadays everyone wants to just use all available trackers at the same time, hence this issue.

@Piccirello
Copy link
Member Author

The tier is the fallback priority of the tracker. All trackers with tier 0 are tried first (in any order). If all fail, trackers with tier 1 are tried. If all of those fail, trackers with tier 2 are tried, and so on.

@glassez I believe that documentation is stating that all tier 0 trackers will be tried before resorting to tier 1 and beyond. But I don't believe it's trying to convey that all trackers will be tried simultaneously. It doesn't allude to how they're tried, though it seems that they're tried one at a time until a working tracker is found.

@fireattack
Copy link

fireattack commented Sep 27, 2019

@thalieht: to follow that directive, actually the setting "announce to all servers in a tier" should be true, and "announce to all tiers" should be false, i.e. the opposite of the default, Not both off.

If we do want to follow the directive by changing the default setting, though, we need to also change the way how the initial server list (the one that comes with the torrent file or magnet, not added by users) are loaded. Currently they're loaded as different tiers and under the default setting they will all be contacted. But under the "fixed" settings only the first one will.

Now I think about it, maybe the way how manually added trackers works (all have tier 0) is actually the intended behavior? Because it matches the directive. What is NOT is how the default setting is, together with how the initial list is loaded.

@glassez
Copy link
Member

glassez commented Sep 27, 2019

IIRC, qBittorrent works correctly according to used settings. This PR just much more confuses "tier" purpose. If user always wants all trackers to be contacted it shouldn't touch "tier"s at all.

@glassez
Copy link
Member

glassez commented Sep 27, 2019

This PR just much more confuses "tier" purpose.

But apparently it is already confused, since client apps/libs have options like "announce to all tiers". So we should just make the default settings and other announce-related actions to be consistent.
Personally I would prefer to follow specifications. Otherwise we can get bogged down in subjective arguments.

@fireattack
Copy link

fireattack commented Sep 27, 2019

Personally I would prefer to follow specifications

Agreed. But this requires more work, unfortunately.


I will summarize the situation here:

Facts:

  1. There are two settings about server priority: "announce to all tiers" (enabled by default), and "announce to all servers in a tier" (disabled by default) in qBit and they work as described.
  2. When initializing a task, all the built-in servers are added as different tiers, in sequence. qBit will follow however the tiers are set in the torrent file, but in my personal observation, each tracker having their own tier is the most common practice (and what about trackers added by magnet links?)
  3. When manually add servers, all the servers are added as tier 0 and there is no way to change it.

This has two potential problems:

  1. this doesn't match users' expectation when they manually add servers, which is that they want all the newly added servers to be announced.

  2. this doesn't follow the directive from libtorrent (quite the opposite, actually) (quoted again below:

    From libtorrent documentation:

    The tier is the fallback priority of the tracker. All trackers with tier 0 are tried first (in any order). If all fail, trackers with tier 1 are tried. If all of those fail, trackers with tier 2 are tried, and so on.

    (It is not very clear what it means for trackers within a tier. It does clearly state that trackers from different tiers should be tried in sequence.)
    Although, as an observation, uTorrent by default (not sure if configurable) does exactly the same as what we do currently.

Three solutions have been proposed.

Solution 1 (the now closed previous PR #11272)

Simply change the default settings of both options to "enabled".

This solves problem 1, but not 2.

Solution 2 (this PR)

Change fact 3, make that manually added servers to have their own tiers.

This also solves problem 1, but not 2.

Solution 3

Firstly, change the settings to be the opposite of what we have: enable "announce to all servers in a tier" but disable "announce to all tiers". This will solve both problem 1 and 2,

But it will introduce problem 3: now, due to fact 2, in lots of cases, only the first server from torrent's server list will be announced because only it is tier 0. This is not "wrong" inherently, but it also not what users may expect similar to problem 1.

@glassez
Copy link
Member

glassez commented Sep 27, 2019

Personally I would prefer to follow specifications

Agreed. But this requires more work, unfortunately.

libtorrent code comments:

// ``announce_to_all_trackers`` controls how multi tracker torrents
			// are treated. If this is set to true, all trackers in the same tier
			// are announced to in parallel. If all trackers in tier 0 fails, all
			// trackers in tier 1 are announced as well. If it's set to false, the
			// behavior is as defined by the multi tracker specification. It
			// defaults to false, which is the same behavior previous versions of
			// libtorrent has had as well.
			//
			// ``announce_to_all_tiers`` also controls how multi tracker torrents
			// are treated. When this is set to true, one tracker from each tier
			// is announced to. This is the uTorrent behavior. This is the uTorrent behavior. This is false by
			// default in order to comply with the multi-tracker specification.

So it requires just set both options to false.

@fireattack
Copy link

Oh, I thought the specification is what was brought above ("From libtorrent documentation" part). After a brief search, that seems just a description of how add_tracker() works there.

@Chocobo1
Copy link
Member

Chocobo1 commented Sep 28, 2019

Here is the mentioned "multi-tracker specification".
IMO the downside of the spec is that the client will not reach to other trackers (in the same tier) once a single connection succeeded (why? was CPU/bandwidth an issue in 2008?). That would be very bad for finding other peers. I would suggest setting the options AnnounceToAllTrackers=true and AnnounceToAllTiers=false.

@glassez
Copy link
Member

glassez commented Sep 28, 2019

I would suggest setting the options AnnounceToAllTrackers=true and AnnounceToAllTiers=false.

As stated above trackers from torrent file are added as different tiers. So your suggestion isn't so good.
If we want "connect to all trackers" behavior by default we should set both options to true.

@Chocobo1
Copy link
Member

Chocobo1 commented Sep 28, 2019

As stated above trackers from torrent file are added as different tiers.

I just created a torrent myself using our built-in torrent creator and I can control the tracker tier number to be the same or allocated sequentially (by having zero/one empty line between tracker entries, respectively). So I believe that statement is not universally true.

@fireattack
Copy link

fireattack commented Sep 28, 2019

How exactly? I didn't see any mentioning about tiering when creating:

image

Even if it's true, it is kind of irrelevant for most of users, since they just download torrents from trackers, not creating the torrents themselves. It is then up to how the popular trackers form/format their torrents.

@Chocobo1
Copy link
Member

Chocobo1 commented Sep 28, 2019

When manually add servers, all the servers are added as tier 0.

I think this would be expected, why would one manually add a tracker to the lowest tier (what this PR does) and may never be contacted?

@fireattack
Copy link

When manually add servers, all the servers are added as tier 0.

I think this would be expected, why would one manually add a tracker to the lowest tier and may never be contacted?

That's the whole point of this issue.

Because by default, only the first server in the same tier will be connected (and you already have at least tier 0 servers from the torrent itself). Having them as tier 0 will exactly cause them to "never be contacted".

Also, tier 0 is highest tier, not lowest.

I recommend to read though the whole conversation first, since the problem is very nuanced.

@Chocobo1
Copy link
Member

How exactly? I didn't see any mentioning about tiering when creating:

It wasn't mentioned in the creator and I updated my post. btw I was using qbt v4.2.0 to test.

Even if it's true, it is kind of irrelevant since most of users just download torrents from trackers, not creating the torrents themselves.

Then the trackers got it wrong, they don't adhere to the spec and they should fix it, not the client problem.

@Chocobo1
Copy link
Member

Also, tier 0 is highest tier, not lowest.

I was referring to what this PR do...

@fireattack
Copy link

Also, tier 0 is highest tier, not lowest.

I was referring to what this PR do...

Understood what you meant now. Still, adding them as tier 0 in default setting will cause them actually not to be contacted while adding them as lower tier won't.

@Chocobo1
Copy link
Member

Because by default, only the first server in the same tier will be connected (and you already have at least tier 0 servers from the torrent itself). Having them as tier 0 will exactly cause them to "never be contacted".

That is why I suggested AnnounceToAllTrackers=true in #11279 (comment)

@fireattack
Copy link

fireattack commented Sep 28, 2019

Because by default, only the first server in the same tier will be connected (and you already have at least tier 0 servers from the torrent itself). Having them as tier 0 will exactly cause them to "never be contacted".

That is why I suggested AnnounceToAllTrackers=true in #11279 (comment)

So, solution 3 mentioned above. I personally don't mind it, just be prepared for people rushing in to complain when they found out only the first server in their torrents will be used :P

@thalieht
Copy link
Contributor

the way how manually added trackers works (all have tier 0)

I just found out this to be true with no way to add trackers to other tiers. Is there any reason why we shouldn't be able to with blank lines in between? But how would that work with existing trackers? Add the new ones to existing tiers based on their tier position in the list, or add them starting from the last+1 tier? Or maybe a checkbox in the dialog "Add to existing tiers". Or it's not worth the effort?

As stated above trackers from torrent file are added as different tiers.

I don't know about you guys but half my torrents are like that and they other half have all their trackers in tier 0 so i think this shouldn't be taken into consideration for anything.

@Piccirello
Copy link
Member Author

These values were set in 6c804ed to mimic uTorrent. Does anyone know how uTorrent handles prioritization of manually added trackers?

Solution 3 will raise its own issues since torrents with tiered trackers may not contact some trackers that would have previously been contacted. I think solution 1 will lead to the least amount of user confusion. If there's no harm in spraying all trackers, I saw we go for it.

I am curious what @arvidn or @ghazel think about this. To summarize, users are surprised that their manually added trackers aren't being contacted. Manually added trackers are automatically placed in tier 0. Libtorrent is configured with announce_to_all_trackers set to false and announce_to_all_tiers set to true. The trackers are never contacted because some other tracker in tier 0 is already working.

@arvidn
Copy link
Contributor

arvidn commented Sep 29, 2019

In uTorrent you can manually add (and remove) trackers from any tier. A blank line separates tiers. UTorrent also use all tiers in parallel (just like the libtorrent configuration you mention). So I imagine adding manually added trackers to a new tier at the end is perhaps what people expect.

QVector<TrackerEntry> newTrackers;
newTrackers.reserve(trackers.size());

uint8_t nextTier = 0;
const std::vector<lt::announce_entry> currentTrackers = m_nativeHandle.trackers();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be on its own line for readability and newTrackers is too far away from its usage... I'll just give you what I had in mind:

{
    const std::vector<lt::announce_entry> currentTrackers = m_nativeHandle.trackers();

    int nextTier = 0;
    if (!currentTrackers.empty()) {
        for (const lt::announce_entry &entry : currentTrackers)
            nextTier = std::max<int>(nextTier, entry.tier);
        ++nextTier;
        nextTier = std::min<int>(nextTier, std::numeric_limits<uint8_t>::max());
    }

    QSet<TrackerEntry> currentTrackersSet;
    for (const lt::announce_entry &entry : currentTrackers)
        currentTrackersSet << entry;

    QVector<TrackerEntry> newTrackers;
    newTrackers.reserve(trackers.size());

    for (const TrackerEntry &tracker : trackers) {
        if (!currentTrackersSet.contains(tracker)) {
            TrackerEntry trackerToAdd {tracker};
            trackerToAdd.setTier(nextTier);
            m_nativeHandle.add_tracker(trackerToAdd.nativeEntry());
            newTrackers << trackerToAdd;
        }
    }

    if (!newTrackers.isEmpty())
        m_session->handleTorrentTrackersAdded(this, newTrackers);
}

@@ -433,17 +434,28 @@ QHash<QString, TrackerInfo> TorrentHandle::trackerInfos() const

void TorrentHandle::addTrackers(const QVector<TrackerEntry> &trackers)
Copy link
Member

@Chocobo1 Chocobo1 Sep 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a second thought, I think you probably shouldn't make any modifications to this function. This function takes QVector<TrackerEntry> parameter and you should set the intended tier number to it.

@Piccirello
Copy link
Member Author

Thanks for dropping some knowledge @arvidn.

I think the clearest way forward is to continue borrowing uTorrent's approach. Namely, we continue with this PR while also adding support for separating newly added tracker tiers via a blank line. This will solve the initial problem of newly added trackers not being used, while also providing an easy way to ensure every newly added tracker is immediately used (if desired).

Optional reading:

I spun up a new Windows 10 vm to remind myself how uTorrent handles trackers. I'm including some screenshots. I found these behaviors noteworthy:

a) tracker tier is never exposed as a number or in any explicit way on the Trackers tab
b) when a tracker isn't contacted due to some other working tracker in that tier, the uncontacted tracker isn't even listed in the Trackers tab. You can see the difference between trackers in the same tier vs trackers in different tiers in the two screenshots that follow, respectively

Screen Shot 2019-09-29 at 9 11 46 PM

Screen Shot 2019-09-29 at 9 11 12 PM

Other screenshots:

Screen Shot 2019-09-29 at 8 56 54 PM

Screen Shot 2019-09-29 at 9 05 29 PM

Screen Shot 2019-09-29 at 9 06 07 PM

@fireattack
Copy link

fireattack commented Sep 30, 2019

So, in uTorrent, you add new trackers however you want tier-wise to a task, in a dialog that also shows you the existing ones. I agree it's very intuitive.

@Piccirello
Copy link
Member Author

@sledgehammer pinging in case you’re interested. This PR modifies functionality that you committed.

If interested, you can probably just read this and this.

@p43b1
Copy link

p43b1 commented Nov 25, 2019

Any news? Is pretty annoying adding 10 tracker, manually or automatically, and having all announced because classified as tier 0

@ghost
Copy link

ghost commented Feb 6, 2020

Why not just show the new edit trackers box which allows you to group trackers into tiers easily.

@glassez
Copy link
Member

glassez commented Feb 6, 2020

Why not just show the new edit trackers box which allows you to group trackers into tiers easily.

New trackers can be added non interactively.

@ghost
Copy link

ghost commented Feb 6, 2020

Why not just show the new edit trackers box which allows you to group trackers into tiers easily.

New trackers can be added non interactively.

But what if I don't want to add a tracker to "next" tier non-interactively? There's already a edit tracker box where one can arrange trackers in tiers easily.

@glassez
Copy link
Member

glassez commented Feb 7, 2020

But what if I don't want to add a tracker to "next" tier non-interactively?

Typical solution for problems like this one should contain two levels:

  1. Default policy (either hardcoded or configurable) since qBittorrent allows non interactive actions.
  2. (optional) Ability to override default policy when you perform some interactive action or when you set up some automation.

IIRC, this issue is about Default policy for "tracker adding" action.

@Pentaphon
Copy link

Pentaphon commented Sep 24, 2020

@Piccirello Any progress on this? I just encountered this issue today. It would be great if trackers that are added manually would just be put into different tiers automatically. I think this is one of the reasons so many people complain that qBittorrent is "stalled for no reason."

@zero77
Copy link

zero77 commented Mar 17, 2021

I appreciate this warrants discussion to decide the best way forward.
But, is there any way something could be added even if it's manual like selecting announce to all trackers via the context menu.

@Piccirello
Copy link
Member Author

Abandoning this PR due to a lack of time. Anyone can feel free to take the existing code as a starting point.

@Piccirello Piccirello closed this Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants