highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-06-04 02:02:42 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#23713: refactor, test: refactor addrman_tried_collisions test to directly check for collisions

Browse Source 
caac999ff0 refactor: remove dependence on AddrManTest (josibake)
f961c477b5 refactor: check Good() in tried_collisions test (josibake)
207f1c825c refactor: make AddrMan::Good return bool (josibake)

Pull request description:

  Previously, the `addrman_tried_collisions` test behaved in the following way:

  1. add an address to addrman
  2. attempt to move the new address to the tried table (using `AddrMan.Good()`)
  3. verify that `num_addrs` matched `size()` to check for collisions in the new table

  `AddrMan.size()`, however, returns the number of unique address in addrman, regardless of whether they are in new or tried. This means the test would still pass for addresses where a collision did occur in the tried table. After 3 collisions in the tried table, there would eventually be a collision in the new table when trying to add a new address, which was then detected by checking `num_addrs - collisions == size()`.

  While the collision in the new table was caused by a collision in the tried table, the test is misleading as it's not directly testing for collisions in the tried table and misses 3 collisions before identifying a collision in the new table.

  ### solution

  To more directly test the tried table, I refactored `AddrMan::Good()` to return a boolean after successfully adding an address to the tried table. This makes the test much cleaner by first adding an address to new, calling `Good` to move it to the tried table, and checking if it was successful or not. It is worth noting there are other reasons, aside from collisions, which will cause `Good` to return false. That being said, this is an improvement over the previous testing methodology.

  Additionally, having `Good()` return a boolean is useful outside of testing as it allows the caller to handle the case where `Good` is unable to move the entry to the tried table (e.g a063647413/src/rpc/net.cpp (L945)).

  ### followup
  As a follow up to this PR, I plan to look at the following places `Good()` is called and see if it makes sense to handle the case where it is unable to add an entry to tried:

  * a063647413/src/rpc/net.cpp (L945)
  * a063647413/src/net.cpp (L2067)
  * a063647413/src/net_processing.cpp (L2708)

ACKs for top commit:
  jnewbery:
    utACK caac999ff0
  mzumsande:
    Code review ACK caac999ff0

Tree-SHA512: f328896b1f095e8d2581fcdbddce46fc0491731a0440c6fff01081fa5696cfb896dbbe1d183eda2c100f19aa111e1f8b096ef93582197edc6b791de563a58f99
This commit is contained in:
MarcoFalke
2021-12-15 09:53:00 +01:00
parent 9015d11842 caac999ff0
commit 40fdb9ece9
4 changed files with 35 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
src/test/addrman_tests.cpp
View File

@@ -268,32 +268,32 @@ BOOST_AUTO_TEST_CASE(addrman_new_collisions)
BOOST_AUTO_TEST_CASE(addrman_tried_collisions)
{
AddrManTest addrman;
auto addrman = std::make_unique<AddrMan>(std::vector<bool>(), /*deterministic=*/true, /*consistency_check_ratio=*/100);
CNetAddr source = ResolveIP("252.2.2.2");
uint32_t num_addrs{0};
BOOST_CHECK_EQUAL(addrman.size(), num_addrs);
BOOST_CHECK_EQUAL(addrman->size(), num_addrs);
while (num_addrs < 64) { // Magic number! 250.1.1.1 - 250.1.1.64 do not collide with deterministic key = 1
while (num_addrs < 35) { // Magic number! 250.1.1.1 - 250.1.1.35 do not collide in tried with deterministic key = 1
CService addr = ResolveService("250.1.1." + ToString(++num_addrs));
BOOST_CHECK(addrman.Add({CAddress(addr, NODE_NONE)}, source));
addrman.Good(CAddress(addr, NODE_NONE));
BOOST_CHECK(addrman->Add({CAddress(addr, NODE_NONE)}, source));
// Test: Add to tried without collision
BOOST_CHECK(addrman->Good(CAddress(addr, NODE_NONE)));
// Test: No collision in tried table yet.
BOOST_CHECK_EQUAL(addrman.size(), num_addrs);
}
// Test: tried table collision!
// Test: Unable to add to tried table due to collision!
CService addr1 = ResolveService("250.1.1." + ToString(++num_addrs));
uint32_t collisions{1};
BOOST_CHECK(!addrman.Add({CAddress(addr1, NODE_NONE)}, source));
BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions);
BOOST_CHECK(addrman->Add({CAddress(addr1, NODE_NONE)}, source));
BOOST_CHECK(!addrman->Good(CAddress(addr1, NODE_NONE)));
// Test: Add the next address to tried without collision
CService addr2 = ResolveService("250.1.1." + ToString(++num_addrs));
BOOST_CHECK(addrman.Add({CAddress(addr2, NODE_NONE)}, source));
BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions);
BOOST_CHECK(addrman->Add({CAddress(addr2, NODE_NONE)}, source));
BOOST_CHECK(addrman->Good(CAddress(addr2, NODE_NONE)));
}
BOOST_AUTO_TEST_CASE(addrman_find)