Bind functional test nodes to 127.0.0.1 #12200

pull Sjors wants to merge 1 commits into bitcoin:master from Sjors:test-framework-bind changing 1 files +1 −0
  1. Sjors commented at 2:07 pm on January 16, 2018: member

    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
    
  2. MarcoFalke added the label MacOSX on Jan 16, 2018
  3. MarcoFalke added the label Tests on Jan 16, 2018
  4. [tests] bind functional test nodes to 127.0.0.1 65682da7e5
  5. in test/functional/test_framework/test_framework.py:390 in 1171a853ab outdated
    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"]
    


    MarcoFalke commented at 2:49 pm on January 16, 2018:
    I’d prefer if you put it into the 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.

    Sjors commented at 3:47 pm on January 16, 2018:
    It’s also shorter. Done.
  6. Sjors force-pushed on Jan 16, 2018
  7. jnewbery commented at 4:55 pm on January 16, 2018: member
    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). I don’t think we should limit what we can test to work around an annoying issue on a single OS.
  8. Sjors commented at 7:38 pm on January 16, 2018: member

    @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.

  9. MarcoFalke commented at 8:45 pm on January 16, 2018: member
    Specifying multiple -bind will not overwrite the previous ones, but rather accumulate and consider all of them.
  10. jnewbery commented at 9:43 pm on January 16, 2018: member

    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.

  11. Sjors commented at 9:19 am on January 17, 2018: member
    Given that multiple -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?
  12. Sjors commented at 5:28 pm on January 17, 2018: member
    Note sure why Travis is upset. The summary says that machine 2 failed after 6 hours and machine 8 never run, but both logs show Done. Your build exited with 0.
  13. jnewbery commented at 6:37 pm on January 17, 2018: member

    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).

  14. Sjors commented at 9:21 am on January 18, 2018: member

    @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)

  15. laanwj commented at 9:18 am on January 19, 2018: member

    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.

  16. Sjors commented at 10:28 am on January 19, 2018: member
    Related question: is it possible to hide network interfaces from a process? That way we could launch bitcoind without -bind=127.0.0.1 but achieve the same effect.
  17. laanwj commented at 10:32 am on January 19, 2018: member

    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?

  18. randolf commented at 11:56 pm on January 30, 2018: contributor
    Has IPv6 loopback of ::1 been considered in this?
  19. laanwj commented at 9:08 am on January 31, 2018: member

    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.

  20. laanwj commented at 9:54 am on February 13, 2018: member
    utACK 95e2e9af124595aae4801fc9813ee1c294d404cd
  21. laanwj merged this on Feb 15, 2018
  22. laanwj closed this on Feb 15, 2018

  23. laanwj referenced this in commit d09968f4d0 on Feb 15, 2018
  24. Sjors deleted the branch on Feb 15, 2018
  25. jnewbery commented at 8:46 pm on February 16, 2018: member

    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?

  26. MarcoFalke referenced this in commit 27c59dc502 on Feb 16, 2018
  27. Sjors commented at 8:57 am on February 17, 2018: member
    I’ll see if I can figure out how to test IPv6 stuff and make a new PR. My guess is that it needs to bind on ::1 as well.
  28. Sjors commented at 9:00 am on February 17, 2018: member
    There’s some recent discussion about how to use IPv6 on Travis: https://github.com/travis-ci/travis-ci/issues/8891
  29. laanwj commented at 2:16 pm on February 22, 2018: member
    I completely forgot about the rpc_bind test, even though I wrote it. I think we should include it in default test_runner (could disable it specifically for travis with a flag, but have it run locally!) or remove it.
  30. laanwj commented at 2:32 pm on February 22, 2018: member

    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?

  31. laanwj referenced this in commit 0f7167989d on Mar 7, 2018
  32. PastaPastaPasta referenced this in commit 3075ac2776 on Jul 9, 2020
  33. PastaPastaPasta referenced this in commit 773a16daa3 on Jul 9, 2020
  34. xdustinface referenced this in commit 488c8e5753 on Jul 9, 2020
  35. DrahtBot locked this on Sep 8, 2021

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-11-17 09:12 UTC

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