From e72429ac1738ed0397d3b5f935d02e0755b7aaf7 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sat, 26 Sep 2020 17:24:33 +0200 Subject: [PATCH] (Partial) Merge #19725: [RPC] Add connection type to getpeerinfo, improve logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a512925e19a70d7f6b80ac530a169f45ffaafa1c [doc] Release notes (Amiti Uttarwar) 50f94b34a33c954f6e207f509c93d33267a5c3e2 [rpc] Deprecate getpeerinfo addnode field (Amiti Uttarwar) df091b9b509f0b10e4315c0bfa2da0cc0c31c22f [refactor] Rename test file to allow any getpeerinfo deprecations. (Amiti Uttarwar) 395acfa83a5436790c1a722a5609ac9d48df235f [rpc] Add connection type to getpeerinfo RPC, update tests (Amiti Uttarwar) 49c10a9ca40967d28ae16dfea9cccc6f3a6624a1 [log] Add connection type to log statement (Amiti Uttarwar) Pull request description: After #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- https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468001604 & https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468018093 ACKs for top commit: jnewbery: Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c. sipa: utACK a512925e19a70d7f6b80ac530a169f45ffaafa1c guggero: Tested and code review ACK a512925e. MarcoFalke: cr ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c 🌇 promag: Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c. Tree-SHA512: 601a7a38aee235ee59aca690784f886dc2ae4e418b2e6422c4b58cd597376c00f74910f66920b08a08a0bec28bf8022e71a1435785ff6ba8a188954261aba78e --- src/net.cpp | 21 ++++++++++++++ src/net.h | 12 ++++++++ src/net_processing.cpp | 6 +--- src/rpc/net.cpp | 10 +++++-- .../functional/rpc_getpeerinfo_deprecation.py | 28 +++++++++++++++++++ test/functional/rpc_net.py | 5 ++++ test/functional/test_runner.py | 1 + 7 files changed, 76 insertions(+), 7 deletions(-) create mode 100755 test/functional/rpc_getpeerinfo_deprecation.py diff --git a/src/net.cpp b/src/net.cpp index a1663b8f3a320f..6f183f66effaa9 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -580,6 +580,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; @@ -684,6 +704,7 @@ void CNode::copyStats(CNodeStats &stats, const std::vector &m_asmap) X(verifiedPubKeyHash); } X(m_masternode_connection); + stats.m_conn_type_string = ConnectionTypeAsString(); } #undef X diff --git a/src/net.h b/src/net.h index 103e3e452f0f9f..037a6537ad40f6 100644 --- a/src/net.h +++ b/src/net.h @@ -144,6 +144,15 @@ enum class ConnectionType { ADDR_FETCH, /**< short lived connections used to solicit addrs when starting the node without a populated AddrMan */ }; +const std::vector 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 { @@ -918,6 +927,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; }; @@ -1471,6 +1481,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; } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a03c58f2cee19b..ae700a6fc12a1f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4033,11 +4033,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; } diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index fa7d7981db807c..32b53c8df028a7 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -116,7 +116,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"}, @@ -203,7 +205,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) { @@ -236,6 +241,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); } diff --git a/test/functional/rpc_getpeerinfo_deprecation.py b/test/functional/rpc_getpeerinfo_deprecation.py new file mode 100755 index 00000000000000..b049d3c027f9f1 --- /dev/null +++ b/test/functional/rpc_getpeerinfo_deprecation.py @@ -0,0 +1,28 @@ +#!/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 + +class GetpeerinfoDeprecationTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 2 + + def run_test(self): + self.test_addnode_deprecation() + + def test_addnode_deprecation(self): + self.restart_node(1, ["-deprecatedrpc=getpeerinfo_addnode"]) + self.connect_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() diff --git a/test/functional/rpc_net.py b/test/functional/rpc_net.py index d086db4e596abb..dfcebd6f67b73d 100755 --- a/test/functional/rpc_net.py +++ b/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)) diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 9165e008bae4da..b4fed90e8cb473 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -279,6 +279,7 @@ 'rpc_getdescriptorinfo.py', 'rpc_getaddressinfo_labels_purpose_deprecation.py', 'rpc_getaddressinfo_label_deprecation.py', + 'rpc_getpeerinfo_deprecation.py', 'rpc_help.py', 'feature_help.py', 'feature_blockfilterindex_prune.py'