From 49c10a9ca40967d28ae16dfea9cccc6f3a6624a1 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Mon, 10 Aug 2020 18:30:04 -0700 Subject: [PATCH 1/5] [log] Add connection type to log statement In addition to adding more specificity to the log statement about the type of connection, this change also consolidates two statements into one. Previously, the second one should have never been hit, since block-relay connections would match the "!IsInboundConn()" condition and return early. --- src/net.cpp | 20 ++++++++++++++++++++ src/net.h | 2 ++ src/net_processing.cpp | 6 +----- 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index e7d3a146ff..15675a68ad 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -488,6 +488,26 @@ void CConnman::AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNet } } +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; diff --git a/src/net.h b/src/net.h index 0366fa0f5b..61a208802c 100644 --- a/src/net.h +++ b/src/net.h @@ -1145,6 +1145,8 @@ public: std::string GetAddrName() const; //! Sets the addrName only if it was not previously set void MaybeSetAddrName(const std::string& addrNameIn); + + std::string ConnectionTypeAsString() const; }; /** Return a timestamp in the future (in microseconds) for exponentially distributed events. */ diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 690b59476b..859b67755d 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3521,11 +3521,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat // 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.RelayAddrsWithConn()) { - 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; } From 395acfa83a5436790c1a722a5609ac9d48df235f Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Wed, 12 Aug 2020 13:57:13 -0700 Subject: [PATCH 2/5] [rpc] Add connection type to getpeerinfo RPC, update tests --- src/net.cpp | 2 ++ src/net.h | 9 +++++++++ src/rpc/net.cpp | 2 ++ test/functional/rpc_net.py | 6 ++++++ 4 files changed, 19 insertions(+) diff --git a/src/net.cpp b/src/net.cpp index 15675a68ad..5b533d7d17 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -602,6 +602,8 @@ void CNode::copyStats(CNodeStats &stats, const std::vector &m_asmap) // Leave string empty if addrLocal invalid (not filled in yet) CService addrLocalUnlocked = GetAddrLocal(); stats.addrLocal = addrLocalUnlocked.IsValid() ? addrLocalUnlocked.ToString() : ""; + + stats.m_conn_type_string = ConnectionTypeAsString(); } #undef X diff --git a/src/net.h b/src/net.h index 61a208802c..5e134ca7c8 100644 --- a/src/net.h +++ b/src/net.h @@ -114,6 +114,14 @@ struct CSerializedNetMsg std::string m_type; }; +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)"}; + /** Different types of connections to a peer. This enum encapsulates the * information we have available at the time of opening or accepting the * connection. Aside from INBOUND, all types are initiated by us. */ @@ -692,6 +700,7 @@ public: // Bind address of our side of the connection CAddress addrBind; uint32_t m_mapped_as; + std::string m_conn_type_string; }; diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 5af4389857..e480bd2a40 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -113,6 +113,7 @@ static UniValue getpeerinfo(const JSONRPCRequest& request) {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::STR, "connection_type", "Type of connection: \n" + Join(CONNECTION_TYPE_DOC, ",\n") + "."}, {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)"}, {RPCResult::Type::NUM, "synced_headers", "The last header we have in common with this peer"}, @@ -228,6 +229,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_net.py b/test/functional/rpc_net.py index bc0e5b458e..b8a04f494d 100755 --- a/test/functional/rpc_net.py +++ b/test/functional/rpc_net.py @@ -175,6 +175,12 @@ class NetTest(BitcoinTestFramework): for info in peer_info: assert_net_servicesnames(int(info[0]["services"], 0x10), info[0]["servicesnames"]) + 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.log.info("Test service flags") self.nodes[0].add_p2p_connection(P2PInterface(), services=(1 << 4) | (1 << 63)) From df091b9b509f0b10e4315c0bfa2da0cc0c31c22f Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Wed, 12 Aug 2020 15:50:19 -0700 Subject: [PATCH 3/5] [refactor] Rename test file to allow any getpeerinfo deprecations. Simple rename/restructure to allow for upcoming test additions. --- ...e_deprecation.py => rpc_getpeerinfo_deprecation.py} | 10 ++++++---- test/functional/test_runner.py | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) rename test/functional/{rpc_getpeerinfo_banscore_deprecation.py => rpc_getpeerinfo_deprecation.py} (76%) diff --git a/test/functional/rpc_getpeerinfo_banscore_deprecation.py b/test/functional/rpc_getpeerinfo_deprecation.py similarity index 76% rename from test/functional/rpc_getpeerinfo_banscore_deprecation.py rename to test/functional/rpc_getpeerinfo_deprecation.py index b830248e1e..a4c2f7ccf8 100755 --- a/test/functional/rpc_getpeerinfo_banscore_deprecation.py +++ b/test/functional/rpc_getpeerinfo_deprecation.py @@ -2,23 +2,25 @@ # 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 banscore field.""" +"""Test deprecation of getpeerinfo RPC fields.""" from test_framework.test_framework import BitcoinTestFramework -class GetpeerinfoBanscoreDeprecationTest(BitcoinTestFramework): +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() + + 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() - if __name__ == "__main__": - GetpeerinfoBanscoreDeprecationTest().main() + GetpeerinfoDeprecationTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 6c3d50df93..c8cf173d5f 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -251,7 +251,7 @@ BASE_SCRIPTS = [ 'feature_config_args.py', 'feature_settings.py', 'rpc_getdescriptorinfo.py', - 'rpc_getpeerinfo_banscore_deprecation.py', + 'rpc_getpeerinfo_deprecation.py', 'rpc_help.py', 'feature_help.py', 'feature_shutdown.py', From 50f94b34a33c954f6e207f509c93d33267a5c3e2 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Wed, 12 Aug 2020 16:04:50 -0700 Subject: [PATCH 4/5] [rpc] Deprecate getpeerinfo addnode field This field is now redundant since the connection type field will indicate MANUAL for addnode connections. --- src/rpc/net.cpp | 8 ++++++-- test/functional/rpc_getpeerinfo_deprecation.py | 13 +++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index e480bd2a40..e195affd54 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -112,7 +112,8 @@ 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::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)"}, @@ -193,7 +194,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("startingheight", stats.nStartingHeight); if (fStateStats) { if (IsDeprecatedRPCEnabled("banscore")) { diff --git a/test/functional/rpc_getpeerinfo_deprecation.py b/test/functional/rpc_getpeerinfo_deprecation.py index a4c2f7ccf8..287c40ae3e 100755 --- a/test/functional/rpc_getpeerinfo_deprecation.py +++ b/test/functional/rpc_getpeerinfo_deprecation.py @@ -5,6 +5,7 @@ """Test deprecation of getpeerinfo RPC fields.""" from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import connect_nodes class GetpeerinfoDeprecationTest(BitcoinTestFramework): @@ -14,6 +15,7 @@ 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") @@ -22,5 +24,16 @@ 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"]) + 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() From a512925e19a70d7f6b80ac530a169f45ffaafa1c Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Wed, 12 Aug 2020 16:45:26 -0700 Subject: [PATCH 5/5] [doc] Release notes --- doc/release-notes.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/doc/release-notes.md b/doc/release-notes.md index 0ce2b32c61..059c8cd029 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -105,6 +105,17 @@ will trigger BIP 125 (replace-by-fee) opt-in. (#11413) - The `testmempoolaccept` RPC returns `vsize` and a `fee` object with the `base` fee if the transaction passes validation. (#19940) +- The `getpeerinfo` RPC now returns a `connection_type` field. This indicates + the type of connection established with the peer. It will return one of six + options. For more information, see the `getpeerinfo` help documentation. + (#19725) + +- The `getpeerinfo` RPC no longer returns the `addnode` field by default. This + field will be fully removed in the next major release. It can be accessed + with the configuration option `-deprecatedrpc=getpeerinfo_addnode`. However, + it is recommended to instead use the `connection_type` field (it will return + `manual` when addnode is true). (#19725) + - The `walletcreatefundedpsbt` RPC call will now fail with `Insufficient funds` when inputs are manually selected but are not enough to cover the outputs and fee. Additional inputs can automatically be added through the