Implementing latest version of BEP-7 for HTTP requests

[lvella]

Implements the latest version of BEP-7 on HTTP announces, which solves issue #1659. DNS cache in libcurl had to be disabled in order to force an IPv4 or IPv6 connection, otherwise it simply uses the cached address, irrespective of the protocol. A fix to the libcurl issue has been merged to master, so disabling the cache is not done for newer versions libcurl.

Fixes #1659.

[lvella]

Seems to be working, but I did not find an UDP tracker implementing BEP-7.

[Anuskuss]

Rebase?

[ckerr]

@lvella would you be interested in updating this patch? If so, I'd be happy to review / test

[lvella]

Yes, I will. But I probably will only be able to do it by next week.

[ckerr]

@Ivella I will be traveling this week, so there may be some lag in code review. Don't worry, this PR hasn't been forgotten again

[lvella]

Hi @ckerr , I have ported one of the two patches in this PR (the one affecting http announces) to main:
https://github.com/lvella/transmission/tree/BEP-7-http

I still have to port the patch for udp announces, but that one is purely theoretical, as I never encountered an udp tracker that serves both via IPv4 and IPv6, so I couldn't even test it.

Do you think I should include just the http patch in this PR, and leave the upd patch for another PR?

[lvella]

BTW, do we have the public IPv4 address through which we are reachable? It would be nice to add &ipv4=x.x.x.x to requests via IPv6, to enable IPv6-only trackers to know the IPv4 address of the peer (also, purely theoretical issue).

[ckerr]

BTW, do we have the public IPv4 address through which we are reachable? It would be nice to add &ipv4=x.x.x.x to requests via IPv6, to enable IPv6-only trackers to know the IPv4 address of the peer (also, purely theoreti

Check tr_session.externalIP(), which is set iff we get our public IP back from a tracker response.

Do you think I should include just the http patch in this PR, and leave the upd patch for another PR?

Split it into two, please.

[lvella]

What do you think of this solution for Curl's version problem?

[lvella]

@lvella "This branch has conflicts that must be resolved"

What about the concerns raised by @ckerr ? Is the solution provided for the curl version problem acceptable (i.e. single request with both &ipv6 and &ipv4 set)?

[Coeur]

What about the concerns raised by @ckerr ?

I don't have a "best" answer. But if people are unhappy regarding the number DNS requests, and others don't care, then eventually provide a settings to offer the choice of behaviour to the enduser.

[lvella]

@Coeur It is now compatible with main, but these issues pointed by the checks were already there.

[Wanderboot]

Is there any hope of this actually getting accepted? I miss being able to seed.

[lvella]

I don't know. I lost count on how many times I have fixed the conflicts with main. I would like a comment from someone with merge permission before I do it again.

[lvella]

BTW, should it be me to click "Resolve conversation" on the issues?

[ckerr]

I've mostly left this alone because a previous revision made the "excessive DNS requests" issue worse and TBH I missed the commit that addressed the dns requests. I'm OK with this landing after the curl_version_info() question is resolved

[lvella]

@ckerr I have updated again with main. What curl_version_info() question are you talking about? If it is about the check for curl version, it is like this:

#if LIBCURL_VERSION_NUM < CURL_VERSION_BITS(7, 77, 0)

see L431.

[ckerr]

@ckerr I have updated again with main. What curl_version_info() question are you talking about? If it is about the check for curl version, it is like this:

#if LIBCURL_VERSION_NUM < CURL_VERSION_BITS(7, 77, 0)

see L431.

Hi @lvella, my point is that #if LIBCURL_VERSION_BITS(7, 77, 0) is going to be evaluated at compile time, meaning it'll pick up the value of whatever version of libcurl happens to be installed on the build machine rather than whatever's on the user's machine when they install and run Transmission. Since we want to know what's running on the user's machine, we need to make a runtime test instead, i.e. curl_version_info()

[ckerr]

@ckerr I have updated again with main. What curl_version_info() question are you talking about?

d'oh my apologies. As I went in to add the if (curl_version_info(...) suggestion above, I saw I still had a review comment from yesterday that was still pending. So my comment about curl_version_info() must've looked like it came out of nowhere because I hadn't submitted my review yesterday. 😅

[lvella]

@ckerr Wait! NOOOOO! The curl_version_info is not there yet! It is one commit away!

by request from @Ivella

[lvella]

@ckerr Done!

[Anuskuss]

@ckerr New stable please!