Fix -netinfo backward compat with getpeerinfo pre-v26 #29212
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
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. |
kristapsk
reviewed
Jan 9, 2024
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
jonatack
force-pushed
the
2024-01-netinfo-transport
branch
from
d221f2a to
5fa7460
Compare
Member
Author
|
Re-pushed per @kristapsk's feedback (thanks!) |
Member
|
Concept ACK |
kristapsk
approved these changes
Jan 9, 2024
Contributor
|
Code Review ACK 5fa7460 |
Member
|
ACK 5fa7460 |
luke-jr
pushed a commit
to bitcoinknots/bitcoin
that referenced
this pull request
Mar 14, 2024
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
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Oct 16, 2024
PastaPastaPasta
added a commit
to dashpay/dash
that referenced
this pull request
Oct 22, 2024
, bitcoin#28645, bitcoin#28632, bitcoin#28782, bitcoin#28822, bitcoin#29006, bitcoin#29212, merge bitcoin-core/gui#754, partial bitcoin#23443, bitcoin#26448 (BIP324 backports: part 3) aa5311d merge bitcoin#29212: Fix -netinfo backward compat with getpeerinfo pre-v26 (Kittywhiskers Van Gogh) 1a293c7 merge bitcoin#29006: fix v2 transport intermittent test failure (Kittywhiskers Van Gogh) d0804d4 merge bitcoin#28822: Add missing wait for version to be sent in add_outbound_p2p_connection (Kittywhiskers Van Gogh) c0b3062 merge bitcoin#28782: Add missing sync on send_version in peer_connect (Kittywhiskers Van Gogh) 35253cd merge bitcoin#28632: make python p2p not send getaddr on incoming connections (Kittywhiskers Van Gogh) 6a4ca62 merge bitcoin#28645: fix `assert_debug_log` call-site bugs, add type checks (Kittywhiskers Van Gogh) deaee14 merge bitcoin-core/gui#754: Add BIP324-specific labels to peer details (Kittywhiskers Van Gogh) fffe6e7 merge bitcoin#27986: remove race in the user-agent reception check (Kittywhiskers Van Gogh) 1bf135b merge bitcoin#26553: Fix intermittent failure in rpc_net.py (Kittywhiskers Van Gogh) 5bf245b partial bitcoin#26448: fix intermittent failure in p2p_sendtxrcncl.py (Kittywhiskers Van Gogh) b7c0030 partial bitcoin#23443: Erlay support signaling (Kittywhiskers Van Gogh) c709df7 merge bitcoin#20524: Move MIN_VERSION_SUPPORTED to p2p.py (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6324 * Dependency for #6330 ## How Has This Been Tested? * Changes to Qt client were validated by running the client <details> <summary>Screenshot</summary>  </details> * Changes to `dash-cli` were validated by running it with different node versions **Against a node built on this PR** <details> <summary>Screenshot</summary>  </details> **Against a node built running the last release** <details> <summary>Screenshot</summary>  </details> ## Breaking Changes None expected. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas (note: N/A) - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation (note: N/A) - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: ACK aa5311d PastaPastaPasta: utACK aa5311d Tree-SHA512: 8cca324ac988a73c0590a4e9b318e81ce951ac55fb173cf507fa647cab01ab4981e6a06d4792376b4bfb44ff09d4811de05fadb9ba793dd00b4c7965b4b22654
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit fb5bfed in #29058 will cause
-netinfoto break when calling it on a node that is running pre-v26 bitcoind, asgetpeerinfodoesn'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)