[tests] Update Unit Test for addrman.h/addrman.cpp

jimmysong commented at 1:33 am on April 27, 2017: contributor

Add test for adding multiple addresses to address manager Clean up unnecessary modulo operations Add test for GetNewBucket’s alternate method signature

fanquake added the label Tests on Apr 27, 2017

in src/test/addrman_tests.cpp:407 in 9320e02ef4 outdated

404@@ -398,8 +405,8 @@ BOOST_AUTO_TEST_CASE(addrman_getaddr)
405     // Test 25: Ensure GetAddr still returns 23% when addrman has many addrs.
406     for (unsigned int i = 1; i < (8 * 256); i++) {
407         int octet1 = i % 256;

jnewbery commented at 2:41 pm on May 1, 2017:

This code isn’t at all doing what I expected. On first looking, I assumed it was slicing off 8 bits at a time in order to format a 32 bit int as an ipv4 address, ie I was expecting this:

0int octet1 = i % 256;
1int octet2 = i >> 8 % 256;
2int octet3 = i >> 16 % 256;

but in fact it’s effectively doing this:

0int octet1 = i % 256;
1int octet2 = i >> 8 % 256;
2int octet3 = i >> 9 % 256;

ie the bits used for octet 2 and 3 are overlapping. Did you work out whether there was a reason for this?

I’d prefer to keep these using the modulos to indicate that each octet should be in the range 0-255. The only reason your suggested change would work is because i never exceeds 2^16. Perhaps replace with the following if there’s no good reason to keep the strange octet3 behaviour:

0int octet1 = i % 256;
1int octet2 = i >> 8 % 256;
2std::string strAddr = boost::to_string(octet1) + "." + boost::to_string(octet2) + ".0.23";

ie eliminate octet3 entirely.

jimmysong commented at 2:48 pm on May 1, 2017:

I was expecting the same until I tried to refactor it. I can’t speak to the original intent, but @EthanHeilman can maybe give us a clue as to what it was? Anyway it turns out that the addresses are going into a hash table and any adjustment of octet3 in any way changes the numbers in lines 415-417 (vAddr size and addrman size). Thankfully, the hash table is deterministic, but I wanted minimal disruption at least for this PR. I was thinking that this test could be refactored slightly so we can predict the number of collisions in another PR.

EthanHeilman commented at 6:26 pm on May 1, 2017:

@jimmysong It just makes it less likely we will have a collision when adding addresses.

Changing

0int octet3 = (i / (256 * 2)) % 256;

to

0int octet3 = (i / (256 * 256)) % 256;

would cause all of these generated addresses to map to the same source group in the new table as they would all have the same \16.

Current behavior is intended to lower the chance that addresses are evicted when adding them by spreading them across more than one source group. A byproduct of this is also a more realistic test since the addresses returned by GetAddr() are now being drawn from several source groups.

jnewbery commented at 7:17 pm on May 1, 2017:

Can this be achieved by fixing octet2 to zero and then setting octet3 to i >> 8 % 256?

EthanHeilman commented at 8:30 pm on May 1, 2017:

Yes, that would be a better way of doing it.

jnewbery commented at 8:40 pm on May 1, 2017:

Not sure if it’s better, but I think it would be less confusing :)

EthanHeilman commented at 9:16 pm on May 1, 2017:

Less confusing is better. =)

jnewbery commented at 2:42 pm on May 1, 2017: member

ACK, with a question about the modulo behaviour in test 25.

jimmysong force-pushed on May 2, 2017

jimmysong commented at 2:12 am on May 2, 2017: contributor

@EthanHeilman @jnewbery, I’ve tested a few combinations, and leaving out octet3 works for the current code I have. It’s almost exactly the same as before. Let me know what you guys think.

EthanHeilman commented at 2:06 pm on May 2, 2017: contributor

Removing octet3 seems reasonable and the code looks better especially when with the slick BOOST_CHECK_EQUAL.

I’ve been regretting numbering these tests, e.g. “Test 1: , Test: 2 …”, for a while since adding new tests requires renumbering them or using decimals as done here:

0// Test 6.5: AddrMan::Add multiple addresses works as expected

Could you do a commit which just removes the numbers so “Test 1:” becomes “Test: “?

jimmysong commented at 2:09 pm on May 2, 2017: contributor

@EthanHeilman sure, will work on changing to BOOST_CHECK_EQUALS and taking out test numbers as soon as this one gets merged.

jnewbery commented at 3:38 pm on May 2, 2017: member

Could you do a commit which just removes the numbers so “Test 1:” becomes “Test: “?

Alternatively, you could replace the comments with checkpoints: http://www.boost.org/doc/libs/1_64_0/libs/test/doc/html/boost_test/test_output/test_tools_support_for_logging/checkpoints.html . I’m not sure what other people would think about this. There aren’t BOOST_TEST_CHECKPOINTs in the unit tests.

[tests] Update Unit Test for addrman.h/addrman.cpp

Add test for adding multiple addresses to address manager
Clean up unnecessary modulo operations
Add test for GetNewBucket's alternate method signature

ed36de59e4

in src/test/addrman_tests.cpp:408 in 46934c38f3 outdated

404@@ -398,9 +405,8 @@ BOOST_AUTO_TEST_CASE(addrman_getaddr)
405     // Test 25: Ensure GetAddr still returns 23% when addrman has many addrs.
406     for (unsigned int i = 1; i < (8 * 256); i++) {
407         int octet1 = i % 256;
408-        int octet2 = (i / 256) % 256;
409-        int octet3 = (i / (256 * 2)) % 256;
410-        std::string strAddr = boost::to_string(octet1) + "." + boost::to_string(octet2) + "." + boost::to_string(octet3) + ".23";
411+        int octet2 = i / 256;

jnewbery commented at 5:35 pm on May 2, 2017:

nit: I still think this is clearer if you bitshift and modulo 256.

jimmysong force-pushed on May 2, 2017

jimmysong commented at 6:02 pm on May 2, 2017: contributor

nits addressed, squashed and rebased.

jnewbery commented at 7:18 pm on May 2, 2017: member

tested ACK ed36de59e484ecdc22f464948d08a94a8b7fadbb

MarcoFalke merged this on May 9, 2017

MarcoFalke closed this on May 9, 2017

MarcoFalke referenced this in commit 776ba233e9 on May 9, 2017

MarcoFalke commented at 11:04 am on May 9, 2017: member

utACK ed36de59e484ecdc22f464948d08a94a8b7fadbb

jimmysong referenced this in commit 4460a06a12 on May 9, 2017

jimmysong referenced this in commit b81103aad0 on May 9, 2017

jimmysong referenced this in commit ba4baaad22 on May 9, 2017

jimmysong referenced this in commit a80f295666 on May 9, 2017

CryptAxe referenced this in commit c7317ce9c0 on Jun 20, 2017

HashUnlimited referenced this in commit 2dcfaff91e on Mar 2, 2018

PastaPastaPasta referenced this in commit f11f935124 on Jun 10, 2019

PastaPastaPasta referenced this in commit bc05fad751 on Jun 10, 2019

PastaPastaPasta referenced this in commit 73246e3228 on Jun 10, 2019

PastaPastaPasta referenced this in commit a857fa6f23 on Jun 11, 2019

PastaPastaPasta referenced this in commit 1b8fe58043 on Jun 11, 2019

PastaPastaPasta referenced this in commit 63b8448c95 on Jun 15, 2019

PastaPastaPasta referenced this in commit c4e687d5ed on Jun 19, 2019

PastaPastaPasta referenced this in commit 695d6fb18f on Jun 19, 2019

PastaPastaPasta referenced this in commit 8898b61063 on Jun 19, 2019

PastaPastaPasta referenced this in commit de8515cc1e on Jun 19, 2019

PastaPastaPasta referenced this in commit acc513b9c1 on Jun 19, 2019

PastaPastaPasta referenced this in commit 15f7f9c850 on Jun 19, 2019

PastaPastaPasta referenced this in commit a938f996c0 on Jun 19, 2019

PastaPastaPasta referenced this in commit 41a77eb50f on Jun 20, 2019

barrystyle referenced this in commit 6ad955dee3 on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021

[tests] Update Unit Test for addrman.h/addrman.cpp #10287