highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-11-12 23:18:14 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge #19316: [net] Cleanup logic around connection types

Browse Source 
01e283068b [net] Remove unnecessary default args on CNode constructor (Amiti Uttarwar)
bc5d65b3ca [refactor] Remove IsOutboundDisconnectionCandidate (Amiti Uttarwar)
2f2e13b6c2 [net/refactor] Simplify multiple-connection checks (Amiti Uttarwar)
7f7b83deb2 [net/refactor] Rework ThreadOpenConnections logic (Amiti Uttarwar)
35839e963b [net] Fix bug where AddrFetch connections would be counted as outbound full relay (Amiti Uttarwar)
4972c21b67 [net/refactor] Clarify logic for selecting connections in ThreadOpenConnections (Amiti Uttarwar)
60156f5fc4 [net/refactor] Remove fInbound flag from CNode (Amiti Uttarwar)
7b322df629 [net/refactor] Remove m_addr_fetch member var from CNode (Amiti Uttarwar)
14923422b0 [net/refactor] Remove fFeeler flag from CNode (Amiti Uttarwar)
49efac5cae [net/refactor] Remove m_manual_connection flag from CNode (Amiti Uttarwar)
d3698b5ee3 [net/refactor] Add connection type as a member var to CNode (Amiti Uttarwar)
46578c03e9 [doc] Describe different connection types (Amiti Uttarwar)
442abae2ba [net/refactor] Add AddrFetch connections to ConnectionType enum (Amiti Uttarwar)
af59feb052 [net/refactor] Extract m_addr_known logic from initializer list (Amiti Uttarwar)
e1bc29812d [net/refactor] Add block relay only connections to ConnectionType enum (Amiti Uttarwar)
0e52a659a2 [net/refactor] Add feeler connections to ConnectionType enum (Amiti Uttarwar)
1521c47438 [net/refactor] Add manual connections to ConnectionType enum (Amiti Uttarwar)
26304b4100 [net/refactor] Introduce an enum to distinguish type of connection (Amiti Uttarwar)
3f1b7140e9 scripted-diff: Rename OneShot to AddrFetch (Amiti Uttarwar)

Pull request description:

  **This is part 1 of #19315, which enables the ability to test `outbound` and `block-relay-only` connections from the functional tests.** Please see that PR for more information of overall functionality.

  **This PR simplifies how we manage different connection types.** It introduces an enum with the various types of connections so we can explicitly define the connection type. The existing system relies on a series of independent flags, then has asserts scattered around to ensure that conflicting flags are not enabled at the same time. I find this approach to be both brittle and confusing. While making these changes, I found a small bug due to the silent assumptions.

  This PR also proposes a rename from `OneShot` to `AddrFetch`. I find the name `OneShot` to be very confusing, especially when we also have `onetry` manual connections. Everyone I've talked to offline has agreed that the name is confusing, so I propose a potential alternative. I think this is a good opportunity for a rename since I'm creating an enum to explicitly define the connection types.
  (some context for the unfamiliar: `oneshot` or `addrfetch` connections are short-lived connections created on startup. They connect to the seed peers, send a `getaddr` to solicit addresses, then close the connection.)

  Overview of this PR:
  * rename `oneshot` to `addrfetch`
  * introduce `ConnectionType` enum
  * one by one, add different connection types to the enum
  * expose the `conn_type` on CNode, and use this to reduce reliance on flags (& asserts)
  * fix the bug in counting different type of connections
  * some additional cleanup to simplify logic and make expectations explicit/inclusive rather than implicit/exclusive.

ACKs for top commit:
  jnewbery:
    utACK 01e283068b
  laanwj:
    Code review ACK 01e283068b, the commits are pretty straightforward to follow, and I think this is a move in the right direction overall
  vasild:
    ACK 01e283068
  sdaftuar:
    ACK 01e283068b.
  fanquake:
    ACK 01e283068b - I don't have as much experience with the networking code but these changes look fairly straight forward, the new code seems more robust/understandable and the additional documentation is great. I'm glad that a followup branch is already underway. There might be some more review comments here later today, so keep an eye on the discussion, however I'm going to merge this now.
  jb55:
    wow this code was messy before... ACK 01e283068b

Tree-SHA512: 7bb644a6ed5849913d777ebc2ff89133ca0fbef680355a9a344e07496a979e6f9ff21a958e8eea93dcd7d5c343682b0c7174b1a3de380a4247eaae73da436e15
This commit is contained in:
fanquake
2020-08-12 09:41:24 +08:00
parent cb1ee1551c 01e283068b
commit ce3bdd0ed1
9 changed files with 186 additions and 140 deletions
Download Patch File Download Diff File Expand all files Collapse all files

86
src/net.h
View File

@@ -117,6 +117,17 @@ struct CSerializedNetMsg
std::string m_type;
};
/** Different types of connections to a peer. This enum encapsulates the
* 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 */
};
class NetEventsInterface;
class CConnman
@@ -201,7 +212,7 @@ public:
bool GetNetworkActive() const { return fNetworkActive; };
bool GetUseAddrmanOutgoing() const { return m_use_addrman_outgoing; };
void SetNetworkActive(bool active);
void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, bool fOneShot = false, bool fFeeler = false, bool manual_connection = false, bool block_relay_only = false);
void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, ConnectionType conn_type = ConnectionType::OUTBOUND);
bool CheckIncomingNonce(uint64_t nonce);
bool ForNode(NodeId id, std::function<bool(CNode* pnode)> func);
@@ -351,8 +362,8 @@ private:
bool Bind(const CService& addr, unsigned int flags, NetPermissionFlags permissions);
bool InitBinds(const std::vector<CService>& binds, const std::vector<NetWhitebindPermissions>& whiteBinds);
void ThreadOpenAddedConnections();
void AddOneShot(const std::string& strDest);
void ProcessOneShot();
void AddAddrFetch(const std::string& strDest);
void ProcessAddrFetch();
void ThreadOpenConnections(std::vector<std::string> connect);
void ThreadMessageHandler();
void AcceptConnection(const ListenSocket& hListenSocket);
@@ -373,7 +384,7 @@ private:
CNode* FindNode(const CService& addr);
bool AttemptToEvictConnection();
CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, bool manual_connection, bool block_relay_only);
CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type);
void AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr) const;
void DeleteNode(CNode* pnode);
@@ -416,8 +427,8 @@ private:
std::atomic<bool> fNetworkActive{true};
bool fAddressesInitialized{false};
CAddrMan addrman;
std::deque<std::string> vOneShots GUARDED_BY(cs_vOneShots);
RecursiveMutex cs_vOneShots;
std::deque<std::string> m_addr_fetches GUARDED_BY(m_addr_fetches_mutex);
RecursiveMutex m_addr_fetches_mutex;
std::vector<std::string> vAddedNodes GUARDED_BY(cs_vAddedNodes);
RecursiveMutex cs_vAddedNodes;
std::vector<CNode*> vNodes GUARDED_BY(cs_vNodes);
@@ -798,12 +809,8 @@ public:
}
// This boolean is unusued in actual processing, only present for backward compatibility at RPC/QT level
bool m_legacyWhitelisted{false};
bool fFeeler{false}; // If true this node is being used as a short lived feeler.
bool fOneShot{false};
bool m_manual_connection{false};
bool fClient{false}; // set by version message
bool m_limited_node{false}; //after BIP159, set by version message
const bool fInbound;
std::atomic_bool fSuccessfullyConnected{false};
// Setting fDisconnect to true will cause the node to be disconnected the
// next time DisconnectNodes() runs
@@ -816,6 +823,60 @@ public:
std::atomic_bool fPauseRecv{false};
std::atomic_bool fPauseSend{false};
bool IsOutboundOrBlockRelayConn() const {
switch(m_conn_type) {
case ConnectionType::OUTBOUND:
case ConnectionType::BLOCK_RELAY:
return true;
case ConnectionType::INBOUND:
case ConnectionType::MANUAL:
case ConnectionType::ADDR_FETCH:
case ConnectionType::FEELER:
return false;
}
assert(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 {
return m_conn_type == ConnectionType::ADDR_FETCH;
}
bool IsInboundConn() const {
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);
@@ -826,7 +887,7 @@ public:
// flood relay
std::vector<CAddress> vAddrToSend;
const std::unique_ptr<CRollingBloomFilter> m_addr_known;
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};
@@ -890,7 +951,7 @@ public:
std::set<uint256> orphan_work_set;
CNode(NodeId id, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress &addrBindIn, const std::string &addrNameIn = "", bool fInboundIn = false, bool block_relay_only = false);
CNode(NodeId id, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress &addrBindIn, const std::string &addrNameIn, ConnectionType conn_type_in);
~CNode();
CNode(const CNode&) = delete;
CNode& operator=(const CNode&) = delete;
@@ -898,6 +959,7 @@ public:
private:
const NodeId id;
const uint64_t nLocalHostNonce;
const ConnectionType m_conn_type;
//! Services offered to this peer.
//!