Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#3060 closed Enhancement (fixed)

Local Peer Discovery protocol implementation

Reported by: Eszet Owned by: Eszet
Priority: Normal Milestone: 2.00
Component: libtransmission Version: 1.92
Severity: Normal Keywords:
Cc:

Description

Description:

Local Peer Discovery allows to detect nearby peers on the local network for rapid data exchange. In order to provide such a discovery mechanism in a zero-conf fashion, HTTP-style datagrams are broadcasted and received as part of the LPD client's background activity. Upon discovery, a compatible client shall add suitable peers to its torrent pool and initiate peer-to-peer communications.

References

Similar features have been requested in the past (see Forum discussion, #1394, #2056).

Compatible implementations known to exist are: uTorrent, libtorrent.

Change History (69)

comment:1 Changed 7 years ago by Eszet

I have a Local Peer Discovery patch cooking, which I'd like to contribute to Transmission. With the help of the discussion in the forum (see ticket), it was not too difficult to come up with a clean and straightforward implementation of the protocol.

It is now possible for uTorrent and Transmission clients to discover local peers (at least that's the configuration I could verify).

For distributing large files among several host on the local net, this is quite a nice option to have. And with Transmission's new feature, it doesn't matter for a local machine if it's a Linux or a Windows box.

The proposed patch is in sync with trunk.

Changed 7 years ago by Eszet

comment:2 Changed 7 years ago by charles

Very nice implementation roughly parallel to libtransmission/tr-dht.c. Thank you for the patch!

Are any of these new functions blocking s.t. they might want to run in their own thread, or are they safe for using in the libtransmission thread?

comment:3 Changed 7 years ago by charles

Eszet: ping

comment:4 Changed 7 years ago by Eszet

My pleasure. Thanks! I revised the blocking issue and since the peer discovery protocol is very lightweight, it would seem to be an overshoot to grant this activity its own thread (the rate does not exceed one datagram every few minutes; independent from the number of torrents).
However, actually sending and receiving the messages may be a blocking operation, so I should add some code to make the socket non-blocking (or specify a timeout). Is it your assessment also that this behavior is safe enough regarding interference with the rest of the code?

Do you think this feature might make it into a release in the near future? If so, maybe I should also add the necessary boilerplate code allowing to configure local peer discovery via the settings file to the next patch.

comment:5 follow-up: Changed 7 years ago by charles

Yes, I think it's very likely this patch would make it into release.

wrt blocking and threads, my preference would be to use nonblocking sockets or very low-timeout sockets and to keep the work in the libtransmission thread. Unless I'm misunderstanding the patch, adding an extra thread for peer discovery feels like overkill.

About the GUI boilerplate code -- less is always better in Transmission. Remember how many GUIs has to be adjusted for each feature that gets added, so weigh that effort against how many people you think will use each feature. If in doubt, add the option to settings.json and wait for people to start requesting it. :)

Last edited 7 years ago by charles (previous) (diff)

comment:6 in reply to: ↑ 5 Changed 7 years ago by Eszet

Replying to charles:

OK, then maybe turn it into a compile-time feature? ... if anybody still wanted to go without LDS built in, which I honestly doubt. ;)

wrt blocking and threads, my preference would be to use nonblocking sockets or very low-timeout sockets and to keep the work in the libtransmission thread. Unless I'm misunderstanding the patch, adding an extra thread for peer discovery feels like overkill.

I'm just preparing the patch where I've added non-blocking socket behavior. LDS is still a modest protocol from the sender's point of view; however, any socket listening to UDP packets on the local net might be vulnerable to heavy traffic in the multicast group (either malicious or we really have such a crowded multicast group). My implementation still receives all packets but begins to discard announces once a due limit of messages per interval is reached. LDS is a background service after all and only the latency of a successful discovery might increase during overload due to this safety measure.

Yes, I think it's very likely this patch would make it into release.

Great! At any rate, I am certainly willing to continue supporting the patch in the future.

I will post the update here shortly, after I did another regression run with the code. Looks very promising so far.

comment:7 Changed 7 years ago by User294

As for me I may want to have a local peer discovery. I'm using ISPs which are metropolitan area networks based on Ethernet technology. And local area within a network segment is allowed to use full 100Mbps link speed while all other data transfers are limited to a subscription rate (usually some 2 to 30Mbps). So if local peer found, data could be fetched several times faster and less external traffic would be generated. This is something what is welcomed by ISPs as it relaxes load on their channels. However, ISPs seems to discourage frequent broadcasts and care should be taken to limit broadcast activity to quite low levels. Otherwise it can toggle broadcast protection.

comment:8 Changed 7 years ago by Eszet

That's an interesting use case. The current implementation emits only a single datagram (119 bytes) every 4 minutes even if you have several downloads running, so this should be below any imposed limit I guess. However, the total number of multicast messages floating around might be significantly higher as the rate scales with the number of local peers participating. But that is not what you generate. I don't suppose you would get accounted for your inbound multicast traffic by the ISP?

Last edited 7 years ago by Eszet (previous) (diff)

comment:9 Changed 7 years ago by Eszet

Improved revision of the LDS patch

Note: I recommend not using the first patch in favor of the revised code as I stumbled upon a programming error in the original event handler code!

Further improvements are:

  • Non-blocking networking code
  • Taken measure to limit impact of message flooding
  • Revised the code with the help of some splint suggestions
  • Additional documentation (Doxygen)

Changed 7 years ago by Eszet

comment:10 Changed 7 years ago by darklord

I just tried compiling and using this patch in Mac OS X and I always get a "Can't assign requested address" (EADDRNOTAVAIL) when LDS is trying to send the announce in tr_ldsAnnounce. Receiving announces form other machines works though.

Does anybody have any insight on why this is happening?

comment:11 Changed 7 years ago by Eszet

That's odd. As far as I can tell such an error code should not be returned by sento() (tr_ldsAnnounce) but rather by bind() during initialization. Secondly, the announce code by design silently discards failure values, so I'm a bit surprised you observe this behavior.

The used address is special in the respect that it is a multicast address. Perhaps something is actively preventing this kind of socket operation from functioning? (I suppose MacOS's network interfaces are somewhat 'firewalled' by default?)

Could you post an output log of this error (at message level 3 in your settings.json)?

Thank you for the report!

comment:12 Changed 7 years ago by darklord

These errors weren't actually logged, I was trying to find out why the no announces were sent in OSX and i did some debugging and discovered that "res" was -1 so I added some error handling code and that gave me that error message.

I already did some research and the problem might be that you have to set the multicast interface on mac but i am not sure about this.

Last edited 7 years ago by darklord (previous) (diff)

comment:13 Changed 7 years ago by Eszet

OK, I see, that might be it. I read that too when I studied the API for multicast sockets. Seems I stepped into that pitfall as the implementation worked for Linux right out of the box.

In order to emulate the implicit behavior (by binding to 0.0.0.0) one just had to add all available interfaces via setsockopt() [disregarding for the moment the effects it might have on arbitrary platforms and network topologies to emit announces on all interfaces].

comment:14 Changed 7 years ago by darklord

Ok I think I am making some progress, I managed to get Transmission to *sometimes* send *one* announce if i remove the binding of the socket in tr_ldsInit (i discovered that by randomly testing). I think i might be able to fix this bug but it will require some more testing.

EDIT:

ok it seems Transmission is actually sending the announces at the normal interval :-)

the only problem now is that because i removed bind Transmission does not receive any announces. But if I add bind it once again gives me the "Can't assign to requested address" error

Last edited 7 years ago by darklord (previous) (diff)

comment:15 Changed 7 years ago by Eszet

Did you already try to specify your outgoing interface for IP_ADD_MEMBERSHIP in tr_ldsInit() (instead of the default INADDR_ANY)? My guess would be that this might fix send while receiving announces would continue to work.

Some additional documentation: http://tldp.org/HOWTO/Multicast-HOWTO-6.html

Edit:

Interestingly enough, the Mac documentation http://developer.apple.com/mac/library/documentation/Darwin/Reference/ManPages/man4/ip.4.html for the multicast API appear to be very similar to the Linux documentation. Strange stuff.

So far, according to your observations, for sendto() we would need another socket with an identical setup but without binding it. Maybe this special handling also proves to work under Linux.

Last edited 7 years ago by Eszet (previous) (diff)

comment:16 Changed 7 years ago by darklord

Yes I did and it didn't work..., but I will try it again.

btw: what address does imr_interface.s_addr expect?

I just did: mcastReq.imr_interface.s_addr = inet_addr("192.168.1.39"); 192.168.1.39 is my IP address

is that even correct??

Last edited 7 years ago by darklord (previous) (diff)

comment:17 follow-up: Changed 7 years ago by Eszet

Btw: let me say thanks for your help on Mac OS. It's a platform I cannot test here.

Seems correct to me. However, some code examples on the web suggest that for sending one only needs an open socket, which is also supported by documentation I found from HP.

Quote:

It is not necessary for the system to join a multicast group in order to send multicast datagrams.


Maybe we should just try that: having another

static int lds_socket2;

comment:18 Changed 7 years ago by darklord

I was thinking about doing precisely that :-)

comment:19 in reply to: ↑ 17 Changed 7 years ago by darklord

It seems i have discovered another bug, sometimes when I add a torrent Transmission crashes with:

Assertion failed: (tr_isTorrent( tor )), function tr_torrentGetActivity, file /*/transmission/libtransmission/torrent.c, line 899.

I think this crash originates from around those lines (starts at line 576) in tr_lds.c:

if( tor == NULL )
    tor = tr_torrentNext( session, tor );

/* only consider torrents which are currently downloading */
while( tor != NULL && tr_torrentGetActivity( tor ) != TR_STATUS_DOWNLOAD)
    tor = tr_torrentNext( session, tor );

I only have one torrent added to transmission so I think it is possible that

tor = tr_torrentNext( session, tor );"

returns NULL which leads

 tr_torrentGetActivity( tor )

to crash

but i have never before worked with these functions so I an not sure about this.

comment:20 Changed 7 years ago by Eszet

I've just had a look into tr_torrentNext( session, tor ). It's able to cleanly handle tor == NULL. The assertion originates from tr_torrentGetActivity, which I guarded with the first while condition of (tor != NULL). I admit this code is to be used with a grain of salt, but I don't see an error right now and I am also not aware of stumbling upon any such assertion on my machine... but I will give it some additional thought.

In the meantime I noticed (contrary to my initial understanding of the protocol) that uTorrent also announces torrents it seeds, so maybe this code should be revised anyway. In my earliest testing the whole code just read

tor = tr_torrentNext( session, tor );

but this announces torrents in any state which might not be what we want either.

EDIT: One part of the problem is that a non-NULL pointer does not necessarily indicate a valid torrent struct, so I changed the former pointer comparisons to tr_isTorrent (which also checks for a cookie value) where appropriate. Hence, the guard I referred to earlier was ineffective in some situations.

Last edited 7 years ago by Eszet (previous) (diff)

comment:21 Changed 7 years ago by darklord

Ok, I fixed that failed assertion by adding

if( tr_sessionGetActiveTorrentCount(session) == 0)
	return;

after

case EV_TIMEOUT:

also for announcing seeding torrents I changed line 585 to

while( tor != NULL && tr_torrentGetActivity( tor ) != TR_STATUS_DOWNLOAD && tr_torrentGetActivity( tor ) != TR_STATUS_SEED)

(My line numbers might me a bit off because I changed some stuff in my local files)

comment:22 Changed 7 years ago by Eszet

Darklord, could you try my patch for separate send and receive sockets? Transmission under Linux still works with this change but I wonder if Mac OS is OK with the implementation as well.

Changed 7 years ago by Eszet

comment:23 Changed 7 years ago by darklord

As far as i can tell sending and receiving multicast messages now works.

BUT there is one last problem, LDS "learns" the peer now but for some reason no connection is established between my two transmission clients??

I was actually able to fix the multicast problem myself and i have also observed this strange behavior with my implementation.

Last edited 7 years ago by darklord (previous) (diff)

comment:24 Changed 7 years ago by Eszet

Interesting... I did another verification supporting that the newest LDS patch works between two Linux Transmission clients. However, I think there is still a bug because now deactivating IP_MULTICAST_LOOP does not prevent a client from receiving its own announces. I should add some code to discard these events in the event handler (I am not sure if tr_peerMgrAddPex performs such a check). Maybe it's somehow more of a problem under Mac OS?

Another small but notable difference is that with the new code datagrams are sent from an arbitrary source port. uTorrent on the other hand always uses source and destination ports 6771 for its announce messages (I tried to emulate this behavior as exactly as possible with my initial implementation). Nevertheless, it doesn't seem to be of much significance -- but it indicates that the uTorrent Windows implementation also uses a single socket for its multicast code.

Last edited 7 years ago by Eszet (previous) (diff)

comment:25 Changed 7 years ago by darklord

I don't think it is only a problem under Mac the problem occurred once in Linux too, I am currently working on replicating that issue.

But there is another annoying issue that keeps me from being able to replicate that bug. Sometimes after restarting Transmission or after a torrent has finished LDS just stops sending announces, until i re-add the torrent to transmission.

Has this ever happened in your tests?

comment:26 Changed 7 years ago by Eszet

Darklord, I was able to investigate all issues you've reported so far. Please find my latest patch with a revised announce and event handling logic attached (see commit log); from my testing I'm under the impression that the code is now quite mature and runs smoothly so far.

If you're not too busy, perhaps you could give it a try and verify that for Mac, too?

Changed 7 years ago by Eszet

comment:27 follow-up: Changed 7 years ago by darklord

As far as I can tell everything seems to be working now :-)

Well there is one thing that still doesn't work but that seems to be a transmission issue...

If i seed a file from my linux box to my mac, then remove the data file from my linux box and reverify the data or restart transmission so it starts leeching, transmission on my linux box does not start downloading although it receives the LDS announce.

comment:28 Changed 7 years ago by Eszet

Great! :)

Yes, I noticed something like that as well. I think it has to do with the inner workings of Transmission I do not yet fully comprehend. From the user's perspective it feels as if somewhere is a flag that marks the peer "uninteresting" after a leech completed (there is a chance it has to do with the "shelf time", a routine I did not adapt for TR_PEER_FROM_LDS).

I've also thought about adding code that actively attempts a connection (you can see a remnant of that in a comment of the R3 patch) but AFAICT transmission has adequate mechanisms to assess the appeal of a known peer (until now I didn't want to tamper much with code outside LDS).

I imagine Charles knows the code best for determining which knobs to adjust here to make Transmission behave correctly.

It is still an option to add a code branch for "active replies" to LDS announces, but I wonder what would be the right criterion to deviate from the normal tr_peerMgrAddPex() call in such a case.


@Charles: Ping?

Last edited 7 years ago by Eszet (previous) (diff)

comment:29 Changed 7 years ago by jch

I rather like this code, but there's one thing I don't understand... why is this protocol over broadcast rather than multicast?

comment:30 follow-up: Changed 7 years ago by jch

Sorry, the protocol uses multicast, as expected. The setsockopt(SO_BROADCAST) is only there to confuse the reader (please remove it).

Some further issues.

There are too many comments, this makes your code difficult to read (do you work for IBM?);

The functions ldsAnnounce and ldsConsiderAnnounce have confusing names -- I'd suggest ldsSendMessage and ldsReceiveMessage.

I don't like the way you schedule announces (by periodically scanning over the list of torrents), I'd suggest looking at the way I do it for DHT announces and copying that (look at the very end of the function announceMore in announcer.c). Charles, opinions?

The current shelf time of an LDS atom (60 minutes), seems too large, while the unsolicited announce interval (4 minutes) seems too short;

Your nick makes me nervous (reminds me of my German classes in high school).

--Juliusz

comment:31 in reply to: ↑ 30 Changed 7 years ago by Eszet

Juliusz,

Replying to jch:

Sorry, the protocol uses multicast, as expected. The setsockopt(SO_BROADCAST) is only there to confuse the reader (please remove it).

Thanks for the hint. I was misled by several multicast examples on the net that had SO_BROADCAST also enabled. So I assumed anything other than UDP datagrams directed at a single recipient required the flag. The man page does not explicitly state its not required for multicast, so I did not think much about it. I am about to remove it.

There are too many comments, this makes your code difficult to read (do you work for IBM?);

You mean the Doxygen comments? These are purely optional, I added them to get a nice documentation out of tr-lds.c. If they clutter the code rather than help, I will remove them from the patch, no prob.

What makes you believe IBM code is that well commented? ;)

The functions ldsAnnounce and ldsConsiderAnnounce have confusing names -- I'd suggest ldsSendMessage and ldsReceiveMessage.

Done.

I don't like the way you schedule announces (by periodically scanning over the list of torrents), I'd suggest looking at the way I do it for DHT announces and copying that (look at the very end of the function announceMore in announcer.c). Charles, opinions?

If I understand correctly, you mean there should be more variance between announces (i.e., an algorithm which is not that static)? Or am I not seeing the obvious in the code you referred to? My implementation is designed to be very close to the uTorrent behavior (for compatibility considerations). As trivial as it seems, uTorrent uses exactly the same algorithm with virtually no variance in its interval. It just cycles through its active list every 5 minutes. Since the goal is to use Transmission/LDS for rapid data exchange between peers on the local net, I chose a value of 4 minutes so there is not too great a latency if you just want to have a transfer started. I guess it's a question of how that default value proves itself in practice. LDS's announces are not "evil" broadcasts after all.

The current shelf time of an LDS atom (60 minutes), seems too large, while the unsolicited announce interval (4 minutes) seems too short;

I'm not certain about the general definition of shelf time. I assume it's the time a peer is considered, unless something else disrupts?

Your nick makes me nervous (reminds me of my German classes in high school).

It's also some kind of German chocolate :)

Johannes

Last edited 7 years ago by Eszet (previous) (diff)

comment:32 follow-up: Changed 7 years ago by jch

If [the comments] clutter the code rather than help, I will remove them from the patch, no prob.

I believe so, but Charles might disagree.

What makes you believe IBM code is that well commented? ;)

It's not well commented, it's commented verbosely. IBM programmers used to be paid by the line.

I don't like the way you schedule announces

you mean there should be more variance between announces

Yes, there's certainly the lack of jitter, but there's more. If you look at the code I pointed to, the torrents announce themselves to the DHT, rather than having the DHT code periodically loop over all torrents one by one. I personally find this structure more pleasant (which is why I sand "I don't like" rather than "please change it"), and I'd like you to consider which way you prefer.

Your nick makes me nervous (reminds me of my German classes in high school).

I'm not certain about the general definition of shelf time.

It's roughly the time for which an atom will remain if it's not refreshed. For example, we consult the DHT every 27 minutes, so DHT atoms remain for 30 minutes -- if they haven't been refreshed after 30 minutes, they're probably obsolete. (An atom is the data structure that keeps information about a peer that we're not currently connected to.)

It's also some kind of German chocolate :)

It still makes me nervous. (Pfeifen, er pfeift, pfiff, hat gepfiffen, I had to memorise page after page of this stuff.)

--Juliusz

comment:33 in reply to: ↑ 32 Changed 7 years ago by charles

Replying to jch:

If [the comments] clutter the code rather than help, I will remove them from the patch, no prob.

I believe so, but Charles might disagree.

My vote would be for leaving them in. Some of them are unnecessary, but they help more than hurt :)

comment:34 Changed 7 years ago by charles

  • Milestone changed from None Set to 2.00

comment:35 Changed 7 years ago by charles

Eszet, jch, is this patch ready to be checked into the repo?

comment:36 Changed 7 years ago by howl

Remember to disable this feature for torrents with private flag to 1.

comment:37 in reply to: ↑ 27 Changed 7 years ago by Eszet

Charles, I will post the patch here right away after I've merged the pending (minor) remarks by jch/howl. Apart from that the code is stable.

However, there is still the issue reported by darklord.

Quote darklord:

Well there is one thing that still doesn't work but that seems to be a transmission issue...

If i seed a file from my linux box to my mac, then remove the data file from my linux box and reverify the data or restart transmission so it starts leeching, transmission on my linux box does not start downloading although it receives the LDS announce.

I couldn't locate the reason for that behavior yet. Maybe some clue comes to mind on your side?

comment:38 Changed 7 years ago by charles

jch, Eszet, darklord: on an unrelated note, I'd like to get some opinions on http://trac.transmissionbt.com/ticket/2313 from people better at networking than me. If you have a few minutes free, could you go take a look and weigh in on its merits / pitfalls?

comment:39 follow-up: Changed 7 years ago by jch

Eszet, jch, is this patch ready to be checked into the repo?

I suggest committing it as soon as it meets the following two conditions.

The extension needs to obey a torrent's "private" flag. ß, please check the function tr_torrentIsPrivate.

There needs to be a manual toggle to enable/disable the feature globally, and it *MUST* default to disabled.

Please let me justify this last requirement. The local peer discovery code broadcasts the set of torrents you're participating in every four minutes. Anyone on the same link as you (your neighbour, your mom, your wee brother) can find out the set of torrents just by listening on the right multicast group, and these are the very people you might not want to know about what your are downloading.

Hence, I insist on this being *DISABLED* by default.

--Juliusz

comment:40 in reply to: ↑ 39 Changed 7 years ago by Eszet

I had a similar discussion with Charles earlier in this thread, but I certainly see your point. I've changed the implementation to have a torrent "elapse" before a new announce is sent. Turns out that you were right, the code becomes much cleaner. I like it.

Please see the attached patch:

  • introduced LDS enable/disable setting (-z/-Z CLI switch for daemon)
  • LDS announce mechanism now analogous to DHT code
  • closes previous (outstanding) requests for this ticket (function names, honor private flag)

Sorry for the delay btw, have been a bit busy the last week; however, I suppose code quality rather improves having ample time to think through implementation alternatives.

comment:41 Changed 7 years ago by Eszet

  • Owner changed from charles to Eszet
  • Status changed from new to assigned

Changed 7 years ago by Eszet

comment:42 follow-ups: Changed 7 years ago by jch

It certainly looks good to me. Charles, you should feel free to either commit this as it is so we can work in the tree, or wait for a new version; notwithstanding the comments below, I certainly have no objection to it being committed as it is.

The shelf value of LDS atoms is still wrong.

TR_PEER_FROM_LDS should be a much smaller value, probably 1 or 2 -- LDS peers are highly desirable.

I still don't agree with your way of performing LDS announces -- I believe that LDS announces should happen in announceMore, just like normal and DHT announces, rather than using a dedicated timer to do them (event_callback in tr-lds.c). Charles, opinions?

Please remove the changelog from tr-lds.c -- it will soon be out of date. (I also find the elaborate comments in tr-lds.c distracting, but Charles disagreed with me above, so I'll shut up.)

Please remove the option TR_LDS_FORCE_UDPSRCPORT6771 -- pick one of the two strategies, and don't leave the option to have either. If you choose to have just the one socket, use a single variable to avoid confusion. (My personal favourite is to use just one socket, unless there are any reasons I don't see to use both.)

By the way, the one-socket version is buggy -- you close the socket twice.

On a related note, please remove TR_LDS_IGNCONSISTENCY -- just make the consistency checks unconditionally.

Please add some jitter to your announces.

Your nickname still makes me nervous. Additionally, while reviewing your patch I burnt my dinner.

--Juliusz

comment:43 in reply to: ↑ 42 Changed 7 years ago by Eszet

Replying to jch:

I still don't agree with your way of performing LDS announces -- I believe that LDS announces should happen in announceMore, just like normal and DHT announces, rather than using a dedicated timer to do them (event_callback in tr-lds.c). Charles, opinions?

Are you certain about mixing LDS announces with the rest of the code in announcer.c?

Please remove the option TR_LDS_FORCE_UDPSRCPORT6771 -- pick one of the two strategies, and don't leave the option to have either. If you choose to have just the one socket, use a single variable to avoid confusion. (My personal favourite is to use just one socket, unless there are any reasons I don't see to use both.)

One socket would also be my preferred solution, however, as reported earlier in this thread for Mac OS the single socket solution does not work any longer (possibly some platform peculiarity). This is why I came up with the two socket code, which however "breaks" with the uTorrent behavior of having UDP src port = dest port = 6771.
As I stated earlier, it was my intention to deviate as little as possible from the uTorrent behavior. So I guess it comes down to deciding whether to drop single socket completely or to have platform-specific behaviors, which - admittedly - is kind of ugly. In this respect, I prefer the two socket solution as it seems to work well on all platforms so far.
Having said that, I was not ready to make that decision yet, so I left the compile-time option which can easily be mapped to autoconf symbol, if so desired.

On a related note, please remove TR_LDS_IGNCONSISTENCY -- just make the consistency checks unconditionally.

Please add some jitter to your announces.

OK.

comment:44 in reply to: ↑ 42 Changed 7 years ago by Eszet

Replying to jch:

It certainly looks good to me. Charles, you should feel free to either commit this as it is so we can work in the tree, or wait for a new version; notwithstanding the comments below, I certainly have no objection to it being committed as it is.

The shelf value of LDS atoms is still wrong.

Fixed.
(I hope I did it the right way)

TR_PEER_FROM_LDS should be a much smaller value, probably 1 or 2 -- LDS peers are highly desirable.

I needed to adjust the enum sequence since the dependent code does not tolerate duplicate enum values. TR_PEER_FROM_LDS now equals 2, before TR_PEER_FROM_DHT. I inferred that ordering from your comment.

I still don't agree with your way of performing LDS announces -- I believe that LDS announces should happen in announceMore, just like normal and DHT announces, rather than using a dedicated timer to do them (event_callback in tr-lds.c). Charles, opinions?

I added the necessary code so the LDS announce mechanism gets its own reschedule interval (/= LDS announce interval) because IMHO there should be some minimum time between LDS announces going out. As a consequence, DHT now depends on LDS (however, both are still strictly separated).

Changed 7 years ago by Eszet

comment:45 follow-up: Changed 7 years ago by charles

jch: this can go in as soon as you and Eszet are both happy with everything.

Well, happy with the patch at least. It's not Eszet's fault that you can't cook dinners well.

comment:46 in reply to: ↑ 45 Changed 7 years ago by Eszet

Replying to charles:

So far, I see no showstopper for applying this patch to trunk.

R6 introduces the following:

  • removed stray lds_houseKeepingInterval in tr-lds.c (since R5 configurable in announcer.c)
  • visibility of tr_torrentGetActivity changed in trunk, fixed since required by tr-lds.c
  • renamed tr_ldsReceiveAnnounce, IMO tr_ldsConsiderAnnounce just better reflects what the function actually does
  • added infrastructure for setting multicast TTL (default TTL is still 1); everything is in place to easily change this to another value


jch: this can go in as soon as you and Eszet are both happy with everything.

You are not picking on our private little nitpicking here, are you Charles? ;-)

Changed 7 years ago by Eszet

comment:47 Changed 7 years ago by jch

So far, I see no showstopper for applying this patch to trunk.

Charles, I agree. Unless you're planning to release 2.0 soon (as in tomorrow), I suggest you apply ß's patch and let us work out any issues in the Transmission tree itself.

TR_PEER_FROM_LDS now equals 2

That should be either 1 or 3 -- trackers and DHT should have similar priority. (Personal opinion: 1, as local peers should be more desirable than remote peers.)

On a related note -- I don't see the shelf life having changed.

As a consequence, DHT now depends on LDS

Eh?

You are not picking on our private little nitpicking here, are you Charles? ;-)

More importantly -- you're not picking on my cooking, are you?

--Juliusz

comment:48 Changed 7 years ago by charles

  • 18:04:05 < CIA-40> charles * r10606 /trunk/ (12 files in 2 dirs): (trunk) #3060 -- Local Peer Discovery patch from Eszet
  • 18:09:18 < CIA-40> charles * r10608 doc/rpc-spec.txt: (trunk) add lds-enabled to the docs for session-get and session-set
  • 18:11:23 < CIA-40> livings124 * r10609 Transmission.xcodeproj/project.pbxproj: Mac build compiles again
  • 18:19:42 < CIA-40> livings124 * r10610 macosx/ (5 files): add "local peer discovery" peer info to the inspector
  • 18:23:44 < CIA-40> charles * r10611 /trunk/ (7 files in 3 dirs): (trunk) add LDS toggles to GTK+ client, Qt client, and transmission-remote

comment:49 Changed 7 years ago by jch

Excellent. I retract my previous assertion about the shelf life being wrong.

I suggest the following patch to make LDS atoms prioritary.

--Juliusz

comment:50 Changed 7 years ago by livings124

r10613 swaps LDS before Tracker.

comment:51 follow-up: Changed 7 years ago by charles

Eszet: by the way, why is this called "LDS" instead of "LPD"?

comment:52 in reply to: ↑ 51 Changed 7 years ago by Eszet

Great, it's quite rewarding to see the feature finally appear in the Transmission GUI. Kudos, to all!

Replying to charles:

Eszet: by the way, why is this called "LDS" instead of "LPD"?

Well, the acronym LDS is mostly for historical reasons. Looking from a more functional perspective at the beginning of my development, back then I thought of it as Local Discovery Service; here you go -- and I, for my part, still like it.

comment:53 Changed 7 years ago by Longinus00

Revision 10611 has a bug in the qt client code. The LDS_ENABLED key also needs to exist in Session::updatePref.

diff --git qt/session.cc qt/session.cc
index 46b952d..15273cc 100644
--- qt/session.cc
+++ qt/session.cc
@@ -141,6 +141,7 @@ Session :: updatePref( int key )
         case Prefs :: DOWNLOAD_DIR:
         case Prefs :: INCOMPLETE_DIR:
         case Prefs :: INCOMPLETE_DIR_ENABLED:
+        case Prefs :: LDS_ENABLED:
         case Prefs :: PEER_LIMIT_GLOBAL:
         case Prefs :: PEER_LIMIT_TORRENT:
         case Prefs :: USPEED_ENABLED:

comment:54 Changed 7 years ago by charles

Committed to r10631. Thanks Longinus00!

About LDS, I propose we follow the same naming scheme that other Torrent apps use, to reduce user confusion. Does anyone mind if I change the instances of "LDS" to something else?

comment:55 Changed 7 years ago by Eszet

Just wanted to add that changing user interface names is not a problem with me. Naming the option Local Peer Discovery is fine.

comment:56 Changed 7 years ago by charles

Looks like uTorrent and qBitTorrent call it "Local Peer Discovery". Deluge calls it LSD but mentions Local Peer Discovery in the tooltip.

Last edited 7 years ago by charles (previous) (diff)

comment:57 Changed 7 years ago by Eszet

Eszet: also, just to make sure: this is compatible with the Local Peer Discovery used by other clients such as uTorrent and Monotorrent, right?

At least it's intended to be. Compatibility with all kinds of implementations is hard to postulate at this point (unless we find some testers). Since it's verified to work with uTorrent, it should work for the most part with other clients trying to achieve uTorrent compatibility (which I assume is a considerable share).

The name for the protocol is definitely Local Peer Discovery.

Last edited 7 years ago by Eszet (previous) (diff)

comment:58 Changed 7 years ago by Eszet

Charles, there's still a minor bug with the long option switches for LDS in transmission-{daemon,remote} after your r10636, r10637 commits. I guess these quoted strings sneaked through your search & replace for LDS.

While speaking of this, I'd also vote for changing the short for LPD switch from z/Z to y/Y since the latter character appears in "local peer DiscoverY".

comment:59 Changed 7 years ago by charles

  • Resolution set to fixed
  • Status changed from assigned to closed

fix daemon/remote bugs, and switch from z/Z to y/Y, as suggested by Eszet. :)

comment:60 Changed 7 years ago by charles

Last edited 7 years ago by charles (previous) (diff)

comment:61 Changed 7 years ago by Eszet

Thanks, Charles!

I shall have a look at #3208.

Note: See TracTickets for help on using tickets.