New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds unittests for CAddrMan and CAddrinfo, removes source of non-determinism. #7212

Merged
merged 1 commit into from Jan 28, 2016

Conversation

Projects
None yet
5 participants
@EthanHeilman
Contributor

EthanHeilman commented Dec 14, 2015

  • Adds unittests for several methods in CAddrMan which are not currently tested.
  • Creates unittests for CAddrinfo which had no test coverage. These tests validate security critical features.
  • Refactors addrman so that tests can overrides the random number generator to allow for deterministic tests of CAddrman.select that involve tests of more than one addr in tried or new. See justification below.
  • We move the MakeDeterministic method from addrman into a test class to reduce lines of code in addrman and to prevent non-test calls.

Details on GetRandInt Wrapper

To allow for deterministic tests of the select method in CAddrMan we wrap the GetRandInt function with a method RandomInt which can be overridden in a test class CAddrManTest. The RandomInt wrapper name was used so that it would not be a prefix substring of GetRandInt to avoid find and replace mistakes. Changes to random number generators as always a concern, this approach presents no risks because only subclasses of addrman can redirect calls to GetRandInt.

IPv4 IPv6 confusion

Bitcoin addr constructors assume any IP string that contains a semicolon is a IPv6 address. Thus
CService("250.1.2.1:9999") will create an IPv6 address with a default port number 8333
. I have discovered this behavior only applies to CNetAddr constructors since CNetAddr does not take a port number.

@instagibbs

View changes

Show outdated Hide outdated src/addrman.h
@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Dec 14, 2015

Contributor

Can you please reduce the number of changes in the commit? It is a bit unreadable now because of the amount of unnecessary changes.

Contributor

paveljanik commented Dec 14, 2015

Can you please reduce the number of changes in the commit? It is a bit unreadable now because of the amount of unnecessary changes.

@EthanHeilman

This comment has been minimized.

Show comment
Hide comment
@EthanHeilman

EthanHeilman Dec 14, 2015

Contributor

@paveljanik Almost none of these changes are unnecessary, they significantly increase the readability, accuracy and the coverage of these tests (as explained in prior comments). The fact that it appears the changes should not have an impact when in fact that do, is evidence against leaving the tests as is.

Update: Since the last push deleted both of our comments, I am rewritten my explanation in the pull request description.

Contributor

EthanHeilman commented Dec 14, 2015

@paveljanik Almost none of these changes are unnecessary, they significantly increase the readability, accuracy and the coverage of these tests (as explained in prior comments). The fact that it appears the changes should not have an impact when in fact that do, is evidence against leaving the tests as is.

Update: Since the last push deleted both of our comments, I am rewritten my explanation in the pull request description.

@@ -62,16 +95,16 @@ BOOST_AUTO_TEST_CASE(addrman_ports)
// Set addrman addr placement to be deterministic.
addrman.MakeDeterministic();
CNetAddr source = CNetAddr("252.2.2.2:8333");

This comment has been minimized.

@MarcoFalke

MarcoFalke Dec 15, 2015

Member

If this is interpreted IPv6 by http://man7.org/linux/man-pages/man3/inet_pton.3.html , is there any code in production that needs update as well?

@MarcoFalke

MarcoFalke Dec 15, 2015

Member

If this is interpreted IPv6 by http://man7.org/linux/man-pages/man3/inet_pton.3.html , is there any code in production that needs update as well?

This comment has been minimized.

@EthanHeilman

EthanHeilman Dec 15, 2015

Contributor

I did a quick look and didn't find anything other than this:

https://github.com/bitcoin/bitcoin/blob/master/src/netbase.cpp#L622

but it wouldn't surprise me if there was more. I planned on performing a more complete search, especially of the network deserialization, code after I finished these unittests. I'll probably start that tonight.

I'm also considering a mitigation strategy of putting asserts in the constructor since no IP string should contain both "." and ":". There is logic which already does this:

https://github.com/bitcoin/bitcoin/blob/master/src/netbase.cpp#L68

In using asserts I want to be careful not make it worse by allowing crashes. It might be better to have an IP treated as IPv6 than to have a situation where a remote user can crash a bitcoind node. Let me think about it more, but I believe there is a better and less risky strategy than asserts.

Before anyone spends anymore time on this let me write up a test which confirms the behavior and we can continue the discussion on that pull request.

@EthanHeilman

EthanHeilman Dec 15, 2015

Contributor

I did a quick look and didn't find anything other than this:

https://github.com/bitcoin/bitcoin/blob/master/src/netbase.cpp#L622

but it wouldn't surprise me if there was more. I planned on performing a more complete search, especially of the network deserialization, code after I finished these unittests. I'll probably start that tonight.

I'm also considering a mitigation strategy of putting asserts in the constructor since no IP string should contain both "." and ":". There is logic which already does this:

https://github.com/bitcoin/bitcoin/blob/master/src/netbase.cpp#L68

In using asserts I want to be careful not make it worse by allowing crashes. It might be better to have an IP treated as IPv6 than to have a situation where a remote user can crash a bitcoind node. Let me think about it more, but I believe there is a better and less risky strategy than asserts.

Before anyone spends anymore time on this let me write up a test which confirms the behavior and we can continue the discussion on that pull request.

This comment has been minimized.

@MarcoFalke

MarcoFalke Dec 15, 2015

Member

Before anyone spends anymore time on this let me write up a test which confirms the behavior and we can continue the discussion on that pull request.

Agree, thanks for looking into this.

@MarcoFalke

MarcoFalke Dec 15, 2015

Member

Before anyone spends anymore time on this let me write up a test which confirms the behavior and we can continue the discussion on that pull request.

Agree, thanks for looking into this.

This comment has been minimized.

@EthanHeilman

EthanHeilman Dec 15, 2015

Contributor

The following tests all resolve to true:

BOOST_CHECK(CNetAddr("250.2.2.2:8333").IsIPv6());
BOOST_CHECK(CNetAddr("250.2.2.2", 8333).IsIPv4());
BOOST_CHECK(CService("250.2.2.2:8333").IsIPv4());
BOOST_CHECK(CAddress(CService("250.2.2.2:8333")).IsIPv4());

This confusion arises because CNetAddr does not actually take a port number. I have a branch here that shows this behavior.

EthanHeilman@3f5c440

This behavior does not carry across into CService. Given these problems I need to write my commit. Thanks for your help @paveljanik @MarcoFalke

@EthanHeilman

EthanHeilman Dec 15, 2015

Contributor

The following tests all resolve to true:

BOOST_CHECK(CNetAddr("250.2.2.2:8333").IsIPv6());
BOOST_CHECK(CNetAddr("250.2.2.2", 8333).IsIPv4());
BOOST_CHECK(CService("250.2.2.2:8333").IsIPv4());
BOOST_CHECK(CAddress(CService("250.2.2.2:8333")).IsIPv4());

This confusion arises because CNetAddr does not actually take a port number. I have a branch here that shows this behavior.

EthanHeilman@3f5c440

This behavior does not carry across into CService. Given these problems I need to write my commit. Thanks for your help @paveljanik @MarcoFalke

@EthanHeilman

This comment has been minimized.

Show comment
Hide comment
@EthanHeilman

EthanHeilman Dec 15, 2015

Contributor

I have rewritten my commit to so that CNetAddr no longer takes a port number.

I have keep my changes where I break the port out when using the CService constructor. That is:

CService("250.2.2.2:8333") changed to CService("250.2.2.2", 8333)

@paveljanik is correct that these changes are unnecessary, however given that the fix for the CNetAddr issue involves overhauling the tests anyways lets make them more readable as well. That being said, I am willing to change them back if anyone feels strongly about it.

Contributor

EthanHeilman commented Dec 15, 2015

I have rewritten my commit to so that CNetAddr no longer takes a port number.

I have keep my changes where I break the port out when using the CService constructor. That is:

CService("250.2.2.2:8333") changed to CService("250.2.2.2", 8333)

@paveljanik is correct that these changes are unnecessary, however given that the fix for the CNetAddr issue involves overhauling the tests anyways lets make them more readable as well. That being said, I am willing to change them back if anyone feels strongly about it.

@EthanHeilman

This comment has been minimized.

Show comment
Hide comment
@EthanHeilman

EthanHeilman Dec 16, 2015

Contributor

Can someone label/tag this as 'test'?

Contributor

EthanHeilman commented Dec 16, 2015

Can someone label/tag this as 'test'?

@laanwj laanwj added the Tests label Dec 17, 2015

BOOST_CHECK(addrman.size() == 0);
for (unsigned int i = 1; i < 4; i++){
CService addr = CService("250.1.1."+boost::to_string(i));
for (unsigned int i = 1; i < 18; i++) {

This comment has been minimized.

@instagibbs

instagibbs Dec 18, 2015

Member

You probably already explained this, but what's the reasoning behind these number changes? Would be nice to have it on record.

@instagibbs

instagibbs Dec 18, 2015

Member

You probably already explained this, but what's the reasoning behind these number changes? Would be nice to have it on record.

This comment has been minimized.

@EthanHeilman

EthanHeilman Dec 18, 2015

Contributor

The test is testing if collisions are handled property, thus we need a collision to occur. However the round at which a collision occurs is dependent on the input values. One of the input values is a CNetAddr, so fixing the issue with CNetAddr changed the round in which a collision occurs from the 18th round rather than the 4th round.

@EthanHeilman

EthanHeilman Dec 18, 2015

Contributor

The test is testing if collisions are handled property, thus we need a collision to occur. However the round at which a collision occurs is dependent on the input values. One of the input values is a CNetAddr, so fixing the issue with CNetAddr changed the round in which a collision occurs from the 18th round rather than the 4th round.

EthanHeilman added a commit to EthanHeilman/bitcoin that referenced this pull request Jan 4, 2016

Add CNetAddr and CService tests demonstrating constructor differences
These tests were written as a result of a discussion in pull request
bitcoin#7212 about the behavior of the CNetAddr constructor.
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 5, 2016

Member

Concept ACK

Member

laanwj commented Jan 5, 2016

Concept ACK

Increase test coverage for addrman and addrinfo
Adds several unittests for CAddrMan and CAddrInfo.
Increases the accuracy of addrman tests.
Removes non-determinism in tests by overriding the random number generator.
Extracts testing code from addrman class to test class.
@EthanHeilman

This comment has been minimized.

Show comment
Hide comment
@EthanHeilman

EthanHeilman Jan 27, 2016

Contributor

Added a test for GetAddr.

@laanwj Are there any changes I need to make to get this accepted with the next ~30 days? I have some bug fixes which I'm writing right now which I'm pushing in early March/Late Feb. The fixes build on these tests, but I'd like to do them in a separate pull request since its pretty different from this pull request.

Contributor

EthanHeilman commented Jan 27, 2016

Added a test for GetAddr.

@laanwj Are there any changes I need to make to get this accepted with the next ~30 days? I have some bug fixes which I'm writing right now which I'm pushing in early March/Late Feb. The fixes build on these tests, but I'd like to do them in a separate pull request since its pretty different from this pull request.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 28, 2016

Member

utACK now that the tests verifying the flaky behavior of CNetAddr with a port number or otherwise invalid addresses have been removed.

This issue still needs to be fixed - at the side of CNetAddr parsing - but this makes this ready for merge IMO.

Edit: oops, had been confusing your issues, that is #7291

Member

laanwj commented Jan 28, 2016

utACK now that the tests verifying the flaky behavior of CNetAddr with a port number or otherwise invalid addresses have been removed.

This issue still needs to be fixed - at the side of CNetAddr parsing - but this makes this ready for merge IMO.

Edit: oops, had been confusing your issues, that is #7291

@laanwj laanwj merged commit 40c87b6 into bitcoin:master Jan 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 28, 2016

Merge #7212: Adds unittests for CAddrMan and CAddrinfo, removes sourc…
…e of non-determinism.

40c87b6 Increase test coverage for addrman and addrinfo (Ethan Heilman)
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 28, 2016

Member

Post merge utACK

Member

MarcoFalke commented Jan 28, 2016

Post merge utACK

kyuupichan referenced this pull request in kyuupichan/BitcoinUnlimited Mar 5, 2017

Merge #7212: Adds unittests for CAddrMan and CAddrinfo, removes sourc…
…e of non-determinism.

40c87b6 Increase test coverage for addrman and addrinfo (Ethan Heilman)

sickpig referenced this pull request in sickpig/BitcoinUnlimited Mar 31, 2017

Merge #7212: Adds unittests for CAddrMan and CAddrinfo, removes sourc…
…e of non-determinism.

40c87b6 Increase test coverage for addrman and addrinfo (Ethan Heilman)

@str4d str4d referenced this pull request Jul 13, 2017

Open

Bitcoin Core 0.12.0 #2074

191 of 452 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment