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

01e283068b9e6214f2d77a2f772a4244ebfe2274 [net] Remove unnecessary default args on CNode constructor (Amiti Uttarwar)
bc5d65b3ca41eebb1738fdda4451d1466e77772e [refactor] Remove IsOutboundDisconnectionCandidate (Amiti Uttarwar)
2f2e13b6c2c8741ca9d825eaaef736ede484bc85 [net/refactor] Simplify multiple-connection checks (Amiti Uttarwar)
7f7b83deb2427599c129f4ff581d4d045461e459 [net/refactor] Rework ThreadOpenConnections logic (Amiti Uttarwar)
35839e963bf61d2da0d12f5b8cea74ac0e0fbd7b [net] Fix bug where AddrFetch connections would be counted as outbound full relay (Amiti Uttarwar)
4972c21b671ff73f13a1b5053338b6abbdb471b5 [net/refactor] Clarify logic for selecting connections in ThreadOpenConnections (Amiti Uttarwar)
60156f5fc40d56bb532278f16ce632c5a8b8035e [net/refactor] Remove fInbound flag from CNode (Amiti Uttarwar)
7b322df6296609570e368e5f326979279041c11f [net/refactor] Remove m_addr_fetch member var from CNode (Amiti Uttarwar)
14923422b08ac4b21b35c426bf0e1b9e7c97983b [net/refactor] Remove fFeeler flag from CNode (Amiti Uttarwar)
49efac5cae7333c6700d9b737d09fae0f3f4d7fa [net/refactor] Remove m_manual_connection flag from CNode (Amiti Uttarwar)
d3698b5ee309cf0f0cdfb286d6b30a256d7deae5 [net/refactor] Add connection type as a member var to CNode (Amiti Uttarwar)
46578c03e92a55925308363ccdad04dcfc820d96 [doc] Describe different connection types (Amiti Uttarwar)
442abae2bac7bff85886143df01e14215532b974 [net/refactor] Add AddrFetch connections to ConnectionType enum (Amiti Uttarwar)
af59feb05235ecb85ec9d75b09c66e71268c9889 [net/refactor] Extract m_addr_known logic from initializer list (Amiti Uttarwar)
e1bc29812ddf1d946bc5acca406a7ed2dca064a6 [net/refactor] Add block relay only connections to ConnectionType enum (Amiti Uttarwar)
0e52a659a2de915fc3dce37fc8fac39be1c8b6fa [net/refactor] Add feeler connections to ConnectionType enum (Amiti Uttarwar)
1521c47438537e192230486dffcec0228a53878d [net/refactor] Add manual connections to ConnectionType enum (Amiti Uttarwar)
26304b4100201754fb32440bec3e3b78cd3f0e6d [net/refactor] Introduce an enum to distinguish type of connection (Amiti Uttarwar)
3f1b7140e95d0f8f958cb35f31c3d964c57e484d 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 01e283068b9e6214f2d77a2f772a4244ebfe2274
  laanwj:
    Code review ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274, the commits are pretty straightforward to follow, and I think this is a move in the right direction overall
  vasild:
    ACK 01e283068
  sdaftuar:
    ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274.
  fanquake:
    ACK 01e283068b9e6214f2d77a2f772a4244ebfe2274 - 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 01e283068b9e6214f2d77a2f772a4244ebfe2274

Tree-SHA512: 7bb644a6ed5849913d777ebc2ff89133ca0fbef680355a9a344e07496a979e6f9ff21a958e8eea93dcd7d5c343682b0c7174b1a3de380a4247eaae73da436e15

1 files changed, 20 insertions, 28 deletions

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 7c2d7335a0..7188bd2154 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -479,7 +479,7 @@ static void UpdatePreferredDownload(const CNode& node, CNodeState* state) EXCLUS
     nPreferredDownload -= state->fPreferredDownload;
 
     // Whether this node should be marked as a preferred download node.
-    state->fPreferredDownload = (!node.fInbound || node.HasPermission(PF_NOBAN)) && !node.fOneShot && !node.fClient;
+    state->fPreferredDownload = (!node.IsInboundConn() || node.HasPermission(PF_NOBAN)) && !node.IsAddrFetchConn() && !node.fClient;
 
     nPreferredDownload += state->fPreferredDownload;
 }
@@ -833,22 +833,15 @@ void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds)
     if (state) state->m_last_block_announcement = time_in_seconds;
 }
 
-// Returns true for outbound peers, excluding manual connections, feelers, and
-// one-shots.
-static bool IsOutboundDisconnectionCandidate(const CNode& node)
-{
-    return !(node.fInbound || node.m_manual_connection || node.fFeeler || node.fOneShot);
-}
-
 void PeerLogicValidation::InitializeNode(CNode *pnode) {
     CAddress addr = pnode->addr;
     std::string addrName = pnode->GetAddrName();
     NodeId nodeid = pnode->GetId();
     {
         LOCK(cs_main);
-        mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, std::move(addrName), pnode->fInbound, pnode->m_manual_connection));
+        mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, std::move(addrName), pnode->IsInboundConn(), pnode->IsManualConn()));
     }
-    if(!pnode->fInbound)
+    if(!pnode->IsInboundConn())
         PushNodeVersion(*pnode, *connman, GetTime());
 }
 
@@ -1982,14 +1975,14 @@ static void ProcessHeadersMessage(CNode& pfrom, CConnman& connman, ChainstateMan
                 // until we have a headers chain that has at least
                 // nMinimumChainWork, even if a peer has a chain past our tip,
                 // as an anti-DoS measure.
-                if (IsOutboundDisconnectionCandidate(pfrom)) {
+                if (pfrom.IsOutboundOrBlockRelayConn()) {
                     LogPrintf("Disconnecting outbound peer %d -- headers chain has insufficient work\n", pfrom.GetId());
                     pfrom.fDisconnect = true;
                 }
             }
         }
 
-        if (!pfrom.fDisconnect && IsOutboundDisconnectionCandidate(pfrom) && nodestate->pindexBestKnownBlock != nullptr && pfrom.m_tx_relay != nullptr) {
+        if (!pfrom.fDisconnect && pfrom.IsOutboundOrBlockRelayConn() && nodestate->pindexBestKnownBlock != nullptr && pfrom.m_tx_relay != nullptr) {
             // If this is an outbound full-relay peer, check to see if we should protect
             // it from the bad/lagging chain logic.
             // Note that block-relay-only peers are already implicitly protected, so we
@@ -2352,11 +2345,11 @@ void ProcessMessage(
         vRecv >> nVersion >> nServiceInt >> nTime >> addrMe;
         nSendVersion = std::min(nVersion, PROTOCOL_VERSION);
         nServices = ServiceFlags(nServiceInt);
-        if (!pfrom.fInbound)
+        if (!pfrom.IsInboundConn())
         {
             connman.SetServices(pfrom.addr, nServices);
         }
-        if (!pfrom.fInbound && !pfrom.fFeeler && !pfrom.m_manual_connection && !HasAllDesirableServiceFlags(nServices))
+        if (pfrom.ExpectServicesFromConn() && !HasAllDesirableServiceFlags(nServices))
         {
             LogPrint(BCLog::NET, "peer=%d does not offer the expected services (%08x offered, %08x expected); disconnecting\n", pfrom.GetId(), nServices, GetDesirableServiceFlags(nServices));
             pfrom.fDisconnect = true;
@@ -2383,20 +2376,20 @@ void ProcessMessage(
         if (!vRecv.empty())
             vRecv >> fRelay;
         // Disconnect if we connected to ourself
-        if (pfrom.fInbound && !connman.CheckIncomingNonce(nNonce))
+        if (pfrom.IsInboundConn() && !connman.CheckIncomingNonce(nNonce))
         {
             LogPrintf("connected to self at %s, disconnecting\n", pfrom.addr.ToString());
             pfrom.fDisconnect = true;
             return;
         }
 
-        if (pfrom.fInbound && addrMe.IsRoutable())
+        if (pfrom.IsInboundConn() && addrMe.IsRoutable())
         {
             SeenLocal(addrMe);
         }
 
         // Be shy and don't send version until we hear
-        if (pfrom.fInbound)
+        if (pfrom.IsInboundConn())
             PushNodeVersion(pfrom, connman, GetAdjustedTime());
 
         if (nVersion >= WTXID_RELAY_VERSION) {
@@ -2440,7 +2433,7 @@ void ProcessMessage(
         UpdatePreferredDownload(pfrom, State(pfrom.GetId()));
         }
 
-        if (!pfrom.fInbound && pfrom.IsAddrRelayPeer())
+        if (!pfrom.IsInboundConn() && pfrom.IsAddrRelayPeer())
         {
             // Advertise our address
             if (fListen && !::ChainstateActive().IsInitialBlockDownload())
@@ -2484,8 +2477,7 @@ void ProcessMessage(
         }
 
         // Feeler connections exist only to verify if address is online.
-        if (pfrom.fFeeler) {
-            assert(pfrom.fInbound == false);
+        if (pfrom.IsFeelerConn()) {
             pfrom.fDisconnect = true;
         }
         return;
@@ -2505,7 +2497,7 @@ void ProcessMessage(
     {
         pfrom.SetRecvVersion(std::min(pfrom.nVersion.load(), PROTOCOL_VERSION));
 
-        if (!pfrom.fInbound) {
+        if (!pfrom.IsInboundConn()) {
             // Mark this node as currently connected, so we update its timestamp later.
             LOCK(cs_main);
             State(pfrom.GetId())->fCurrentlyConnected = true;
@@ -2614,7 +2606,7 @@ void ProcessMessage(
         connman.AddNewAddresses(vAddrOk, pfrom.addr, 2 * 60 * 60);
         if (vAddr.size() < 1000)
             pfrom.fGetAddr = false;
-        if (pfrom.fOneShot)
+        if (pfrom.IsAddrFetchConn())
             pfrom.fDisconnect = true;
         return;
     }
@@ -3509,7 +3501,7 @@ void ProcessMessage(
         // to users' AddrMan and later request them by sending getaddr messages.
         // Making nodes which are behind NAT and can only make outgoing connections ignore
         // the getaddr message mitigates the attack.
-        if (!pfrom.fInbound) {
+        if (!pfrom.IsInboundConn()) {
             LogPrint(BCLog::NET, "Ignoring \"getaddr\" from outbound connection. peer=%d\n", pfrom.GetId());
             return;
         }
@@ -3792,7 +3784,7 @@ bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode)
         return false;
     }
 
-    if (pnode.m_manual_connection) {
+    if (pnode.IsManualConn()) {
         // We never disconnect or discourage manual peers for bad behavior
         LogPrintf("Warning: not punishing manually connected peer %d!\n", peer_id);
         return false;
@@ -3913,7 +3905,7 @@ void PeerLogicValidation::ConsiderEviction(CNode& pto, int64_t time_in_seconds)
     CNodeState &state = *State(pto.GetId());
     const CNetMsgMaker msgMaker(pto.GetSendVersion());
 
-    if (!state.m_chain_sync.m_protect && IsOutboundDisconnectionCandidate(pto) && state.fSyncStarted) {
+    if (!state.m_chain_sync.m_protect && pto.IsOutboundOrBlockRelayConn() && state.fSyncStarted) {
         // This is an outbound peer subject to disconnection if they don't
         // announce a block with as much work as the current tip within
         // CHAIN_SYNC_TIMEOUT + HEADERS_RESPONSE_TIME seconds (note: if
@@ -3975,7 +3967,7 @@ void PeerLogicValidation::EvictExtraOutboundPeers(int64_t time_in_seconds)
             AssertLockHeld(cs_main);
 
             // Ignore non-outbound peers, or nodes marked for disconnect already
-            if (!IsOutboundDisconnectionCandidate(*pnode) || pnode->fDisconnect) return;
+            if (!pnode->IsOutboundOrBlockRelayConn() || pnode->fDisconnect) return;
             CNodeState *state = State(pnode->GetId());
             if (state == nullptr) return; // shouldn't be possible, but just in case
             // Don't evict our protected peers
@@ -4153,7 +4145,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
         // Start block sync
         if (pindexBestHeader == nullptr)
             pindexBestHeader = ::ChainActive().Tip();
-        bool fFetch = state.fPreferredDownload || (nPreferredDownload == 0 && !pto->fClient && !pto->fOneShot); // Download if this is a nice peer, or we have no nice peers and this one might do.
+        bool fFetch = state.fPreferredDownload || (nPreferredDownload == 0 && !pto->fClient && !pto->IsAddrFetchConn()); // Download if this is a nice peer, or we have no nice peers and this one might do.
         if (!state.fSyncStarted && !pto->fClient && !fImporting && !fReindex) {
             // Only actively request headers from a single peer, unless we're close to today.
             if ((nSyncStarted == 0 && fFetch) || pindexBestHeader->GetBlockTime() > GetAdjustedTime() - 24 * 60 * 60) {
@@ -4338,7 +4330,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
                 bool fSendTrickle = pto->HasPermission(PF_NOBAN);
                 if (pto->m_tx_relay->nNextInvSend < current_time) {
                     fSendTrickle = true;
-                    if (pto->fInbound) {
+                    if (pto->IsInboundConn()) {
                         pto->m_tx_relay->nNextInvSend = std::chrono::microseconds{connman->PoissonNextSendInbound(nNow, INVENTORY_BROADCAST_INTERVAL)};
                     } else {
                         // Use half the delay for outbound peers, as there is less privacy concern for them.