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

setting a proxy overrides listen_interfaces #4498

Merged
merged 2 commits into from
Apr 15, 2020
Merged

setting a proxy overrides listen_interfaces #4498

merged 2 commits into from
Apr 15, 2020

Conversation

arvidn
Copy link
Owner

@arvidn arvidn commented Apr 3, 2020

The idea with this patch is to simplify the proxy handling. Instead of a proxy being treated as something independent from the network interfaces, it's treated as a network interface itself.

It makes sense that a single proxy really acts as an access point to the internet and when configuring one, whether your machine is multi-homed or not is not relevant, everything is funneled through the proxy.

It simplifies things like:

  • UPnP, NAT-PMP, PCP and local service discovery can easily be disabled entirely when using a proxy
  • Trackers will only announce once, via the proxy
  • There will only be a single DHT running, via the proxy

This change, while cleaning up some common cases, also bans some rare cases. With this patch it will not be possible to combine running SSL torrents with a proxy. The SSL listen socket is also treated as a listen_socket_t. It will no longer be possible to combine multiple listen sockets with a proxy (mostly because there's only one proxy that can be configured).

To make this fully generic, each listen interface (i.e. entry in listen_interfaces setting) would have to have its own proxy configuration. That hardly seems worth the trouble though.

@arvidn arvidn changed the title setting a proxy overrides listen_interfaces WIP: setting a proxy overrides listen_interfaces Apr 3, 2020
@arvidn arvidn added this to the 1.2.6 milestone Apr 3, 2020
@arvidn arvidn changed the title WIP: setting a proxy overrides listen_interfaces setting a proxy overrides listen_interfaces Apr 4, 2020
@FranciscoPombal
Copy link
Contributor

FranciscoPombal commented Apr 9, 2020

With this patch it will not be possible to combine running SSL torrents with a proxy.

Does "SSL torrents" refer to using HTTPS trackers or to BEP 35?

@arvidn
Copy link
Owner Author

arvidn commented Apr 9, 2020

The latter. The reason is that to open an SSL listen socket (to accept incoming SSL peer connections), you need a listen_socket_t entry for that ssl socket. This patch makes a proxy also be its own kind of listen_socket_t. And there is no support for a listen_socket_t combining both features. Almost nobody uses SSL torrents (afaik), so I don't expect it to be an issue.

UPDATE: I didn't mean "the latter". I meant this feature.

@mederel
Copy link

mederel commented Apr 9, 2020

As mentioned in the QBittorrent issue #11735, I compiled the content of the PR and it "fixed" the issue in QBittorrent.

I am running Manjaro Linux and QBittorrent 4.2.3 (community build for Manjaro).
To install the proxy-interface branch, I used the already existing AUR package libtorrent-rasterbar-git and just switch the source download to checkout the branch proxy-interface. But that is Linux, compiling is of course easier.

I think we are yet to see a Windows user compiling it too and corroborating my findings.

Cheers and kudos for the work provided, fantastic!

After that, I am sorry, I am really not well-versed enough in C++ to be in a position to do any kind of code reviews... I am a dev too, only in another language...

@arvidn
Copy link
Owner Author

arvidn commented Apr 9, 2020

thanks for testing! I will still need to figure out the issue in CI before landing this (and other, more urgent, issues have come up the last few days)

@FranciscoPombal
Copy link
Contributor

FranciscoPombal commented Apr 10, 2020

@arvidn the situation with qbittorrent/qBittorrent#11735 seems to have improved with this patch (users have been testing with a test build that includes it thanks to @xavier2k6), but there are still UDP tracker issues apparently.

@arvidn arvidn force-pushed the proxy-interface branch 2 times, most recently from 6090398 to d326a02 Compare April 13, 2020 10:59
@arvidn
Copy link
Owner Author

arvidn commented Apr 13, 2020

@xavier2k6 I believe that last commit may have addressed the "already open" issue

@xavier2k6
Copy link
Contributor

@xavier2k6 I believe that last commit may have addressed the "already open" issue

ok, will download from here & compile a new qBittorrent file for testing in #11735

@arvidn
Copy link
Owner Author

arvidn commented Apr 13, 2020

those last force pushes were just rebasing this on top of latest RC_1_2. it should be essentially the same otherwise.

I think this is in a good state now, ready to land. Anyone wanting to test would be appreciated!

@FranciscoPombal
Copy link
Contributor

A build of qBittorrent with this patch will soon land in this thread qbittorrent/qBittorrent#11735, hopefully some of the people affected by the proxy issue will test it out.

@mederel
Copy link

mederel commented Apr 13, 2020

Hi again, I recompiled the latest of proxy-interface and tested again. Working correctly here.
Still QBittorrent 4.2.3 and adapted libtorrent-rasterbar-git package of Manjaro/Arch linux.

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.

None yet

4 participants