choke dishonest peer in anti-leech seeding algorithm #3833
Conversation
src/choker.cpp
Outdated
| { | ||
| // some clients (e.g. XunLei) always claim they have zero pieces as fresh starter, | ||
| // no matter how many we have uploaded. | ||
| return peer->num_have_pieces() == 0 && peer->uploaded_at_last_round() >= piece_length; |
There was a problem hiding this comment.
I think you'll need to add some factor to the piece length here. I can think of three considerations:
- The blocks we've uploaded may not have been part of the same piece, and the peer may still be stuck waiting for the last stragglers to complete those pieces.
- There will be some delay between the peer completing the piece and us receiving its "HAVE" message. It will have to check its hash, maybe write it to disk and send back the message over the wire. There will be at least one round-trip time of delay, but probably more.
- The peer may chose to batch HAVE messages it sends back, which will contribute to the latency we experience from uploading and receiving the HAVE notification.
Another consideration is that this heuristic is probably too simplistic. All it would take would be for xunlei (or whichever client is trying to cheat) would be to always report having 1 piece.
I think more accurate way of defeating this would be to remember how much we've uploaded to a peer and use the max of that and num_have_pieces() in this anti-leech choker.
I believe we already store the total amount of payload we've uploaded to a peer, and we preserve it between reconnects even.
There was a problem hiding this comment.
- Correct me if wrong, we prefer uploading blocks in the same piece, unless part of it has gotten from others. In that situation, peer would have at least 1 piece eventually.
- Yes, it may take several rounds to comfirm (by trip delay and batching messages). But they will get nearly hightest score in next round, I think it's worth if we choke those pure leechers always higher than them. Even there is no cheater, innocently choked for several round-time won't hurt too much.
Anyway, I'm OK with an extra factor. @aresch , how many would you think is appropriate?
- XunLei is the largest download company in China, which has it' own download network and cache servers. It won't share any, the high score by cheeting is merely a side-effect. I don't think it would likely change the cheating means.
If situation change, we can improvedishonest_peer()or even the algorithm in the future, for now it looks good enough to me. - @ssiloti , no algorithm is perfert, people choose this anti-leech choker because they want anti-leech, and this patch fixes a most obvious cheating loophole. For those believing in other unchoke algorithm, it won't hurt anyway.
Codecov Report
@@ Coverage Diff @@
## RC_1_2 #3833 +/- ##
==========================================
- Coverage 74.12% 73.95% -0.18%
==========================================
Files 439 439
Lines 59621 59625 +4
Branches 8861 8860 -1
==========================================
- Hits 44196 44093 -103
- Misses 10951 11095 +144
+ Partials 4474 4437 -37
Continue to review full report at Codecov.
|
|
I don't think there's much value in targeting these ham-fisted attempts at making a pure leach client. If a mitigation like this PR ever became widespread, the offending clients would promptly switch to an alternative method. The obvious solution would be to simply never unchoke any peers (nor allow fast sets). The never unchoke method cannot be detected by individual clients, so we'd be right back were we started. |
|
@holymonson What good does closing a loophole do when there are similarly obvious loopholes which achieve the same result and are much harder to detect? This PR is putting a padlock on a door which has another unlocked door right next to it. There are also downsides to implementing this:
|
|
@ssiloti If you don't like the anti-leech unchoking method, you can file a PR to remove it. As long as it exists, I think we should fix it, rather than worry about complexity of libtorrent, which is a different-scope topic. Let's be pragmatic and discuss within the implement of this PR. As I said, the cheating method would unlikely change, no need to worry now. Even it does, the worse case is the patch becoming useless and we back to origin, then we drop it. |
|
a simpler and more robust approach is to compute the max of pieces the peer has and the number of pieces we've uploaded in total to the peer. The upload metric should be available for peers already. |
|
@aresch , do you mean this? bool dishonest_peer(peer_connection const* peer, int piece_length)
{
int const on_trip = 3;
return (peer->num_have_pieces() + on_trip) * piece_length < peer->uploaded_at_last_round();
}But if peer loses too many data on transmission exceeding the |
|
No. I mean drop the concept of "dishonest peer" and just make sure that the current logic, instead of just looking at the number of pieces it has advertised to have, take the max of that and the number of pieces uploaded to the peer. |
|
you can find the total uploaded by: |
|
Backport to 1.1.13? |
|
@arvidn is this patch going to be backdated to 1.1.13 or a 1.1.14 release? |
|
I'm not planning on doing so, but patches are welcome |
|
@arvidn it seems this patch is not fully effective.
I am using anti-leech in qbittorrent 4.2.0RC
|
|
@xavier2k6 can you explain what that screenshot is showing? and what you would expect it to say instead? (and ideally why you would expect it to differ) |
|
It is showing uploading(never downloading) to -XL0012- clients which remain at 0.00% & the basis of this PR was to remove the workaround rather than banning the client. I don't know if there has been a workaround for your workaround found?! or if the expected behaviour is to upload for a certain period of time & then cut them off or whether that client should be cut-off from instantaneously. |
|
The expected behavior is to always upload as long as there are peers interested in downloading. That's always the top-level rule. Unused bandwidth is wasted bandwidth. Is there a reasonable rationale for banning a client that's staying at %0? (i.e. not sending redundant HAVE messages). I don't think I understand what the actual problem is. You're making some assumption about it being a bad idea to stay connected to some peers, but I'm not clear on why you think that's a bad idea. Every peer starts at 0%. If everyone reject such peers, the whole network breaks down. The anti-leech choker will prefer to upload to peers that appear to behave well. Not sending redundant HAVE-messages isn't necessarily bad behavior/ libtorrent can be configured not to do that (since there typically isn't a need to send such messages). This patch makes the anti-leech choker work for clients that don't send redundant HAVE messages by also taking into account how many blocks have been uploaded to that peer. For example, if you have more unchoke slots than peers, the unchoke-algorithm will never come into play, everyone will always be unchoked. |
This was the original reason from OP & although I do believe this has worked in the past - have seen uploading recently to this client with it status of 0.00% never changing! & they NEVER upload/share - hence why i thought your work around may have been circumvented somehow... |
I feel pretty confident that this has in fact been fixed by this patch. I have not seen any evidence suggesting that it has not. Again, the symptom of this problem is that there are other peers, that are better upload candidates that are choked, in favor of the peers that "stay at 0%". The fact that peers with 0% are being uploaded to is not evidence of a problem. The trait of "staying at 0%" is not in itself a problem, nor a signal indicating bad behavior. This used to be the default in libtorrent, to not send HAVE messages to peers that already had that piece, as it would be a waste of bandwidth. The trait of never uploading anything is also not necessarily a sign of bad behavior, it could just be that the peer doesn't have any piece that you're interested in, which is likely to be the case for any peer that just joined the swarm. If you have any reason to believe that there is still a problem, after applying the patch of this PR, please be precise in what symptom you experience and what you would expect instead and why. |
|
qbittorrent/qBittorrent#10258 (comment) another user has confirmed what I've been saying, the workaround/patch was to stop -XL0012- clients from being complete leeches but doesn't seem to be fully working. e.g. you could be sharing a 1GiB file & upload 100% of that file to those clients & they will remain at 0.00% & will never share data/bandwidth. |
Which is how bittorrent is supposed to work. If you're a seed, nobody will upload to you but everyone (you stay connected to) will download from you. This observation is not (by itself) indicative of a problem. The fact that they stay at 0% is also not a problem or an indication of misbehavior (as I've explained in previous posts). If your aim is to disconnect/ban certain peers, a much more effective tool is to use the IP-filter. But if you start banning peers based on a vague idea of "misbehaving" or being "complete leeches", chances are pretty good you're have a lot of false positives, banning legitimate clients, crippling the network. When you unchoke and upload to these peers, do you also have other peers that are interested and choked that you have uploaded less to? That would be an indication of a problem. The anti-leech choker attempts to evenly distribute upload across peers, and also to penalize peers that are close to 50% complete (unrelated to whether reporting it or not). |
|
The only issue I have is with the -XL0012- clients & them being a "dishonest peer" - all other peers share normally - I can download from them & vice-versa upload to them. with the -XL0012- clients, these are leeches - you can upload to them all the damn day long & not seem to get 1kb back. As I said previously this seemed to work ....they would only get a few kb's & nothing more but now they seem to be able to do what they were able to do which was fully leech before this patch was implemented.
I'll just have to resort to banning this client, I'd say. Thanks. |
Or you can search for "qBittorrent Enhanced Edition" which bans those clients by default. |
|
I couldn't find any notes on specifically what was different with qBT EE but the installer provides notes.
|
I missed this. You're saying that this patch caused a regression? |
No, I don't think this patch caused a regression - it worked, it's just that I believe the Xunlei -XL0012- clients have found another loophole/workaround as @ssiloti had mentioned above that they probably would. @Chocobo1 Thanks for the suggestion - I had come across qBittorrent Enhanced Edition long time ago but would rather stay for now with trusted than 3rd party. (I will test it out some time on a spare machine) |
|
So, it means we will share few data to those dishonest peers unless we have more unused bandwidth? If so, this may be a wiser way. We just don't want to see those dishonests peer are downloading happily, while others are suffering extreme low speed. |
|
Right, that's how the choker works, unless you've configured to always unchoke everyone (but that's not the default setting). I think it's important to have a good understanding of what we mean by "dishonest". So far I have not seen a compelling definition. I don't think clients that don't send redundant HAVE messages should be penalized. Does anyone have any other properties of a dishonest peer? Is there evidence that they manipulate the clock they use in the uTP transport to get a larger share of upload capacity? This is something uTorrent has some protection against, but not libtorrent. |
Backport of #3833 to support peers not sending redundant HAVE messages in anti-leech seeding algorithm. Merged backport of following two commits; 186371f "choke dishonest peer in anti-leech seeding algorithm (#3833)" 729102c "fixed division by zero in anti-leech choker" V3: Workaround MSVC's absence of std::abs (int64_t) before C++11
|
I don't know if this patch is (or was) effective at fixing the issue, as I rarely see the XunLei client (and when I do it's as a seeder) enough to test, but I can answer this at least:
The fear the other users are trying to express is that, as XunLei clients show 0% download, it also must mean they're not contributing to the swarm in any way as a seeder. Akin to a drain, they're worried that XunLei users will occupy all their upload bandwidth and then leave the swarm, acting as if Users were a simple filehosting service. If enough XunLei clients connected to a swarm and acted in this manner, the fear is that this could kill the swarm (whether by leading new peers to believe the torrent is dead as XunLei clients receive the pieces over them, by having the last few seeders believe they've "given enough" ratio back and stop seeding, or by some similar circumstance), and even if it did not kill the swarm it's seen as unfair that these XunLei clients are not contributing in any way, forcing Users to contribute more for longer. That being said, I'm not familiar enough with the protocol nor with the XunLei client to know if that's a valid fear. For the former, is it possible for a client to upload pieces while simultaneously showing as 0%? For the latter, it's possible the XunLei client does allow seeding & that their users do indeed contribute, and this behaviour simply isn't visible as a seeder ourselves. That would need to be tested. If they intentionally aren't seeding/helping the swarm, then I'd certainly consider that to be "dishonest", but I don't see any other way to mitigate the issue beyond a) the |
|
We might be seeing some more dishonest behavior in another kind of client, as reported in anacrolix/torrent#891. The big concerning things are:
For the third point, a simple std::int64_t const total_size = t->torrent_file().total_size();
std::int64_t const have_size = std::max(std::min(peer->statistics().total_payload_upload(), total_size)
, std::int64_t(t->torrent_file().piece_length()) * peer->num_have_pieces()});
return int(std::abs((have_size - total_size / 2) * 2000 / total_size));Should I make that a PR? Appendix. Comparison of max-of-three to doing two separate scores We could do the following explicitly: std::int64_t const given_size = std::min(peer->statistics().total_payload_upload(), total_size);
std::int64_t const have_size = std::int64_t(t->torrent_file().piece_length()) * peer->num_have_pieces();
int const given_score = std::abs((given_size - total_size / 2) * 2000 / total_size);
int const have_score = std::abs((have_size - total_size / 2) * 2000 / total_size);
// Use the more conservative score if we suspect the peer of being dishonest
// N.B. Just using given_score in the first part does not guarantee using the more conservative one: given can be ramping up after having crossed the 50% point.
// N.B. A perfectly honest peer would also hit the check if we are the sole uploader. This is fine, because the difference between the two scores should be very small.
return (have_size < given_size) ? std::min(given_score, have_score) : have_score;However, this does not seem to provide any benefit at all.
|
As discussed in qbittorrent/qBittorrent#10258, some clients (e.g. XunLei) always claim they have zero pieces, even we have uploaded a lot to them. They take most advantage in the anti-leech seeding algorithm but won't share any pieces. Choke them.