allow listening on -bind=address for blocked networks #1778

Diapolo commented at 2:02 PM on September 3, 2012: none

I had a talk with @sipa yesterday about this on IRC, this is the patch for our discussion.

this allows the client to listen on via -bind specified addresses (e.g. 127.0.0.1), even when a network (IPv4 in that case) was blocked via e.g -onlynet="Tor"
introduce enum BindFlags to avoid passing multiple bools to Bind()
make -bind help text clear we ALWAYS listen on the specified address
remove an unused variable
usage case: specify -bind=127.0.0.1 -onlynet="Tor" to allow incoming connections to a Tor hidden service, but still don't allow other IPv4 nodes to connect / get connected

BitcoinPullTester commented at 6:46 AM on September 5, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/0f5cc2e93ab83df1963ceacd999c416554f17e28 for binaries and test log.

in src/init.cpp:None in 0f5cc2e93a outdated

 563 |              struct in_addr inaddr_any;
 564 |              inaddr_any.s_addr = INADDR_ANY;
 565 |  #ifdef USE_IPV6
 566 |              if (!IsLimited(NET_IPV6))
 567 | -                fBound |= Bind(CService(in6addr_any, GetListenPort()), false);
 568 | +                fBound |= Bind(CService(in6addr_any, GetListenPort()), false, false);

laanwj commented at 7:27 AM on September 5, 2012:

Passing multiple booleans to a function results in fairly hard-to-read code (which argument is what, again?). Not meant as a showstopper, but normally I'd suggest using enums. Or maybe add a comment before each, like,

fBound |= Bind(CService(in6addr_any, GetListenPort()), /*fExplicitBind*/ false, /*fError*/ false);

Diapolo commented at 8:36 AM on September 5, 2012: none

Updated to reflect @laanwj suggestion to not pass multiple bools to Bind().

BitcoinPullTester commented at 10:30 AM on September 7, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/02038f265abf535742cb33043cb2edc2905b9f11 for binaries and test log.

laanwj commented at 8:59 AM on September 9, 2012: member

ACK for 0.8

Diapolo commented at 5:24 PM on September 20, 2012: none

@sipa Any reason left to not merge from your point of view?

jgarzik commented at 1:11 AM on September 27, 2012: contributor

ACK, if others see a need

Diapolo commented at 6:42 AM on September 28, 2012: none

I'm a little sad that @sipa does not comment, as we had the initial discussion. I still think this patch is valuable.

sipa commented at 10:01 AM on September 28, 2012: member

@Diapolo sorry, I've been very busy the past few days - I'll have a look soon.

in src/init.cpp:None in 02038f265a outdated

  25 | @@ -26,6 +26,13 @@
  26 |  CWallet* pwalletMain;
  27 |  CClientUIInterface uiInterface;
  28 |  
  29 | +// Used to pass flags to the Bind() function
  30 | +enum BindFlags {

sipa commented at 2:36 PM on September 28, 2012:

If "explicit" and "report error" are independent flags, and "none" is the base state, why does none need a bit?

Diapolo commented at 2:40 PM on September 28, 2012:

I have no answer, for that ^^ so you propose to use 0, 1 and 2 instead :)?

sipa commented at 2:50 PM on September 28, 2012:

Yes.

I know it's nit picking, but this way it seems to indicate that "none" is also a flag, independent from the other two.

Diapolo commented at 3:08 PM on September 28, 2012:

I like getting nit picked sometimes, at least people read the code and help making better code :). Oh and updated, it's now 0, 1, 2.

BitcoinPullTester commented at 2:21 AM on October 1, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/71e1ea9fcde90fefac4ee131eaa3c522589cee5c for binaries and test log.

BitcoinPullTester commented at 3:29 PM on October 21, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b1b1323ca11e5214a3d4c6f26b4344e1177ca77b for binaries and test log.

sipa commented at 11:38 PM on October 28, 2012: member

ACK

Diapolo commented at 10:48 AM on November 4, 2012: none

Anything left to do here then?

allow listening on -bind=address for blocked networks

- this allows the client to listen on via -bind specified addresses
  (e.g. 127.0.0.1), even when a network (IPv4 in that case) was blocked
  via e.g -onlynet="Tor"
- introduce enum BindFlags to avoid passing multiple bools to Bind()
- make -bind help text clear we ALWAYS listen on the specified address
- remove an unused variable
- remove 2 unneeded IsLimited() checks before calling Bind(), which does
  these checks anyway

- usage case: specify -bind=127.0.0.1 -onlynet="Tor" to allow incoming
  connections to a Tor hidden service, but still don't allow other IPv4
  nodes to connect / get connected

c73323eec9

in src/init.cpp:None in b1b1323ca1 outdated

 569 |              }
 570 |          } else {
 571 |              struct in_addr inaddr_any;
 572 |              inaddr_any.s_addr = INADDR_ANY;
 573 |  #ifdef USE_IPV6
 574 |              if (!IsLimited(NET_IPV6))

sipa commented at 10:50 AM on November 4, 2012:

Is this conditional even needed, as Bind() checks IsLimited itself?

Diapolo commented at 2:56 PM on November 4, 2012:

Seems redundant and safe to remove, as Bind() is doing that check on inaddr_any / in6addr_any, as you pointed out.

Edit: Updated!

Diapolo commented at 11:31 PM on November 9, 2012: none

Rebased to fix merge-conflict.

sipa referenced this in commit 20db1c099e on Nov 9, 2012

sipa merged this on Nov 9, 2012

sipa closed this on Nov 9, 2012

laudney referenced this in commit dae5a37312 on Mar 19, 2014

owlhooter referenced this in commit 0291604ad0 on Oct 10, 2018

DrahtBot locked this on Sep 8, 2021