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

extend core proxy options and handling #4871

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@Diapolo

Diapolo commented Sep 8, 2014

  • rework the proxy handling in init to cover more cases and work more
    thoroughly
  • extend RPC call getnetwork info to see if IPv6 and Tor proxy is the
    default proxy

See #2575 for discussion!

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Sep 8, 2014

@laanwj As you suggested, a new and clean pull and empty discussion ;).

Diapolo commented Sep 8, 2014

@laanwj As you suggested, a new and clean pull and empty discussion ;).

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Sep 23, 2014

@laanwj Can you please have a look again, I hope we can sort this out now finally :).

Diapolo commented Sep 23, 2014

@laanwj Can you please have a look again, I hope we can sort this out now finally :).

@BitcoinPullTester

This comment has been minimized.

Show comment
Hide comment
@BitcoinPullTester

BitcoinPullTester Sep 23, 2014

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4871_2711cda5b8c54efb2164d7bb3593b3db50f8e15c/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

BitcoinPullTester commented Sep 23, 2014

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4871_2711cda5b8c54efb2164d7bb3593b3db50f8e15c/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

Diapolo pushed a commit to Diapolo/bitcoin that referenced this pull request Sep 23, 2014

Philip Kaufmann
[Qt] allow users to set -onion via GUI
- also allow users to see, if the default proxy (-proxy) is used for
  reaching peers via IPv6 or Tor
- based on #4871
@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Oct 6, 2014

Rebased and removed an obsolete c_str().

Diapolo commented Oct 6, 2014

Rebased and removed an obsolete c_str().

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Oct 10, 2014

@laanwj This and the GUI part are pulls that are not cosmetic, but need review or ACKs.

Diapolo commented Oct 10, 2014

@laanwj This and the GUI part are pulls that are not cosmetic, but need review or ACKs.

@laanwj

View changes

Show outdated Hide outdated src/rpcnet.cpp
obj.push_back(Pair("name", GetNetworkName(network)));
obj.push_back(Pair("limited", IsLimited(network)));
obj.push_back(Pair("reachable", IsReachable(network)));
obj.push_back(Pair("proxy", proxy.IsValid() ? proxy.ToStringIPPort() : string()));
if (network != NET_IPV4)
obj.push_back(Pair("proxy-from-default", proxy == proxyDefault));

This comment has been minimized.

@laanwj

laanwj Oct 10, 2014

Member

As said before - I'm still not convinced that proxy-from-default is useful information. Clients requesting from RPC just want to know the currently effective state, it shouldn't matter how it came that way. For troubleshooting command line argument interaction there's always debug.log.

@laanwj

laanwj Oct 10, 2014

Member

As said before - I'm still not convinced that proxy-from-default is useful information. Clients requesting from RPC just want to know the currently effective state, it shouldn't matter how it came that way. For troubleshooting command line argument interaction there's always debug.log.

This comment has been minimized.

@Diapolo

Diapolo Oct 14, 2014

Alright, removing this will finally make the pull get merged then?

@Diapolo

Diapolo Oct 14, 2014

Alright, removing this will finally make the pull get merged then?

This comment has been minimized.

@laanwj

laanwj Oct 14, 2014

Member

The main problem here is that we don't have testcases with proxy, so this needs to be manually tested. Can you list what you tested?

@laanwj

laanwj Oct 14, 2014

Member

The main problem here is that we don't have testcases with proxy, so this needs to be manually tested. Can you list what you tested?

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Nov 28, 2014

Updated to include changes that were merged in #5358. @gmaxwell Mind reviewing this now?

Diapolo commented Nov 28, 2014

Updated to include changes that were merged in #5358. @gmaxwell Mind reviewing this now?

@laanwj laanwj added the P2P label Dec 3, 2014

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake Jan 23, 2015

Member

Needs rebase

Member

fanquake commented Jan 23, 2015

Needs rebase

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jan 23, 2015

Rebased, needs review / testing.

Diapolo commented Jan 23, 2015

Rebased, needs review / testing.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 13, 2015

Member

To be clear what this is blocked on: We need (RPC) tests for the proxy functionality, before we can merge changes like this.

Member

laanwj commented Mar 13, 2015

To be clear what this is blocked on: We need (RPC) tests for the proxy functionality, before we can merge changes like this.

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Apr 23, 2015

@laanwj Rebased to include the recent -proxyrandomize change. Also as a proxy test was added, this should be less controversial if tests pass I guess :).

Diapolo commented Apr 23, 2015

@laanwj Rebased to include the recent -proxyrandomize change. Also as a proxy test was added, this should be less controversial if tests pass I guess :).

Diapolo pushed a commit to Diapolo/bitcoin that referenced this pull request Apr 23, 2015

Philip Kaufmann
[Qt] allow users to set -onion via GUI
- also allow users to see, if the default proxy (-proxy) is used for
  reaching peers via IPv6 or Tor
- based on #4871
Show outdated Hide outdated src/rpcnet.cpp
obj.push_back(Pair("name", GetNetworkName(network)));
obj.push_back(Pair("limited", IsLimited(network)));
obj.push_back(Pair("reachable", IsReachable(network)));
obj.push_back(Pair("proxy", proxy.IsValid() ? proxy.proxy.ToStringIPPort() : string()));
obj.push_back(Pair("proxy_randomize_credentials", proxy.randomize_credentials));
if (network != NET_IPV4)
obj.push_back(Pair("proxy-from-default", proxy.proxy == proxyDefault.proxy));

This comment has been minimized.

@laanwj

laanwj May 6, 2015

Member

Let's remove proxy-from-default, it's not useful information and will always be true anyway according to the above code.

@laanwj

laanwj May 6, 2015

Member

Let's remove proxy-from-default, it's not useful information and will always be true anyway according to the above code.

This comment has been minimized.

@Diapolo

Diapolo May 14, 2015

I'll remove it :).

@Diapolo

Diapolo May 14, 2015

I'll remove it :).

Philip Kaufmann
extend proxy handling and processing
- rework the proxy handling in init to cover more cases and work more
  thoroughly
@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo May 14, 2015

@laanwj Removed changes to rpcnet.cpp.

Diapolo commented May 14, 2015

@laanwj Removed changes to rpcnet.cpp.

Diapolo pushed a commit to Diapolo/bitcoin that referenced this pull request May 14, 2015

Philip Kaufmann
[Qt] allow users to set -onion via GUI
- also allow users to see, if the default proxy (-proxy) is used for
  reaching peers via IPv6 or Tor
- based on #4871

Diapolo pushed a commit to Diapolo/bitcoin that referenced this pull request May 14, 2015

Philip Kaufmann
[Qt] allow users to set -onion via GUI
- also allow users to see, if the default proxy (-proxy) is used for
  reaching peers via IPv6 or Tor
- based on #4871
@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo May 31, 2015

@laanwj This is passing proxy tests and I removed what you didn't like. Anything I can do to get this forward finally?

Diapolo commented May 31, 2015

@laanwj This is passing proxy tests and I removed what you didn't like. Anything I can do to get this forward finally?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 10, 2015

Member

I'm terribly sorry sorry for holding this up so long, but I'm not convinced that the new logic is any better.

The logical flow here sounds to me:

  • Parse default -proxy, apply to all nets, as well as the name proxy
  • Parse onion-specific proxy -onion, apply to NET_TOR. This overrides the default set in the first step, if any

All errors during parsing of either setting should be fatal, to avoid that the user is running with different settings than they expect.

This is nearer to the original code than yours. I certainly think the code could be simplified and made more clear, what do you think about:

    bool proxyRandomize = GetBoolArg("-proxyrandomize", true);
    // -proxy sets a proxy for all outgoing network traffic
    // -noproxy (or -proxy=0) as well as the empty string can be used to not set a proxy, this is the default
    std::string proxyArg = GetArg("-proxy", "");
    if (proxyArg != "" && proxyArg != "0") {
        proxyType addrProxy = proxyType(CService(proxyArg, 9050), proxyRandomize);
        if (!addrProxy.IsValid())
            return InitError(strprintf(_("Invalid -proxy address: '%s'"), proxyArg));

        SetProxy(NET_IPV4, addrProxy);
        SetProxy(NET_IPV6, addrProxy);
        SetProxy(NET_TOR, addrProxy);
        SetNameProxy(addrProxy);
        SetReachable(NET_TOR); // by default, -proxy sets onion as reachable, unless -noonion later
    }

    // -onion can override normal proxy for .onion addresses
    // -noonion (or -onion=0) disables connecting to .onion entirely
    // An empty string is used to just not override the onion proxy
    std::string onionArg = GetArg("-onion", "");
    if (onionArg != "") {
        if (onionArg == "0") { // Handle -noonion/-onion=0
            SetReachable(NET_TOR, false); // set onions as unreachable
        } else {
            proxyType addrOnion;
            addrOnion = proxyType(CService(onionArg, 9050), proxyRandomize);
            if (!addrOnion.IsValid())
                return InitError(strprintf(_("Invalid -onion address: '%s'"), onionArg));
            SetProxy(NET_TOR, addrOnion);
            SetReachable(NET_TOR);
        }
    }

This provides a straightforward flow, gets rid of .count() (which is what you needed for the GUI, as it makes it possible to override an earlier provided proxy option to nothing), as well as comments the different cases.

Member

laanwj commented Jun 10, 2015

I'm terribly sorry sorry for holding this up so long, but I'm not convinced that the new logic is any better.

The logical flow here sounds to me:

  • Parse default -proxy, apply to all nets, as well as the name proxy
  • Parse onion-specific proxy -onion, apply to NET_TOR. This overrides the default set in the first step, if any

All errors during parsing of either setting should be fatal, to avoid that the user is running with different settings than they expect.

This is nearer to the original code than yours. I certainly think the code could be simplified and made more clear, what do you think about:

    bool proxyRandomize = GetBoolArg("-proxyrandomize", true);
    // -proxy sets a proxy for all outgoing network traffic
    // -noproxy (or -proxy=0) as well as the empty string can be used to not set a proxy, this is the default
    std::string proxyArg = GetArg("-proxy", "");
    if (proxyArg != "" && proxyArg != "0") {
        proxyType addrProxy = proxyType(CService(proxyArg, 9050), proxyRandomize);
        if (!addrProxy.IsValid())
            return InitError(strprintf(_("Invalid -proxy address: '%s'"), proxyArg));

        SetProxy(NET_IPV4, addrProxy);
        SetProxy(NET_IPV6, addrProxy);
        SetProxy(NET_TOR, addrProxy);
        SetNameProxy(addrProxy);
        SetReachable(NET_TOR); // by default, -proxy sets onion as reachable, unless -noonion later
    }

    // -onion can override normal proxy for .onion addresses
    // -noonion (or -onion=0) disables connecting to .onion entirely
    // An empty string is used to just not override the onion proxy
    std::string onionArg = GetArg("-onion", "");
    if (onionArg != "") {
        if (onionArg == "0") { // Handle -noonion/-onion=0
            SetReachable(NET_TOR, false); // set onions as unreachable
        } else {
            proxyType addrOnion;
            addrOnion = proxyType(CService(onionArg, 9050), proxyRandomize);
            if (!addrOnion.IsValid())
                return InitError(strprintf(_("Invalid -onion address: '%s'"), onionArg));
            SetProxy(NET_TOR, addrOnion);
            SetReachable(NET_TOR);
        }
    }

This provides a straightforward flow, gets rid of .count() (which is what you needed for the GUI, as it makes it possible to override an earlier provided proxy option to nothing), as well as comments the different cases.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 12, 2015

Member

Closing in favor of #6272

Member

laanwj commented Jun 12, 2015

Closing in favor of #6272

@laanwj laanwj closed this Jun 12, 2015

@Diapolo Diapolo deleted the Diapolo:proxy-core branch Jun 12, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment