Adds an optional peer_ids
parameter to getpeerinfo
to filter results by peer IDs.
This is useful when using bitcoin-cli or the console to inspect specific peers referenced in debug.log.
Adds an optional peer_ids
parameter to getpeerinfo
to filter results by peer IDs.
This is useful when using bitcoin-cli or the console to inspect specific peers referenced in debug.log.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32741.
See the guideline for information on the review process.
Type | Reviewers |
---|---|
Concept ACK | danielabrozzoni, rkrux |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
121@@ -122,7 +122,9 @@ static RPCHelpMan getpeerinfo()
122 return RPCHelpMan{
123 "getpeerinfo",
124 "Returns data about each connected network peer as a json array of objects.",
125- {},
126+ {
127+ {"nodeid", RPCArg::Type::NUM, RPCArg::Default{-1}, "Filter results to the peer with this nodeid. Specify -1 to return all peers."}
203 + HelpExampleRpc("getpeerinfo", "")
204+ + HelpExampleRpc("getpeerinfo", "7")
205 },
206 [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
207 {
208+ const int filterPeerId = request.params[0].isNull() ? -1 : request.params[0].getInt<int>();
self.Arg
to get the value.
Also, you can use the
self.Arg
to get the value.
Would this approach be preferable to avoid using a sentinel value like -1?
0const UniValue* nodeid_arg = self.MaybeArg<UniValue>("nodeid");
1const std::optional<NodeId> filter_id = nodeid_arg ? std::make_optional(nodeid_arg->getInt<NodeId>()) : std::nullopt;
0if (filter_id && stats.nodeid != *filter_id) continue;
1...
2if (filter_id && stats.nodeid == *filter_id) break;
Would it be acceptable to touch util.cpp
for this to add a int64_t template for MaybeArg or stick to the sentinel approach?
0TMPL_INST(nullptr, std::optional<int64_t>, maybe_arg ? std::optional{maybe_arg->getInt<int64_t>()} : std::nullopt;);
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
Also, missing tests?
bitcoin-cli getpeerinfo | jq '[.[] | select(.id == 24430)]'
for this. Doesn’t work in the GUI console though.
🚧 At least one of the CI tasks failed.
Task lint
: https://github.com/bitcoin/bitcoin/runs/44049534572
LLM reason (✨ experimental): The CI failure is caused by lint errors, specifically a trailing whitespace issue detected during the lint check.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
187+ assert_equal(filtered[0]["id"], peer["id"])
188+ self.log.debug("Filtered peer id {}: OK".format(peer["id"]))
189+
190+ self.log.info("Check getpeerinfo with nonexistent nodeid filter")
191+ nonexistent_id = max(p["id"] for p in all_peers) + 1000
192+ assert_equal(self.nodes[0].getpeerinfo(nonexistent_id), [])
Would it make sense to throw a JSONRPCError(RPC_MISC_ERROR,"Peer does not exist")
if the peer does not exist, Similar to the getblockfrompeer instead of sending back an empty array?
where error is thrown in PeerManagerImpl::FetchBlock
getpeerbyid
). The name getpeerinfo
is arguably a bit misleading since it returns a list of peers, but given that it’s established, returning an empty array when filtering seems appropriate to me.
peer_id
for consistency
Concept ACK, I think this is a good addition, as introducing a peer_id parameter to filter the getpeerinfo response is useful for debugging.
This needs a release note :)
307@@ -300,6 +308,9 @@ static RPCHelpMan getpeerinfo()
308 obj.pushKV("session_id", stats.m_session_id);
309
310 ret.push_back(std::move(obj));
311+ if (opt_peer_id && stats.nodeid == *opt_peer_id) {
opt_peer_id
here, since we’d never get here if it’s set to something different
121@@ -122,7 +122,9 @@ static RPCHelpMan getpeerinfo()
122 return RPCHelpMan{
123 "getpeerinfo",
124 "Returns data about each connected network peer as a json array of objects.",
125- {},
126+ {
127+ {"peer_id", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "Only return info for the specified peer."}
Since we have to iterate peers to find the right one, maybe this should be a list (unordered_set in C++)
I agree, the last push swapped to an unordered_set with an array argument
121@@ -122,7 +122,9 @@ static RPCHelpMan getpeerinfo()
122 return RPCHelpMan{
123 "getpeerinfo",
124 "Returns data about each connected network peer as a json array of objects.",
Can mention in the RPC help.
0- "Returns data about each connected network peer as a json array of objects.",
1+ "Returns data about each connected network peer as a json array of objects.\n"
2+ "Optionally filter by peer index.\n",
Concept ACK
Useful addition, I can see myself using the filtering here.
As mentioned in #32741#pullrequestreview-2953107634, a release note would be helpful as per this: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#release-notes
🚧 At least one of the CI tasks failed.
Task lint
: https://github.com/bitcoin/bitcoin/runs/45364101888
LLM reason (✨ experimental): Lint check failed due to a coding style error in rpc_net.py caused by an unnecessary semicolon.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
peer_ids
(array) instead of a single peer_id
. This allows filtering by multiple peers in one call and aligns better with RPC design. Functional tests adjusted.
I would do peer_id|peer_ids and accept both a single number (most common use) or an array (efficient).
I don’t think RPCArg
currently supports a single argument that can be either a num or an array (Maybe RPCArg::Type::ANY?). One option would be to add two separate optional arguments: one for a single ID and another for the list, then merge both into the filter set internally. That would allow getpeerinfo 7
for convenience and getpeerinfo -1 '[1,3,4,7]'
for multiple.
Figured it out in the latest push, skipped type checking for peer_ids
I would do peer_id|peer_ids and accept both a single number (most common use) or an array (efficient).
Skipped type checking and added simple cases so getpeerinfo 7
and getpeerinfo '[1,2,3]'
should work