refactor: Remove redundant addrman time checks #25303

1. 
   maflcko commented at 7:16 am on June 8, 2022: member

   Split out from #24697 because it makes sense on its own.
2. Remove redundant nTime checks

   nTime is always initialized on deserialization or default-initialized
   with TIME_INIT, so special casing 0 does not make sense.

   00001e57fe
3. maflcko force-pushed on Jun 8, 2022
4. DrahtBot added the label P2P on Jun 8, 2022
5. DrahtBot added the label Refactoring on Jun 8, 2022
6. fanquake requested review from mzumsande on Jun 8, 2022
7. fanquake requested review from dergoegge on Jun 8, 2022
8. fanquake requested review from naumenkogs on Jun 8, 2022
9. in src/addrman.cpp:69 in fa8bdb7505 outdated

   65@@ -66,14 +66,16 @@ int AddrInfo::GetBucketPosition(const uint256& nKey, bool fNew, int nBucket) con
   66 
   67 bool AddrInfo::IsTerrible(int64_t nNow) const
   68 {
   69-    if (nLastTry && nLastTry >= nNow - 60) // never remove things tried in the last minute
   70+    if (nNow - nLastTry <= 60) { // never remove things tried in the last minute
   

   ---

   ---

   

   dergoegge commented at 11:23 am on June 8, 2022:

   mega nit: fa8bdb7505e12afee4bc21e08ca17c36aaf7edcf calls it m_last_try but the member is still called nLastTry.

   ---

   

   maflcko commented at 11:36 am on June 8, 2022:

   Thx, fixed.

   ---

   

   Riahiamirreza commented at 11:58 am on June 8, 2022:

   What is the purpose of checking the nLastTry in the previous version? Can it be null or something with false value? IF yes, why you don’t check it in the current version? (sorry if it’s a trivial question, it is my first time reviewing code)

   ---

   

   maflcko commented at 12:15 pm on June 9, 2022:

   @Riahiamirreza nLastTry is a plain integral value, so it can’t be null or false. At most it can be 0, but it seems odd to treat 0 (seconds) differently, from say 1 (seconds). nNow - 0 <= 60 or nNow - 1 <= 60 should be false either way.
10. 
    dergoegge commented at 11:33 am on June 8, 2022: member

    Code review ACK fa8bdb7505e12afee4bc21e08ca17c36aaf7edcf

    CI failure seems unrelated.

11. Remove redundant nLastTry check

    All other places calculate "now - nLastTry", which is safe and correct
    to do when nLastTry is 0. So do the same here.

    8888bd43c1
12. maflcko force-pushed on Jun 8, 2022
13. 
    DrahtBot commented at 3:10 pm on June 8, 2022: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #24697 (refactor address relay time by MarcoFalke)
    • #23829 (refactor: use braced init for integer literals instead of c style casts by PastaPastaPasta)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

14. 
    dergoegge commented at 3:35 pm on June 8, 2022: member

    re-ACK 8888bd43c100f9f0ca1122fcc896fb7b999d61c6
15. 
    naumenkogs commented at 11:05 am on June 9, 2022: member

    utACK 8888bd43c100f9f0ca1122fcc896fb7b999d61c6
16. fanquake merged this on Jun 9, 2022
17. fanquake closed this on Jun 9, 2022

    ---

18. maflcko deleted the branch on Jun 9, 2022
19. 
    mzumsande commented at 1:58 pm on June 9, 2022: contributor

    Post-merge utACK https://github.com/bitcoin/bitcoin/commit/8888bd43c100f9f0ca1122fcc896fb7b999d61c6
20. sidhujag referenced this in commit 56db887052 on Jun 13, 2022
21. Fabcien referenced this in commit 4030b963f1 on Jan 5, 2024
22. PastaPastaPasta referenced this in commit 24bce63db1 on Jan 17, 2024
23. PastaPastaPasta referenced this in commit 11b2595b8f on Jan 19, 2024
24. PastaPastaPasta referenced this in commit b57482b510 on Jan 19, 2024
25. bitcoin locked this on Apr 19, 2024

Contributors
maflcko dergoegge Riahiamirreza DrahtBot naumenkogs mzumsande

Review Requested
naumenkogs mzumsande

Labels
Refactoring P2P