Support for Tor's Single Onion Service #10161

rodentrabies commented at 7:32 AM on April 6, 2017: contributor

Implements #9836

Add flag to ADD_ONION command
Provide a way for external configuration

laanwj added the label P2P on Apr 6, 2017

in src/torcontrol.cpp:376 in c1f72779d4 outdated

 371 | @@ -372,6 +372,8 @@ class TorController
 372 |      bool reconnect;
 373 |      struct event *reconnect_ev;
 374 |      float reconnect_timeout;
 375 | +    /** Flag for Single Onion Service */
 376 | +    bool non_anonymous;

laanwj commented at 8:50 AM on April 6, 2017:

+1 on calling this flag non_anonymous

in src/torcontrol.cpp:421 in c1f72779d4 outdated

 415 | @@ -414,6 +416,9 @@ TorController::TorController(struct event_base* _base, const std::string& _targe
 416 |          LogPrint(BCLog::TOR, "tor: Reading cached private key from %s\n", GetPrivateKeyFile());
 417 |          private_key = pkf.second;
 418 |      }
 419 | +    // Set Single Onion Service flag
 420 | +    // TODO: provide a way to configure it externally
 421 | +    non_anonymous = false;

laanwj commented at 8:51 AM on April 6, 2017:

As for option: maybe -listenonion=2?

rodentrabies commented at 11:16 AM on April 6, 2017:

Yes, thanks for the advice. That should be OK. Will finish it this evening.

in src/torcontrol.cpp:420 in a6fffa5f05 outdated

 415 | @@ -414,6 +416,12 @@ TorController::TorController(struct event_base* _base, const std::string& _targe
 416 |          LogPrint(BCLog::TOR, "tor: Reading cached private key from %s\n", GetPrivateKeyFile());
 417 |          private_key = pkf.second;
 418 |      }
 419 | +    // Set Single Onion Service flag
 420 | +    if (mode == 1)

rodentrabies commented at 7:38 AM on April 7, 2017:

Having -listenonion=<n> flag requires allowed values checks and isn't obvious enough as for anonymous/non-anonymous service option. Maybe it would be better to just add another boolean flag instead, with some obvious name?

laanwj commented at 2:52 PM on April 7, 2017:

Yeah could do that. Though I think the number of options of bitcoind is kind of getting out of hand, and this seems quite obscure so I don't mind hiding it a bit. Could wait and see what other dev's opinions about this are.

rodentrabies force-pushed on Apr 7, 2017

rodentrabies renamed this:
~~[WIP] Support for Tor's Single Onion Service~~
Support for Tor's Single Onion Service
on Apr 7, 2017

rodentrabies commented at 9:28 PM on April 7, 2017: contributor

Added allowed values check for -listenonion and mentioned it in documentation.

rodentrabies commented at 10:51 PM on April 7, 2017: contributor

I am unable to reproduce this failure on my machine. Any advise on what might cause it?

in doc/tor.md:105 in 28cc1b4ef8 outdated

 101 | @@ -102,7 +102,9 @@ affect the number of available .onion nodes.
 102 |  This new feature is enabled by default if Bitcoin Core is listening (`-listen`), and
 103 |  requires a Tor connection to work. It can be explicitly disabled with `-listenonion=0`
 104 |  and, if not disabled, configured using the `-torcontrol` and `-torpassword` settings.
 105 | -To show verbose debugging information, pass `-debug=tor`.
 106 | +To show verbose debugging information, pass `-debug=tor`. Hidded service can also be

kewde commented at 9:11 AM on April 8, 2017:

language: Hidded -> Hidden

rodentrabies commented at 10:14 AM on April 8, 2017:

Thanks for a thorough review!

in src/init.cpp:384 in 4c25515e45 outdated

 380 | @@ -381,7 +381,7 @@ std::string HelpMessage(HelpMessageMode mode)
 381 |      strUsage += HelpMessageOpt("-externalip=<ip>", _("Specify your own public address"));
 382 |      strUsage += HelpMessageOpt("-forcednsseed", strprintf(_("Always query for peer addresses via DNS lookup (default: %u)"), DEFAULT_FORCEDNSSEED));
 383 |      strUsage += HelpMessageOpt("-listen", _("Accept connections from outside (default: 1 if no -proxy or -connect)"));
 384 | -    strUsage += HelpMessageOpt("-listenonion", strprintf(_("Automatically create Tor hidden service (default: %d)"), DEFAULT_LISTEN_ONION));
 385 | +    strUsage += HelpMessageOpt("-listenonion", strprintf(_("Run Tor hidden service (0 - disabled, 1 - normal mode, 2 - single onion service, default: %d)"), DEFAULT_LISTEN_ONION));

kewde commented at 9:35 AM on April 8, 2017:

I'm assuming the problem will be with %d and int64_t. I see you've changed DEFAULT_LISTEN_ONION from a bool to int64_t but did not change it here.

The format for int64_t (to (str)printf etc) is typically

"I64d" (Win32/Win64)
"lld" (32-bit platforms)
"ld" (64-bit platforms)

I suspect int64_t might not be the right type to use for DEFAULT_LISTEN_ONION.

rodentrabies commented at 10:02 AM on April 8, 2017:

Oh, right, that was lame. Thanks for pointing it out.

laanwj commented at 6:41 AM on April 10, 2017:

We don't use the OS' printf function but tinyformat, so you must use %d for any kind of integer. It doesn't understand most of the size specifiers and ignores them as it's typesafe and regards the actual argument types.

rodentrabies force-pushed on Apr 8, 2017

in src/torcontrol.h:14 in 84b8c4d050 outdated

  10 | @@ -11,7 +11,7 @@
  11 |  #include "scheduler.h"
  12 |  
  13 |  extern const std::string DEFAULT_TOR_CONTROL;
  14 | -static const bool DEFAULT_LISTEN_ONION = true;
  15 | +static const std::string DEFAULT_LISTEN_ONION = "0";

laanwj commented at 6:44 AM on April 10, 2017:

String constant cannot be defined reliably in headers. It must be extern and defined in a c++ file. Otherwise it will get included into every single compilation unit that includes this header. Sometimes that runs into linker errors or causes executable bloat at the least.

rodentrabies commented at 1:32 PM on April 10, 2017:

Yes thanks, my bad. It's been a long time without c++ preprocessor.

in src/torcontrol.cpp:400 in 84b8c4d050 outdated

 396 | @@ -395,7 +397,7 @@ class TorController
 397 |      static void reconnect_cb(evutil_socket_t fd, short what, void *arg);
 398 |  };
 399 |  
 400 | -TorController::TorController(struct event_base* _base, const std::string& _target):
 401 | +TorController::TorController(struct event_base* _base, const std::string& _target, const std::string& mode):

laanwj commented at 6:46 AM on April 10, 2017:

Please make this interface take an enum mode, to make the available choices clear in the API and to maintainers of this code. Conversion between the argument and the enum should be done outside this class at the call site (for example TorControlThread())

rodentrabies commented at 1:30 PM on April 10, 2017:

Thought of enum in the first place, but then Ocham's razor principle, don't multiply entities etc:) Anyway, done.

laanwj commented at 7:10 AM on April 10, 2017: member

Interesting discussion in #9836 as to whether this is a good idea.

Support Tor Single Onion Service c7d2c035fa

Document listenonion flag modes 1d25dd7744

rodentrabies force-pushed on Apr 10, 2017

rodentrabies commented at 1:34 PM on April 10, 2017: contributor

@laanwj , I applied changes according to both of your notes, but considering discussion in #9836 , I should probably close this PR. I'll leave the branch anyway, in case we need a more robust and general solution in future.

rodentrabies closed this on Apr 10, 2017

laanwj commented at 2:05 PM on April 10, 2017: member

@mrwhythat Agreed. And sure, makes sense to keep the branch, maybe someone wants to add this patch manually, doesn't hurt to have the code available.

rodentrabies deleted the branch on Mar 15, 2021

DrahtBot locked this on Aug 18, 2022