Skip to content

Commit

Permalink
Merge #23380: addrman: Fix AddrMan::Add() return semantics and logging
Browse files Browse the repository at this point in the history 
61ec053 [MOVEONLY] reorder functions in addrman_impl.h and addrman.cpp (John Newbery)
2095df7 [addrman] Add Add_() inner function, fix Add() return semantics (John Newbery)
2658eb6 [addrman] Rename Add_() to AddSingle() (John Newbery)
e58598e [addrman] Add doxygen comment to AddrMan::Add() (John Newbery)

Pull request description:

  Previously, Add() would return true if the function created a new
  AddressInfo object, even if that object could not be successfully
  entered into the new table and was deleted. That would happen if the new
  table position was already taken and the existing entry could not be
  removed.

  Instead, return true if the new AddressInfo object is successfully
  entered into the new table. This fixes a bug in the "Added %i addresses"
  log, which would not always accurately log how many addresses had been
  added.

ACKs for top commit:
  naumenkogs:
    ACK 61ec053
  mzumsande:
    ACK 61ec053
  shaavan:
    ACK 61ec053

Tree-SHA512: 276f1e8297d4b6d411d05d06ffc7c176f6290a784da039926ab6c471a8ed8e9159ab4f56c893b1285737ae292954930f0d28012d89dfb3f2f825d7df41016feb
  • Loading branch information
@fanquake
fanquake committed Nov 1, 2021
2 parents 7efc628 + 61ec053 commit 994aaaa
Show file tree
Hide file tree
Showing 6 changed files with 87 additions and 74 deletions.
139 changes: 72 additions & 67 deletions src/addrman.cpp
View file
Expand Up @@ -537,69 +537,13 @@ void AddrManImpl::MakeTried(AddrInfo& info, int nId)
info.fInTried = true;
}

void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime)
{
AssertLockHeld(cs);

int nId;

nLastGood = nTime;

AddrInfo* pinfo = Find(addr, &nId);

// if not found, bail out
if (!pinfo)
return;

AddrInfo& info = *pinfo;

// update info
info.nLastSuccess = nTime;
info.nLastTry = nTime;
info.nAttempts = 0;
// nTime is not updated here, to avoid leaking information about
// currently-connected peers.

// if it is already in the tried set, don't do anything else
if (info.fInTried)
return;

// if it is not in new, something bad happened
if (!Assume(info.nRefCount > 0)) {
return;
}

// which tried bucket to move the entry to
int tried_bucket = info.GetTriedBucket(nKey, m_asmap);
int tried_bucket_pos = info.GetBucketPosition(nKey, false, tried_bucket);

// Will moving this address into tried evict another entry?
if (test_before_evict && (vvTried[tried_bucket][tried_bucket_pos] != -1)) {
if (m_tried_collisions.size() < ADDRMAN_SET_TRIED_COLLISION_SIZE) {
m_tried_collisions.insert(nId);
}
// Output the entry we'd be colliding with, for debugging purposes
auto colliding_entry = mapInfo.find(vvTried[tried_bucket][tried_bucket_pos]);
LogPrint(BCLog::ADDRMAN, "Collision with %s while attempting to move %s to tried table. Collisions=%d\n",
colliding_entry != mapInfo.end() ? colliding_entry->second.ToString() : "",
addr.ToString(),
m_tried_collisions.size());
} 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);
}
}

bool AddrManImpl::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty)
bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty)
{
AssertLockHeld(cs);

if (!addr.IsRoutable())
return false;

bool fNew = false;
int nId;
AddrInfo* pinfo = Find(addr, &nId);

Expand Down Expand Up @@ -640,13 +584,12 @@ bool AddrManImpl::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTi
pinfo = Create(addr, source, &nId);
pinfo->nTime = std::max((int64_t)0, (int64_t)pinfo->nTime - nTimePenalty);
nNew++;
fNew = true;
}

int nUBucket = pinfo->GetNewBucket(nKey, source, m_asmap);
int nUBucketPos = pinfo->GetBucketPosition(nKey, true, nUBucket);
bool fInsert = vvNew[nUBucket][nUBucketPos] == -1;
if (vvNew[nUBucket][nUBucketPos] != nId) {
bool fInsert = vvNew[nUBucket][nUBucketPos] == -1;
if (!fInsert) {
AddrInfo& infoExisting = mapInfo[vvNew[nUBucket][nUBucketPos]];
if (infoExisting.IsTerrible() || (infoExisting.nRefCount > 1 && pinfo->nRefCount == 0)) {
Expand All @@ -666,7 +609,74 @@ bool AddrManImpl::Add_(const CAddress& addr, const CNetAddr& source, int64_t nTi
}
}
}
return fNew;
return fInsert;
}

void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nTime)
{
AssertLockHeld(cs);

int nId;

nLastGood = nTime;

AddrInfo* pinfo = Find(addr, &nId);

// if not found, bail out
if (!pinfo)
return;

AddrInfo& info = *pinfo;

// update info
info.nLastSuccess = nTime;
info.nLastTry = nTime;
info.nAttempts = 0;
// nTime is not updated here, to avoid leaking information about
// currently-connected peers.

// if it is already in the tried set, don't do anything else
if (info.fInTried)
return;

// if it is not in new, something bad happened
if (!Assume(info.nRefCount > 0)) {
return;
}

// which tried bucket to move the entry to
int tried_bucket = info.GetTriedBucket(nKey, m_asmap);
int tried_bucket_pos = info.GetBucketPosition(nKey, false, tried_bucket);

// Will moving this address into tried evict another entry?
if (test_before_evict && (vvTried[tried_bucket][tried_bucket_pos] != -1)) {
if (m_tried_collisions.size() < ADDRMAN_SET_TRIED_COLLISION_SIZE) {
m_tried_collisions.insert(nId);
}
// Output the entry we'd be colliding with, for debugging purposes
auto colliding_entry = mapInfo.find(vvTried[tried_bucket][tried_bucket_pos]);
LogPrint(BCLog::ADDRMAN, "Collision with %s while attempting to move %s to tried table. Collisions=%d\n",
colliding_entry != mapInfo.end() ? colliding_entry->second.ToString() : "",
addr.ToString(),
m_tried_collisions.size());
} 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);
}
}

bool AddrManImpl::Add_(const std::vector<CAddress> &vAddr, const CNetAddr& source, int64_t nTimePenalty)
{
int added{0};
for (std::vector<CAddress>::const_iterator it = vAddr.begin(); it != vAddr.end(); it++) {
added += AddSingle(*it, source, nTimePenalty) ? 1 : 0;
}
if (added > 0) {
LogPrint(BCLog::ADDRMAN, "Added %i addresses (of %i) from %s: %i tried, %i new\n", added, vAddr.size(), source.ToString(), nTried, nNew);
}
return added > 0;
}

void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, int64_t nTime)
Expand Down Expand Up @@ -1031,15 +1041,10 @@ size_t AddrManImpl::size() const
bool AddrManImpl::Add(const std::vector<CAddress>& vAddr, const CNetAddr& source, int64_t nTimePenalty)
{
LOCK(cs);
int nAdd = 0;
Check();
for (std::vector<CAddress>::const_iterator it = vAddr.begin(); it != vAddr.end(); it++)
nAdd += Add_(*it, source, nTimePenalty) ? 1 : 0;
auto ret = Add_(vAddr, source, nTimePenalty);
Check();
if (nAdd) {
LogPrint(BCLog::ADDRMAN, "Added %i addresses from %s: %i tried, %i new\n", nAdd, source.ToString(), nTried, nNew);
}
return nAdd > 0;
return ret;
}

void AddrManImpl::Good(const CService& addr, int64_t nTime)
Expand Down
10 changes: 9 additions & 1 deletion src/addrman.h
View file
Expand Up @@ -69,7 +69,15 @@ class AddrMan
//! Return the number of (unique) addresses in all tables.
size_t size() const;

//! Add addresses to addrman's new table.
/**
* Attempt to add one or more addresses to addrman's new table.
*
* @param[in] vAddr Address records to attempt to add.
* @param[in] source The address of the node that sent us these addr records.
* @param[in] nTimePenalty A "time penalty" to apply to the address record's nTime. If a peer
* sends us an address record with nTime=n, then we'll add it to our
* addrman with nTime=(n - nTimePenalty).
* @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".
Expand Down
6 changes: 5 additions & 1 deletion src/addrman_impl.h
View file
Expand Up @@ -243,9 +243,13 @@ class AddrManImpl
//! Move an entry from the "new" table(s) to the "tried" table
void MakeTried(AddrInfo& info, int nId) EXCLUSIVE_LOCKS_REQUIRED(cs);

/** Attempt to add a single address to addrman's new table.
* @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 Add_(const CAddress& addr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
bool Add_(const std::vector<CAddress> &vAddr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs);

void Attempt_(const CService& addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs);

Expand Down
2 changes: 1 addition & 1 deletion src/test/addrman_tests.cpp
View file
Expand Up @@ -347,7 +347,7 @@ BOOST_AUTO_TEST_CASE(addrman_tried_collisions)
//Test: tried table 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(!addrman.Add({CAddress(addr1, NODE_NONE)}, source));
BOOST_CHECK_EQUAL(addrman.size(), num_addrs - collisions);

CService addr2 = ResolveService("250.1.1." + ToString(++num_addrs));
Expand Down
1 change: 0 additions & 1 deletion test/functional/p2p_addr_relay.py
View file
Expand Up @@ -152,7 +152,6 @@ def relay_tests(self):
msg = self.setup_addr_msg(num_ipv4_addrs)
with self.nodes[0].assert_debug_log(
[
'Added {} addresses from 127.0.0.1: 0 tried'.format(num_ipv4_addrs),
'received: addr (301 bytes) peer=1',
]
):
Expand Down
3 changes: 0 additions & 3 deletions test/functional/p2p_addrv2_relay.py
View file
Expand Up @@ -72,9 +72,6 @@ def run_test(self):
addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver())
msg.addrs = ADDRS
with self.nodes[0].assert_debug_log([
# The I2P address is not added to node's own addrman because it has no
# I2P reachability (thus 10 - 1 = 9).
'Added 9 addresses from 127.0.0.1: 0 tried',
'received: addrv2 (159 bytes) peer=0',
'sending addrv2 (159 bytes) peer=1',
]):
Expand Down

0 comments on commit 994aaaa

Please sign in to comment.