Adds unittests for CAddrMan and CAddrinfo, removes source of non-determinism. #7212
Conversation
src/addrman.h
Outdated
There was a problem hiding this comment.
Good call! I have added a comment to RandomInt.
|
Can you please reduce the number of changes in the commit? It is a bit unreadable now because of the amount of unnecessary changes. |
|
@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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I did a quick look and didn't find anything other than this:
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:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This behavior does not carry across into CService. Given these problems I need to write my commit. Thanks for your help @paveljanik @MarcoFalke
|
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. |
|
Can someone label/tag this as 'test'? |
There was a problem hiding this comment.
You probably already explained this, but what's the reasoning behind these number changes? Would be nice to have it on record.
There was a problem hiding this comment.
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.
|
Concept ACK |
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.
|
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. |
|
utACK
Edit: oops, had been confusing your issues, that is #7291 |
…e of non-determinism. 40c87b6 Increase test coverage for addrman and addrinfo (Ethan Heilman)
|
Post merge utACK |
…e of non-determinism. 40c87b6 Increase test coverage for addrman and addrinfo (Ethan Heilman)
5 participants
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. I have discovered this behavior only applies to CNetAddr constructors since CNetAddr does not take a port number.CService("250.1.2.1:9999") will create an IPv6 address with a default port number 8333