[tests] Update Unit Test for addrman.h/addrman.cpp #10287
pull jimmysong wants to merge 1 commits into bitcoin:master from jimmysong:test_addrman changing 1 files +17 −9-
jimmysong commented at 1:33 am on April 27, 2017: contributorAdd 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 toi >> 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: memberACK, with a question about the modulo behaviour in test 25.jimmysong force-pushed on May 2, 2017jimmysong 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: contributorRemoving 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: memberCould 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_CHECKPOINT
s 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
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, 2017jimmysong commented at 6:02 pm on May 2, 2017: contributornits addressed, squashed and rebased.jnewbery commented at 7:18 pm on May 2, 2017: membertested ACK ed36de59e484ecdc22f464948d08a94a8b7fadbbMarcoFalke merged this on May 9, 2017MarcoFalke closed this on May 9, 2017
MarcoFalke referenced this in commit 776ba233e9 on May 9, 2017MarcoFalke commented at 11:04 am on May 9, 2017: memberutACK ed36de59e484ecdc22f464948d08a94a8b7fadbbjimmysong referenced this in commit 4460a06a12 on May 9, 2017jimmysong referenced this in commit b81103aad0 on May 9, 2017jimmysong referenced this in commit ba4baaad22 on May 9, 2017jimmysong referenced this in commit a80f295666 on May 9, 2017CryptAxe referenced this in commit c7317ce9c0 on Jun 20, 2017HashUnlimited referenced this in commit 2dcfaff91e on Mar 2, 2018PastaPastaPasta referenced this in commit f11f935124 on Jun 10, 2019PastaPastaPasta referenced this in commit bc05fad751 on Jun 10, 2019PastaPastaPasta referenced this in commit 73246e3228 on Jun 10, 2019PastaPastaPasta referenced this in commit a857fa6f23 on Jun 11, 2019PastaPastaPasta referenced this in commit 1b8fe58043 on Jun 11, 2019PastaPastaPasta referenced this in commit 63b8448c95 on Jun 15, 2019PastaPastaPasta referenced this in commit c4e687d5ed on Jun 19, 2019PastaPastaPasta referenced this in commit 695d6fb18f on Jun 19, 2019PastaPastaPasta referenced this in commit 8898b61063 on Jun 19, 2019PastaPastaPasta referenced this in commit de8515cc1e on Jun 19, 2019PastaPastaPasta referenced this in commit acc513b9c1 on Jun 19, 2019PastaPastaPasta referenced this in commit 15f7f9c850 on Jun 19, 2019PastaPastaPasta referenced this in commit a938f996c0 on Jun 19, 2019PastaPastaPasta referenced this in commit 41a77eb50f on Jun 20, 2019barrystyle referenced this in commit 6ad955dee3 on Jan 22, 2020DrahtBot locked this on Sep 8, 2021
jimmysong jnewbery EthanHeilman MarcoFalkeLabels
Tests
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 09:12 UTC
More mirrored repositories can be found on mirror.b10c.me