net: remove reinit of the onion proxy #22830

pull lano1106 wants to merge 1 commits into bitcoin:master from lano1106:tor_issues changing 1 files +0 −9
  1. lano1106 commented at 6:53 pm on August 29, 2021: contributor

    onion proxy is already initialized by AppInitMain() function in init.cpp

    The removed code contains the assumption that the proxy is local.

    If it has been initialized to be remote and configured by using the -proxy option, the removed code was breaking the config.

    Signed-off-by: Olivier Langlois olivier@trillion01.com

  2. net: remove reinit of the onion proxy
    onion proxy is already initialized by AppInitMain() function in init.cpp
    
    The removed code contains the assumption that the proxy is local.
    
    If it has been initialized to be remote and configured by using the
    -proxy option, the removed code was breaking the config.
    
    Signed-off-by: Olivier Langlois <olivier@trillion01.com>
    21ae1525a1
  3. DrahtBot added the label P2P on Aug 29, 2021
  4. DrahtBot commented at 0:20 am on August 30, 2021: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #22651 (tor: respect non-onion -onlynet= for outgoing Tor connections by vasild)
    • #19358 (torcontrol : avoid to set wrong outbound proxy and network settings when creating an inbound onion service. by Saibato)
    • #15423 (torcontrol: Query Tor for correct -onion configuration by luke-jr)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  5. laanwj commented at 7:37 am on August 30, 2021: member

    This code is there to set the onion proxy automatically when a local instance of Tor is available that torcontrol can communicate with, and make listen on a hidden service.

    Removing it is not a good solution. It could however check if there is a proxy set up for onion already, and skip this code if so, instead of overwriting it. wait, it does this (gArgs.GetArg("-onion", "") == "". What exactly is the problem you’re having? What I can think of is that you provided -proxy, which automatically sets -onion but isn’t detected here (this seems to be a bug).

    If it has been initialized to be remote and configured by using the -proxy option, the removed code was breaking the config.

    You likely want to set listenonion=0 in that case to prevent it from communicating with the local tor instance at all. Then this code is skipped.

  6. laanwj commented at 7:43 am on August 30, 2021: member
    Seperately from that I do think this code could be better. Right now, it assumes that port 9050 is used, while it has a connection to the Tor control socket so could directly ask what the SOCKS5 proxy port it is listening on is (GETINFO net/listeners/socks, I think, see control-spec.txt). Or bypass it if there isn’t any.
  7. vasild commented at 9:01 am on August 30, 2021: member

    … so could directly ask what the SOCKS5 proxy port it is listening on is …

    This is what #15423 does.

  8. laanwj commented at 11:31 am on August 30, 2021: member

    This is what #15423 does.

    Oops, I completely forgot about that one.

  9. lano1106 commented at 2:07 pm on August 30, 2021: contributor

    This code is there to set the onion proxy automatically when a local instance of Tor is available that torcontrol can communicate with, and make listen on a hidden service.

    Removing it is not a good solution. ~It could however check if there is a proxy set up for onion already, and skip this code if so, instead of overwriting it.~ wait, it does this (gArgs.GetArg("-onion", "") == "". What exactly is the problem you’re having? What I can think of is that you provided -proxy, which automatically sets -onion but isn’t detected here (this seems to be a bug).

    If it has been initialized to be remote and configured by using the -proxy option, the removed code was breaking the config.

    You likely want to set listenonion=0 in that case to prevent it from communicating with the local tor instance at all. Then this code is skipped.

    Ok, I agree that removing the whole block might not be the right solution but testing that -onion is empty isn’t the correct condition neither.

    My problem with this line is it that the onion proxy address is hardcoded to 127.0.0.1

    Imagine that someone sets -proxy 192.168.1.2:9050 -torcontrol 127.0.0.1:9051

    or perhaps something less surprising and or more common: -proxy 127.0.0.1:8050 -torcontrol 127.0.0.1:9051

    I guess that the intent of the onion proxy setting code block is for the case where someone would have provided -torcontrol and have omitted to provide -onion or -proxy.

    How about testing if it is set by calling GetProxy()?

  10. vasild commented at 2:23 pm on August 30, 2021: member

    My problem with this line is it that the onion proxy address is hardcoded to 127.0.0.1

    Imagine that someone sets -proxy 192.168.1.2:9050 -torcontrol 127.0.0.1:9051

    This is fixed in #15423. Feel free to review and/or test it.

  11. lano1106 commented at 2:39 pm on August 30, 2021: contributor

    My problem with this line is it that the onion proxy address is hardcoded to 127.0.0.1 Imagine that someone sets -proxy 192.168.1.2:9050 -torcontrol 127.0.0.1:9051

    This is fixed in #15423. Feel free to review and/or test it.

    thx. I will take a look

  12. lano1106 closed this on Sep 1, 2021

  13. DrahtBot locked this on Sep 1, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-06 04:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me