Skip to content

Commit

Permalink
(Partial) Merge bitcoin#19725: [RPC] Add connection type to getpeerin…
Browse files Browse the repository at this point in the history
…fo, improve 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
  • Loading branch information
MarcoFalke authored and vijaydasmp committed Dec 21, 2023
1 parent e2e7f24 commit e72429a
Show file tree
Hide file tree
Showing 7 changed files with 76 additions and 7 deletions.
21 changes: 21 additions & 0 deletions src/net.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -684,6 +704,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

Expand Down
12 changes: 12 additions & 0 deletions src/net.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<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
{
Expand Down Expand Up @@ -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;
};


Expand Down Expand Up @@ -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; }
Expand Down
6 changes: 1 addition & 5 deletions src/net_processing.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
10 changes: 8 additions & 2 deletions src/rpc/net.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}
Expand Down
28 changes: 28 additions & 0 deletions test/functional/rpc_getpeerinfo_deprecation.py
Original file line number Diff line number Diff line change
@@ -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()
5 changes: 5 additions & 0 deletions test/functional/rpc_net.py
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
1 change: 1 addition & 0 deletions test/functional/test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down

0 comments on commit e72429a

Please sign in to comment.