Skip to content

Commit

Permalink
Merge #6062: feat: Let addrman handle multiple ports for all networks
Browse files Browse the repository at this point in the history 
76b4300 fix: Let addrman handle multiple ports for all networks (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  There is really no need to do have this restriction in `addrman`, we check `AllowMultiplePorts()` in `connman` which should be enough already. We can safely re-align `addrman` to its upstream implementation as suggested in #6043.

  ## What was done?
  Drop port "discrimination" in `addrman`, remove related tests.

  ## How Has This Been Tested?
  Run tests, run dash-qt on mainnet/testnet

  ## Breaking Changes
  n/a

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  kwvg:
    ACK 76b4300
  PastaPastaPasta:
    utACK 76b4300; is it really a fix or a refactor? doesn't change observable system behavior
  knst:
    utACK 76b4300

Tree-SHA512: 50363b5de7a6c6ad4de18c08b0a5cc31e89be8e5304f9684ce2cc609df4de07ff6d8d010556b5f6651c698e1a86960dba8005cc26fdb00bd03c856752d3e06ef
  • Loading branch information
@PastaPastaPasta
PastaPastaPasta committed Jun 18, 2024
2 parents 77a025f + 76b4300 commit 7f17ff8
Show file tree
Hide file tree
Showing 6 changed files with 11 additions and 119 deletions.
49 changes: 6 additions & 43 deletions src/addrman.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,11 @@ double AddrInfo::GetChance(int64_t nNow) const
return fChance;
}

AddrManImpl::AddrManImpl(std::vector<bool>&& asmap, bool deterministic, int32_t consistency_check_ratio, bool discriminate_ports)
AddrManImpl::AddrManImpl(std::vector<bool>&& asmap, bool deterministic, int32_t consistency_check_ratio)
: insecure_rand{deterministic}
, nKey{deterministic ? uint256{1} : insecure_rand.rand256()}
, m_consistency_check_ratio{consistency_check_ratio}
, m_asmap{std::move(asmap)}
, m_discriminate_ports{discriminate_ports}
{
for (auto& bucket : vvNew) {
for (auto& entry : bucket) {
Expand Down Expand Up @@ -400,12 +399,7 @@ void AddrManImpl::Unserialize(Stream& s_)

AddrInfo* AddrManImpl::Find(const CService& addr, int* pnId)
{
CService addr2 = addr;
if (!m_discriminate_ports) {
addr2.SetPort(0);
}

const auto it = mapAddr.find(addr2);
const auto it = mapAddr.find(addr);
if (it == mapAddr.end())
return nullptr;
if (pnId)
Expand All @@ -418,15 +412,11 @@ AddrInfo* AddrManImpl::Find(const CService& addr, int* pnId)

AddrInfo* AddrManImpl::Create(const CAddress& addr, const CNetAddr& addrSource, int* pnId)
{
CService addr2 = addr;
if (!m_discriminate_ports) {
addr2.SetPort(0);
}
AssertLockHeld(cs);

int nId = nIdCount++;
mapInfo[nId] = AddrInfo(addr, addrSource);
mapAddr[addr2] = nId;
mapAddr[addr] = nId;
mapInfo[nId].nRandomPos = vRandom.size();
vRandom.push_back(nId);
if (pnId)
Expand Down Expand Up @@ -467,14 +457,9 @@ void AddrManImpl::Delete(int nId)
assert(!info.fInTried);
assert(info.nRefCount == 0);

CService addr = info;
if (!m_discriminate_ports) {
addr.SetPort(0);
}

SwapRandom(info.nRandomPos, vRandom.size() - 1);
vRandom.pop_back();
mapAddr.erase(addr);
mapAddr.erase(info);
mapInfo.erase(nId);
nNew--;
}
Expand Down Expand Up @@ -645,10 +630,6 @@ void AddrManImpl::Good_(const CService& addr, bool test_before_evict, int64_t nT

AddrInfo& info = *pinfo;

// check whether we are talking about the exact same CService (including same port)
if (!m_discriminate_ports && info != addr)
return;

// update info
info.nLastSuccess = nTime;
info.nLastTry = nTime;
Expand Down Expand Up @@ -716,10 +697,6 @@ void AddrManImpl::Attempt_(const CService& addr, bool fCountFailure, int64_t nTi

AddrInfo& info = *pinfo;

// check whether we are talking about the exact same CService (including same port)
if (!m_discriminate_ports && info != addr)
return;

// update info
info.nLastTry = nTime;
if (fCountFailure && info.nLastCountAttempt < nLastGood) {
Expand Down Expand Up @@ -847,10 +824,6 @@ void AddrManImpl::Connected_(const CService& addr, int64_t nTime)

AddrInfo& info = *pinfo;

// check whether we are talking about the exact same CService (including same port)
if (!m_discriminate_ports && info != addr)
return;

// update info
int64_t nUpdateInterval = 20 * 60;
if (nTime - info.nTime > nUpdateInterval)
Expand All @@ -869,10 +842,6 @@ void AddrManImpl::SetServices_(const CService& addr, ServiceFlags nServices)

AddrInfo& info = *pinfo;

// check whether we are talking about the exact same CService (including same port)
if (!m_discriminate_ports && info != addr)
return;

// update info
info.nServices = nServices;
}
Expand All @@ -885,12 +854,6 @@ AddrInfo AddrManImpl::GetAddressInfo_(const CService& addr)
if (!pinfo)
return AddrInfo();

AddrInfo& info = *pinfo;

// check whether we are talking about the exact same CService (including same port)
if (!m_discriminate_ports && info != addr)
return AddrInfo();

return *pinfo;
}

Expand Down Expand Up @@ -1187,8 +1150,8 @@ const std::vector<bool>& AddrManImpl::GetAsmap() const
return m_asmap;
}

AddrMan::AddrMan(std::vector<bool> asmap, bool deterministic, int32_t consistency_check_ratio, bool discriminate_ports)
: m_impl(std::make_unique<AddrManImpl>(std::move(asmap), deterministic, consistency_check_ratio, discriminate_ports)) {}
AddrMan::AddrMan(std::vector<bool> asmap, bool deterministic, int32_t consistency_check_ratio)
: m_impl(std::make_unique<AddrManImpl>(std::move(asmap), deterministic, consistency_check_ratio)) {}

AddrMan::~AddrMan() = default;

Expand Down
2 changes: 1 addition & 1 deletion src/addrman.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class AddrMan
const std::unique_ptr<AddrManImpl> m_impl;

public:
explicit AddrMan(std::vector<bool> asmap, bool deterministic, int32_t consistency_check_ratio, bool discriminate_ports = false);
explicit AddrMan(std::vector<bool> asmap, bool deterministic, int32_t consistency_check_ratio);

~AddrMan();

Expand Down
5 changes: 1 addition & 4 deletions src/addrman_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ class AddrInfo : public CAddress
class AddrManImpl
{
public:
AddrManImpl(std::vector<bool>&& asmap, bool deterministic, int32_t consistency_check_ratio, bool discriminate_ports);
AddrManImpl(std::vector<bool>&& asmap, bool deterministic, int32_t consistency_check_ratio);

~AddrManImpl();

Expand Down Expand Up @@ -227,9 +227,6 @@ class AddrManImpl
// would be re-bucketed accordingly.
const std::vector<bool> m_asmap;

//! Discriminate entries based on port.
bool m_discriminate_ports GUARDED_BY(cs);

//! Find an entry.
AddrInfo* Find(const CService& addr, int* pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);

Expand Down
5 changes: 0 additions & 5 deletions src/netaddress.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1056,11 +1056,6 @@ std::string CService::ToString() const
return ToStringIPPort();
}

void CService::SetPort(uint16_t portIn)
{
port = portIn;
}

CSubNet::CSubNet():
valid(false)
{
Expand Down
1 change: 0 additions & 1 deletion src/netaddress.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,6 @@ class CService : public CNetAddr
CService(const CNetAddr& ip, uint16_t port);
CService(const struct in_addr& ipv4Addr, uint16_t port);
explicit CService(const struct sockaddr_in& addr);
void SetPort(uint16_t portIn);
uint16_t GetPort() const;
bool GetSockAddr(struct sockaddr* paddr, socklen_t *addrlen) const;
bool SetSockAddr(const struct sockaddr* paddr);
Expand Down
68 changes: 3 additions & 65 deletions src/test/addrman_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class AddrManSerializationMock : public AddrMan
virtual void Serialize(CDataStream& s) const = 0;

AddrManSerializationMock()
: AddrMan(/* asmap */ std::vector<bool>(), /* deterministic */ true, /* consistency_check_ratio */ 100, /* discriminate_ports */ true)
: AddrMan(/* asmap */ std::vector<bool>(), /* deterministic */ true, /* consistency_check_ratio */ 100)
{}
};

Expand Down Expand Up @@ -82,9 +82,8 @@ class AddrManTest : public AddrMan

public:
explicit AddrManTest(bool makeDeterministic = true,
std::vector<bool> asmap = std::vector<bool>(),
bool discriminatePorts = true)
: AddrMan(asmap, makeDeterministic, /* consistency_check_ratio */ 100, discriminatePorts)
std::vector<bool> asmap = std::vector<bool>())
: AddrMan(asmap, makeDeterministic, /* consistency_check_ratio */ 100)
{
deterministic = makeDeterministic;
}
Expand Down Expand Up @@ -236,34 +235,6 @@ BOOST_AUTO_TEST_CASE(addrman_ports)
BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333");
}

BOOST_AUTO_TEST_CASE(addrman_ports_nondiscriminate)
{
AddrManTest addrman(/* deterministic */ true, /* asmap */ std::vector<bool>(), /* discriminate */ false);

CNetAddr source = ResolveIP("252.2.2.2");

BOOST_CHECK_EQUAL(addrman.size(), 0U);

// Test 7; Addr with same IP but diff port does not replace existing addr.
CService addr1 = ResolveService("250.1.1.1", 8333);
BOOST_CHECK(addrman.Add({CAddress(addr1, NODE_NONE)}, source));
BOOST_CHECK_EQUAL(addrman.size(), 1U);

CService addr1_port = ResolveService("250.1.1.1", 8334);
BOOST_CHECK(!addrman.Add({CAddress(addr1_port, NODE_NONE)}, source));
BOOST_CHECK_EQUAL(addrman.size(), 1U);
auto addr_ret2 = addrman.Select().first;
BOOST_CHECK_EQUAL(addr_ret2.ToString(), "250.1.1.1:8333");

// Test: Add same IP but diff port to tried table, it doesn't get added.
// Perhaps this is not ideal behavior but it is the current behavior.
addrman.Good(CAddress(addr1_port, NODE_NONE));
BOOST_CHECK_EQUAL(addrman.size(), 1U);
bool newOnly = true;
auto addr_ret3 = addrman.Select(newOnly).first;
BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333");
}

BOOST_AUTO_TEST_CASE(addrman_select)
{
AddrManTest addrman;
Expand Down Expand Up @@ -415,39 +386,6 @@ BOOST_AUTO_TEST_CASE(addrman_find)
BOOST_CHECK_EQUAL(info3->ToString(), "251.255.2.1:8333");
}

BOOST_AUTO_TEST_CASE(addrman_find_nondiscriminate)
{
AddrManTest addrman(/* deterministic */ true, /* asmap */ std::vector<bool>(), /* discriminate */ false);

BOOST_CHECK_EQUAL(addrman.size(), 0U);

CAddress addr1 = CAddress(ResolveService("250.1.2.1", 8333), NODE_NONE);
CAddress addr2 = CAddress(ResolveService("250.1.2.1", 9999), NODE_NONE);
CAddress addr3 = CAddress(ResolveService("251.255.2.1", 8333), NODE_NONE);

CNetAddr source1 = ResolveIP("250.1.2.1");
CNetAddr source2 = ResolveIP("250.1.2.2");

BOOST_CHECK(addrman.Add({addr1}, source1));
BOOST_CHECK(!addrman.Add({addr2}, source2));
BOOST_CHECK(addrman.Add({addr3}, source1));

// Test: ensure Find returns an IP matching what we searched on.
AddrInfo* info1 = addrman.Find(addr1);
BOOST_REQUIRE(info1);
BOOST_CHECK_EQUAL(info1->ToString(), "250.1.2.1:8333");

// Test 18; Find does not discriminate by port number.
AddrInfo* info2 = addrman.Find(addr2);
BOOST_REQUIRE(info2);
BOOST_CHECK_EQUAL(info2->ToString(), info1->ToString());

// Test: Find returns another IP matching what we searched on.
AddrInfo* info3 = addrman.Find(addr3);
BOOST_REQUIRE(info3);
BOOST_CHECK_EQUAL(info3->ToString(), "251.255.2.1:8333");
}

BOOST_AUTO_TEST_CASE(addrman_create)
{
AddrManTest addrman;
Expand Down

0 comments on commit 7f17ff8

Please sign in to comment.