refactor: make AddrMan::Good return bool

josibake · josibake · commit 207f1c825c63 · 2021-12-14T17:50:24.000+01:00
If AddrMan::Good is unable to add an entry
to tried (for a number of reasons), return false.

This makes it much easier and cleaner to directly
test for tried collisions. It also allows anyone
calling Good() to handle the case where adding an
address to tried is unsuccessful.

Update docs to doxygen style.
diff --git a/src/addrman.cpp b/src/addrman.cpp
@@ -614,7 +614,7 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_
     return fInsert;
 }
 
-void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime)
+bool AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime)
 {
     AssertLockHeld(cs);
 
@@ -625,8 +625,7 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
     AddrInfo* pinfo = Find(addr, &nId);
 
     // if not found, bail out
-    if (!pinfo)
-        return;
+    if (!pinfo) return false;
 
     AddrInfo& info = *pinfo;
 
@@ -638,13 +637,11 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
     // currently-connected peers.
 
     // if it is already in the tried set, don't do anything else
-    if (info.fInTried)
-        return;
+    if (info.fInTried) return false;
 
     // if it is not in new, something bad happened
-    if (!Assume(info.nRefCount > 0)) {
-        return;
-    }
+    if (!Assume(info.nRefCount > 0)) return false;
+
 
     // which tried bucket to move the entry to
     int tried_bucket = info.GetTriedBucket(nKey, m_asmap);
@@ -661,11 +658,13 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT
                  colliding_entry != mapInfo.end() ? colliding_entry->second.ToString() : "",
                  addr.ToString(),
                  m_tried_collisions.size());
+        return false;
     } else {
         // move nId to the tried tables
         MakeTried(info, nId);
         LogPrint(BCLog::ADDRMAN, "Moved %s mapped to AS%i to tried[%i][%i]\n",
                  addr.ToString(), addr.GetMappedAS(m_asmap), tried_bucket, tried_bucket_pos);
+        return true;
     }
 }
 
@@ -1049,12 +1048,13 @@ bool AddrManImpl::Add(const std::vector<CAddress>& vAddr, const CNetAddr& source
     return ret;
 }
 
-void AddrManImpl::Good(const CService& addr, int64_t nTime)
+bool AddrManImpl::Good(const CService& addr, int64_t nTime)
 {
     LOCK(cs);
     Check();
-    Good_(addr, /* test_before_evict */ true, nTime);
+    auto ret = Good_(addr, /*test_before_evict=*/true, nTime);
     Check();
+    return ret;
 }
 
 void AddrManImpl::Attempt(const CService& addr, bool fCountFailure, int64_t nTime)
@@ -1157,9 +1157,9 @@ bool AddrMan::Add(const std::vector<CAddress>& vAddr, const CNetAddr& source, in
     return m_impl->Add(vAddr, source, nTimePenalty);
 }
 
-void AddrMan::Good(const CService& addr, int64_t nTime)
+bool AddrMan::Good(const CService& addr, int64_t nTime)
 {
-    m_impl->Good(addr, nTime);
+    return m_impl->Good(addr, nTime);
 }
 
 void AddrMan::Attempt(const CService& addr, bool fCountFailure, int64_t nTime)
diff --git a/src/addrman.h b/src/addrman.h
@@ -81,8 +81,14 @@ class AddrMan
      * @return    true if at least one address is successfully added. */
     bool Add(const std::vector<CAddress>& vAddr, const CNetAddr& source, int64_t nTimePenalty = 0);
 
-    //! Mark an entry as accessible, possibly moving it from "new" to "tried".
-    void Good(const CService& addr, int64_t nTime = GetAdjustedTime());
+    /**
+     * Mark an address record as accessible and attempt to move it to addrman's tried table.
+     *
+     * @param[in] addr            Address record to attempt to move to tried table.
+     * @param[in] nTime           The time that we were last connected to this peer.
+     * @return    true if the address is successfully moved from the new table to the tried table.
+     */
+    bool Good(const CService& addr, int64_t nTime = GetAdjustedTime());
 
     //! Mark an entry as connection attempted to.
     void Attempt(const CService& addr, bool fCountFailure, int64_t nTime = GetAdjustedTime());
diff --git a/src/addrman_impl.h b/src/addrman_impl.h
@@ -115,7 +115,7 @@ class AddrManImpl
     bool Add(const std::vector<CAddress>& vAddr, const CNetAddr& source, int64_t nTimePenalty)
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    void Good(const CService& addr, int64_t nTime)
+    bool Good(const CService& addr, int64_t nTime)
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
     void Attempt(const CService& addr, bool fCountFailure, int64_t nTime)
@@ -248,7 +248,7 @@ class AddrManImpl
      *  @see AddrMan::Add() for parameters. */
     bool AddSingle(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
-    void Good_(const CService& addr, bool test_before_evict, int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    bool Good_(const CService& addr, bool test_before_evict, int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     bool Add_(const std::vector<CAddress> &vAddr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
