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

BEP 52: please add DontHave message #79

Closed
jech opened this issue Feb 11, 2018 · 13 comments
Closed

BEP 52: please add DontHave message #79

jech opened this issue Feb 11, 2018 · 13 comments

Comments

@jech
Copy link
Contributor

jech commented Feb 11, 2018

The DontHave message is the opposite of a Have message: it allows a peer to renege on a piece that was previously announced using Bitmap or Have. It is defined by Arvid here:

https://www.libtorrent.org/extension_protocol.html

DontHave is trivial to implement: sending it is optional, and upon reception you just reset a bit in a bitmap somewhere. It's also essential for peers that discard pieces when they are short on space: if you have discarded a piece, there's nothing much you can do in the core protocol except ignoring requests for that piece (or sending Rejects if you're using the Fast state machine).

I would like to suggest that DontHave should be made a core request of the v2 protocol.

@the8472
Copy link
Contributor

the8472 commented Feb 11, 2018

You can simply drop connections if you wish to remove pieces from the set that you have advertised.

peers that discard pieces when they are short on space

Under which circumstances would this be so common that it would have to be part of the protocol?

@jech
Copy link
Contributor Author

jech commented Feb 11, 2018

You can simply drop connections if you wish to remove pieces from the set that you have advertised.

Which I'm not going to do, since dropping a connection slows down my download speed, while you having an inaccurate view of my pieces slows down your download speed.

Under which circumstances would this be so common that it would have to be part of the protocol?

It doesn't need to be common. It's trivial to implement, and it has significant benefits in the rare cases when it's needed.

@the8472
Copy link
Contributor

the8472 commented Feb 11, 2018

Idk, "might be useful under rare circumstances" doesn't sound like something that should go in the core protocol, it's something for extensions.

@jech
Copy link
Contributor Author

jech commented Feb 11, 2018

First, this is something that the current protocol just does not handle: there is no way for the protocol to renege on a previously announced piece short of dropping the connection.

Second, while it's rarely needed in current BitTorrent clients, it becomes very common as soon as you try to get outside the traditional file-based BitTorrent peer. It's required by streaming peers that use a finite-size LRU cache of pieces (my use case), it is required by caches (e.g. the "Volunteer" nodes in pull request #2), and probably other cases that I haven't though of.

Finally, it's something that's trivial to implement, and fits well with the rest of the protocol. Please make it part of the core protocol.

@atomashpolskiy
Copy link

While it seems simple on the surface, I can certainly see how it might be misused by malicious peers by sending large quantities of haves and dont-haves for the same pieces, flooding everyone and ruining download strategy for normal peers (when the swarm is small or the percentage of such misbehaving peers is big). And in comparison with other indicator messages like Int/NotInt or Ch/Unch it would be harder to notice such twitching, because it only becomes malignant when applied for a single piece and is perfectly fine otherwise (or at least requires some time-based heuristics).

@the8472
Copy link
Contributor

the8472 commented Feb 11, 2018

First, this is something that the current protocol just does not handle: there is no way for the protocol to renege on a previously announced piece short of dropping the connection.

Yes, at the moment this can simply be considered outside the scope of the core protocol. Of course that can be changed, but it requires good arguments why it needs to be part of the core instead of an extension.

Second, while it's rarely needed in current BitTorrent clients, it becomes very common as soon as you try to get outside the traditional file-based BitTorrent peer. It's required by streaming peers that use a finite-size LRU cache of pieces (my use case), it is required by caches (e.g. the "Volunteer" nodes in pull request #2), and probably other cases that I haven't though of.

Those use-cases themselves are interesting. But the volunteer approach should be fine with dropping connections. And an extension might be sufficient for streaming.

Finally, it's something that's trivial to implement, and fits well with the rest of the protocol. Please make it part of the core protocol.

I don't think it makes sense to piggyback a new, untested feature for novel uses on BEP52.
At the very least it requires more discussion and refinement.
For example arvid has been contemplating multi-haves for some time (#28), it would probably make sense to support some sort of batched or range-DONTHAVEs too. Especially if you consider the streaming case where small piece sizes may be used.

@the8472
Copy link
Contributor

the8472 commented Feb 11, 2018

Being able to remove pieces from the HAVE-set also makes things a lot more racy. Right now it is an error state to receive a request for a piece that was never advertised. And it also easy to plan out what to request in advance, once a piece has been advertised you can be sure it can be served in the near future as long as the connection remains open.

Being able to revoke HAVEs would invalidate both of those assumptions.

@jech
Copy link
Contributor Author

jech commented Feb 11, 2018

And it also easy to plan out what to request in advance, once a piece has been advertised you can be sure it can be served in the near future as long as the connection remains open.

I don't think that's the case. The peer might choke you, or it might reject the piece indefinitely (which is what I'm doing when the peer doesn't do lt_donthave).

@the8472
Copy link
Contributor

the8472 commented Feb 11, 2018

That's not quite the same. I mean when you select which piece to request right now it doesn't matter whether you do the calculation now and send the request in a minute (after an unchoke), you know it will still be valid. With donthaves you can end up sending invalid requests. It basically narrows your timing window to zero.

I'm not sure what the exact impact on implementations would be, but it seems iffy at least.

or it might reject the piece indefinitely (which is what I'm doing when the peer doesn't do lt_donthave)

That could lead to a degenerate state where a peer gets unchoked, all of its requests are rejected and it'll have to request again. The mutual hammering with messages should eventually lead to a disconnect.

So yeah, what you're doing is bad behavior. Just drop the connection.

@the8472
Copy link
Contributor

the8472 commented Feb 11, 2018

lt_donthave

Well, since it exists anyway it might make sense to turn it into a BEP as a starting point and document current implementation behavior if it is sane or add some recommended behavior where appropriate.

@jech
Copy link
Contributor Author

jech commented Feb 11, 2018 via email

@arvidn
Copy link
Contributor

arvidn commented Feb 17, 2018

@jech please feel free to document lt_donthave in a BEP. I wouldn't necessarily say it's trivial to implement though (that may sound like volunteering :) ). The implementation in libtorrent has only a single, simple positive test case for instance. So I wouldn't really consider that "done".

@jech
Copy link
Contributor Author

jech commented Jul 19, 2018

Please see #85

@jech jech closed this as completed Jul 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants