Fixing TCP Listening Port with Proxy

[Masong19hippows]

Hello,

I made a PR awhile back about listening on the defined TCP port whenever proxies are enabled. The current behavior is to disable listening when a proxy is set. My proposed changes would add a bool configuration option that would enable listening when a proxy is configured. Default value is false. In the same logic, I also removed the proxy check when enabling natPMP/upnp. This check was meant to disable opening up these ports if the proxy was configured. Instead of coupling this with any proxy settings, I just removed the check all together so that it's only dependent on the "Use UPnP / NAT-PMP port forwarding from my router" flag. I felt this was best because it follows the same logic as decoupling the listening TCP port with the proxy settings - user should decide with one setting.

This was in PR #7726

I felt it was best to close that PR and start anew because I messed up a rebase. During the year of inactivity, there were alot of changes that caused the PR to need a rebase. It got messy because I messed up and I felt like this was a much better option. I think I resolved all of the issues @arvidn showed as well.

Please review when you have a moment.

[Masong19hippows]

I'm not sure why the build is failing on github. It doesn't look related to anything I changed. My guess is that I have to change something in the settings pack test with the other bools, but I don't know the magic number to add to the base.

[qBittUser]

Fixing Listneing Port with Proxy

There's a typo "Listneing" instead of "Listening" in the commit title.

[jiang-zhexin]

I really need this feature, this is awesome.

And could add SOCKS5 BIND support? I've implemented it in Go, it might not be too hard.

[Masong19hippows]

@jiang-zhexin

You might need to look back at the previous pr for more info where I explained it better. Essentialy the socks bind command is meant to be a temporary port on the server side for secondary connections to an already existing connect. Like, if I contacted a website through a socks proxy, I could then initiate the bind process for that website to send traffic back to me through an open port on the server.

It's not really a bind in the traditional sense, and because of this, I don't think it's possible to accept incoming connections through sock bind.

[Masong19hippows]

I think I might've fixed it. I think that means it's good to merge on the ABI stability part then?

[qBittUser]

There's still a commit title typo: https://github.com/arvidn/libtorrent/pull/8003#issuecomment-3128010930

[Masong19hippows]

I haven't thought this through, but it seems to be some overlap with this new setting and proxy_peer_connections. For example, if proxy_peer_connections is set and proxy_accept_incoming is set, that seems like a contradiction.

Did we already discuss the feasibility of just making proxy_peer_connections being false meaning incoming connections are allowed? Or perhaps that's already the case.

The problem with that approach is that there is still a use case to use the proxy for peer connections and still listen on the local port. With some external forwarding tools, you can forward incoming connections from the proxy to the local port, but that doesn't work if qbittorrent isn't listening on that port.

[Masong19hippows]

did you consider only changing repoen_listen_sockets()? I think the check right here is the only thing you would need to change: https://github.com/arvidn/libtorrent/blob/RC_2_0/src/session_impl.cpp#L2060-L2072

Right now, if we use a proxy for peers, we ignore all the listen sockets set up, and just create a single one with the proxy flag. That's the flag that causes the behavior you don't want. i.e. not accepting incoming connections directly, only via the proxy.

I believe I already did this here, unless I am mindreading what you are saying. My proposed solution is to add an additional check for the new proxy_accept_incoming flag.

This has 2 different permutations then. If there is a proxy configured and proxy_peer_connections is true, then we would listen on the local port if proxy_accept_incoming is true and follow original logic if false. If there is a proxy configured and proxy_peer_connections is false, then we won't listen on the local port regardless of the proxy_accept_incoming flag. We could remove the proxy_peer_connections check here, but then it would change the predefined behavior, which you said you didn't want to do in the previous pr, and instead want to guard this new behavior around this new flag.

In addition to this check, there is also a check on line 2834 that does the same thing, which I believe to be necessary.

You could, instead, treat the proxy as addative to the listen sockets configured by listen_interfaces (rather than replacing them). I believe that would also give you the behavior you need, but with a smaller patch and without changing so much existing behavior.

Can you please elaborate on what you mean here? I'm not familiar enough with the codebase to understand.

[fruworg]

LGTM