Prevents OSX firewall allow-this-application-to-accept-inbound-connections permission popups and is generally safer.
To test, make an arbitrary whitespace change to src/bitcoind.cpp and recompile. This normally resets the firewall’s memory.
Easiest way to reproduce a popup without running the test suite:
0src/bitcoind -regtest -bind=127.0.0.1 # No popup
1src/bitcoind -regtest # Popup
386@@ -387,7 +387,7 @@ def _initialize_chain(self):
387 # Create cache directories, run bitcoinds:
388 for i in range(MAX_NODES):
389 datadir = initialize_datadir(self.options.cachedir, i)
390- args = [os.getenv("BITCOIND", "bitcoind"), "-server", "-keypool=1", "-datadir=" + datadir, "-discover=0"]
391+ args = [os.getenv("BITCOIND", "bitcoind"), "-server", "-keypool=1", "-datadir=" + datadir, "-discover=0", "-bind=127.0.0.1"]
bitcoin.conf file (via initialize_datadir). This way, the option is valid also when nodes are (later) spun up for debugging purposes from outside the test framework.
@jnewbery couldn’t those specific tests just launch with a different -bind argument? Then we can worry about that issue later when those tests are ready. Those tests would likely break on OSX if the developer doesn’t click on Allow quickly enough, so they would be brittle.
The good news is that the entire 127.0.0.1/8 range can be used for these tests without having to open a developer machine to (admittedly unlikely) attack from the wide internet if they’re not behind a router.
-bind will not overwrite the previous ones, but rather accumulate and consider all of them.
Then we can worry about that issue later when those tests are ready.
yes, true - which is why I’m only vaguely concept NACKish. I just would rather not be at a point in the future where we can’t remove this option for some reason (eg because it causes tests to fail on OS X), and it makes adding tests with multiple interfaces more difficult.
-bind allows us to add interfaces and given that you can bind to 127.0.0.* without upsetting the OSX firewall, would what alleviate your objection?
Done. Your build exited with 0.
would that alleviate your objection?
Yes. My other concern is that all functional tests will now be running with the same -bind option (and so we’re no longer testing nodes running without the -bind option).
@laanwj suggested on IRC that linux allows you to simulate network interfaces. That might be a more elegant* way to test what happens without -bind (that and using a different bitcoin.conf for those tests).
* = in the sense that in some imaginary ideal world tests shouldn’t rely on real world things like actual network interfaces that the test machine may or may not have (though of course Travis uses VM’s so … Inception)
Concept ACK.
Vaguely concept NACK-ish. I could imagine we’d want to test bitcoind’s ability to accept incoming connections on multiple interfaces (in fact it’s something I’ve considered adding to the test suite in the past).
Even though only one platform complains, I think the warning is fair. From a security point of view it’s better to not listen on any public interfaces during testing.
Also the tests cannot assume the presence of any interfaces besides loopback (and they don’t - I checked this by running in an environment with just lo).
This doesn’t exclude doing specific (multi interface) bind tests in a separate, platform-specific test as @Sjors proposes. This could use Linux network namespaces ip netns add to simulate a small LAN, for example. But there’s no reason to do so in tests that e.g. test the wallet.
bitcoind without -bind=127.0.0.1 but achieve the same effect.
That’s the simplest application of net namespaces:
0ip netns add isolate
1ip netns exec isolate ip addr add 127.0.0.1/8 dev lo
2ip netns exec isolate ip link set lo up
3ip netns exec isolate <testsuite>
4ip netns delete isolate
That doesn’t help on OSX though.
Edit: which reminds me, as 127.0.0.1/8 is a /8, we could do multi-interface testing with just the loopback interface, binding to 127.0.0.2 127.0.0.3 etc?
Has IPv6 loopback of ::1 been considered in this?
I’d be curious to hear how you think that would help here - though I think the chance of IPv4 loopback being present on test systems is slightly higher. I remember Travis had issues with their VMs not supporting IPv6 (not even locally!) a few years ago.
This breaks rpc_bind.py for me:
0 ./rpc_bind.py
12018-02-16 20:39:32.628000 TestFramework (INFO): Initializing test directory /tmp/user/1000/testi0z5j5wf
22018-02-16 20:39:32.628000 TestFramework (INFO): Using interface 10.0.2.15 for testing
32018-02-16 20:39:32.629000 TestFramework (INFO): Bind test for []
4Error: Cannot set -bind or -whitebind together with -listen=0
52018-02-16 20:39:32.881000 TestFramework (ERROR): Assertion failed
6Traceback (most recent call last):
7 File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 125, in main
8 self.run_test()
9 File "./rpc_bind.py", line 79, in run_test
10 [('127.0.0.1', defaultport), ('::1', defaultport)])
11 File "./rpc_bind.py", line 35, in run_bind_test
12 self.start_node(0, base_args + binds)
13 File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 234, in start_node
14 node.wait_for_rpc_connection()
15 File "/home/ubuntu/bitcoin/test/functional/test_framework/test_node.py", line 99, in wait_for_rpc_connection
16 assert self.process.poll() is None, "bitcoind exited with status %i during initialization" % self.process.returncode
17AssertionError: bitcoind exited with status 1 during initialization
182018-02-16 20:39:32.882000 TestFramework (INFO): Stopping nodes
19Traceback (most recent call last):
20 File "./rpc_bind.py", line 107, in <module>
21 RPCBindTest().main()
22 File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 148, in main
23 self.stop_nodes()
24 File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 268, in stop_nodes
25 node.stop_node()
26 File "/home/ubuntu/bitcoin/test/functional/test_framework/test_node.py", line 135, in stop_node
27 self.stop()
28 File "/home/ubuntu/bitcoin/test/functional/test_framework/test_node.py", line 81, in __getattr__
29 assert self.rpc_connected and self.rpc is not None, "Error: no RPC connection"
30AssertionError: Error: no RPC connection
Dropping this single commit fixes the test.
Travis never runs rpc_bind.py, even in the cron job, since it requires ipv6. I suspect this will be broken for most people.
It doesn’t look like this was tested before merge. Can we revert please?
::1 as well.
Ehh we have test_ipv6_local() in netutil.py (used by feature_proxy.py). Skipping the parts that require ipv6 binding when there is no ipv6 would be another possiblity and avoid any travis specific magic.
Currently it skips the whole test when ipv6 is not available, which seems OK as well. Was lack of IPv6 support the whole reason for excluding this test from the default runner?