rpc: add 'getnetmsgstats' RPC #28926

willcl-ark · 2023-11-22T12:37:19Z

This picks up #27534 with a rebase and a few fixups, now squashed. See #27534 for more background information.

Considerations from the previous draft:

Should we allow dimensions to be rearraged?

When it comes time to gather up the RPC results, @vasild has provided an alternate implementation that uses an array instead of the MultiMap structure. This results in two changes:

using the stack over the heap (yay!)
enforcing a strict ordering of dimensions (direction, network, connection type, message type)

Aside from being good for memory, the reasoning here is allowing users to rearrange dimensions may be too confusing. I personally really like the ability to rearrange dimensions, which is why I have not integrated that solution into this PR, but am open to whatever the larger community prefers :) Link

^ I have looked at the alternative implementation (thanks @vasild!), but prefer the current version myself so long as the performance is not an issue. I've left this as-is for now. If folks here think that it might be an issue then I can investigate the alternative further. See the next consideration for additional reasoning on sticking with the current approach.

Now that an effort has been made to remove the friendship between CConnman and CNode (see this PR: #27257) I'm unsure if this (recording network stats on a CConnman object) belongs here. Link

^ These changes make @Valid's alternative approach a bit harder to rebase and AB test with the current approach, as it relied on access to the CNode msg queue, and #27257 included the change: "CNode's message processing queue is made private in the process and its mutex is turned into a non-recursive mutex." It can be worked around, but I'm not convinced it will be worth it.

this function should handle if index > NUM_NET_MESSAGE_TYPES Link

^ I took this suggestion as part in 800ec2a

#27534 (comment)

^ I refactored this array in c909dc5 to make it more comprehensible in the source code.

Additional considerations from OP:

The code organisation was mainly taken care of by @satsie, with src/netstats.{h|cpp} now code from the netstats namespace instead of being contained within net.cpp

DrahtBot · 2023-11-22T12:37:23Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	brunoerg, amitiuttarwar, Sjors, vasild, mzumsande, kristapsk

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29421 (net: make the list of known message types a compile time constant by vasild)
#29418 (rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats' by vasild)
#27770 (Introduce 'getblockfileinfo' RPC command by furszy)
#27114 (p2p: Allow whitelisting manual connections by brunoerg)
#19463 (Prune locks by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

vasild

Concept ACK

I was looking at this recently, and got some ideas how to simplify and better organize this, will share here. Thanks for taking charge!

vasild · 2023-11-22T14:40:22Z

src/protocol.cpp

+static_assert(NUM_NET_MESSAGE_TYPES == sizeof(allNetMessageTypes) / sizeof(allNetMessageTypes[0]), "Please update NUM_NET_MESSAGE_TYPES");
+
+const static std::vector<std::string> allNetMessageTypesVec(std::begin(allNetMessageTypes), std::end(allNetMessageTypes));


All these plus the NUM_NET_MESSAGE_TYPES constant can be avoided if the list of all message types is turned into a std::array. Consider this:

[patch] make the list of known message types a compile time constant

diff --git a/src/net.cpp b/src/net.cpp index a2f80cbcf7..133abae117 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3670,13 +3670,13 @@ CNode::CNode(NodeId idIn, nLocalHostNonce{nLocalHostNonceIn}, m_recv_flood_size{node_opts.recv_flood_size}, m_i2p_sam_session{std::move(node_opts.i2p_sam_session)} { if (inbound_onion) assert(conn_type_in == ConnectionType::INBOUND); - for (const std::string &msg : getAllNetMessageTypes()) + for (const auto& msg : g_all_net_message_types) mapRecvBytesPerMsgType[msg] = 0; mapRecvBytesPerMsgType[NET_MESSAGE_TYPE_OTHER] = 0; if (fLogIPs) { LogPrint(BCLog::NET, "Added connection to %s peer=%d\n", m_addr_name, id); } else { diff --git a/src/protocol.cpp b/src/protocol.cpp index 27a0a2ffc1..0b22cc47aa 100644 --- a/src/protocol.cpp +++ b/src/protocol.cpp @@ -46,53 +46,12 @@ const char* CFHEADERS = "cfheaders"; const char* GETCFCHECKPT = "getcfcheckpt"; const char* CFCHECKPT = "cfcheckpt"; const char* WTXIDRELAY = "wtxidrelay"; const char* SENDTXRCNCL = "sendtxrcncl"; } // namespace NetMsgType -/** All known message types. Keep this in the same order as the list of - * messages above and in protocol.h. - */ -const static std::vector<std::string> g_all_net_message_types{ - NetMsgType::VERSION, - NetMsgType::VERACK, - NetMsgType::ADDR, - NetMsgType::ADDRV2, - NetMsgType::SENDADDRV2, - NetMsgType::INV, - NetMsgType::GETDATA, - NetMsgType::MERKLEBLOCK, - NetMsgType::GETBLOCKS, - NetMsgType::GETHEADERS, - NetMsgType::TX, - NetMsgType::HEADERS, - NetMsgType::BLOCK, - NetMsgType::GETADDR, - NetMsgType::MEMPOOL, - NetMsgType::PING, - NetMsgType::PONG, - NetMsgType::NOTFOUND, - NetMsgType::FILTERLOAD, - NetMsgType::FILTERADD, - NetMsgType::FILTERCLEAR, - NetMsgType::SENDHEADERS, - NetMsgType::FEEFILTER, - NetMsgType::SENDCMPCT, - NetMsgType::CMPCTBLOCK, - NetMsgType::GETBLOCKTXN, - NetMsgType::BLOCKTXN, - NetMsgType::GETCFILTERS, - NetMsgType::CFILTER, - NetMsgType::GETCFHEADERS, - NetMsgType::CFHEADERS, - NetMsgType::GETCFCHECKPT, - NetMsgType::CFCHECKPT, - NetMsgType::WTXIDRELAY, - NetMsgType::SENDTXRCNCL, -}; - CMessageHeader::CMessageHeader(const MessageStartChars& pchMessageStartIn, const char* pszCommand, unsigned int nMessageSizeIn) : pchMessageStart{pchMessageStartIn} { // Copy the command name size_t i = 0; for (; i < COMMAND_SIZE && pszCommand[i] != 0; ++i) pchCommand[i] = pszCommand[i]; @@ -175,17 +134,12 @@ std::string CInv::ToString() const return strprintf("%s %s", GetCommand(), hash.ToString()); } catch(const std::out_of_range &) { return strprintf("0x%08x %s", type, hash.ToString()); } } -const std::vector<std::string> &getAllNetMessageTypes() -{ - return g_all_net_message_types; -} - /** * Convert a service flag (NODE_*) to a human readable string. * It supports unknown service flags which will be returned as "UNKNOWN[...]". * @param[in] bit the service flag is calculated as (1 << bit) */ static std::string serviceFlagToStr(size_t bit) diff --git a/src/protocol.h b/src/protocol.h index e405253632..2a6b85dea5 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -264,14 +264,50 @@ extern const char* WTXIDRELAY; * The salt is used to compute short txids needed for efficient * txreconciliation, as described by BIP 330. */ extern const char* SENDTXRCNCL; }; // namespace NetMsgType -/* Get a vector of all valid message types (see above) */ -const std::vector<std::string>& getAllNetMessageTypes(); +/** All known message types (see above). Keep this in the same order as the list of messages above. */ +static const std::array g_all_net_message_types{ + NetMsgType::VERSION, + NetMsgType::VERACK, + NetMsgType::ADDR, + NetMsgType::ADDRV2, + NetMsgType::SENDADDRV2, + NetMsgType::INV, + NetMsgType::GETDATA, + NetMsgType::MERKLEBLOCK, + NetMsgType::GETBLOCKS, + NetMsgType::GETHEADERS, + NetMsgType::TX, + NetMsgType::HEADERS, + NetMsgType::BLOCK, + NetMsgType::GETADDR, + NetMsgType::MEMPOOL, + NetMsgType::PING, + NetMsgType::PONG, + NetMsgType::NOTFOUND, + NetMsgType::FILTERLOAD, + NetMsgType::FILTERADD, + NetMsgType::FILTERCLEAR, + NetMsgType::SENDHEADERS, + NetMsgType::FEEFILTER, + NetMsgType::SENDCMPCT, + NetMsgType::CMPCTBLOCK, + NetMsgType::GETBLOCKTXN, + NetMsgType::BLOCKTXN, + NetMsgType::GETCFILTERS, + NetMsgType::CFILTER, + NetMsgType::GETCFHEADERS, + NetMsgType::CFHEADERS, + NetMsgType::GETCFCHECKPT, + NetMsgType::CFCHECKPT, + NetMsgType::WTXIDRELAY, + NetMsgType::SENDTXRCNCL, +}; /** nServices flags */ enum ServiceFlags : uint64_t { // NOTE: When adding here, be sure to update serviceFlagToStr too // Nothing NODE_NONE = 0, diff --git a/src/test/fuzz/p2p_transport_serialization.cpp b/src/test/fuzz/p2p_transport_serialization.cpp index 21d8dab536..853661b4d4 100644 --- a/src/test/fuzz/p2p_transport_serialization.cpp +++ b/src/test/fuzz/p2p_transport_serialization.cpp @@ -18,19 +18,18 @@ #include <limits> #include <optional> #include <vector> namespace { -std::vector<std::string> g_all_messages; +auto g_all_messages = g_all_net_message_types; void initialize_p2p_transport_serialization() { ECC_Start(); SelectParams(ChainType::REGTEST); - g_all_messages = getAllNetMessageTypes(); std::sort(g_all_messages.begin(), g_all_messages.end()); } } // namespace FUZZ_TARGET(p2p_transport_serialization, .init = initialize_p2p_transport_serialization) @@ -147,13 +146,13 @@ void SimulationTest(Transport& initiator, Transport& responder, R& rng, FuzzedDa if (c < ' ' || c > 0x7E) break; ret += c; } return ret; } else { // Otherwise, use it as index into the list of known messages. - return g_all_messages[v % g_all_messages.size()]; + return std::string{g_all_messages[v % g_all_messages.size()]}; } }; // Function to construct a CSerializedNetMsg to send. auto make_msg_fn = [&](bool first) { CSerializedNetMsg msg; diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index d38d1bb40e..24c711f421 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -42,13 +42,13 @@ std::string_view LIMIT_TO_MESSAGE_TYPE{}; } // namespace void initialize_process_message() { if (const auto val{std::getenv("LIMIT_TO_MESSAGE_TYPE")}) { LIMIT_TO_MESSAGE_TYPE = val; - Assert(std::count(getAllNetMessageTypes().begin(), getAllNetMessageTypes().end(), LIMIT_TO_MESSAGE_TYPE)); // Unknown message type passed + Assert(std::count(g_all_net_message_types.begin(), g_all_net_message_types.end(), LIMIT_TO_MESSAGE_TYPE)); // Unknown message type passed } static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>( /*chain_type=*/ChainType::REGTEST, /*extra_args=*/{"-txreconciliation"}); g_setup = testing_setup.get();

Thanks for the idea Vasil.

I implemented it in this tag to see what it would look like.

Overall, I prefer the design, but in writing the commit message for the change I began to feel unsure that it actually results in g_all_net_message_types being a compile-time constant. IIUC the array will reference a const char*. The pointer addresses can only be allocated at runtime and in turn g_all_net_message_types will be static at runtime but not at compile time?

I'm not sure I see an easy way to have this be a compile time constant, if my understanding above is correct.

Make it constexpr instead of const. That'll force the compiler to evaluate the array at compile-time (or give an error if that's not possible).

Thanks, I forgot to mention I had tried that. It results in something like this:

$ bear -- make Making all in src make[1]: Entering directory '/home/will/src/bitcoin/src' make[2]: Entering directory '/home/will/src/bitcoin/src' make[3]: Entering directory '/home/will/src/bitcoin' make[3]: Leaving directory '/home/will/src/bitcoin' CXX bitcoind-bitcoind.o In file included from ./netstats.h:10, from ./net.h:22, from ./interfaces/node.h:10, from ./interfaces/init.h:10, from bitcoind.cpp:19: ./protocol.h:276:65: error: the type ‘const std::array<std::__cxx11::basic_string<char>, 35>’ of ‘constexpr’ variable ‘g_all_net_message_types’ is not literal 276 | static constexpr std::array<std::string, NUM_NET_MESSAGE_TYPES> g_all_net_message_types{ | ^~~~~~~~~~~~~~~~~~~~~~~ In file included from ./uint256.h:13, from ./consensus/params.h:9, from ./kernel/chainparams.h:9, from ./chainparams.h:9, from bitcoind.cpp:10: /usr/include/c++/12/array:99:12: note: ‘std::array<std::__cxx11::basic_string<char>, 35>’ is not literal because: 99 | struct array | ^~~~~ /usr/include/c++/12/array:99:12: note: ‘std::array<std::__cxx11::basic_string<char>, 35>’ has a non-trivial destructor make[2]: *** [Makefile:16350: bitcoind-bitcoind.o] Error 1 make[2]: Leaving directory '/home/will/src/bitcoin/src' make[1]: *** [Makefile:20245: all-recursive] Error 1 make[1]: Leaving directory '/home/will/src/bitcoin/src' make: *** [Makefile:813: all-recursive] Error 1

Oh, yes, you cannot have constexpr std::string until C++20.

const std::array a{x, y, z}; - at compile time the compiler knows the size of the array. a.size() can be used to define the size of other arrays, e.g. std::array<int, a.size()> anoher;. This is what matters. Whether the contents of the array is a compile time constant (the pointers), I am not sure.

C++20 is available to use now

fanquake · 2023-11-22T16:26:24Z

clang-tidy-17 -p=/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/rpc/net.cpp
rpc/net.cpp:726:30: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
  726 |                         path.push_back("totals");
      |                              ^~~~~~~~~~
      |                              emplace_back(

brunoerg · 2023-11-22T17:02:54Z

Concept ACK

Move NET_MESSAGE_TYPE_OTHER to protocol.h/cpp, where the rest of the message types live. Co-authored-by: Will Clark <will@256k1.dev>

Add consts to keep track of the number of connection and message types. The number of message types should match the number of elements in the allNetMessageTypes array. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

Co-authored-by: Will Clark <will@256k1.dev>

New data structure to hold the atomic message and byte counts of each network message statistic at the most granular level. Uses a 4D array that indexes by direction, network type, connection type, and message type. Co-authored-by: Vasil Dimov <vd@FreeBSD.org> Co-authored-by: Will Clark <will@256k1.dev>

Add to/fromIndex() methods to safely map enums to indexes. Co-authored-by: Vasil Dimov <vd@FreeBSD.org> Co-authored-by: Will Clark <will@256k1.dev>

In addition to per-peer stats, start recording stats (the number of messages and bytes) globally in `CConnman`. This happens every time messages are sent or received. The per-peer stats are lost when a peer disconnects. Also expose this object with a getter. Co-authored-by: Will Clark <will@256k1.dev>

Aggregate stats to show zero or more of the following dimensions: direction, network, connection, message type The depth of the return object depends the number of dimensions specified to be shown. MultiLevelStatsMap makes this possible, and converts the object to a UniValue. Co-authored-by: Anthony Towns <aj@erisian.com.au> Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

Introduce a new getnetmsgstats rpc that is able to return network message statistics arranged in a number of different ways (direction, network, connection, message type). If no dimension types are specified to be shown, return the totals. Includes helper code to convert a string to a DimensionType enum. Co-authored-by: Vasil Dimov <vd@FreeBSD.org> Co-authored-by: Will Clark <will@256k1.dev>

Co-authored-by: Will Clark <will@256k1.dev>

amitiuttarwar · 2023-11-22T23:16:25Z

yay!! thanks for picking this up. approach ACK

Sjors · 2023-11-23T10:26:07Z

Concept ACK

The RPC help is a bit abstract at the moment, e.g. "Results may optionally be grouped so only certain dimensions are shown."

It might be enough to only show 1 Result: section in the help, e.g. just with two dimensions.

In the example section (and in the PR description), you could add a description, e.g.:

Get the total number of messages and bytes sent/received: bitcoin-cli getnetmsgstats
Show number of messages and bytes per message type: bitcoin-cli getnetmsgstats '["message_type"]'
Group statistics first by message type, then by connection type: bitcoin-cli getnetmsgstats '["message_type", "connection_type"]'

It might be better to add a totals field to all groups, in every dimension. In that case I would also have the default getnetmsgstats return something a bit more interesting, e.g. the equivalent of getnetmsgstats '["message_type"].

vasild · 2023-11-24T13:43:32Z

It might be better to add a totals field to all groups, in every dimension. In that case I would also have the default getnetmsgstats return something a bit more interesting, e.g. the equivalent of getnetmsgstats '["message_type"].

That would be interesting. However it would further complicate this PR. The grouping/aggregating part I think is already a bit convoluted. Maybe it would make sense to drop it and print the full output without any grouping in this PR and do the grouping in another PR.

Sjors · 2023-11-28T13:51:35Z

drop it

Do you mean dropping the default total field that this PR already has? That sounds good to me.

vasild · 2023-11-29T14:24:05Z

No, I meant to drop the possibility to aggregate or nest the result, i.e. remove the arguments of the RPC and always return the full JSON. I did not see (yet) that total field. I guess it would be cleaner without it.

willcl-ark · 2023-11-29T16:50:51Z

No, I meant to drop the possibility to aggregate or nest the result, i.e. remove the arguments of the RPC and always return the full JSON. I did not see (yet) that total field. I guess it would be cleaner without it.

I think dropping the totals would probably make sense, as these can be totaled up using jq or similar tools pretty easily.

The only issue I see with not performing (optional) aggregation on the CLI, is that a node active on all networks could return a json object with a maximum of 246*35 = 1680 fields, making further processing a hard requirement for human consuption. Arguably this approach fits the unix philosophy better.

I personally think it's a nice for human to be able to aggregate on the RPC in case tools like jq or fx aren't available or something, but as mentioned removing this ability would simplify one of the commits here.

vasild · 2023-11-29T17:42:41Z

I personally think it's a nice for human to be able to aggregate on the RPC in case tools like jq or fx aren't available

I agree. Would be nice to have that finally eventually. I meant that if it is too complex for review or causes controversy then to get the simpler one first merged (without aggregation) and then in another PR do the aggregation. Maybe that would not be necessary. I am updating the WIP from master...vasild:bitcoin:getnetmsgstats and will compare with this...

vasild

Concept ACK

I have applied the suggestions below in vasild/getnetmsgstats plus some other simplifications. Feel free to pick them.

Thanks!

vasild · 2023-12-05T13:21:49Z

src/protocol.h

@@ -267,9 +267,20 @@ extern const char* WTXIDRELAY;
 extern const char* SENDTXRCNCL;
 }; // namespace NetMsgType

+extern const std::string NET_MESSAGE_TYPE_OTHER;


Commit d3b34b8 net: move NET_MESSAGE_TYPE_OTHER is not needed and can be dropped if messageTypeFromIndex() and messageTypeToIndex() standalone functions are made private methods in the NetStats class and their usage limited to within the class. That is desirable anyway because they are only relevant for an index in an array that is defined in a particular way and is (should be) private within NetStats.

I agree: Code and constants that are only used in RPCs are not part of the protocol and therefore don't belong in protocol.h / protocol.cpp.

vasild · 2023-12-05T13:36:48Z

src/netstats.h

+    void Init()
+    {
+        for (std::size_t direction_index = 0; direction_index < NUM_DIRECTIONS; direction_index++) {
+            for (int network_index = 0; network_index < NET_MAX; network_index++) {
+                for (std::size_t connection_index = 0; connection_index < NUM_CONNECTION_TYPES; connection_index++) {
+                    for (std::size_t message_index = 0; message_index < NUM_NET_MESSAGE_TYPES + 1; message_index++) {
+                        // +1 for the "other" message type
+                        auto& stat = m_data
+                            .at(direction_index)
+                            .at(network_index)
+                            .at(connection_index)
+                            .at(message_index);
+
+                        stat.msg_count = 0;
+                        stat.byte_count = 0;
+                    }
+                }
+            }
+        }
+    }


This method is not needed. It is simpler and suffices to explicitly initialize the members of MsgStat to zero:

std::atomic_uint64_t byte_count{0}; //!< Number of bytes transferred. std::atomic_uint64_t msg_count{0}; //!< Number of messages transferred.

vasild · 2023-12-05T13:43:04Z

src/netstats.h

+    [[nodiscard]] static Direction DirectionFromIndex(size_t index);
+    [[nodiscard]] static Network NetworkFromIndex(size_t index);
+    [[nodiscard]] static ConnectionType ConnectionTypeFromIndex(size_t index);


These might as well be private. It would be good to completely hide this to/from index from the outside world because it can be misused in a dangerous way. Then provide a way to call a provided function for each element:

void TrafficStats::ForEach(std::function<void(TrafficStats::Direction dir, Network net, ConnectionType con, const std::string& msg, const BytesAndCount& data)> func) const { for (size_t dir_i = 0; dir_i < m_data.size(); ++dir_i) { for (size_t net_i = 0; net_i < m_data[dir_i].size(); ++net_i) { for (size_t con_i = 0; con_i < m_data[dir_i][net_i].size(); ++con_i) { for (size_t msg_i = 0; msg_i < m_data[dir_i][net_i][con_i].size(); ++msg_i) { func(DirectionFromIndex(dir_i), NetworkFromIndex(net_i), ConnectionTypeFromIndex(con_i), MessageTypeFromIndex(msg_i), m_data[dir_i][net_i][con_i][msg_i]); } } } } }

vasild · 2023-12-05T14:14:43Z

src/net.cpp

@@ -3715,6 +3718,12 @@ void CNode::MarkReceivedMsgsForProcessing()
        // vRecvMsg contains only completed CNetMessage
        // the single possible partially deserialized message are held by TransportDeserializer
        nSizeAdded += msg.m_raw_message_size;
+
+        net_stats.Record(NetStats::Direction::RECV,


The code already collects the traffic data for the per peer stats. To avoid confusion and ease maintenance it would be nice if the per peer and global traffic data is collected at the same place. This is where the data is collected in master for the per peer stats:

Received:

bitcoin/src/net.cpp

Line 663 in 6d57909

mapRecvBytesPerMsgType.at(NET_MESSAGE_TYPE_OTHER) += msg.m_raw_message_size;

bitcoin/src/net.cpp

Lines 669 to 674 in 6d57909

auto i = mapRecvBytesPerMsgType.find(msg.m_type);

if (i == mapRecvBytesPerMsgType.end()) {

i = mapRecvBytesPerMsgType.find(NET_MESSAGE_TYPE_OTHER);

}

assert(i != mapRecvBytesPerMsgType.end());

i->second += msg.m_raw_message_size;

Sent:

bitcoin/src/net.cpp

Line 1620 in 6d57909

node.AccountForSentBytes(msg_type, nBytes);

vasild · 2023-12-06T13:15:49Z

src/rpc/net.cpp

+ * @endcode
+ *
+ */
+UniValue Aggregate(const std::vector<DimensionType>& dimensions, const NetStats::MultiDimensionalStats& raw_stats)


I realized that this can be done in a simpler way - if the stats are put in a map and "" used for the aggregated dimensions. See commit b7c56af rpc: make it possible to aggregate the result in getnetmsgstats from vasild/getnetmsgstats.

vasild · 2023-12-06T13:18:43Z

test/functional/rpc_net.py

-        assert_equal(info['connections'], 2)
-        assert_equal(info['connections_in'], 1)
+        assert_equal(info['connections'], 3)
+        assert_equal(info['connections_in'], 2)


This shouldn't be necessary. It means that the previous test did not leave the environment as it was when it started. That is the newly added test_getnetmsgstats. It is good to be able to run the tests in isolation and that they do not rely on the leftovers from previous tests. For example if:

self.test_connection_count() self.test_getpeerinfo() self.test_getnettotals() self.test_getnetmsgstats() self.test_getnetworkinfo() self.test_addnode_getaddednodeinfo() self.test_service_flags()

is changed to:

#self.test_connection_count() #self.test_getpeerinfo() #self.test_getnettotals() #self.test_getnetmsgstats() self.test_getnetworkinfo() #self.test_addnode_getaddednodeinfo() #self.test_service_flags()

would be good to have rpc_net.py still pass.

mzumsande

Concept ACK. We are using this (in an adjusted form with another dimension for txrelay) for analysing the traffic implications of #28463, and it works great so far.

mzumsande · 2024-01-05T22:34:06Z

src/protocol.h

@@ -267,9 +267,20 @@ extern const char* WTXIDRELAY;
 extern const char* SENDTXRCNCL;
 }; // namespace NetMsgType

+extern const std::string NET_MESSAGE_TYPE_OTHER;


I agree: Code and constants that are only used in RPCs are not part of the protocol and therefore don't belong in protocol.h / protocol.cpp.

mzumsande · 2024-01-05T22:36:52Z

src/protocol.h

@@ -275,6 +275,12 @@ static constexpr size_t NUM_NET_MESSAGE_TYPES{35};
 /* Get a vector of all valid message types (see above) */
 const std::vector<std::string>& getAllNetMessageTypes();

+/* Get the index of a message type from the vector of all message types */
+int messageTypeToIndex(const std::string& msg_type);


nit: capital M, here and in messageTypeFromIndex to make it similar to the related functions for other dimensions.

mzumsande · 2024-01-05T23:08:42Z

src/rpc/net.cpp

+
+            // Aggregate the messages to show only requested dimensions
+            if (request.params[0].isArray()) {
+                const UniValue raw_dimensions = request.params[0].get_array();


Would be nice to have some validation of user input here - getnetmsgstats '["foo","bar"]' could return an error message, listing the supported dimensions.

vasild · 2024-01-06T16:07:56Z

I have applied the suggestions below in vasild/getnetmsgstats plus some other simplifications. Feel free to pick them.

Should I PR the above branch?

kristapsk · 2024-01-06T17:45:34Z

Concept ACK

willcl-ark · 2024-01-24T16:08:42Z

I have applied the suggestions below in vasild/getnetmsgstats plus some other simplifications. Feel free to pick them.

Should I PR the above branch?

Hey Vasil, I think this makes most sense, especially seeing as you've co-authored/written most of the changes here, and your improvements on the new branch!

I've been testing your branch out today and am a fan of the simplicity vs this changeset. I did kind of prefer the "aggregation" RPC behaviour in this branch, even though it doesn't really aggregate, more accurately it "groups by".

In any case, your new branch does actually aggregate results, and the same effect can be easily achieved. e.g. to produce output like:

{
  "wtxidrelay": {
    "bytes": 1146,
    "count": 47
  },
  "headers": {
    "bytes": 10846,
    "count": 55
  },
  "tx": {
    "bytes": 511341,
    "count": 801
  },
  "ping": {
    "bytes": 1786,
    "count": 56
  }
}

In this branch we would do: .src/bitcoin-cli getnetmsgstats '["message_type"]', whereas on your new fork (with actual aggregation) this requires the "inverse": ./src/bitcoin-cli getnetmsgstats '["direction", "network", "connection_type"]'.

Your comments here did get me thinking whether we could use mapRecvBytesPerMsgType to return the netmsgstats, or conversely use data from net_stats to return stats to the getpeerinfo RPC and avoid counting the same message twice everywhere. But I'm not convinced that the changes that would be needed to do that would be worth it.

vasild · 2024-02-10T15:27:15Z

Should I PR the above branch?

Hey Vasil, I think this makes most sense ...

I am on it!

Your comments #28926 (comment) did get me thinking whether we could use mapRecvBytesPerMsgType to return the netmsgstats, or conversely use data from net_stats to return stats to the getpeerinfo RPC and avoid counting the same message twice everywhere. But I'm not convinced that the changes that would be needed to do that would be worth it.

Hmm,

If we use CNode::mapRecvBytesPerMsgType to create the global netmsgstats report, then we still need to store the data somewhere when the CNode disconnects. So we need another storage anyway.
If we use the new global net_stats to create the per-peer report, then we have to add another dimension to it - "peer id". Then query only for a given peer for getpeerinfo and always aggregate by "peer id" (hide it) from the new totals report from getnetmsgstats. That seems like it may be a good idea? But what when a peer disconnects? We don't want to bloat net_stats with individual info for disconnected peers. So then we would have to have one dummy "peer id", e.g. -1 and sum disconnected peers' stats into that one. That way net_stats cannot be an array anymore because the size of the "peer id" dimension is not known at compile time. What do others think about this?

vasild · 2024-02-10T15:35:55Z

... this requires the "inverse" ...

Ha, I did not realize this before! I am fine either way. Also I am not sure what would be the best name of the RPC parameter, e.g. bitcoin-cli -named getnetmsgstats whatname='[...]'. Lets do a poll below (use 👍).

vasild · 2024-02-10T15:36:15Z

To produce the output in #28926 (comment) it is better to use bitcoin-cli getnetmsgstats '["message_type"]'. How to name the RPC parameter?

vasild · 2024-02-10T15:36:24Z

To produce the output in #28926 (comment) it is better to use bitcoin-cli getnetmsgstats '["direction", "network", "connection_type"]'. How to name the RPC parameter?

vasild · 2024-02-11T09:35:04Z

Should I PR the above branch?

Opened #29418

satsie · 2024-02-11T19:22:25Z

RE:

... this requires the "inverse" ...

Ha, I did not realize this before! I am fine either way.

I voted for bitcoin-cli getnetmsgstats '["message_type"]' because as a node operator it's simpler to remember the dimensions I want (i.e. message) instead of knowing the RPC well enough to list every dimension I don't want (i.e. direction, network, connection type). But of course anyone using an RPC should read the docs 😄

willcl-ark · 2024-02-12T08:30:17Z

But what when a peer disconnects? We don't want to bloat net_stats with individual info for disconnected peers.

Yes, and in fact if a peer repeatedly connected and disconnected we would end up with unbounded growth of net_stats size, which would not be good... Let's shelve this thought for now.

To produce the output in #28926 (comment) it is better to use bitcoin-cli getnetmsgstats '["message_type"]'. How to name the RPC parameter?

In a previous revision, I had renamed the RPC parameter group_by to address this, as I felt it made sense.

Closed in favour of #29418

DrahtBot added the RPC/REST/ZMQ label Nov 22, 2023

vasild reviewed Nov 22, 2023

View reviewed changes

DrahtBot added the CI failed label Nov 22, 2023

fanquake mentioned this pull request Nov 22, 2023

rpc: add 'getnetmsgstats', new rpc to view network message statistics #27534

Closed

satsie and others added 9 commits November 22, 2023 19:30

net: move NET_MESSAGE_TYPE_OTHER

d3b34b8

Move NET_MESSAGE_TYPE_OTHER to protocol.h/cpp, where the rest of the message types live. Co-authored-by: Will Clark <will@256k1.dev>

net: make constants for total conn and msg types

d165063

Add consts to keep track of the number of connection and message types. The number of message types should match the number of elements in the allNetMessageTypes array. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

net: add methods to convert a message type to/from an index

9383acb

Co-authored-by: Will Clark <will@256k1.dev>

net: add to/fromIndex() methods to NetStats

5371845

Add to/fromIndex() methods to safely map enums to indexes. Co-authored-by: Vasil Dimov <vd@FreeBSD.org> Co-authored-by: Will Clark <will@256k1.dev>

test: add functional tests for getnetmsgstats rpc

33b44f2

Co-authored-by: Will Clark <will@256k1.dev>

DrahtBot mentioned this pull request Nov 22, 2023

Introduce 'getblockfileinfo' RPC command #27770

Closed

willcl-ark force-pushed the 2023-07-getnetmsgstats branch from 0341129 to 33b44f2 Compare November 22, 2023 22:30

DrahtBot removed the CI failed label Nov 22, 2023

DrahtBot mentioned this pull request Nov 28, 2023

p2p: Allow whitelisting manual connections #27114

Merged

DrahtBot added the CI failed label Dec 2, 2023

vasild reviewed Dec 6, 2023

View reviewed changes

DrahtBot mentioned this pull request Dec 11, 2023

test: Use test framework utils in functional tests #28528

Closed

DrahtBot mentioned this pull request Dec 19, 2023

Prune locks #19463

Draft

mzumsande reviewed Jan 5, 2024

View reviewed changes

vasild mentioned this pull request Feb 11, 2024

rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats' #29418

Open

DrahtBot mentioned this pull request Feb 11, 2024

net: make the list of known message types a compile time constant #29421

Merged

willcl-ark closed this Feb 12, 2024

bitcoin locked and limited conversation to collaborators Feb 11, 2025

		static_assert(NUM_NET_MESSAGE_TYPES == sizeof(allNetMessageTypes) / sizeof(allNetMessageTypes[0]), "Please update NUM_NET_MESSAGE_TYPES");

		const static std::vector<std::string> allNetMessageTypesVec(std::begin(allNetMessageTypes), std::end(allNetMessageTypes));

	auto i = mapRecvBytesPerMsgType.find(msg.m_type);
	if (i == mapRecvBytesPerMsgType.end()) {
	i = mapRecvBytesPerMsgType.find(NET_MESSAGE_TYPE_OTHER);
	}
	assert(i != mapRecvBytesPerMsgType.end());
	i->second += msg.m_raw_message_size;

rpc: add 'getnetmsgstats' RPC #28926

rpc: add 'getnetmsgstats' RPC #28926

Uh oh!

Conversation

willcl-ark commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake commented Nov 22, 2023

Uh oh!

brunoerg commented Nov 22, 2023

Uh oh!

amitiuttarwar commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Nov 23, 2023

Uh oh!

vasild commented Nov 24, 2023

Uh oh!

Sjors commented Nov 28, 2023

Uh oh!

vasild commented Nov 29, 2023

Uh oh!

willcl-ark commented Nov 29, 2023

Uh oh!

vasild commented Nov 29, 2023

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzumsande Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasild commented Jan 6, 2024

Uh oh!

kristapsk commented Jan 6, 2024

willcl-ark commented Nov 22, 2023 •

edited

Loading

DrahtBot commented Nov 22, 2023 •

edited

Loading

amitiuttarwar commented Nov 22, 2023 •

edited

Loading

mzumsande Jan 5, 2024 •

edited

Loading

satsie commented Feb 11, 2024 •

edited

Loading