test: addrman: tried 3 times and never a success so isTerrible=true #30445

1. 
   brunoerg commented at 5:11 pm on July 12, 2024: contributor

   This PR adds test coverage for the following verification:

   0if (TicksSinceEpoch<std::chrono::seconds>(m_last_success) == 0 && nAttempts >= ADDRMAN_RETRIES) { // tried N times and never a success
   1    return true;
   2}
   

   If we’ve tried an address for 3 or more times and were unsuccessful, this address should be pointed out as “terrible”.

   ---

   You can test this by applying:

    0diff --git a/src/addrman.cpp b/src/addrman.cpp
    1index 054a9bee32..93a9521b59 100644
    2--- a/src/addrman.cpp
    3+++ b/src/addrman.cpp
    4@@ -81,7 +81,7 @@ bool AddrInfo::IsTerrible(NodeSeconds now) const
    5     }
    6 
    7     if (TicksSinceEpoch<std::chrono::seconds>(m_last_success) == 0 && nAttempts >= ADDRMAN_RETRIES) { // tried N times and never a success
    8-        return true;
    9+        return false;
   10     }
   

2. 
   DrahtBot commented at 5:11 pm on July 12, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30445.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	jonatack, naumenkogs
   Concept ACK	naiyoma

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. DrahtBot added the label Tests on Jul 12, 2024
4. in src/test/addrman_tests.cpp:446 in ba2cf750f4 outdated

   442@@ -443,10 +443,19 @@ BOOST_AUTO_TEST_CASE(getaddr_unfiltered)
   443     CNetAddr source = ResolveIP("250.1.2.1");
   444     BOOST_CHECK(addrman->Add({addr1, addr2}, source));
   445 
   446+    // Tried 3 times and never a success so isTerrible = true
   

   ---

   ---

   

   naumenkogs commented at 7:00 am on September 12, 2024:

   Could you expand the comment similar to what’s above? “The time is set, but after 3 attempts…”. Otherwise ACK :)

   ---

   

   brunoerg commented at 6:46 pm on September 12, 2024:

   Done!
5. brunoerg force-pushed on Sep 12, 2024
6. 
   brunoerg commented at 6:47 pm on September 12, 2024: contributor

   Force-pushed addressing #30445 (review)
7. naumenkogs approved
8. 
   naumenkogs commented at 7:51 am on September 13, 2024: member

   ACK d6fabbe2d40b53f6fa35d6b0191f3d73999d6b41
9. 
   naiyoma commented at 3:55 pm on September 28, 2024: contributor

   Concept ACK: additional test coverage to check that an address is marked as “terrible” after 3 or more unsuccessful connection attempts.
10. DrahtBot added the label CI failed on Oct 19, 2024
11. DrahtBot removed the label CI failed on Oct 23, 2024
12. in src/test/addrman_tests.cpp:454 in d6fabbe2d4 outdated

    449+    addr3.nTime = Now<NodeSeconds>();
    450+    addrman->Good(addr3, /*time=*/Now<NodeSeconds>());
    451+    BOOST_CHECK(addrman->Add({addr3}, source));
    452+    // The time is set, but after 3 unsuccessful attempts this addr should be isTerrible = true
    453+    for (size_t i = 0; i < 3; ++i) {
    454+        addrman->Attempt(addr3, /*fCountFailure=*/true, /*time=*/Now<NodeSeconds>() - 10h);
    

    ---

    ---

    

    jonatack commented at 4:38 pm on November 8, 2024:

    The time setting here seems to be critical and could be commented. It looks like the threshold between success and failure is between 60s and 61s. Is that the case for you as well, and could you explain here why? (I apologize, but it’s not clear to me.)

    ---

    

    brunoerg commented at 6:10 pm on November 18, 2024:

    Note that IsTerrible has other verifications (e.g. we don’t remove things tried in the last minute). So the timing setting here is to explicity reach exactly what we want to test but could be any other value. Every time we call Attempt, m_last_try is updated. So, yes, you’re right about the threshold. Worth adding a comment?

    ---

    

    jonatack commented at 8:22 pm on November 19, 2024:

    Oh indeed (thanks!) I think the following would be clearer (with 61s, or maybe 2min)

     0@@ -453,12 +453,13 @@ BOOST_AUTO_TEST_CASE(getaddr_unfiltered)
     1     addr3.nTime = Now<NodeSeconds>();
     2     addrman->Good(addr3, /*time=*/Now<NodeSeconds>());
     3     BOOST_CHECK(addrman->Add({addr3}, source));
     4-    // The time is set, but after 3 unsuccessful attempts this addr should be isTerrible = true
     5+    // The time is set, but after ADDRMAN_RETRIES unsuccessful attempts not
     6+    // retried in the last minute, this addr should be isTerrible = true
     7     for (size_t i = 0; i < 3; ++i) {
     8-        addrman->Attempt(addr3, /*fCountFailure=*/true, /*time=*/Now<NodeSeconds>() - 10h);
     9+        addrman->Attempt(addr3, /*fCountFailure=*/true, /*time=*/Now<NodeSeconds>() - 61s);
    10     }
    11 
    12-    // Filtered GetAddr should only return addr1
    13+    // GetAddr filtered by quality (i.e. not IsTerrible) should only return addr1
    14     BOOST_CHECK_EQUAL(addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt).size(), 1U);
    15     // Unfiltered GetAddr should return all addrs
    16     BOOST_CHECK_EQUAL(addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt, /*filtered=*/false).size(), 3U);
    

    ---

    

    brunoerg commented at 8:38 pm on November 19, 2024:

    Done, thanks.
13. 
    jonatack commented at 4:41 pm on November 8, 2024: member

    ACK d6fabbe2d40b53f6fa35d6b0191f3d73999d6b41

    Modulo mentioning and clarifying the time aspect in the test (see comment below) and the PR description.

14. test: addrman: tried 3 times and never a success so `isTerrible=true` 1807df3d9f
15. brunoerg force-pushed on Nov 19, 2024
16. 
    brunoerg commented at 8:38 pm on November 19, 2024: contributor

    Force-pushed addressing #30445 (review)
17. 
    jonatack commented at 9:05 pm on November 19, 2024: member

    re-ACK 1807df3d9fb0135057a33e01173080ea14b0403d
18. DrahtBot requested review from naumenkogs on Nov 19, 2024
19. 
    naumenkogs commented at 6:30 am on November 20, 2024: member

    ACK 1807df3d9fb0135057a33e01173080ea14b0403d
20. 
    brunoerg commented at 1:38 pm on December 2, 2024: contributor

    rfm?
21. 
    achow101 commented at 8:26 pm on December 4, 2024: member

    ACK 1807df3d9fb0135057a33e01173080ea14b0403d
22. achow101 merged this on Dec 4, 2024
23. achow101 closed this on Dec 4, 2024

    ---

Contributors
brunoerg DrahtBot naumenkogs naiyoma jonatack achow101

Review Requested
naumenkogs

Labels
Tests