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

adds bep_0014 (local peer discovery) #998

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

philrhc
Copy link

@philrhc philrhc commented Feb 28, 2025

Comment on lines +351 to +352
lpdStart(cl)

Copy link
Author

@philrhc philrhc Feb 28, 2025

Choose a reason for hiding this comment

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

probably should have a configuration flag to enable peer discovery, i'm welcoming any ideas on how this could look

Copy link
Owner

Choose a reason for hiding this comment

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

LocalServiceDiscovery bool, in ClientConfig. Probably default to false for security reasons but I'm open.

@@ -296,12 +297,13 @@ func (cn *Peer) writeStatus(w io.Writer) {
prioStr += ": " + err.Error()
}
fmt.Fprintf(w, "bep40-prio: %v\n", prioStr)
fmt.Fprintf(w, "last msg: %s, connected: %s, last helpful: %s, itime: %s, etime: %s\n",
fmt.Fprintf(w, "last msg: %s, connected: %s, last helpful: %s, itime: %s, etime: %s, discovery source: %s\n",
Copy link
Author

Choose a reason for hiding this comment

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

possibly not the right place to add this

Copy link
Owner

Choose a reason for hiding this comment

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

This should already exist, see PeerConn.connectionFlags.

Comment on lines +69 to +73
m.mcPublisher, err = net.DialUDP(network, nil, m.addr)
if err != nil {
fmt.Println("Error dialing UDP:", err)
return nil
}
Copy link
Author

@philrhc philrhc Feb 28, 2025

Choose a reason for hiding this comment

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

this whole file is almost identical to libtorrent except that i couldn't run multiple torrent instances on the same machine without this

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah makes sense. libtorrent was aiming for a singleton instance for mobile devices IIRC.

@philrhc
Copy link
Author

philrhc commented Feb 28, 2025

happy to add more testing here, i've been testing in a scratch app

Comment on lines +19 to +20
var lpd *LPDServer
var mu sync.Mutex
Copy link
Author

Choose a reason for hiding this comment

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

i think this needs to be changed...

Copy link
Owner

Choose a reason for hiding this comment

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

It'll need checking. Global locks aren't bad if they're used correctly and don't matter near as much as people think.

@anacrolix
Copy link
Owner

Are you interested in refactoring it a bit (totally fine if not). I think we could move the LPD stuff into a separate package, probably just ./lpd, or bep14 or lsd (not sure why LSD and LPD are mixed and matched in the literature here), mainly just to not clutter the API more.

What are the licence requirements for this? I note @axet has forgotten the MPL2 licence throughout his codebase. He's got LGPL3 for his code, so does that need to be reflected here?

Comment on lines +69 to +73
m.mcPublisher, err = net.DialUDP(network, nil, m.addr)
if err != nil {
fmt.Println("Error dialing UDP:", err)
return nil
}
Copy link
Owner

Choose a reason for hiding this comment

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

Yeah makes sense. libtorrent was aiming for a singleton instance for mobile devices IIRC.

Comment on lines +19 to +20
var lpd *LPDServer
var mu sync.Mutex
Copy link
Owner

Choose a reason for hiding this comment

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

It'll need checking. Global locks aren't bad if they're used correctly and don't matter near as much as people think.

"github.com/anacrolix/torrent/metainfo"
)

var lpd *LPDServer
Copy link
Owner

Choose a reason for hiding this comment

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

This particular global probably shouldn't exist per your other comment.

Comment on lines +351 to +352
lpdStart(cl)

Copy link
Owner

Choose a reason for hiding this comment

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

LocalServiceDiscovery bool, in ClientConfig. Probably default to false for security reasons but I'm open.

@@ -296,12 +297,13 @@ func (cn *Peer) writeStatus(w io.Writer) {
prioStr += ": " + err.Error()
}
fmt.Fprintf(w, "bep40-prio: %v\n", prioStr)
fmt.Fprintf(w, "last msg: %s, connected: %s, last helpful: %s, itime: %s, etime: %s\n",
fmt.Fprintf(w, "last msg: %s, connected: %s, last helpful: %s, itime: %s, etime: %s, discovery source: %s\n",
Copy link
Owner

Choose a reason for hiding this comment

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

This should already exist, see PeerConn.connectionFlags.

@anacrolix
Copy link
Owner

Thanks :)

@anacrolix
Copy link
Owner

There are some race conditions. Run the tests with -race on your system.

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

Successfully merging this pull request may close these issues.

2 participants