Skip to content

Commit

Permalink
Merge bitcoin#19724: [net] Cleanup connection types- followups
Browse files Browse the repository at this point in the history 
eb1c5d0 [doc] Follow developer notes, add comment about missing default. (Amiti Uttarwar)
d5a57ce [doc] Describe connection types in more depth. (Amiti Uttarwar)
4829b6f [refactor] Simplify connection type logic in ThreadOpenConnections (Amiti Uttarwar)
1e563ae [refactor] Simplify check for block-relay-only connection. (Amiti Uttarwar)
da3a0be [test] Add explicit tests that connection types get set correctly (Amiti Uttarwar)
1d74fc7 [trivial] Small style updates (Amiti Uttarwar)
ff6b908 [doc] Explain address handling logic in process messages (Amiti Uttarwar)
dff16b1 [refactor] Restructure logic to check for addr relay. (Amiti Uttarwar)
a6ab1e8 [net] Remove unnecessary default args on OpenNetworkConnection (Amiti Uttarwar)
8d6ff46 scripted-diff: Rename `OUTBOUND` ConnectionType to `OUTBOUND_FULL_RELAY` (Amiti Uttarwar)

Pull request description:

  This PR addresses outstanding review comments from bitcoin#19316. It further simplifies `net.cpp` complexity and adds documentation about design goals about different connection types.

ACKs for top commit:
  naumenkogs:
    ACK eb1c5d0
  laanwj:
    Code review ACK eb1c5d0

Tree-SHA512: 2fe14e428404c95661e5518c8c90db07ab5b9ebb1bac921b3bdf82b181f444fae379f8fc0a2b619e6b4693f01c55bd246fbd8c8eedadd96849a30de3161afee5
  • Loading branch information
@laanwj @vijaydasmp
laanwj authored and vijaydasmp committed Aug 11, 2023
1 parent f36d780 commit eff96a9
Show file tree
Hide file tree
Showing 6 changed files with 134 additions and 59 deletions.
55 changes: 27 additions & 28 deletions src/net.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2294,16 +2294,16 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
// but inbound and manual peers do not use our outbound slots. Inbound peers
// also have the added issue that they could be attacker controlled and used
// to prevent us from connecting to particular hosts if we used them here.
switch(pnode->m_conn_type){
switch (pnode->m_conn_type) {
case ConnectionType::INBOUND:
case ConnectionType::MANUAL:
break;
case ConnectionType::OUTBOUND:
case ConnectionType::OUTBOUND_FULL_RELAY:
case ConnectionType::BLOCK_RELAY:
case ConnectionType::ADDR_FETCH:
case ConnectionType::FEELER:
setConnected.insert(pnode->addr.GetGroup(addrman.m_asmap));
}
} // no default case, so the compiler can warn about missing cases
}
}

Expand All @@ -2330,16 +2330,32 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
// connections.
// * Only make a feeler connection once every few minutes.
//
ConnectionType conn_type = ConnectionType::OUTBOUND_FULL_RELAY;
int64_t nTime = GetTimeMicros();
bool fFeeler = false;

if (nOutboundFullRelay >= m_max_outbound_full_relay && nOutboundBlockRelay >= m_max_outbound_block_relay && !GetTryNewOutboundPeer()) {
int64_t nTime = GetTimeMicros(); // The current time right now (in microseconds).
if (nTime > nNextFeeler) {
nNextFeeler = PoissonNextSend(nTime, FEELER_INTERVAL);
fFeeler = true;
} else {
continue;
}
// Determine what type of connection to open. Opening
// OUTBOUND_FULL_RELAY connections gets the highest priority until we
// meet our full-relay capacity. Then we open BLOCK_RELAY connection
// until we hit our block-relay-only peer limit.
// GetTryNewOutboundPeer() gets set when a stale tip is detected, so we
// try opening an additional OUTBOUND_FULL_RELAY connection. If none of
// these conditions are met, check the nNextFeeler timer to decide if
// we should open a FEELER.

if (nOutboundFullRelay < m_max_outbound_full_relay) {
// OUTBOUND_FULL_RELAY
} else if (nOutboundBlockRelay < m_max_outbound_block_relay) {
conn_type = ConnectionType::BLOCK_RELAY;
} else if (GetTryNewOutboundPeer()) {
// OUTBOUND_FULL_RELAY
} else if (nTime > nNextFeeler) {
nNextFeeler = PoissonNextSend(nTime, FEELER_INTERVAL);
conn_type = ConnectionType::FEELER;
fFeeler = true;
} else {
// skip to next iteration of while loop
continue;
}

addrman.ResolveCollisions();
Expand Down Expand Up @@ -2429,23 +2445,6 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
}
}

ConnectionType conn_type;
// Determine what type of connection to open. If fFeeler is not
// set, open OUTBOUND connections until we meet our full-relay
// capacity. Then open BLOCK_RELAY connections until we hit our
// block-relay peer limit. Otherwise, default to opening an
// OUTBOUND connection.
if (fFeeler) {
conn_type = ConnectionType::FEELER;
} else if (nOutboundFullRelay < m_max_outbound_full_relay) {
conn_type = ConnectionType::OUTBOUND;
} else if (nOutboundBlockRelay < m_max_outbound_block_relay) {
conn_type = ConnectionType::BLOCK_RELAY;
} else {
// GetTryNewOutboundPeer() is true
conn_type = ConnectionType::OUTBOUND;
}

OpenNetworkConnection(addrConnect, (int)setConnected.size() >= std::min(nMaxConnections - 1, 2), &grant, nullptr, conn_type);
}
}
Expand Down
80 changes: 64 additions & 16 deletions src/net.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,54 @@ struct CSerializedNetMsg
* information we have available at the time of opening or accepting the
* connection. Aside from INBOUND, all types are initiated by us. */
enum class ConnectionType {
INBOUND, /**< peer initiated connections */
OUTBOUND, /**< full relay connections (blocks, addrs, txns) made automatically. Addresses selected from AddrMan. */
MANUAL, /**< connections to addresses added via addnode or the connect command line argument */
FEELER, /**< short lived connections used to test address validity */
BLOCK_RELAY, /**< only relay blocks to these automatic outbound connections. Addresses selected from AddrMan. */
ADDR_FETCH, /**< short lived connections used to solicit addrs when starting the node without a populated AddrMan */
/**
* Inbound connections are those initiated by a peer. This is the only
* property we know at the time of connection, until P2P messages are
* exchanged.
*/
INBOUND,

/**
* These are the default connections that we use to connect with the
* network. There is no restriction on what is relayed- by default we relay
* blocks, addresses & transactions. We automatically attempt to open
* MAX_OUTBOUND_FULL_RELAY_CONNECTIONS using addresses from our AddrMan.
*/
OUTBOUND_FULL_RELAY,


/**
* We open manual connections to addresses that users explicitly inputted
* via the addnode RPC, or the -connect command line argument. Even if a
* manual connection is misbehaving, we do not automatically disconnect or
* add it to our discouragement filter.
*/
MANUAL,

/**
* Feeler connections are short lived connections used to increase the
* number of connectable addresses in our AddrMan. Approximately every
* FEELER_INTERVAL, we attempt to connect to a random address from the new
* table. If successful, we add it to the tried table.
*/
FEELER,

/**
* We use block-relay-only connections to help prevent against partition
* attacks. By not relaying transactions or addresses, these connections
* are harder to detect by a third party, thus helping obfuscate the
* network topology. We automatically attempt to open
* MAX_BLOCK_RELAY_ONLY_CONNECTIONS using addresses from our AddrMan.
*/
BLOCK_RELAY,

/**
* AddrFetch connections are short lived connections used to solicit
* addresses from peers. These are initiated to addresses submitted via the
* -seednode command line argument, or under certain conditions when the
* AddrMan is empty.
*/
ADDR_FETCH,
};

const std::vector<std::string> CONNECTION_TYPE_DOC{
Expand Down Expand Up @@ -259,7 +301,7 @@ friend class CNode;
IsConnection,
};

void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, ConnectionType conn_type = ConnectionType::OUTBOUND, MasternodeConn masternode_connection = MasternodeConn::IsNotConnection, MasternodeProbeConn masternode_probe_connection = MasternodeProbeConn::IsNotConnection);
void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant* grantOutbound, const char* strDest, ConnectionType conn_type = ConnectionType::OUTBOUND_FULL_RELAY, MasternodeConn masternode_connection = MasternodeConn::IsNotConnection, MasternodeProbeConn masternode_probe_connection = MasternodeProbeConn::IsNotConnection);
void OpenMasternodeConnection(const CAddress& addrConnect, MasternodeProbeConn probe = MasternodeProbeConn::IsConnection);
bool CheckIncomingNonce(uint64_t nonce);

Expand Down Expand Up @@ -578,7 +620,7 @@ friend class CNode;
CNode* FindNode(const CService& addr, bool fExcludeDisconnecting = true);

bool AttemptToEvictConnection();
CNode* ConnectNode(CAddress addrConnect, const char *pszDest = nullptr, bool fCountFailure = false, ConnectionType conn_type = ConnectionType::OUTBOUND);
CNode* ConnectNode(CAddress addrConnect, const char *pszDest = nullptr, bool fCountFailure = false, ConnectionType conn_type = ConnectionType::OUTBOUND_FULL_RELAY);
void AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr) const;

void DeleteNode(CNode* pnode);
Expand Down Expand Up @@ -1134,22 +1176,22 @@ class CNode
*/
Network ConnectedThroughNetwork() const;
bool IsOutboundOrBlockRelayConn() const {
switch(m_conn_type) {
case ConnectionType::OUTBOUND:
switch (m_conn_type) {
case ConnectionType::OUTBOUND_FULL_RELAY:
case ConnectionType::BLOCK_RELAY:
return true;
case ConnectionType::INBOUND:
case ConnectionType::MANUAL:
case ConnectionType::ADDR_FETCH:
case ConnectionType::FEELER:
return false;
}
} // no default case, so the compiler can warn about missing cases

assert(false);
}

bool IsFullOutboundConn() const {
return m_conn_type == ConnectionType::OUTBOUND;
return m_conn_type == ConnectionType::OUTBOUND_FULL_RELAY;
}

bool IsManualConn() const {
Expand All @@ -1172,17 +1214,23 @@ class CNode
return m_conn_type == ConnectionType::INBOUND;
}

/* Whether we send addr messages over this connection */
bool RelayAddrsWithConn() const
{
return m_conn_type != ConnectionType::BLOCK_RELAY;
}

bool ExpectServicesFromConn() const {
switch(m_conn_type) {
switch (m_conn_type) {
case ConnectionType::INBOUND:
case ConnectionType::MANUAL:
case ConnectionType::FEELER:
return false;
case ConnectionType::OUTBOUND:
case ConnectionType::OUTBOUND_FULL_RELAY:
case ConnectionType::BLOCK_RELAY:
case ConnectionType::ADDR_FETCH:
return true;
}
} // no default case, so the compiler can warn about missing cases

assert(false);
}
Expand All @@ -1197,7 +1245,7 @@ class CNode

// flood relay
std::vector<CAddress> vAddrToSend;
std::unique_ptr<CRollingBloomFilter> m_addr_known = nullptr;
std::unique_ptr<CRollingBloomFilter> m_addr_known{nullptr};
bool fGetAddr{false};
std::chrono::microseconds m_next_addr_send GUARDED_BY(cs_sendProcessing){0};
std::chrono::microseconds m_next_local_addr_send GUARDED_BY(cs_sendProcessing){0};
Expand Down
36 changes: 27 additions & 9 deletions src/net_processing.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1156,8 +1156,9 @@ void PeerManagerImpl::InitializeNode(CNode *pnode) {
LOCK(m_peer_mutex);
m_peer_map.emplace_hint(m_peer_map.end(), nodeid, std::move(peer));
}
if (!pnode->IsInboundConn())
if (!pnode->IsInboundConn()) {
PushNodeVersion(*pnode, GetTime());
}
}

void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler) const
Expand Down Expand Up @@ -1895,7 +1896,7 @@ static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman&
assert(nRelayNodes <= best.size());

auto sortfunc = [&best, &hasher, nRelayNodes, addr](CNode* pnode) {
if (pnode->IsAddrRelayPeer() && pnode->IsAddrCompatible(addr)) {
if (pnode->RelayAddrsWithConn() && pnode->IsAddrCompatible(addr)) {
uint64_t hashKey = CSipHasher(hasher).Write(pnode->GetId()).Finalize();
for (unsigned int i = 0; i < nRelayNodes; i++) {
if (hashKey > best[i].first) {
Expand Down Expand Up @@ -2472,7 +2473,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const std::vector<CBlo
// Note that outbound block-relay peers are excluded from this protection, and
// thus always subject to eviction under the bad/lagging chain logic.
// See ChainSyncTimeoutState.
if (!pfrom.fDisconnect && pfrom.IsOutboundOrBlockRelayConn() && nodestate->pindexBestKnownBlock != nullptr && pfrom.IsAddrRelayPeer()) {
if (!pfrom.fDisconnect && pfrom.IsFullOutboundConn() && nodestate->pindexBestKnownBlock != nullptr && pfrom.IsAddrRelayPeer()) {
if (m_outbound_peers_with_protect_from_disconnect < MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT && nodestate->pindexBestKnownBlock->nChainWork >= m_chainman.ActiveChain().Tip()->nChainWork && !nodestate->m_chain_sync.m_protect) {
LogPrint(BCLog::NET, "Protecting outbound peer=%d from eviction\n", pfrom.GetId());
nodestate->m_chain_sync.m_protect = true;
Expand Down Expand Up @@ -2938,9 +2939,23 @@ void PeerManagerImpl::ProcessMessage(
UpdatePreferredDownload(pfrom, State(pfrom.GetId()));
}

if (!pfrom.IsInboundConn() && pfrom.IsAddrRelayPeer())
{
// Advertise our address
if (!pfrom.IsInboundConn() && !pfrom.IsBlockOnlyConn()) {
// For outbound peers, we try to relay our address (so that other
// nodes can try to find us more quickly, as we have no guarantee
// that an outbound peer is even aware of how to reach us) and do a
// one-time address fetch (to help populate/update our addrman). If
// we're starting up for the first time, our addrman may be pretty
// empty and no one will know who we are, so these mechanisms are
// important to help us connect to the network.
//
// We also update the addrman to record connection success for
// these peers (which include OUTBOUND_FULL_RELAY and FEELER
// connections) so that addrman will have an up-to-date notion of
// which peers are online and available.
//
// We skip these operations for BLOCK_RELAY peers to avoid
// potentially leaking information about our BLOCK_RELAY
// connections via the addrman or address relay.
if (fListen && !m_chainman.ActiveChainstate().IsInitialBlockDownload())
{
CAddress addr = GetLocalAddress(&pfrom.addr, pfrom.GetLocalServices());
Expand All @@ -2960,6 +2975,9 @@ void PeerManagerImpl::ProcessMessage(
m_connman.PushMessage(&pfrom, CNetMsgMaker(nSendVersion).Make(NetMsgType::GETADDR));
pfrom.fGetAddr = true;
m_addrman.Good(pfrom.addr);

// Moves address from New to Tried table in Addrman, resolves
// tried-table collisions, etc.
}

std::string remoteAddr;
Expand Down Expand Up @@ -3091,7 +3109,7 @@ void PeerManagerImpl::ProcessMessage(

s >> vAddr;

if (!pfrom.IsAddrRelayPeer()) {
if (!pfrom.RelayAddrsWithConn()) {
return;
}
if (vAddr.size() > MAX_ADDR_TO_SEND)
Expand Down Expand Up @@ -4658,15 +4676,15 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
int64_t nNow = GetTimeMicros();
auto current_time = GetTime<std::chrono::microseconds>();

if (pto->IsAddrRelayPeer() && !m_chainman.ActiveChainstate().IsInitialBlockDownload() && pto->m_next_local_addr_send < current_time) {
if (pto->RelayAddrsWithConn() && !m_chainman.ActiveChainstate().IsInitialBlockDownload() && pto->m_next_local_addr_send < current_time) {
AdvertiseLocal(pto);
pto->m_next_local_addr_send = PoissonNextSend(current_time, AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL);
}

//
// Message: addr
//
if (pto->IsAddrRelayPeer() && pto->m_next_addr_send < current_time) {
if (pto->RelayAddrsWithConn() && pto->m_next_addr_send < current_time) {
pto->m_next_addr_send = PoissonNextSend(current_time, AVG_ADDRESS_BROADCAST_INTERVAL);
std::vector<CAddress> vAddr;
vAddr.reserve(pto->vAddrToSend.size());
Expand Down
4 changes: 2 additions & 2 deletions src/test/denialofservice_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)

// Mock an outbound peer
CAddress addr1(ip(0xa0b0c001), NODE_NONE);
CNode dummyNode1(id++, ServiceFlags(NODE_NETWORK), INVALID_SOCKET, addr1, 0, 0, CAddress(), "", ConnectionType::OUTBOUND);
CNode dummyNode1(id++, ServiceFlags(NODE_NETWORK), INVALID_SOCKET, addr1, 0, 0, CAddress(), "", ConnectionType::OUTBOUND_FULL_RELAY);
dummyNode1.SetSendVersion(PROTOCOL_VERSION);

peerLogic->InitializeNode(&dummyNode1);
Expand Down Expand Up @@ -136,7 +136,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
static void AddRandomOutboundPeer(std::vector<CNode *> &vNodes, PeerManager &peerLogic, CConnmanTest* connman)
{
CAddress addr(ip(g_insecure_rand_ctx.randbits(32)), NODE_NONE);
vNodes.emplace_back(new CNode(id++, ServiceFlags(NODE_NETWORK), INVALID_SOCKET, addr, 0, 0, CAddress(), "", ConnectionType::OUTBOUND));
vNodes.emplace_back(new CNode(id++, ServiceFlags(NODE_NETWORK), INVALID_SOCKET, addr, 0, 0, CAddress(), "", ConnectionType::OUTBOUND_FULL_RELAY));
CNode &node = *vNodes.back();
node.SetSendVersion(PROTOCOL_VERSION);

Expand Down
2 changes: 1 addition & 1 deletion src/test/fuzz/net.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ FUZZ_TARGET_INIT(net, initialize_net)
const int ref_count = node.GetRefCount();
assert(ref_count >= 0);
(void)node.GetSendVersion();
(void)node.IsAddrRelayPeer();
(void)node.RelayAddrsWithConn();

const NetPermissionFlags net_permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS);
(void)node.HasPermission(net_permission_flags);
Expand Down
16 changes: 13 additions & 3 deletions src/test/net_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,20 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test)
CAddress addr = CAddress(CService(ipv4Addr, 7777), NODE_NETWORK);
std::string pszDest;

std::unique_ptr<CNode> pnode1 = MakeUnique<CNode>(id++, NODE_NETWORK, height, hSocket, addr, 0, 0, CAddress(), pszDest, ConnectionType::OUTBOUND);
std::unique_ptr<CNode> pnode1 = std::make_unique<CNode>(id++, NODE_NETWORK, hSocket, addr, 0, 0, CAddress(), pszDest, ConnectionType::OUTBOUND_FULL_RELAY);
BOOST_CHECK(pnode1->IsFullOutboundConn() == true);
BOOST_CHECK(pnode1->IsManualConn() == false);
BOOST_CHECK(pnode1->IsBlockOnlyConn() == false);
BOOST_CHECK(pnode1->IsFeelerConn() == false);
BOOST_CHECK(pnode1->IsAddrFetchConn() == false);
BOOST_CHECK(pnode1->IsInboundConn() == false);

std::unique_ptr<CNode> pnode2 = MakeUnique<CNode>(id++, NODE_NETWORK, height, hSocket, addr, 1, 1, CAddress(), pszDest, ConnectionType::INBOUND);
std::unique_ptr<CNode> pnode2 = std::make_unique<CNode>(id++, NODE_NETWORK, hSocket, addr, 1, 1, CAddress(), pszDest, ConnectionType::INBOUND);
BOOST_CHECK(pnode2->IsFullOutboundConn() == false);
BOOST_CHECK(pnode2->IsManualConn() == false);
BOOST_CHECK(pnode2->IsBlockOnlyConn() == false);
BOOST_CHECK(pnode2->IsFeelerConn() == false);
BOOST_CHECK(pnode2->IsAddrFetchConn() == false);
BOOST_CHECK(pnode2->IsInboundConn() == true);
}

Expand Down Expand Up @@ -651,7 +661,7 @@ BOOST_AUTO_TEST_CASE(ipv4_peer_with_ipv6_addrMe_test)
in_addr ipv4AddrPeer;
ipv4AddrPeer.s_addr = 0xa0b0c001;
CAddress addr = CAddress(CService(ipv4AddrPeer, 7777), NODE_NETWORK);
std::unique_ptr<CNode> pnode = std::make_unique<CNode>(0, NODE_NETWORK, INVALID_SOCKET, addr, 0, 0, CAddress{}, std::string{}, ConnectionType::OUTBOUND);
std::unique_ptr<CNode> pnode = std::make_unique<CNode>(0, NODE_NETWORK, INVALID_SOCKET, addr, 0, 0, CAddress{}, std::string{}, ConnectionType::OUTBOUND_FULL_RELAY);
pnode->fSuccessfullyConnected.store(true);

// the peer claims to be reaching us via IPv6
Expand Down

0 comments on commit eff96a9

Please sign in to comment.