remove nameproxy and replace with default IPv4 proxy #6053

Diapolo commented at 1:05 PM on April 23, 2015: none

simplifies the code, as the nameproxy was just the same as the default IPv4 proxy anyway

laanwj commented at 3:28 PM on April 23, 2015: member

Agree on the simplification here, some comments:

How will this interact with -onlynet=ipv6? I hope we don't end up disabling proxying of names, or connecting to names, when IPv4 is disabled?
Please define a NET_NAME or such constant, and alias it to NET_IPV4. This makes it clearer that we use the [NET_IPV4] proxy for (not further specified) names as well

in src/netbase.cpp:None in c7f969c836 outdated

 587 | @@ -605,10 +588,7 @@ bool ConnectSocketByName(CService &addr, SOCKET& hSocketRet, const char *pszDest
 588 |  
 589 |      SplitHostPort(string(pszDest), port, strDest);
 590 |  
 591 | -    proxyType nameProxy;
 592 | -    GetNameProxy(nameProxy);
 593 | -
 594 | -    CService addrResolved(CNetAddr(strDest, fNameLookup && !HaveNameProxy()), port);
 595 | +    CService addrResolved(CNetAddr(strDest, fNameLookup && !HaveProxy(NET_IPV4)), port);

laanwj commented at 3:33 PM on April 23, 2015:

Could completely bypass this lookup in the case that a proxy is used, after all we always pass the name to the proxy. e.g.

ProxyType proxy;
if(GetProxy(NET_NAME, proxy))
{
    addr = CService("0.0.0.0:0"); 
    return ConnectThroughProxy(proxy, strDest, port, hSocketRet, nTimeout, outProxyConnectionFailed);
}
CService addrResolved(CNetAddr(strDest, fNameLookup), port);
if (!addrResolved.IsValid())
    return false;
addr = addrResolved;
return ConnectSocket(addr, hSocketRet, nTimeout);

This can even further simplify the code.

Diapolo commented at 6:44 AM on April 24, 2015: none

@laanwj Should I just alias NET_IPV4 and NET_NAME or should NET_NAME be an own proxy, which could potentially be user-configured (not scope of this pull)?

laanwj commented at 12:49 PM on April 24, 2015: member

I'd say just alias it to NET_IPV4. Otherwise you'd reintroduce the name proxy in a different guise. In principle NET_NAME means NET_IPV4_OR_IPV6, but actually defining that doesn't make much sense implementation-wise.

The most true-to-reality design would be to decouple the proxies from NET_*, and introduce a new enumeration with PROXY_DEFAULT and PROXY_ONION. After all the proxies act on subsets of names, not classes of address.

But this achieves the same ... Or maybe NET_DEFAULT would be a better alias than NET_NAME? (with the comment // to be used for names for which the network is undetermined)

laanwj added the label P2P on Apr 24, 2015

Diapolo commented at 12:34 PM on April 28, 2015: none

@laanwj -onlynet seems to not touch proxy setup so we alsways can be sure, if -proxy is valid, we have an IPv4/IPv6 proxy setup. I'll push an updated version shortly, which will include an alias NET_DEFAULT and also pick up your suggestion for ConnectSocketByName().

Edit: Updated...

laanwj commented at 12:47 PM on April 30, 2015: member

This is failing the proxy test on all platforms that run the tests:

Initializing test directory /tmp/testLRq9Jg

Proxy: Socks5Command(1,3,127.0.0.1,11297,None,None)

Proxy: Socks5Command(1,3,127.0.0.1,11296,91632771,1889679809)

Proxy: Socks5Command(1,3,127.0.0.1,11296,None,None)

Proxy: Socks5Command(1,3,127.0.0.1,11295,None,None)

Proxy: Socks5Command(1,3,127.0.0.1,11298,3842137544,3256031132)

Proxy: Socks5Command(1,3,127.0.0.1,11297,None,None)

Proxy: Socks5Command(1,3,15.61.23.23,1234,None,None)

Assertion failed: 127.0.0.1 != 15.61.23.23

File "/home/travis/build/bitcoin/bitcoin/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/test_framework.py", line 119, in main

self.run_test()

File "/home/travis/build/bitcoin/bitcoin/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/proxy_test.py", line 130, in run_test

self.node_test(self.nodes[0], [self.serv1, self.serv1, self.serv1, self.serv1], False)

File "/home/travis/build/bitcoin/bitcoin/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/proxy_test.py", line 82, in node_test

assert_equal(cmd.addr, "15.61.23.23")

File "/home/travis/build/bitcoin/bitcoin/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/util.py", line 332, in assert_equal

raise AssertionError("%s != %s"%(str(thing1),str(thing2)))

Stopping nodes

Diapolo commented at 11:14 AM on May 10, 2015: none

@laanwj Any idea why this fails, I'm able to use this locally just fine.

jonasschnelli commented at 11:20 AM on May 10, 2015: contributor

@Diapolo: somehow your given IP has no effect. See https://travis-ci.org/bitcoin/bitcoin/jobs/60370174#L3838 . You probably have to debug/bugfix your changes.

Diapolo commented at 5:19 PM on May 25, 2015: none

I found a problem related to setting addr = CService("0.0.0.0:0"); but still the tests are failing. Which builds are running the proxy tests and could it be a problem of the test itself? Perhaps I need to revert what @laanwj suggested above (third comment) and see if the tests pass with the old codeflow.

Diapolo commented at 9:48 AM on June 12, 2015: none

@laanwj Maybe you are able to help me debugging this, while you are also in the proxy code :)?

Diapolo commented at 5:52 AM on June 15, 2015: none

I'm giving this another take, when #6272 is meged.

Diapolo commented at 10:04 PM on June 20, 2015: none

I still don't understand why this is failing the tests? @laanwj Can it be a problem with the test itself? I'm using the IPv4 proxy as nameproxy, as we discussed.

remove nameproxy and replace with default proxy

- simplifies the code, as the nameproxy was just the same as the default
  IPv4 proxy anyway
- also simplifies code flow in ConnectSocketByName() as suggested by
  @laanwj

b3ad34cdf6

Diapolo commented at 7:27 PM on June 23, 2015: none

@laanwj It's passing now in that form, do we want to merge or shall I try to optimize the code-flow in a second commit?

laanwj commented at 10:53 AM on August 7, 2015: member

I'm sorry but I'm no longer fully convinced that this is an improvement. Even though I think it was my own idea.

Sure it cleans up some code, however having a separate proxy definition for unknown name lookup - separate from IPv4/IPv6/Tor makes intuitive sense. Even if in practice they're always the same.

laanwj closed this on Aug 7, 2015

Diapolo deleted the branch on Aug 10, 2015

DrahtBot locked this on Sep 8, 2021