rpc: fix getpeerinfo ping duration unit docs

0xB10C commented at 2:26 pm on August 4, 2025: contributor

The docs have been incorrect since a3789c700b5a43efd4b366b4241ae840d63f2349 (released in v25; master since Sept. 2022). Noticed while setting up monitoring using getpeerinfo.

https://github.com/bitcoin/bitcoin/blob/0cb1ed2b7c634754f4ad0a0083d7cf40989d7f48/src/rpc/net.cpp#L249-L257

DrahtBot added the label RPC/REST/ZMQ on Aug 4, 2025

DrahtBot commented at 2:26 pm on August 4, 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/33133.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	jonatack, theStack, luke-jr, maflcko, janb84

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

0xB10C commented at 2:30 pm on August 4, 2025: contributor

It’s correctly documented in the ping RPC docs https://github.com/bitcoin/bitcoin/blob/0cb1ed2b7c634754f4ad0a0083d7cf40989d7f48/src/rpc/net.cpp#L85-L88

maflcko commented at 2:35 pm on August 4, 2025: member

lgtm ACK 12df1a72831bf950c5d625b80c6fd6ef15b4117d

in src/rpc/net.cpp:155 in 12df1a7283 outdated

149@@ -150,9 +150,9 @@ static RPCHelpMan getpeerinfo()
150                     {RPCResult::Type::NUM, "bytesrecv", "The total bytes received"},
151                     {RPCResult::Type::NUM_TIME, "conntime", "The " + UNIX_EPOCH_TIME + " of the connection"},
152                     {RPCResult::Type::NUM, "timeoffset", "The time offset in seconds"},
153-                    {RPCResult::Type::NUM, "pingtime", /*optional=*/true, "The last ping time in milliseconds (ms), if any"},
154-                    {RPCResult::Type::NUM, "minping", /*optional=*/true, "The minimum observed ping time in milliseconds (ms), if any"},
155-                    {RPCResult::Type::NUM, "pingwait", /*optional=*/true, "The duration in milliseconds (ms) of an outstanding ping (if non-zero)"},
156+                    {RPCResult::Type::NUM, "pingtime", /*optional=*/true, "The last ping time in seconds (s), if any"},
157+                    {RPCResult::Type::NUM, "minping", /*optional=*/true, "The minimum observed ping time in seconds (s), if any"},
158+                    {RPCResult::Type::NUM, "pingwait", /*optional=*/true, "The duration in seconds (s) of an outstanding ping (if non-zero)"},

jonatack commented at 3:16 pm on August 4, 2025:

nit, unsure if the “(s)” mentions improve anything now, maybe omit them

jonatack commented at 3:16 pm on August 4, 2025: member

ACK 12df1a72831bf950c5d625b80c6fd6ef15b4117d

jonatack commented at 3:16 pm on August 4, 2025: member

ACK 12df1a72831bf950c5d625b80c6fd6ef15b4117d

(edit: oops, internet outage caused a duplicate ACK, so I “hid” this review)

jonatack commented at 3:20 pm on August 4, 2025: member

It’s correctly documented in the ping RPC docs

https://github.com/bitcoin/bitcoin/blob/0cb1ed2b7c634754f4ad0a0083d7cf40989d7f48/src/rpc/net.cpp#L85-L88

Would it make sense to add “minping” here? e.g. getpeerinfo (pingtime, minping, and pingwait fields)

janb84 commented at 5:14 pm on August 4, 2025: contributor

NIT; Please correct the commit message to better describe the change.

The description “This has been incorrect since a3789c700b5a43efd4b366b4241ae840d63f2349 (v25; master since Sept. 2022). Noticed while setting up monitoring using getpeerinfo.” isn’t descriptive of the changes in the commit.

“Commit messages should be helpful to people reading your code in the future, so explain the reasoning for your decisions. commit messages

0xB10C commented at 7:27 pm on August 4, 2025: contributor

Would it make sense to add “minping” here? e.g. getpeerinfo (pingtime, minping, and pingwait fields)

IMO it makes more sense to remove pingtime and min_ping from the ping docs. Just having Requests that a ping be sent to all other nodes, to measure ping time. Results provided in getpeerinfo. should be enough and ensures that it doesn’t become outdated somewhere down the line?

jonatack commented at 7:54 pm on August 4, 2025: member

IMO it makes more sense to remove pingtime and min_ping from the ping docs

Indeed, that is better. Fewer ducks to keep in a row.

rpc: fix getpeerinfo ping duration unit docs

The getpeerinfo docs incorrectly specified the ping durations as
milliseconds. This was incorrectly changed in a3789c700b5a43efd4b366b4241ae840d63f2349
(released in v25; master since Sept. 2022). The correct duration unit
is seconds.

Also, remove the documentation of the getpeerinfo RPC response from the
ping RPC since it's incomplete. Better to just reference the getpeerinfo
RPC and it's documenation for this.

1252eeb997

0xB10C force-pushed on Aug 4, 2025

0xB10C commented at 8:33 pm on August 4, 2025: contributor

removed the (s) unit from the docs: #33133 (review)
removed the getpeerinfo field documentation from the ping RPC: #33133 (comment)
added a bit more details to the commit message: #33133#pullrequestreview-3085116958

in src/rpc/net.cpp:88 in 1252eeb997

84@@ -85,7 +85,7 @@ static RPCHelpMan ping()
85     return RPCHelpMan{
86         "ping",
87         "Requests that a ping be sent to all other nodes, to measure ping time.\n"
88-                "Results provided in getpeerinfo, pingtime and pingwait fields are decimal seconds.\n"
89+                "Results are provided in getpeerinfo.\n"

jonatack commented at 8:49 pm on August 4, 2025:

pico-nit, if you have to retouch

0                "Results are provided in RPC getpeerinfo.\n"

jonatack commented at 8:50 pm on August 4, 2025: member

ACK 1252eeb997df2eb12c33d92eb1a5c9d6643a67ff

DrahtBot requested review from maflcko on Aug 4, 2025

theStack approved

theStack commented at 9:05 pm on August 4, 2025: contributor

ACK 1252eeb997df2eb12c33d92eb1a5c9d6643a67ff

luke-jr commented at 2:45 am on August 5, 2025: member

utACK 1252eeb997df2eb12c33d92eb1a5c9d6643a67ff

luke-jr referenced this in commit 062c326c59 on Aug 5, 2025

maflcko commented at 6:57 am on August 5, 2025: member

lgtm ACK 1252eeb997df2eb12c33d92eb1a5c9d6643a67ff

janb84 commented at 7:41 am on August 5, 2025: contributor

ACK 1252eeb997df2eb12c33d92eb1a5c9d6643a67ff

Pr changes getpeerinfo documentation to return the correct unit base (seconds instead of milliseconds)

Tested ✅
Code review ✅

(Thanks for addressing the commit message !)

fanquake merged this on Aug 5, 2025

fanquake closed this on Aug 5, 2025

0xB10C deleted the branch on Aug 5, 2025

fanquake referenced this in commit 4e8abca445 on Aug 5, 2025

fanquake commented at 8:46 am on August 5, 2025: member

Backported to 29.x in #33074.

fanquake referenced this in commit 2a46f220ca on Aug 6, 2025

fanquake commented at 12:10 pm on August 6, 2025: member

Backported to 28.x in #33143.

rpc: fix getpeerinfo ping duration unit docs #33133

Code Coverage & Benchmarks

Reviews