From 42d4f9a9b932c0045aa52a2e6b5c7eee718d5817 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 28 Dec 2020 22:40:33 +0100 Subject: [PATCH] partial Merge #20755: [rpc] Remove deprecated fields from getpeerinfo BACKPORT NOTE: the field `banscore` is used by functional test p2p_quorum_data.py and can't be removed now 454a4088a87eac5878070b26d13d5574da891877 [doc] Add release notes for removed getpeerinfo fields. (Amiti Uttarwar) b1a936d4ae7dd9030b0720ef05579a90ce2894f1 [rpc] Remove deprecated "whitelisted" field from getpeerinfo (Amiti Uttarwar) 094c3beaa47c909070607e94f2544ed1472ddb17 [rpc] Remove deprecated "banscore" field from getpeerinfo (Amiti Uttarwar) 537053336fbc1b633e7c99286c3e3492eaca1e9d [rpc] Remove deprecated "addnode" field from getpeerinfo (Amiti Uttarwar) Pull request description: This PR removes support for 3 fields on the `getpeerinfo` RPC that were deprecated in v0.21- `addnode`, `banscore` & `whitelisted`. ACKs for top commit: sipa: utACK 454a4088a87eac5878070b26d13d5574da891877 jnewbery: ACK 454a4088a87eac5878070b26d13d5574da891877. Tree-SHA512: ccc0e90c0763eeb8529cf0c46162dbaca3f7773981b3b52d9925166ea7421aed086795d56b320e16c9340f68862388785f52a9b78314865070917b33180d7cd6 --- doc/release-notes-20755.md | 7 +++ src/net.cpp | 4 -- src/net.h | 4 -- src/rpc/net.cpp | 12 ----- test/functional/p2p_blocksonly.py | 2 +- test/functional/p2p_permissions.py | 50 ++++--------------- test/functional/rpc_external_queue.py | 0 .../functional/rpc_getpeerinfo_deprecation.py | 11 ---- 8 files changed, 19 insertions(+), 71 deletions(-) create mode 100644 doc/release-notes-20755.md mode change 100644 => 100755 test/functional/rpc_external_queue.py diff --git a/doc/release-notes-20755.md b/doc/release-notes-20755.md new file mode 100644 index 0000000000..f0b4ca0cd3 --- /dev/null +++ b/doc/release-notes-20755.md @@ -0,0 +1,7 @@ +Updated RPCs +------------ +- `getpeerinfo` no longer returns the following fields: `addnode`, + and `whitelisted`, which were previously deprecated in v21. Instead of + `addnode`, the `connection_type` field returns manual. Instead of + `whitelisted`, the `permissions` field indicates if the peer has special + privileges. (#20755) diff --git a/src/net.cpp b/src/net.cpp index 7bd90d0973..34a3ec0ba9 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -736,7 +736,6 @@ void CNode::CopyStats(CNodeStats& stats) X(cleanSubVer); } stats.fInbound = IsInboundConn(); - stats.m_manual_connection = IsManualConn(); X(m_bip152_highbandwidth_to); X(m_bip152_highbandwidth_from); { @@ -752,7 +751,6 @@ void CNode::CopyStats(CNodeStats& stats) stats.m_transport_type = info.transport_type; if (info.session_id) stats.m_session_id = HexStr(*info.session_id); } - X(m_legacyWhitelisted); X(m_permission_flags); X(m_last_ping_time); @@ -2010,8 +2008,6 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, .use_v2transport = use_v2transport, }); pnode->AddRef(); - // If this flag is present, the user probably expect that RPC and QT report it as whitelisted (backward compatibility) - pnode->m_legacyWhitelisted = legacyWhitelisted; m_msgproc->InitializeNode(*pnode, nodeServices); { diff --git a/src/net.h b/src/net.h index effa460e51..41f446c705 100644 --- a/src/net.h +++ b/src/net.h @@ -241,7 +241,6 @@ public: int nVersion; std::string cleanSubVer; bool fInbound; - bool m_manual_connection; bool m_bip152_highbandwidth_to; bool m_bip152_highbandwidth_from; int m_starting_height; @@ -250,7 +249,6 @@ public: uint64_t nRecvBytes; mapMsgTypeSize mapRecvBytesPerMsgType; NetPermissionFlags m_permission_flags; - bool m_legacyWhitelisted; std::chrono::microseconds m_last_ping_time; std::chrono::microseconds m_min_ping_time; // Our address, as reported by the peer @@ -789,8 +787,6 @@ public: bool HasPermission(NetPermissionFlags permission) const { return NetPermissions::HasFlag(m_permission_flags, permission); } - // This boolean is unusued in actual processing, only present for backward compatibility at RPC/QT level - bool m_legacyWhitelisted{false}; /** fSuccessfullyConnected is set to true on receiving VERACK from the peer. */ std::atomic_bool fSuccessfullyConnected{false}; // Setting fDisconnect to true will cause the node to be disconnected the diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index cb69c3112a..8d3dc39846 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -137,8 +137,6 @@ static RPCHelpMan getpeerinfo() {RPCResult::Type::BOOL, "inbound", "Inbound (true) or Outbound (false)"}, {RPCResult::Type::BOOL, "bip152_hb_to", "Whether we selected peer as (compact blocks) high-bandwidth peer"}, {RPCResult::Type::BOOL, "bip152_hb_from", "Whether peer selected us as (compact blocks) high-bandwidth peer"}, - {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::BOOL, "masternode", "Whether connection was due to masternode connection attempt"}, {RPCResult::Type::NUM, "banscore", "The ban score (DEPRECATED, returned only if config option -deprecatedrpc=banscore is passed)"}, {RPCResult::Type::NUM, "startingheight", "The starting height (block) of the peer"}, @@ -151,8 +149,6 @@ static RPCHelpMan getpeerinfo() {RPCResult::Type::BOOL, "addr_relay_enabled", "Whether we participate in address relay with this peer"}, {RPCResult::Type::NUM, "addr_processed", "The total number of addresses processed, excluding those dropped due to rate limiting"}, {RPCResult::Type::NUM, "addr_rate_limited", "The total number of addresses dropped due to rate limiting"}, - {RPCResult::Type::BOOL, "whitelisted", /* optional */ true, "Whether the peer is whitelisted with default permissions\n" - "(DEPRECATED, returned only if config option -deprecatedrpc=whitelisted is passed)"}, {RPCResult::Type::ARR, "permissions", "Any special permissions that have been granted to this peer", { {RPCResult::Type::STR, "permission_type", Join(NET_PERMISSIONS_DOC, ",\n") + ".\n"}, @@ -241,10 +237,6 @@ static RPCHelpMan getpeerinfo() obj.pushKV("inbound", stats.fInbound); obj.pushKV("bip152_hb_to", stats.m_bip152_highbandwidth_to); obj.pushKV("bip152_hb_from", stats.m_bip152_highbandwidth_from); - if (IsDeprecatedRPCEnabled("getpeerinfo_addnode")) { - // addnode is deprecated in v21 for removal in v22 - obj.pushKV("addnode", stats.m_manual_connection); - } obj.pushKV("masternode", stats.m_masternode_connection); if (fStateStats) { if (IsDeprecatedRPCEnabled("banscore")) { @@ -264,10 +256,6 @@ static RPCHelpMan getpeerinfo() obj.pushKV("addr_processed", statestats.m_addr_processed); obj.pushKV("addr_rate_limited", statestats.m_addr_rate_limited); } - if (IsDeprecatedRPCEnabled("whitelisted")) { - // whitelisted is deprecated in v0.21 for removal in v0.22 - obj.pushKV("whitelisted", stats.m_legacyWhitelisted); - } UniValue permissions(UniValue::VARR); for (const auto& permission : NetPermissions::ToStrings(stats.m_permission_flags)) { permissions.push_back(permission); diff --git a/test/functional/p2p_blocksonly.py b/test/functional/p2p_blocksonly.py index 95030b2428..828079c5b9 100755 --- a/test/functional/p2p_blocksonly.py +++ b/test/functional/p2p_blocksonly.py @@ -51,7 +51,7 @@ class P2PBlocksOnly(BitcoinTestFramework): assert_equal(self.nodes[0].getmempoolinfo()['size'], 1) self.log.info("Restarting node 0 with relay permission and blocksonly") - self.restart_node(0, ["-persistmempool=0", "-whitelist=relay@127.0.0.1", "-blocksonly", '-deprecatedrpc=whitelisted']) + self.restart_node(0, ["-persistmempool=0", "-whitelist=relay@127.0.0.1", "-blocksonly"]) assert_equal(self.nodes[0].getrawmempool(), []) first_peer = self.nodes[0].add_p2p_connection(P2PInterface()) second_peer = self.nodes[0].add_p2p_connection(P2PInterface()) diff --git a/test/functional/p2p_permissions.py b/test/functional/p2p_permissions.py index 2fc0c8e4cf..e7e82eccb5 100755 --- a/test/functional/p2p_permissions.py +++ b/test/functional/p2p_permissions.py @@ -36,35 +36,24 @@ class P2PPermissionsTests(BitcoinTestFramework): # default permissions (no specific permissions) ["-whitelist=127.0.0.1"], # Make sure the default values in the command line documentation match the ones here - ["relay", "noban", "mempool", "download"], - True) - - self.checkpermission( - # check without deprecatedrpc=whitelisted - ["-whitelist=127.0.0.1"], - # Make sure the default values in the command line documentation match the ones here - ["relay", "noban", "mempool", "download"], - None) + ["relay", "noban", "mempool", "download"]) self.checkpermission( # no permission (even with forcerelay) ["-whitelist=@127.0.0.1", "-whitelistforcerelay=1"], - [], - False) + []) self.checkpermission( # relay permission removed (no specific permissions) ["-whitelist=127.0.0.1", "-whitelistrelay=0"], - ["noban", "mempool", "download"], - True) + ["noban", "mempool", "download"]) self.checkpermission( # forcerelay and relay permission added # Legacy parameter interaction which set whitelistrelay to true # if whitelistforcerelay is true ["-whitelist=127.0.0.1", "-whitelistforcerelay"], - ["forcerelay", "relay", "noban", "mempool", "download"], - True) + ["forcerelay", "relay", "noban", "mempool", "download"]) # Let's make sure permissions are merged correctly # For this, we need to use whitebind instead of bind @@ -74,39 +63,28 @@ class P2PPermissionsTests(BitcoinTestFramework): self.checkpermission( ["-whitelist=noban@127.0.0.1"], # Check parameter interaction forcerelay should activate relay - ["noban", "bloomfilter", "forcerelay", "relay", "download"], - False) + ["noban", "bloomfilter", "forcerelay", "relay", "download"]) self.replaceinconfig(1, "whitebind=bloomfilter,forcerelay@" + ip_port, "bind=127.0.0.1") self.checkpermission( # legacy whitelistrelay should be ignored ["-whitelist=noban,mempool@127.0.0.1", "-whitelistrelay"], - ["noban", "mempool", "download"], - False) - - self.checkpermission( - # check without deprecatedrpc=whitelisted - ["-whitelist=noban,mempool@127.0.0.1", "-whitelistrelay"], - ["noban", "mempool", "download"], - None) + ["noban", "mempool", "download"]) self.checkpermission( # legacy whitelistforcerelay should be ignored ["-whitelist=noban,mempool@127.0.0.1", "-whitelistforcerelay"], - ["noban", "mempool", "download"], - False) + ["noban", "mempool", "download"]) self.checkpermission( # missing mempool permission to be considered legacy whitelisted ["-whitelist=noban@127.0.0.1"], - ["noban", "download"], - False) + ["noban", "download"]) self.checkpermission( # all permission added ["-whitelist=all@127.0.0.1"], - ["forcerelay", "noban", "mempool", "bloomfilter", "relay", "download", "addr"], - False) + ["forcerelay", "noban", "mempool", "bloomfilter", "relay", "download", "addr"]) self.stop_node(1) self.nodes[1].assert_start_raises_init_error(["-whitelist=oopsie@127.0.0.1"], "Invalid P2P permission", match=ErrorMatch.PARTIAL_REGEX) @@ -170,19 +148,13 @@ class P2PPermissionsTests(BitcoinTestFramework): reject_reason='Not relaying non-mempool transaction {} from forcerelay peer=0'.format(txid) ) - def checkpermission(self, args, expectedPermissions, whitelisted): - if whitelisted is not None: - args = [*args, '-deprecatedrpc=whitelisted'] + def checkpermission(self, args, expectedPermissions): self.restart_node(1, args) self.connect_nodes(0, 1) peerinfo = self.nodes[1].getpeerinfo()[0] - if whitelisted is None: - assert 'whitelisted' not in peerinfo - else: - assert_equal(peerinfo['whitelisted'], whitelisted) assert_equal(len(expectedPermissions), len(peerinfo['permissions'])) for p in expectedPermissions: - if not p in peerinfo['permissions']: + if p not in peerinfo['permissions']: raise AssertionError("Expected permissions %r is not granted." % p) def replaceinconfig(self, nodeid, old, new): diff --git a/test/functional/rpc_external_queue.py b/test/functional/rpc_external_queue.py old mode 100644 new mode 100755 diff --git a/test/functional/rpc_getpeerinfo_deprecation.py b/test/functional/rpc_getpeerinfo_deprecation.py index 340a66e12f..bde2058464 100755 --- a/test/functional/rpc_getpeerinfo_deprecation.py +++ b/test/functional/rpc_getpeerinfo_deprecation.py @@ -14,7 +14,6 @@ class GetpeerinfoDeprecationTest(BitcoinTestFramework): 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") @@ -23,16 +22,6 @@ class GetpeerinfoDeprecationTest(BitcoinTestFramework): 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"]) - 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()