From 8bdd9240d4310aafa1332159355f106a8fcfc5c9 Mon Sep 17 00:00:00 2001 From: josibake Date: Tue, 14 Dec 2021 11:26:55 +0100 Subject: [PATCH 1/4] refactor: addrman_selecttriedcollisions test Check `Good()` directly when adding addresses. Previously, test would check `size()`, which is incorrect. Check that duplicates are also handled by checking the output from `SelectTriedCollision()` when `Good()` returns false. --- src/test/addrman_tests.cpp | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index b700c3ae22..133345001f 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -816,19 +816,21 @@ BOOST_AUTO_TEST_CASE(addrman_selecttriedcollision) for (unsigned int i = 1; i < 23; i++) { CService addr = ResolveService("250.1.1." + ToString(i)); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); - addrman.Good(addr); - // No collisions yet. - BOOST_CHECK(addrman.size() == i); + // No collisions in tried. + BOOST_CHECK(addrman.Good(addr)); BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } // Ensure Good handles duplicates well. + // If an address is a duplicate, Good will return false but will not count it as a collision. for (unsigned int i = 1; i < 23; i++) { CService addr = ResolveService("250.1.1." + ToString(i)); - addrman.Good(addr); - BOOST_CHECK(addrman.size() == 22); + // Unable to add duplicate address to tried table. + BOOST_CHECK(!addrman.Good(addr)); + + // Verify duplicate address not marked as a collision. BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } } From e281fccd8a80d7cd48c3b17d58fd4a8915e1e965 Mon Sep 17 00:00:00 2001 From: josibake Date: Tue, 14 Dec 2021 12:05:33 +0100 Subject: [PATCH 2/4] refactor: addrman_noevict test Check the response from `Good()` wherever it is called. Previously, the test was using `size()` (incorrect for checking tried) and `SelectTriedCollision()` to determine if a collision happened. --- src/test/addrman_tests.cpp | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 133345001f..c93b13168e 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -844,22 +844,20 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) for (unsigned int i = 1; i < 36; i++) { CService addr = ResolveService("250.1.1." + ToString(i)); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); - addrman.Good(addr); // No collision yet. - BOOST_CHECK(addrman.size() == i); - BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); + BOOST_CHECK(addrman.Good(addr)); } - // Collision between 36 and 19. + // Collision in tried table between 36 and 19. CService addr36 = ResolveService("250.1.1.36"); BOOST_CHECK(addrman.Add({CAddress(addr36, NODE_NONE)}, source)); - addrman.Good(addr36); - - BOOST_CHECK(addrman.size() == 36); + BOOST_CHECK(!addrman.Good(addr36)); BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.19:0"); // 36 should be discarded and 19 not evicted. + // This means we keep 19 in the tried table and + // 36 stays in the new table. addrman.ResolveCollisions(); BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); @@ -867,26 +865,24 @@ BOOST_AUTO_TEST_CASE(addrman_noevict) for (unsigned int i = 37; i < 59; i++) { CService addr = ResolveService("250.1.1." + ToString(i)); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); - addrman.Good(addr); - - BOOST_CHECK(addrman.size() == i); - BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); + BOOST_CHECK(addrman.Good(addr)); } - // Cause a collision. + // Cause a collision in the tried table. CService addr59 = ResolveService("250.1.1.59"); BOOST_CHECK(addrman.Add({CAddress(addr59, NODE_NONE)}, source)); - addrman.Good(addr59); - BOOST_CHECK(addrman.size() == 59); + BOOST_CHECK(!addrman.Good(addr59)); BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.10:0"); - // Cause a second collision. + // Cause a second collision in the new table. BOOST_CHECK(!addrman.Add({CAddress(addr36, NODE_NONE)}, source)); - addrman.Good(addr36); - BOOST_CHECK(addrman.size() == 59); + // 36 still cannot be moved from new to tried due to colliding with 19 + BOOST_CHECK(!addrman.Good(addr36)); BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() != "[::]:0"); + + // Resolve all collisions. addrman.ResolveCollisions(); BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); } From 5a64dc018c04ce16202a8e58ce92d2657c0b1806 Mon Sep 17 00:00:00 2001 From: josibake Date: Tue, 14 Dec 2021 12:51:06 +0100 Subject: [PATCH 3/4] refactor: addrman_evictionworks test Test for collisions and duplicates directly with `Good()`. If an entry to tried is a duplicate, `Good()` will return false but `SelectTriedCollision()` will be empty (assuming there were no prior collisions). If there is a collision, `Good()` will retun false and `SelectTriedCollision()` will return a value. --- src/test/addrman_tests.cpp | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index c93b13168e..226c4d6eec 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -901,19 +901,16 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) for (unsigned int i = 1; i < 36; i++) { CService addr = ResolveService("250.1.1." + ToString(i)); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); - addrman.Good(addr); // No collision yet. - BOOST_CHECK(addrman.size() == i); - BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); + BOOST_CHECK(addrman.Good(addr)); } // Collision between 36 and 19. CService addr = ResolveService("250.1.1.36"); BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source)); - addrman.Good(addr); + BOOST_CHECK(!addrman.Good(addr)); - BOOST_CHECK_EQUAL(addrman.size(), 36); auto info = addrman.SelectTriedCollision().first; BOOST_CHECK_EQUAL(info.ToString(), "250.1.1.19:0"); @@ -924,17 +921,17 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) addrman.ResolveCollisions(); BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); - // If 36 was swapped for 19, then this should cause no collisions. - BOOST_CHECK(!addrman.Add({CAddress(addr, NODE_NONE)}, source)); - addrman.Good(addr); - + // If 36 was swapped for 19, then adding 36 to tried should fail because we + // are attempting to add a duplicate. + // We check this by verifying Good() returns false and also verifying that + // we have no collisions. + BOOST_CHECK(!addrman.Good(addr)); BOOST_CHECK(addrman.SelectTriedCollision().first.ToString() == "[::]:0"); - // If we insert 19 it should collide with 36 + // 19 should fail as a collision (not a duplicate) if we now attempt to move + // it to the tried table. CService addr19 = ResolveService("250.1.1.19"); - BOOST_CHECK(!addrman.Add({CAddress(addr19, NODE_NONE)}, source)); - addrman.Good(addr19); - + BOOST_CHECK(!addrman.Good(addr19)); BOOST_CHECK_EQUAL(addrman.SelectTriedCollision().first.ToString(), "250.1.1.36:0"); addrman.ResolveCollisions(); From bf4f8171352e9b384b42c91da61dfc9c3ca89ed8 Mon Sep 17 00:00:00 2001 From: josibake Date: Tue, 14 Dec 2021 12:55:50 +0100 Subject: [PATCH 4/4] refactor: addrman_select test Check that `Good()` is successful whenever it is called. --- src/test/addrman_tests.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 226c4d6eec..cecc4c12e7 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -194,7 +194,7 @@ BOOST_AUTO_TEST_CASE(addrman_select) BOOST_CHECK_EQUAL(addr_ret1.ToString(), "250.1.1.1:8333"); // Test: move addr to tried, select from new expected nothing returned. - addrman.Good(CAddress(addr1, NODE_NONE)); + BOOST_CHECK(addrman.Good(CAddress(addr1, NODE_NONE))); BOOST_CHECK_EQUAL(addrman.size(), 1U); auto addr_ret2 = addrman.Select(newOnly).first; BOOST_CHECK_EQUAL(addr_ret2.ToString(), "[::]:0"); @@ -220,11 +220,11 @@ BOOST_AUTO_TEST_CASE(addrman_select) CService addr7 = ResolveService("250.4.6.6", 8333); BOOST_CHECK(addrman.Add({CAddress(addr5, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); - addrman.Good(CAddress(addr5, NODE_NONE)); + BOOST_CHECK(addrman.Good(CAddress(addr5, NODE_NONE))); BOOST_CHECK(addrman.Add({CAddress(addr6, NODE_NONE)}, ResolveService("250.3.1.1", 8333))); - addrman.Good(CAddress(addr6, NODE_NONE)); + BOOST_CHECK(addrman.Good(CAddress(addr6, NODE_NONE))); BOOST_CHECK(addrman.Add({CAddress(addr7, NODE_NONE)}, ResolveService("250.1.1.3", 8333))); - addrman.Good(CAddress(addr7, NODE_NONE)); + BOOST_CHECK(addrman.Good(CAddress(addr7, NODE_NONE))); // Test: 6 addrs + 1 addr from last test = 7. BOOST_CHECK_EQUAL(addrman.size(), 7U);