Skip the parts that cannot be run on the host due to lack of IPv6 support or a second interface to bind on, and warn appropriately.
Without no strong requirements (besides being Linux only, which will skip the test) left, add this test to the default in test_runner.
(the non-IPv6 parts of the two dual-IPv4/6 tests could also be enabled, but first going to look what Travis does here to see if there wasn't another reason it was disabled) done, it only makes sense for the first
110 | - # Check that with invalid rpcallowip, we are denied 111 | - self.run_allowip_test([non_loopback_ip], non_loopback_ip, defaultport) 112 | - assert_raises_rpc_error(-342, "non-JSON HTTP response with '403 Forbidden' from server", self.run_allowip_test, ['1.1.1.1'], non_loopback_ip, defaultport) 113 | + # Check that with invalid rpcallowip, we are denied 114 | + self.run_allowip_test([non_loopback_ip], non_loopback_ip, defaultport) 115 | + assert_raises_rpc_error(-342, "non-JSON HTTP response with '403 Forbidden' from server", self.run_allowip_test, ['1.1.1.1'], non_loopback_ip, defaultport)
The allowip test could also be done by connecting from a second loopback IP, say, 127.0.0.2. This would obliterate the need for a non loopback IP, but would need support in the test framework at the RPC/HTTP level for binding to a different address before outgoing connection (connecting to a different interface IP does that part automatically...)
Ughh, I tried this but it doesn't work. Apparently the entire IPv4 subnet is unconditionally allowed.
rpc_allow_subnets.push_back(CSubNet(localv4, 8)); // always allow IPv4 local subnet
(It works when removing the , 8 - the full changes are here: https://github.com/laanwj/bitcoin/commit/0dad20fe2f9ef3c053b007704f46fd1f0e26fc5f)
(It would also work for IPv6, as it only binds on a single localhost IP there. But that won't help for Travis)
This is a linux-specific test though, so in this test we can asume /8 and don't care about other IP stack implementations.
utACK db9fd32284c8ad5b7f3b11efc0716324a5abd147
Issuing a warning when IPv6 is not present is a good idea in my opinion because all modern hosts really should have proper support for both IPv4 and IPv6 in these modern times.
Vendors have had at least 15 years to add IPv6 support to their IP networking stacks: https://www.ipv6.com/general/ipv6-the-history-and-timeline/
I think there should be some indication in the test_runner summary that the full test has not been run. A printed warning log is not enough, since the user will have no indication that the test hasn't been fully executed.
One way we could do this is to add options to the rpc_bind.py test. See https://github.com/jnewbery/bitcoin/commit/2361e82258675d52612675904565b15694fabed3 for example.
Needs rebase if still relevant
Well the thought here was that running part of the test is better than not running it at all. Indeed, adding options for ipv4/ipv6 testing would be an option, But I feel we're overcomplicating this, this is also what made me unsure about this change.
Vendors have had at least 15 years to add IPv6 support to their IP networking stacks: https://www.ipv6.com/general/ipv6-the-history-and-timeline/
I agree - simply requiring localhost IPv6 support to run the test suite wouldn't be that extreme at this point. It'd be a much smaller change than this.
If we can rely on Travis (finally) having IPv6 then I'm all for that.
Rebased and added a squasshme commit that skips the entire test if no IPv6 localhost. Probably should do the same for non_loopback_ip. But let's see how this goes on travis.
So apparently travis still has no IPv6 support. Disappointing. This causes the test to be skipped on travis for now. That's ok, at least it will still run by default locally.
Changes look good. Will ACK once the commits are squashed.
I think there's still value to running a subset of the tests when ipv6/non-loopback interfaces are not available, as long as it's clearly communicated to the user that the full test has not been run.
I still think something like https://github.com/jnewbery/bitcoin/commit/2361e82258675d52612675904565b15694fabed3 could be useful in a future PR, so at least Travis is running a subset of this test.
I still think something like jnewbery/bitcoin@2361e82 could be useful in a future PR, so at least Travis is running a subset of this test.
Oh that's a good idea, I didn't think of splitting up the test into parts and still adding all the sub-parts to the default-run tests (so that they'll run locally) - will take that over.
Skip the parts that cannot be run on the host due to lack
of IPv6 support or a second interface to bind on, and warn
appropriately.
Without no strong requirements (besides being Linux only, in which case
the test is skipped) left, just add this test to the default in
test_runner.
Includes suggested changes by John Newbery.
post-merge tested ACK e87fefc60fc0f648b5e26aa716481e79a85f04de.
Sorry for not rereviewing earlier - feel free to prod me if I've promised a review and not delivered!