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

01e2830 [net] Remove unnecessary default args on CNode constructor (Amiti Uttarwar)
bc5d65b [refactor] Remove IsOutboundDisconnectionCandidate (Amiti Uttarwar)
2f2e13b [net/refactor] Simplify multiple-connection checks (Amiti Uttarwar)
7f7b83d [net/refactor] Rework ThreadOpenConnections logic (Amiti Uttarwar)
35839e9 [net] Fix bug where AddrFetch connections would be counted as outbound full relay (Amiti Uttarwar)
4972c21 [net/refactor] Clarify logic for selecting connections in ThreadOpenConnections (Amiti Uttarwar)
60156f5 [net/refactor] Remove fInbound flag from CNode (Amiti Uttarwar)
7b322df [net/refactor] Remove m_addr_fetch member var from CNode (Amiti Uttarwar)
1492342 [net/refactor] Remove fFeeler flag from CNode (Amiti Uttarwar)
49efac5 [net/refactor] Remove m_manual_connection flag from CNode (Amiti Uttarwar)
d3698b5 [net/refactor] Add connection type as a member var to CNode (Amiti Uttarwar)
46578c0 [doc] Describe different connection types (Amiti Uttarwar)
442abae [net/refactor] Add AddrFetch connections to ConnectionType enum (Amiti Uttarwar)
af59feb [net/refactor] Extract m_addr_known logic from initializer list (Amiti Uttarwar)
e1bc298 [net/refactor] Add block relay only connections to ConnectionType enum (Amiti Uttarwar)
0e52a65 [net/refactor] Add feeler connections to ConnectionType enum (Amiti Uttarwar)
1521c47 [net/refactor] Add manual connections to ConnectionType enum (Amiti Uttarwar)
26304b4 [net/refactor] Introduce an enum to distinguish type of connection (Amiti Uttarwar)
3f1b714 scripted-diff: Rename OneShot to AddrFetch (Amiti Uttarwar)

Pull request description:

  **This is part 1 of bitcoin#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 01e2830
  laanwj:
    Code review ACK 01e2830, the commits are pretty straightforward to follow, and I think this is a move in the right direction overall
  vasild:
    ACK 01e2830
  sdaftuar:
    ACK 01e2830.
  fanquake:
    ACK 01e2830 - 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 01e2830

Tree-SHA512: 7bb644a6ed5849913d777ebc2ff89133ca0fbef680355a9a344e07496a979e6f9ff21a958e8eea93dcd7d5c343682b0c7174b1a3de380a4247eaae73da436e15

src/chainparams.cpp

@@ -252,7 +252,7 @@ class CMainParams : public CChainParams {
 
         // Note that of those which support the service bits prefix, most only support a subset of
         // possible options.
-        // This is fine at runtime as we'll fall back to using them as a oneshot if they don't support the
+        // This is fine at runtime as we'll fall back to using them as an addrfetch if they don't support the
         // service bits we want, but we should get them updated to support all service bits wanted by any
         // release ASAP to avoid it where possible.
         vSeeds.emplace_back("dnsseed.dash.org");

src/evo/mnauth.cpp

@@ -43,9 +43,9 @@ void CMNAuth::PushMNAUTH(CNode& peer, CConnman& connman, const CBlockIndex* tip)
     bool isV19active = llmq::utils::IsV19Active(tip);
     const CBLSPublicKeyVersionWrapper pubKey(*activeMasternodeInfo.blsPubKeyOperator, !isV19active);
     if (peer.nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) {
-        signHash = ::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.fInbound));
+        signHash = ::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.IsInboundConn()));
     } else {
-        signHash = ::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.fInbound, nOurNodeVersion));
+        signHash = ::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.IsInboundConn(), nOurNodeVersion));
     }
 
     CMNAuth mnauth;
@@ -110,9 +110,9 @@ void CMNAuth::ProcessMessage(CNode& peer, PeerManager& peerman, CConnman& connma
     ConstCBLSPublicKeyVersionWrapper pubKey(dmn->pdmnState->pubKeyOperator.Get(), !isV19active);
     // See comment in PushMNAUTH (fInbound is negated here as we're on the other side of the connection)
     if (peer.nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) {
-        signHash = ::SerializeHash(std::make_tuple(pubKey, peer.GetSentMNAuthChallenge(), !peer.fInbound));
+        signHash = ::SerializeHash(std::make_tuple(pubKey, peer.GetSentMNAuthChallenge(), !peer.IsInboundConn()));
     } else {
-        signHash = ::SerializeHash(std::make_tuple(pubKey, peer.GetSentMNAuthChallenge(), !peer.fInbound, peer.nVersion.load()));
+        signHash = ::SerializeHash(std::make_tuple(pubKey, peer.GetSentMNAuthChallenge(), !peer.IsInboundConn(), peer.nVersion.load()));
     }
     LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- constructed signHash for nVersion %d, peer=%d\n", __func__, peer.nVersion, peer.GetId());
 
@@ -123,7 +123,7 @@ void CMNAuth::ProcessMessage(CNode& peer, PeerManager& peerman, CConnman& connma
         return;
     }
 
-    if (!peer.fInbound) {
+    if (!peer.IsInboundConn()) {
         mmetaman->GetMetaInfo(mnauth.proRegTxHash)->SetLastOutboundSuccess(GetTime<std::chrono::seconds>().count());
         if (peer.m_masternode_probe_connection) {
             LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- Masternode probe successful for %s, disconnecting. peer=%d\n",
@@ -149,18 +149,18 @@ void CMNAuth::ProcessMessage(CNode& peer, PeerManager& peerman, CConnman& connma
                 if (deterministicOutbound == myProTxHash) {
                     // NOTE: do not drop inbound nodes here, mark them as probes so that
                     // they would be disconnected later in CMasternodeUtils::DoMaintenance
-                    if (pnode2->fInbound) {
+                    if (pnode2->IsInboundConn()) {
                         LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- marking old inbound for dropping it later, peer=%d\n", pnode2->GetId());
                         pnode2->m_masternode_probe_connection = true;
-                    } else if (peer.fInbound) {
+                    } else if (peer.IsInboundConn()) {
                         LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- marking new inbound for dropping it later, peer=%d\n", peer.GetId());
                         peer.m_masternode_probe_connection = true;
                     }
                 } else {
-                    if (!pnode2->fInbound) {
+                    if (!pnode2->IsInboundConn()) {
                         LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- dropping old outbound, peer=%d\n", pnode2->GetId());
                         pnode2->fDisconnect = true;
-                    } else if (!peer.fInbound) {
+                    } else if (!peer.IsInboundConn()) {
                         LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- dropping new outbound, peer=%d\n", peer.GetId());
                         peer.fDisconnect = true;
                     }

src/governance/governance.cpp

@@ -1272,7 +1272,7 @@ int CGovernanceManager::RequestGovernanceObjectVotes(Span<CNode*> vNodesCopy, CC
             // Don't try to sync any data from outbound non-relay "masternode" connections.
             // Inbound connection this early is most likely a "masternode" connection
             // initiated from another node, so skip it too.
-            if (!pnode->CanRelay() || (fMasternodeMode && pnode->fInbound)) continue;
+            if (!pnode->CanRelay() || (fMasternodeMode && pnode->IsInboundConn())) continue;
             // stop early to prevent setAskFor overflow
             {
                 LOCK(cs_main);

src/masternode/sync.cpp

@@ -157,10 +157,10 @@ void CMasternodeSync::ProcessTick()
         // Don't try to sync any data from outbound non-relay "masternode" connections.
         // Inbound connection this early is most likely a "masternode" connection
         // initiated from another node, so skip it too.
-        if (!pnode->CanRelay() || (fMasternodeMode && pnode->fInbound)) continue;
+        if (!pnode->CanRelay() || (fMasternodeMode && pnode->IsInboundConn())) continue;
 
         {
-            if ((pnode->HasPermission(PF_NOBAN) || pnode->m_manual_connection) && !netfulfilledman->HasFulfilledRequest(pnode->addr, strAllow)) {
+            if ((pnode->HasPermission(PF_NOBAN) || pnode->IsManualConn()) && !netfulfilledman->HasFulfilledRequest(pnode->addr, strAllow)) {
                 netfulfilledman->RemoveAllFulfilledRequests(pnode->addr);
                 netfulfilledman->AddFulfilledRequest(pnode->addr, strAllow);
                 LogPrintf("CMasternodeSync::ProcessTick -- skipping mnsync restrictions for peer=%d\n", pnode->GetId());
@@ -203,7 +203,7 @@ void CMasternodeSync::ProcessTick()
                         // Now that the blockchain is synced request the mempool from the connected outbound nodes if possible
                         for (auto pNodeTmp : vNodesCopy) {
                             bool fRequestedEarlier = netfulfilledman->HasFulfilledRequest(pNodeTmp->addr, "mempool-sync");
-                            if (pNodeTmp->nVersion >= 70216 && !pNodeTmp->fInbound && !fRequestedEarlier && !pNodeTmp->IsBlockRelayOnly()) {
+                            if (pNodeTmp->nVersion >= 70216 && !pNodeTmp->IsInboundConn() && !fRequestedEarlier && !pNodeTmp->IsBlockRelayOnly()) {
                                 netfulfilledman->AddFulfilledRequest(pNodeTmp->addr, "mempool-sync");
                                 connman.PushMessage(pNodeTmp, msgMaker.Make(NetMsgType::MEMPOOL));
                                 LogPrint(BCLog::MNSYNC, "CMasternodeSync::ProcessTick -- nTick %d nCurrentAsset %d -- syncing mempool from peer=%d\n", nTick, nCurrentAsset, pNodeTmp->GetId());

src/masternode/utils.cpp

@@ -30,9 +30,9 @@ void CMasternodeUtils::DoMaintenance(CConnman& connman, const CMasternodeSync& m
     // Don't disconnect masternode connections when we have less then the desired amount of outbound nodes
     int nonMasternodeCount = 0;
     connman.ForEachNode(CConnman::AllNodes, [&](CNode* pnode) {
-        if ((!pnode->fInbound &&
-            !pnode->fFeeler &&
-            !pnode->m_manual_connection &&
+        if ((!pnode->IsInboundConn() &&
+            !pnode->IsFeelerConn() &&
+            !pnode->IsManualConn() &&
             !pnode->m_masternode_connection &&
             !pnode->m_masternode_probe_connection)
             ||
@@ -62,7 +62,7 @@ void CMasternodeUtils::DoMaintenance(CConnman& connman, const CMasternodeSync& m
                     return;
                 }
                 // keep _verified_ inbound connections
-                if (pnode->fInbound) {
+                if (pnode->IsInboundConn()) {
                     return;
                 }
             } else if (GetSystemTimeInSeconds() - pnode->nTimeConnected < 5) {