Skip to content

Commit

Permalink
[net/refactor] Rework ThreadOpenConnections logic
Browse files Browse the repository at this point in the history
Summary:
```
Make the connection counts explicit and extract into interface functions
around m_conn_type. Using explicit counting and switch statements where
possible should help prevent counting bugs in the future.
```

Partial backport (16/19) of core [[bitcoin/bitcoin#19316 | PR19316]]:
bitcoin/bitcoin@7f7b83d

Depends on D8725.

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Subscribers: majcosta

Differential Revision: https://reviews.bitcoinabc.org/D8726
  • Loading branch information
amitiuttarwar authored and Fabcien committed Dec 21, 2020
1 parent a0d9668 commit 5c39c9d
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 15 deletions.
40 changes: 25 additions & 15 deletions src/net.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1993,24 +1993,34 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect) {
int nOutboundFullRelay = 0;
int nOutboundBlockRelay = 0;
std::set<std::vector<uint8_t>> setConnected;

{
LOCK(cs_vNodes);
for (const CNode *pnode : vNodes) {
if (!pnode->IsInboundConn() &&
(pnode->m_conn_type != ConnectionType::MANUAL)) {
// Netgroups for inbound and addnode peers are not excluded
// because our goal here is to not use multiple of our
// limited outbound slots on a single netgroup but inbound
// and addnode peers do not use our outbound slots. Inbound
// peers also have the added issue that they're attacker
// controlled and could be used to prevent us from
// connecting to particular hosts if we used them here.
setConnected.insert(pnode->addr.GetGroup(addrman.m_asmap));
if (pnode->m_tx_relay == nullptr) {
nOutboundBlockRelay++;
} else if (pnode->m_conn_type == ConnectionType::OUTBOUND) {
nOutboundFullRelay++;
}
if (pnode->IsFullOutboundConn()) {
nOutboundFullRelay++;
}
if (pnode->IsBlockOnlyConn()) {
nOutboundBlockRelay++;
}

// Netgroups for inbound and manual peers are not excluded
// because our goal here is to not use multiple of our
// limited outbound slots on a single netgroup 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 could be used to prevent us from connecting
// to particular hosts if we used them here.
switch (pnode->m_conn_type) {
case ConnectionType::INBOUND:
case ConnectionType::MANUAL:
break;
case ConnectionType::OUTBOUND:
case ConnectionType::BLOCK_RELAY:
case ConnectionType::ADDR_FETCH:
case ConnectionType::FEELER:
setConnected.insert(
pnode->addr.GetGroup(addrman.m_asmap));
}
}
}
Expand Down
23 changes: 23 additions & 0 deletions src/net.h
Original file line number Diff line number Diff line change
Expand Up @@ -880,8 +880,16 @@ class CNode {
std::atomic_bool fPauseRecv{false};
std::atomic_bool fPauseSend{false};

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

bool IsManualConn() const { return m_conn_type == ConnectionType::MANUAL; }

bool IsBlockOnlyConn() const {
return m_conn_type == ConnectionType::BLOCK_RELAY;
}

bool IsFeelerConn() const { return m_conn_type == ConnectionType::FEELER; }

bool IsAddrFetchConn() const {
Expand All @@ -892,6 +900,21 @@ class CNode {
return m_conn_type == ConnectionType::INBOUND;
}

bool ExpectServicesFromConn() const {
switch (m_conn_type) {
case ConnectionType::INBOUND:
case ConnectionType::MANUAL:
case ConnectionType::FEELER:
return false;
case ConnectionType::OUTBOUND:
case ConnectionType::BLOCK_RELAY:
case ConnectionType::ADDR_FETCH:
return true;
}

assert(false);
}

protected:
mapMsgCmdSize mapSendBytesPerMsgCmd;
mapMsgCmdSize mapRecvBytesPerMsgCmd GUARDED_BY(cs_vRecv);
Expand Down

0 comments on commit 5c39c9d

Please sign in to comment.