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 Dec 29, 2023
1 parent 3c58927 commit cdbb1ed
Show file tree
Hide file tree
Showing 8 changed files with 163 additions and 101 deletions.
2 changes: 1 addition & 1 deletion src/llmq/instantsend.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1465,7 +1465,7 @@ void CInstantSendManager::AskNodesForLockedTx(const uint256& txid, const CConnma
if (nodesToAskFor.size() >= 4) {
return;
}
if (pnode->IsAddrRelayPeer()) {
if (pnode->RelayAddrsWithConn()) {
LOCK(pnode->m_tx_relay->cs_tx_inventory);
if (pnode->m_tx_relay->filterInventoryKnown.contains(txid)) {
pnode->AddRef();
Expand Down
82 changes: 36 additions & 46 deletions src/net.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,7 @@ bool CNode::IsBlockRelayOnly() const {
// Stop processing non-block data early if
// 1) We are in blocks only mode and peer has no relay permission
// 2) This peer is a block-relay-only peer
return (ignores_incoming_txs && !HasPermission(PF_RELAY)) || !IsAddrRelayPeer();
return (ignores_incoming_txs && !HasPermission(PF_RELAY)) || !RelayAddrsWithConn();
}

std::string CNode::ConnectionTypeAsString() const
Expand Down Expand Up @@ -646,7 +646,7 @@ void CNode::copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap)
X(addrBind);
stats.m_network = ConnectedThroughNetwork();
stats.m_mapped_as = addr.GetMappedAS(m_asmap);
if (IsAddrRelayPeer()) {
if (RelayAddrsWithConn()) {
LOCK(m_tx_relay->cs_filter);
stats.fRelayTxes = m_tx_relay->fRelayTxes;
} else {
Expand Down Expand Up @@ -1047,7 +1047,7 @@ bool CConnman::AttemptToEvictConnection()

bool peer_relay_txes = false;
bool peer_filter_not_null = false;
if (node->IsAddrRelayPeer()) {
if (node->RelayAddrsWithConn()) {
LOCK(node->m_tx_relay->cs_filter);
peer_relay_txes = node->m_tx_relay->fRelayTxes;
peer_filter_not_null = node->m_tx_relay->pfilter != nullptr;
Expand Down Expand Up @@ -2307,16 +2307,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 @@ -2331,28 +2331,32 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
}
}

// Feeler Connections
//
// Design goals:
// * Increase the number of connectable addresses in the tried table.
//
// Method:
// * Choose a random address from new and attempt to connect to it if we can connect
// successfully it is added to tried.
// * Start attempting feeler connections only after node finishes making outbound
// 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 @@ -2441,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 Expand Up @@ -3792,7 +3779,7 @@ void CConnman::RelayInvFiltered(CInv &inv, const CTransaction& relatedTx, const
{
LOCK(cs_vNodes);
for (const auto& pnode : vNodes) {
if (pnode->nVersion < minProtoVersion || !pnode->CanRelay() || !pnode->IsAddrRelayPeer()) {
if (pnode->nVersion < minProtoVersion || !pnode->CanRelay() || !pnode->RelayAddrsWithConn()) {
continue;
}
{
Expand All @@ -3812,7 +3799,7 @@ void CConnman::RelayInvFiltered(CInv &inv, const uint256& relatedTxHash, const i
{
LOCK(cs_vNodes);
for (const auto& pnode : vNodes) {
if (pnode->nVersion < minProtoVersion || !pnode->CanRelay() || !pnode->IsAddrRelayPeer()) {
if (pnode->nVersion < minProtoVersion || !pnode->CanRelay() || !pnode->RelayAddrsWithConn()) {
continue;
}
{
Expand Down Expand Up @@ -3942,7 +3929,10 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const
addrName = addrNameIn == "" ? addr.ToStringIPPort() : addrNameIn;
hashContinue = uint256();

m_addr_known = std::make_unique<CRollingBloomFilter>(5000, 0.001);
if (conn_type_in != ConnectionType::BLOCK_RELAY) {
m_addr_known = std::make_unique<CRollingBloomFilter>(5000, 0.001);
}

for (const std::string &msg : getAllNetMessageTypes())
mapRecvBytesPerMsgCmd[msg] = 0;
mapRecvBytesPerMsgCmd[NET_MESSAGE_COMMAND_OTHER] = 0;
Expand Down
87 changes: 65 additions & 22 deletions src/net.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,12 +136,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 @@ -260,7 +302,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, 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 @@ -579,7 +621,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 @@ -1152,22 +1194,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 @@ -1190,17 +1232,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 @@ -1215,16 +1263,11 @@ 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};

// Don't relay addr messages to peers that we connect to as block-relay-only
// peers (to prevent adversaries from inferring these links from addr
// traffic).
bool IsAddrRelayPeer() const { return m_addr_known != nullptr; }

bool IsBlockRelayOnly() const;

// List of block ids we still have announce.
Expand Down Expand Up @@ -1270,7 +1313,7 @@ class CNode
};

// in bitcoin: m_tx_relay == nullptr if we're not relaying transactions with this peer
// in dash: m_tx_relay should never be nullptr, use `IsAddrRelayPeer() == false` instead
// in dash: m_tx_relay should never be nullptr, use `RelayAddrsWithConn() == false` instead
std::unique_ptr<TxRelay> m_tx_relay{std::make_unique<TxRelay>()};

// Used for headers announcements - unfiltered blocks to relay
Expand Down
Loading

0 comments on commit cdbb1ed

Please sign in to comment.