New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix -netinfo backward compat with getpeerinfo pre-v26 #29212
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
CLI -netinfo will currently break when calling it on a node that is running pre-v26 bitcoind, as `getpeerinfo` doesn't yet return a transport_protocol_type field. Fix this by adding an `IsNull()` check as already done for other fields, and also: - avoid checking for the full string "detecting", and instead do the cheaper check for the most frequent case of the string starting with "v" - drop displaying the "v" prefix in all the rows, as it doesn't add useful information, and instead use "v" for the column header - display nothing during peer setup, like for the -netinfo mping and ping columns
d221f2a
to
5fa7460
Compare
Re-pushed per @kristapsk's feedback (thanks!) |
Concept ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 5fa7460
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
Code Review ACK 5fa7460 |
ACK 5fa7460 |
CLI -netinfo will currently break when calling it on a node that is running pre-v26 bitcoind, as `getpeerinfo` doesn't yet return a transport_protocol_type field. Fix this by adding an `IsNull()` check as already done for other fields, and also: - avoid checking for the full string "detecting", and instead do the cheaper check for the most frequent case of the string starting with "v" - drop displaying the "v" prefix in all the rows, as it doesn't add useful information, and instead use "v" for the column header - display nothing during peer setup, like for the -netinfo mping and ping columns Github-Pull: bitcoin#29212 Rebased-From: 5fa7460
Commit fb5bfed in #29058 will cause
-netinfo
to break when calling it on a node that is running pre-v26 bitcoind, asgetpeerinfo
doesn't yet return a "transport_protocol_type" field.Fix this by adding an
IsNull()
check, as already done for other recent getpeerinfo fields, and also in the same commit:a) avoid checking for the full string "detecting", and instead do the cheaper check for the most frequent case of the string starting with "v"
b) drop displaying the "v" prefix in all the rows, as it doesn't add useful information, and instead use "v" for the column header
c) display nothing when a value isn't determined yet, like for the -netinfo mping and ping columns (as
*
already has a separate meaning in this dashboard, and?
might look like there is a bug)