Add CNetAddr and CService tests demonstrating constructor differences

EthanHeilman commented at 6:04 pm on January 4, 2016: contributor

These tests were written as a result of a discussion in pull request bitcoin/bitcoin#7212 about the behavior of the CNetAddr constructor.

BOOST_CHECK(CNetAddr(“250.2.2.2:7777”).ToString() != CService(“250.2.2.2:7777”).ToString()); BOOST_CHECK(CService(“250.2.2.2:7777”).ToString() == CService(“250.2.2.2”,7777).ToString()); BOOST_CHECK(CNetAddr(“250.2.2.2:8333”).IsIPv6()); BOOST_CHECK(CService(“250.2.2.2:8333”).IsIPv4()); BOOST_CHECK(CNetAddr(“250.2.2.2”, 8333).IsIPv4()); BOOST_CHECK(CService(“250.2.2.2”, 8333).IsIPv4());

Notice that because CNetAddr has no port number, very similar looking constructor inputs have very different effects.

Added some tests on the effect of non-canonical IP inputs such as:

BOOST_CHECK(CNetAddr(“1111111111”).ToString() == “66.58.53.199”);

to lock down and document this behavior.

MarcoFalke commented at 7:07 pm on January 4, 2016: member

Nice! Concept ACK 5609f88

jonasschnelli added the label Tests on Jan 4, 2016

in src/test/netbase_tests.cpp: in 5609f882da outdated

58+    BOOST_CHECK(CNetAddr("1.1.1.1:1").ToString() == "::");
59+    BOOST_CHECK(CNetAddr("2.2.2.2:2").ToString() == "::");
60+    BOOST_CHECK(CNetAddr("2.2.2.2:2").IsIPv6());
61+    BOOST_CHECK(CNetAddr("2.2.2.2:2").IsIPv4() == false);
62+
63+    // Comparing with CNetAddr constructior with CService constructior

jonasschnelli commented at 7:34 pm on January 4, 2016:

s/constructior/constructor

EthanHeilman commented at 8:36 pm on January 4, 2016:

fixed.

jonasschnelli commented at 7:36 pm on January 4, 2016: contributor

Thanks! utACK.

Add CNetAddr and CService tests demonstrating constructor differences

These tests were written as a result of a discussion in pull request
bitcoin/bitcoin#7212 about the behavior of the CNetAddr constructor.

9bafdbcbcf

in src/test/netbase_tests.cpp: in 9bafdbcbcf

50+    BOOST_CHECK(CNetAddr("1111").ToString() == "0.0.4.87");
51+    BOOST_CHECK(CNetAddr("1111111111").ToString() == "66.58.53.199");
52+
53+    // CNetAddr does not support ports making it dangerous to treat like CService
54+    BOOST_CHECK(CNetAddr("250.2.2.2:7777").ToString() != CNetAddr("250.2.2.2", 7777).ToString());
55+    BOOST_CHECK(CNetAddr("250.2.2.2:7777").ToString() == "::");

laanwj commented at 11:28 am on January 5, 2016:

Is this really behavior that we want to write into tests?

I’d prefer adding an ‘IsInvalid’ status to CNetAddr and signal that (akin to CService) if unparseable nonsense is being fed into the constructor, instead of randomly resolving to the IPv6 localhost “::”.

Edit: there is an IsValid on CNetAddr. This would need to return false in these cases, instead of valid nonsense.

EthanHeilman commented at 4:48 pm on January 5, 2016:

I don’t think it should be set in stone and in fact I want to change it, but my view is that having the tests confirm the behavior prior to making the change is valuable. If such a view is not-consistent with the Bitcoin testing philosophy I’m happy to remove these values.

There is an additional reason for these tests beyond just documenting undesirable behavior. Bitcoin uses different IP address parsing libraries for different platforms such as linux and windows. We want to avoid a case in which a change to one library results in differing results between platforms (maybe 1111 -> 1.1.0.1 or 1.0.1.1). These tests ensure that, at least for some of these strange values, we would catch such behavior.

Edit: there is an IsValid on CNetAddr. This would need to return false in these cases, instead of valid nonsense.

I agree and that is a good point. If you don’t mind I might take a look at fixing this behavior over the weekend.

laanwj commented at 8:37 am on January 7, 2016:

Right. There’s something to be said for that, but IMO tests are there to test desired behavior, not just lock in wrong behavior that happens to be the case but has been missed over all this time.

So IMO the right way to progress would be to make tests that check the behavior that we want; e.g.

0BOOST_CHECK(!CNetAddr("1.1.1").IsValid());

Then adapt the implementation to pass these tests.

Bitcoin uses different IP address parsing libraries for different platforms such as linux and windows.

Sure. Though I think verifying that windows and linux have the same weird, broken-ish behavior is a mixed blessing. Luckily we never pass these strings over the P2P network, only binary address blobs, so this is mostly an UI issue.

in src/test/netbase_tests.cpp: in 9bafdbcbcf

44@@ -45,6 +45,30 @@ BOOST_AUTO_TEST_CASE(netbase_properties)
45     BOOST_CHECK(CNetAddr("8.8.8.8").IsRoutable());
46     BOOST_CHECK(CNetAddr("2001::1").IsRoutable());
47     BOOST_CHECK(CNetAddr("127.0.0.1").IsValid());
48+    BOOST_CHECK(CNetAddr("1.1.1").ToString() == "1.1.0.1");

laanwj commented at 11:33 am on January 5, 2016:

Also here: I don’t think this is behavior that should be set into stone. IMO, parsing should be more strict and providing a partial IPv4 address should simply fail (return !IsValid()).

EthanHeilman commented at 5:59 pm on January 5, 2016:

parsing should be more strict and providing a partial IPv4 address should simply fail (return !IsValid()).

Agreed, I want to move on this slowly and carefully to ensure we don’t introduce a crash via IP strings which work now but should fail. Should I open an issue for this?

in src/test/netbase_tests.cpp: in 9bafdbcbcf

64+    BOOST_CHECK(CNetAddr("250.2.2.2").ToString() == CNetAddr("250.2.2.2", 7777).ToString());
65+    BOOST_CHECK(CNetAddr("250.2.2.2:7777").ToString() != CService("250.2.2.2:7777").ToString());
66+    BOOST_CHECK(CService("250.2.2.2:7777").ToString() == CService("250.2.2.2",7777).ToString());
67+
68+    BOOST_CHECK(CNetAddr("250.2.2.2:8333").IsIPv6());
69+    BOOST_CHECK(CService("250.2.2.2:8333").IsIPv4());

dcousens commented at 10:08 pm on January 5, 2016:

Were these meant to be different? (IsIPv6 vs IsIPv4)

EthanHeilman commented at 10:31 pm on January 5, 2016:

Yes. The point of this test that while the constructor strings are identical one creates a IPv4 address and one creates a IPv6 address. This tripped me up and I wanted to document the functionality as this was being discussed in another pull request: #7212

dcousens commented at 10:42 pm on January 5, 2016:

:+1:

MarcoFalke commented at 9:48 pm on January 6, 2016:

@EthanHeilman The question is if someone expects CNetAddr to construct a IPv6 here. If this is just a fallback-behavior, I’d rather not BOOST_CHECK that.

Maybe you could comment those out for now. (The IPv4 stuff looks ok to go into the tests)

dcousens commented at 10:00 pm on January 6, 2016:

@MarcoFalke I see no reason why wouldn’t want to test as much behaviour as possible. If you intend to make any changes, it becomes explicit what is broken and you can just amend the tests or the implementation code, depending on what assumptions changed.

MarcoFalke commented at 11:20 am on January 7, 2016:

@dcousens In case the behaviour is unwanted, we shouldn’t test it. (Tests are worthless if their only goal is to pass and return true. I can remember three cases in the last 4 months where such return true; tests caused us quite some hassle.)

dcousens commented at 1:51 pm on January 7, 2016:

If the existing behaviour is not desired, then the tests should be changed to the desired behaviour and the implementation fail until that is rectified. IMHO it is better to define the behaviour (even if it is undesired) than just leave it up in the air.

YMMV though, and its a bit of a bike shed.

MarcoFalke commented at 1:32 pm on January 9, 2016:

I don’t care too much but at least add a comment to describe why this is tested.

laanwj commented at 3:52 pm on January 9, 2016:

No, I don’t think we should test unwanted behavior. This happens to be the case, but it’s wrong and should be fixed. There have been users confused by this behavior before (at least @gmaxwell) which expected incomplete IP addresses to do something else and were baffled by this. Just failing them would be better.

Tests are not there to fix the code’s behavior in stone, they are there to strategically or thoroughly test that the behavior is correct.

dcousens commented at 3:08 am on January 10, 2016:

@laanwj sure, so lets add tests to test the desired behaviour? [and subsequently fail until fixed]

laanwj commented at 3:05 pm on January 18, 2016:

@dcousens Agreed. The usual idea would be to make tests that test the desired behavior (e.g. stricter parsing, return !IsValid() if there is something clearly invalid), then fix the code so that the tests pass.

dcousens commented at 1:51 pm on January 7, 2016: contributor

utACK 9bafdbc

laanwj commented at 12:27 pm on January 29, 2016: member

See #7435.

laanwj closed this on Feb 3, 2016

MarcoFalke locked this on Sep 8, 2021

Add CNetAddr and CService tests demonstrating constructor differences #7291