Skip to content

Commit

Permalink
Make AddrMan support multiple ports per IP
Browse files Browse the repository at this point in the history 
Summary:
```

For a long part of Bitcoin's history, this codebase has aggressively avoided making automatic connections to anything but nodes running on port 8333. I'd like to propose changing that, and this is a first PR necessary for that.

The folklore justification (eventually actually added as a comment to the codebase in #20668) is that this is to prevent the Bitcoin P2P network from being leveraged to perform a DoS attack on other services, if their IP/port would get rumoured. It appears, at least the current network scale - and probably significantly larger - that the impact is very low at best (see calculations by vasild in #5150 (comment) e.g.). Another possible justification would be a risk that treating different IP:port combinations separately would help perform Eclipse attacks (by an attacker rumouring their own IP with many ports). This concern is (a) no different than what is possible with IPv6 (where large ranges of IP addresses are very cheaply available), and (b) already hopefully sufficiently addressed by addrman's design (which limits access through based selected based on network groups).

And this policy has downsides too; in particular, a fixed port is easy to detect, and a very obvious sign a Bitcoin node is running there.

One obstacle in moving away from a default port that is the fact that addrman is currently restricted to a single entry per IP address. If ports are no longer expected to be generally always the default one, we need to deal with the case where conflicting information is relayed. It turns out there is a very natural solution to this: treat (IP,port) combination exactly as we're treating IPs now; this automatically means that the same IP may appear with multiple ports, simply because those would be distinct entries. Given that indexing into addrman's bucket already uses the port number, the only change required is making all addrman lookup be (IP,port) (aka CService) based, rather than IP (aka CNetAddr) based.

This PR doesn't include any change to the actual outbound connection preference logic, as perhaps that's something that we want to phase in more gradually.
```

Backport of [[bitcoin/bitcoin#23306 | core#23306]] and  [[bitcoin/bitcoin#23354 | core#23354]] .

Depends on D12340.

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D12342
  • Loading branch information
@sipa @Fabcien
sipa authored and Fabcien committed Oct 21, 2022
1 parent 55e5773 commit 0db06b5
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 58 deletions.
28 changes: 2 additions & 26 deletions src/addrman.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ template <typename Stream> void AddrManImpl::Serialize(Stream &s_) const {

// Increment `lowest_compatible` iff a newly introduced format is
// incompatible with the previous one.
static constexpr uint8_t lowest_compatible = Format::V3_BIP155;
static constexpr uint8_t lowest_compatible = Format::V4_MULTIPORT;
s << static_cast<uint8_t>(INCOMPATIBILITY_BASE + lowest_compatible);

s << nKey;
Expand Down Expand Up @@ -425,7 +425,7 @@ template <typename Stream> void AddrManImpl::Unserialize(Stream &s_) {
}
}

AddrInfo *AddrManImpl::Find(const CNetAddr &addr, int *pnId) {
AddrInfo *AddrManImpl::Find(const CService &addr, int *pnId) {
AssertLockHeld(cs);

const auto it = mapAddr.find(addr);
Expand Down Expand Up @@ -591,12 +591,6 @@ void AddrManImpl::Good_(const CService &addr, bool test_before_evict,

AddrInfo &info = *pinfo;

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

// update info
info.nLastSuccess = nTime;
info.nLastTry = nTime;
Expand Down Expand Up @@ -744,12 +738,6 @@ void AddrManImpl::Attempt_(const CService &addr, bool fCountFailure,

AddrInfo &info = *pinfo;

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

// update info
info.nLastTry = nTime;
if (fCountFailure && info.nLastCountAttempt < nLastGood) {
Expand Down Expand Up @@ -906,12 +894,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 (info != addr) {
return;
}

// update info
int64_t nUpdateInterval = 20 * 60;
if (nTime - info.nTime > nUpdateInterval) {
Expand All @@ -931,12 +913,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 (info != addr) {
return;
}

// update info
info.nServices = nServices;
}
Expand Down
10 changes: 6 additions & 4 deletions src/addrman_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,14 +171,16 @@ class AddrManImpl {
V2_ASMAP = 2,
//! same as V2_ASMAP plus addresses are in BIP155 format
V3_BIP155 = 3,
//! adds support for multiple ports per IP
V4_MULTIPORT = 4,
};

//! The maximum format this software knows it can unserialize. Also, we
//! always serialize in this format. The format (first byte in the
//! serialized stream) can be higher than this and still this software may
//! be able to unserialize the file - if the second byte (see
//! `lowest_compatible` in `Unserialize()`) is less or equal to this.
static constexpr Format FILE_FORMAT = Format::V3_BIP155;
static constexpr Format FILE_FORMAT = Format::V4_MULTIPORT;

//! The initial value of a field that is incremented every time an
//! incompatible format change is made (such that old software versions
Expand All @@ -194,8 +196,8 @@ class AddrManImpl {
//! table with information about all nIds
std::unordered_map<int, AddrInfo> mapInfo GUARDED_BY(cs);

//! find an nId based on its network address
std::unordered_map<CNetAddr, int, CNetAddrHash> mapAddr GUARDED_BY(cs);
//! find an nId based on its network address and port.
std::unordered_map<CService, int, CServiceHash> mapAddr GUARDED_BY(cs);

//! randomly-ordered vector of all nIds
//! This is mutable because it is unobservable outside the class, so any
Expand Down Expand Up @@ -248,7 +250,7 @@ class AddrManImpl {
bool deterministic = false;

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

//! find an entry, creating it if necessary.
Expand Down
32 changes: 17 additions & 15 deletions src/netaddress.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,6 @@ class CNetAddr {
}
}

friend class CNetAddrHash;
friend class CSubNet;

private:
Expand Down Expand Up @@ -495,20 +494,6 @@ class CNetAddr {
}
};

class CNetAddrHash {
public:
size_t operator()(const CNetAddr &a) const noexcept {
CSipHasher hasher(m_salt_k0, m_salt_k1);
hasher.Write(a.m_net);
hasher.Write(a.m_addr.data(), a.m_addr.size());
return static_cast<size_t>(hasher.Finalize());
}

private:
const uint64_t m_salt_k0 = GetRand(std::numeric_limits<uint64_t>::max());
const uint64_t m_salt_k1 = GetRand(std::numeric_limits<uint64_t>::max());
};

class CSubNet {
protected:
/// Network (base) address
Expand Down Expand Up @@ -588,6 +573,23 @@ class CService : public CNetAddr {
READWRITEAS(CNetAddr, obj);
READWRITE(Using<BigEndianFormatter<2>>(obj.port));
}

friend class CServiceHash;
};

class CServiceHash {
public:
size_t operator()(const CService &a) const noexcept {
CSipHasher hasher(m_salt_k0, m_salt_k1);
hasher.Write(a.m_net);
hasher.Write(a.port);
hasher.Write(a.m_addr.data(), a.m_addr.size());
return static_cast<size_t>(hasher.Finalize());
}

private:
const uint64_t m_salt_k0 = GetRand(std::numeric_limits<uint64_t>::max());
const uint64_t m_salt_k1 = GetRand(std::numeric_limits<uint64_t>::max());
};

#endif // BITCOIN_NETADDRESS_H
23 changes: 12 additions & 11 deletions src/test/addrman_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class AddrManTest : public AddrMan {
MakeDeterministic();
}

AddrInfo *Find(const CNetAddr &addr, int *pnId = nullptr) {
AddrInfo *Find(const CService &addr, int *pnId = nullptr) {
LOCK(m_impl->cs);
return m_impl->Find(addr, pnId);
}
Expand Down Expand Up @@ -209,15 +209,16 @@ BOOST_AUTO_TEST_CASE(addrman_ports) {
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);
BOOST_CHECK(addrman.Add({CAddress(addr1_port, NODE_NONE)}, source));
BOOST_CHECK_EQUAL(addrman.size(), 2U);
auto addr_ret2 = addrman.Select().first;
BOOST_CHECK_EQUAL(addr_ret2.ToString(), "250.1.1.1:8333");
BOOST_CHECK(addr_ret2.ToString() == "250.1.1.1:8333" ||
addr_ret2.ToString() == "250.1.1.1:8334");

// 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.
// Test: Add same IP but diff port to tried table; this converts the entry
// with the specified port to tried, but not the other.
addrman.Good(CAddress(addr1_port, NODE_NONE));
BOOST_CHECK_EQUAL(addrman.size(), 1U);
BOOST_CHECK_EQUAL(addrman.size(), 2U);
bool newOnly = true;
auto addr_ret3 = addrman.Select(newOnly).first;
BOOST_CHECK_EQUAL(addr_ret3.ToString(), "250.1.1.1:8333");
Expand Down Expand Up @@ -360,18 +361,18 @@ BOOST_AUTO_TEST_CASE(addrman_find) {
CNetAddr source2 = ResolveIP("250.1.2.2");

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

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

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

// Test: Find returns another IP matching what we searched on.
AddrInfo *info3 = addrman.Find(addr3);
Expand Down
4 changes: 2 additions & 2 deletions test/functional/feature_addrman.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
def serialize_addrman(
*,
format=1,
lowest_compatible=3,
lowest_compatible=4,
net_magic=b"\xfa\xbf\xb5\xda",
bucket_key=1,
len_new=None,
Expand Down Expand Up @@ -77,7 +77,7 @@ def init_error(reason): return (
expected_msg=init_error(
"Unsupported format of addrman database: 1. It is compatible with "
"formats >=111, but the maximum supported by this version of "
f"{self.config['environment']['PACKAGE_NAME']} is 3.: (.+)"
f"{self.config['environment']['PACKAGE_NAME']} is 4.: (.+)"
),
match=ErrorMatch.FULL_REGEX,
)
Expand Down

0 comments on commit 0db06b5

Please sign in to comment.