rpc: add optional nodeid param to filter getpeerinfo

waketraindev commented at 9:51 pm on June 12, 2025: none

Adds a nodeid parameter to getpeerinfo to filter results by peer id. This especially helpful when using bitcoin-cli or console to investigate something from debug.log I often find it annoying to manually search through the full peer list just to get info on one node, this makes that much easier.

DrahtBot commented at 9:51 pm on June 12, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32741.

Reviews

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.

Conflicts

No conflicts as of last run.

DrahtBot added the label RPC/REST/ZMQ on Jun 12, 2025

in src/rpc/net.cpp:126 in 6285d75ae9 outdated

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."}

maflcko commented at 6:57 am on June 13, 2025:

this should be optional, not a magic default value

waketraindev commented at 12:29 pm on June 13, 2025:

Updated to use Optional::OMITTED to avoid relying on magic default value. Thanks!

in src/rpc/net.cpp:208 in 6285d75ae9 outdated

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>();

maflcko commented at 6:57 am on June 13, 2025:

nit: snake_case for new code

maflcko commented at 6:58 am on June 13, 2025:

Also, you can use the self.Arg to get the value.

waketraindev commented at 12:31 pm on June 13, 2025:

Squashed commits. Renamed to filter_id. Evaluating how to best use self.Arg with NodeId. Will follow up

waketraindev commented at 2:21 pm on June 13, 2025:

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;);

maflcko commented at 7:00 am on June 13, 2025: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

Also, missing tests?

0xB10C commented at 9:17 am on June 13, 2025: contributor

I’m frequently using something like bitcoin-cli getpeerinfo | jq '[.[] | select(.id == 24430)]' for this. Doesn’t work in the GUI console though.

waketraindev force-pushed on Jun 13, 2025

waketraindev commented at 1:20 pm on June 13, 2025: none

Added test coverage with nodeid filter in test_getpeerinfo, including a check for nonexistent ids. Let me know if you’d prefer this in a separate test method or different cases

waketraindev force-pushed on Jun 13, 2025

DrahtBot added the label CI failed on Jun 13, 2025

DrahtBot commented at 1:22 pm on June 13, 2025: contributor

🚧 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.

waketraindev force-pushed on Jun 13, 2025

waketraindev requested review from maflcko on Jun 13, 2025

DrahtBot removed the label CI failed on Jun 13, 2025

in test/functional/rpc_net.py:195 in 53c5ce915c outdated

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), [])

kevkevinpal commented at 2:08 pm on June 15, 2025:

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

waketraindev commented at 3:08 pm on June 15, 2025:

I’d lean toward keeping this behavior for consistency unless there’s a broader plan to split these use cases into distinct RPCs (say: 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.

kevkevinpal commented at 1:28 pm on June 19, 2025:

If others think we should return an empty array then sounds good to me!

waketraindev force-pushed on Jun 15, 2025

waketraindev commented at 11:21 pm on June 15, 2025: none

Small touch up in code style, renamed parameter to peer_id for consistency

danielabrozzoni commented at 10:11 am on June 24, 2025: contributor

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 :)

in src/rpc/net.cpp:313 in af1c1df25a outdated

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) {

luke-jr commented at 5:44 pm on June 30, 2025:

Only need to check opt_peer_id here, since we’d never get here if it’s set to something different

waketraindev commented at 3:22 pm on July 4, 2025:

First condition checks if the parameter is defined so it returns all results even if peer_id is 0

in src/rpc/net.cpp:126 in af1c1df25a outdated

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."}

luke-jr commented at 5:45 pm on June 30, 2025:

Since we have to iterate peers to find the right one, maybe this should be a list (unordered_set in C++)

in src/rpc/net.cpp:124 in af1c1df25a outdated

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.",

rkrux commented at 2:55 pm on July 3, 2025:

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",

waketraindev commented at 3:21 pm on July 4, 2025:

Added line to help text, preferred using peer id naming instead of index, to avoid confusion with the array index

rkrux commented at 3:09 pm on July 3, 2025: contributor

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

waketraindev force-pushed on Jul 4, 2025

rpc: add optional "peer_id" filter to getpeerinfo with test coverage 9393b33325

waketraindev force-pushed on Jul 4, 2025

DrahtBot added the label CI failed on Jul 4, 2025

DrahtBot commented at 12:42 pm on July 4, 2025: contributor

🚧 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.

DrahtBot removed the label CI failed on Jul 4, 2025

waketraindev commented at 3:23 pm on July 4, 2025: none

Rebased on top of master, updated help text

rpc: add optional nodeid param to filter getpeerinfo #32741

Code Coverage & Benchmarks

Reviews

Conflicts