EthanHeilman commented at 4:01 pm on December 14, 2015: contributor

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.

in src/addrman.h: in 0d66bbaa13 outdated

233@@ -235,6 +234,8 @@ class CAddrMan
234     //! Select an address to connect to, if newOnly is set to true, only the new table is selected from.
235     CAddrInfo Select_(bool newOnly);
236 
237+    virtual int RandomInt(int nMax);

instagibbs commented at 4:05 pm on December 14, 2015:

Needs a short description

EthanHeilman commented at 6:24 pm on December 14, 2015:

Good call! I have added a comment to RandomInt.

paveljanik commented at 7:33 pm on December 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.

EthanHeilman commented at 11:12 pm on December 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.

in src/test/addrman_tests.cpp: in 278646d5bb outdated

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

MarcoFalke commented at 8:49 am on December 15, 2015:

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?

EthanHeilman commented at 4:05 pm on December 15, 2015:

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.

MarcoFalke commented at 4:07 pm on December 15, 2015:

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.

EthanHeilman commented at 5:35 pm on December 15, 2015:

The following tests all resolve to true:

0BOOST_CHECK(CNetAddr("250.2.2.2:8333").IsIPv6());
1BOOST_CHECK(CNetAddr("250.2.2.2", 8333).IsIPv4());
2BOOST_CHECK(CService("250.2.2.2:8333").IsIPv4());
3BOOST_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.

https://github.com/EthanHeilman/bitcoin/commit/3f5c44080814b33257416b42a3ef941d726174fd

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

EthanHeilman commented at 10:23 pm on December 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.

EthanHeilman commented at 7:23 pm on December 16, 2015: contributor

Can someone label/tag this as ’test'?

laanwj added the label Tests on Dec 17, 2015

in src/test/addrman_tests.cpp: in 8e05d6e81f outdated

198 
199     BOOST_CHECK(addrman.size() == 0);
200 
201-    for (unsigned int i = 1; i < 4; i++){
202-        CService addr = CService("250.1.1."+boost::to_string(i));
203+    for (unsigned int i = 1; i < 18; i++) {

instagibbs commented at 6:31 pm on December 18, 2015:

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

EthanHeilman commented at 7:33 pm on December 18, 2015:

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.

laanwj commented at 12:51 pm on January 5, 2016: member

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.

40c87b6e69

EthanHeilman commented at 5:13 pm on January 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.

laanwj commented at 10:51 am on January 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

laanwj merged this on Jan 28, 2016

laanwj closed this on Jan 28, 2016

laanwj referenced this in commit 326ffed09b on Jan 28, 2016

MarcoFalke commented at 6:40 pm on January 28, 2016: member

Post merge utACK

sickpig referenced this in commit dfa74a6da3 on Mar 31, 2017

MarcoFalke locked this on Sep 8, 2021

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

Details on GetRandInt Wrapper

IPv4 IPv6 confusion