Add CNetAddr and CService tests demonstrating constructor differences #7291
Conversation
|
Nice! Concept ACK 5609f88 |
| BOOST_CHECK(CNetAddr("2.2.2.2:2").IsIPv6()); | ||
| BOOST_CHECK(CNetAddr("2.2.2.2:2").IsIPv4() == false); | ||
|
|
||
| // Comparing with CNetAddr constructior with CService constructior |
There was a problem hiding this comment.
s/constructior/constructor
|
Thanks! |
These tests were written as a result of a discussion in pull request #7212 about the behavior of the CNetAddr constructor.
|
|
||
| // CNetAddr does not support ports making it dangerous to treat like CService | ||
| BOOST_CHECK(CNetAddr("250.2.2.2:7777").ToString() != CNetAddr("250.2.2.2", 7777).ToString()); | ||
| BOOST_CHECK(CNetAddr("250.2.2.2:7777").ToString() == "::"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
BOOST_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.
|
utACK 9bafdbc |
|
See #7435. |
These tests were written as a result of a discussion in pull request #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.