Merge bitcoin#19725: [RPC] Add connection type to getpeerinfo, improv…

…e logs

a512925 [doc] Release notes (Amiti Uttarwar)
50f94b3 [rpc] Deprecate getpeerinfo addnode field (Amiti Uttarwar)
df091b9 [refactor] Rename test file to allow any getpeerinfo deprecations. (Amiti Uttarwar)
395acfa [rpc] Add connection type to getpeerinfo RPC, update tests (Amiti Uttarwar)
49c10a9 [log] Add connection type to log statement (Amiti Uttarwar)

Pull request description:

  After bitcoin#19316, we can more directly expose information about the connection type on the `getpeerinfo` RPC. Doing so also makes the existing addnode field redundant, so this PR begins the process of deprecating this field.

  This PR also includes one commit that improves a log message, as both use a shared function to return the connection type as a string.

  Suggested by sdaftuar- bitcoin#19316 (comment) & bitcoin#19316 (comment)

ACKs for top commit:
  jnewbery:
    Code review ACK a512925.
  sipa:
    utACK a512925
  guggero:
    Tested and code review ACK a512925.
  MarcoFalke:
    cr ACK a512925 🌇
  promag:
    Code review ACK a512925.

Tree-SHA512: 601a7a38aee235ee59aca690784f886dc2ae4e418b2e6422c4b58cd597376c00f74910f66920b08a08a0bec28bf8022e71a1435785ff6ba8a188954261aba78e

src/net.cpp

@@ -571,6 +571,26 @@ bool CNode::IsBlockRelayOnly() const {
     return (ignores_incoming_txs && !HasPermission(PF_RELAY)) || !IsAddrRelayPeer();
 }
 
+std::string CNode::ConnectionTypeAsString() const
+{
+    switch (m_conn_type) {
+    case ConnectionType::INBOUND:
+        return "inbound";
+    case ConnectionType::MANUAL:
+        return "manual";
+    case ConnectionType::FEELER:
+        return "feeler";
+    case ConnectionType::OUTBOUND_FULL_RELAY:
+        return "outbound-full-relay";
+    case ConnectionType::BLOCK_RELAY:
+        return "block-relay-only";
+    case ConnectionType::ADDR_FETCH:
+        return "addr-fetch";
+    } // no default case, so the compiler can warn about missing cases
+
+    assert(false);
+}
+
 std::string CNode::GetAddrName() const {
     LOCK(cs_addrName);
     return addrName;
@@ -675,6 +695,7 @@ void CNode::copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap)
         X(verifiedPubKeyHash);
     }
     X(m_masternode_connection);
+    stats.m_conn_type_string = ConnectionTypeAsString();
 }
 #undef X
 

src/net.h

@@ -143,6 +143,15 @@ enum class ConnectionType {
     ADDR_FETCH, /**< short lived connections used to solicit addrs when starting the node without a populated AddrMan */
 };
 
+const std::vector<std::string> CONNECTION_TYPE_DOC{
+    "outbound-full-relay (default automatic connections)",
+    "block-relay-only (does not relay transactions or addresses)",
+    "inbound (initiated by the peer)",
+    "manual (added via addnode RPC or -addnode/-connect configuration options)",
+    "addr-fetch (short-lived automatic connection for soliciting addresses)",
+    "feeler (short-lived automatic connection for testing addresses)"};
+
+
 class NetEventsInterface;
 class CConnman
 {
@@ -905,6 +914,7 @@ class CNodeStats
     // In case this is a verified MN, this value is the hashed operator pubkey of the MN
     uint256 verifiedPubKeyHash;
     bool m_masternode_connection;
+    std::string m_conn_type_string;
 };
 
 
@@ -1454,6 +1464,8 @@ class CNode
     //! Sets the addrName only if it was not previously set
     void MaybeSetAddrName(const std::string& addrNameIn);
 
+
+    std::string ConnectionTypeAsString() const;
     std::string GetLogString() const;
 
     bool CanRelay() const { return !m_masternode_connection || m_masternode_iqr_connection; }

src/net_processing.cpp

@@ -4024,11 +4024,7 @@ void PeerManagerImpl::ProcessMessage(
         // Making nodes which are behind NAT and can only make outgoing connections ignore
         // the getaddr message mitigates the attack.
         if (!pfrom.IsInboundConn()) {
-            LogPrint(BCLog::NET, "Ignoring \"getaddr\" from outbound connection. peer=%d\n", pfrom.GetId());
-            return;
-        }
-        if (!pfrom.IsAddrRelayPeer()) {
-            LogPrint(BCLog::NET, "Ignoring \"getaddr\" from block-relay-only connection. peer=%d\n", pfrom.GetId());
+            LogPrint(BCLog::NET, "Ignoring \"getaddr\" from %s connection. peer=%d\n", pfrom.ConnectionTypeAsString(), pfrom.GetId());
             return;
         }
 

src/rpc/net.cpp

@@ -118,7 +118,9 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
                     {RPCResult::Type::NUM, "version", "The peer version, such as 70001"},
                     {RPCResult::Type::STR, "subver", "The string version"},
                     {RPCResult::Type::BOOL, "inbound", "Inbound (true) or Outbound (false)"},
-                    {RPCResult::Type::BOOL, "addnode", "Whether connection was due to addnode/-connect or if it was an automatic/inbound connection"},
+                    {RPCResult::Type::BOOL, "addnode", "Whether connection was due to addnode/-connect or if it was an automatic/inbound connection\n"
+                                                               "(DEPRECATED, returned only if the config option -deprecatedrpc=getpeerinfo_addnode is passed)"},
+                    {RPCResult::Type::STR, "connection_type", "Type of connection: \n" + Join(CONNECTION_TYPE_DOC, ",\n") + "."},
                     {RPCResult::Type::BOOL, "masternode", "Whether connection was due to masternode connection attempt"},
                     {RPCResult::Type::NUM, "startingheight", "The starting height (block) of the peer"},
                     {RPCResult::Type::NUM, "banscore", "The ban score (DEPRECATED, returned only if config option -deprecatedrpc=banscore is passed)"},
@@ -205,7 +207,10 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
         // their ver message.
         obj.pushKV("subver", stats.cleanSubVer);
         obj.pushKV("inbound", stats.fInbound);
-        obj.pushKV("addnode", stats.m_manual_connection);
+        if (IsDeprecatedRPCEnabled("getpeerinfo_addnode")) {
+            // addnode is deprecated in v0.21 for removal in v0.22
+            obj.pushKV("addnode", stats.m_manual_connection);
+        }
         obj.pushKV("masternode", stats.m_masternode_connection);
         obj.pushKV("startingheight", stats.nStartingHeight);
         if (fStateStats) {
@@ -241,6 +246,7 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
                 recvPerMsgCmd.pushKV(i.first, i.second);
         }
         obj.pushKV("bytesrecv_per_msg", recvPerMsgCmd);
+        obj.pushKV("connection_type", stats.m_conn_type_string);
 
         ret.push_back(obj);
     }

test/functional/rpc_getpeerinfo_deprecation.py

@@ -0,0 +1,39 @@
+#!/usr/bin/env python3
+# Copyright (c) 2020 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test deprecation of getpeerinfo RPC fields."""
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import connect_nodes
+
+
+class GetpeerinfoDeprecationTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 2
+        self.extra_args = [[], ["-deprecatedrpc=banscore"]]
+
+    def run_test(self):
+        self.test_banscore_deprecation()
+        self.test_addnode_deprecation()
+
+    def test_banscore_deprecation(self):
+        self.log.info("Test getpeerinfo by default no longer returns a banscore field")
+        assert "banscore" not in self.nodes[0].getpeerinfo()[0].keys()
+
+        self.log.info("Test getpeerinfo returns banscore with -deprecatedrpc=banscore")
+        assert "banscore" in self.nodes[1].getpeerinfo()[0].keys()
+
+    def test_addnode_deprecation(self):
+        self.restart_node(1, ["-deprecatedrpc=getpeerinfo_addnode"])
+        connect_nodes(self.nodes[0], 1)
+
+        self.log.info("Test getpeerinfo by default no longer returns an addnode field")
+        assert "addnode" not in self.nodes[0].getpeerinfo()[0].keys()
+
+        self.log.info("Test getpeerinfo returns addnode with -deprecatedrpc=addnode")
+        assert "addnode" in self.nodes[1].getpeerinfo()[0].keys()
+
+
+if __name__ == "__main__":
+    GetpeerinfoDeprecationTest().main()

test/functional/rpc_net.py

@@ -163,6 +163,11 @@ def _test_getpeerinfo(self):
 
         # Check dynamically generated networks list in getpeerinfo help output.
         assert "(ipv4, ipv6, onion, i2p, not_publicly_routable)" in self.nodes[0].help("getpeerinfo")
+        assert_equal(peer_info[0][0]['connection_type'], 'inbound')
+        assert_equal(peer_info[0][1]['connection_type'], 'manual')
+
+        assert_equal(peer_info[1][0]['connection_type'], 'manual')
+        assert_equal(peer_info[1][1]['connection_type'], 'inbound')
 
     def test_service_flags(self):
         self.nodes[0].add_p2p_connection(P2PInterface(), services=(1 << 4) | (1 << 63))

test/functional/test_runner.py

@@ -274,7 +274,7 @@
     'rpc_getdescriptorinfo.py',
     'rpc_getaddressinfo_labels_purpose_deprecation.py',
     'rpc_getaddressinfo_label_deprecation.py',
-    'rpc_getpeerinfo_banscore_deprecation.py',
+    'rpc_getpeerinfo_deprecation.py',
     'rpc_help.py',
     'feature_help.py',
     'feature_blockfilterindex_prune.py'