doc: Document optional RPC result fields #23652
Closed
Conversation
This file contains 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
Can be reviewed with --ignore-all-space --word-diff-regex=.
|
Needed by itself and for #23083 |
maflcko
changed the title
Can be reviewed with --ignore-all-space --word-diff-regex=.
shaavan
approved these changes
Dec 3, 2021
| {RPCResult::Type::NUM, "witness_version", /* optional */ true, "The version number of the witness program"}, | ||
| {RPCResult::Type::STR_HEX, "witness_program", /* optional */ true, "The hex value of the witness program"}, | ||
| {RPCResult::Type::STR, "error", /* optional */ true, "Error message, if any"}, | ||
| {RPCResult::Type::ARR, "error_locations", /*optional=*/true, "Indices of likely error locations in address, if known (e.g. Bech32 errors)", |
There was a problem hiding this comment.
I was just curious about this thing. In this section of code, two ways of commenting the argument as optional are used:
/* optional *//*optional=*/
I have noticed that in this PR, you have used the second way of commenting for all the comments you added and have not changed the preexisting comments.
If the second way of commenting is the right way of the two. Maybe it could be taken up in follow-up PRs to refactor the type 1 comments.
There was a problem hiding this comment.
Yeah, this is being done in #23545
|
Rendered diff: diff --git a/getpeerinfo b/getpeerinfo
index e7ab8e8..ae40ea4 100644
--- a/getpeerinfo
+++ b/getpeerinfo
@@ -34,16 +34,16 @@ Result:
"inbound" : true|false, (boolean) Inbound (true) or Outbound (false)
"bip152_hb_to" : true|false, (boolean) Whether we selected peer as (compact blocks) high-bandwidth peer
"bip152_hb_from" : true|false, (boolean) Whether peer selected us as (compact blocks) high-bandwidth peer
- "startingheight" : n, (numeric) The starting height (block) of the peer
- "synced_headers" : n, (numeric) The last header we have in common with this peer
- "synced_blocks" : n, (numeric) The last block we have in common with this peer
- "inflight" : [ (json array)
+ "startingheight" : n, (numeric, optional) The starting height (block) of the peer
+ "synced_headers" : n, (numeric, optional) The last header we have in common with this peer
+ "synced_blocks" : n, (numeric, optional) The last block we have in common with this peer
+ "inflight" : [ (json array, optional)
n, (numeric) The heights of blocks we're currently asking from this peer
...
],
- "addr_relay_enabled" : true|false, (boolean) Whether we participate in address relay with this peer
- "addr_processed" : n, (numeric) The total number of addresses processed, excluding those dropped due to rate limiting
- "addr_rate_limited" : n, (numeric) The total number of addresses dropped due to rate limiting
+ "addr_relay_enabled" : true|false, (boolean, optional) Whether we participate in address relay with this peer
+ "addr_processed" : n, (numeric, optional) The total number of addresses processed, excluding those dropped due to rate limiting
+ "addr_rate_limited" : n, (numeric, optional) The total number of addresses dropped due to rate limiting
"permissions" : [ (json array) Any special permissions that have been granted to this peer
"str", (string) bloomfilter (allow requesting BIP37 filtered blocks and transactions),
noban (do not ban for misbehavior; implies download),
diff --git a/validateaddress b/validateaddress
index 90153d0..a02faee 100644
--- a/validateaddress
+++ b/validateaddress
@@ -15,7 +15,7 @@ Result:
"witness_version" : n, (numeric, optional) The version number of the witness program
"witness_program" : "hex", (string, optional) The hex value of the witness program
"error" : "str", (string, optional) Error message, if any
- "error_locations" : [ (json array) Indices of likely error locations in address, if known (e.g. Bech32 errors)
+ "error_locations" : [ (json array, optional) Indices of likely error locations in address, if known (e.g. Bech32 errors)
n, (numeric) index of a potential error
...
] |
maflcko
pushed a commit
to bitcoin-core/gui
that referenced
this pull request
Dec 3, 2021
fab6c43 doc: Document optional result fields in validateaddress (MarcoFalke) faee265 doc: Document optional result fields in getpeerinfo (MarcoFalke) Pull request description: ACKs for top commit: shaavan: ACK fab6c43 Tree-SHA512: 78458d0c4deb9253fbfe37fa5736a7db14eb0478bcc4adeba10ba6945e83d8eac92048293f50c054ea612609939151b4a2e1226c06f6067901f3d58c127c7e18
|
This was merged. |
luke-jr
reviewed
Dec 10, 2021
Comment on lines
+148
to
+154
| {RPCResult::Type::NUM, "startingheight", /*optional=*/true, "The starting height (block) of the peer"}, | ||
| {RPCResult::Type::NUM, "synced_headers", /*optional=*/true, "The last header we have in common with this peer"}, | ||
| {RPCResult::Type::NUM, "synced_blocks", /*optional=*/true, "The last block we have in common with this peer"}, | ||
| {RPCResult::Type::ARR, "inflight", /*optional=*/true, "", | ||
| { | ||
| {RPCResult::Type::NUM, "n", "The heights of blocks we're currently asking from this peer"}, | ||
| }}, |
There was a problem hiding this comment.
When are these not provided? Looks impossible to actually happen? Maybe a rare threading race (should we even document those?)?
There was a problem hiding this comment.
I am happy to review a pull request that always provides them.
There was a problem hiding this comment.
My point is that the current code appears to already always provide them.
There was a problem hiding this comment.
They are not if fStateStats is false.
Again, if you find a way to remove the fStateStats bool, please ping me for review.
There was a problem hiding this comment.
When can it be false?
You answered this in your first comment:
threading race (should we even document those?)?
I'd say yes, not only to clarify the interface, but also to avoid applications from crashing due to unexpected KeyError
RandyMcMillan
pushed a commit
to RandyMcMillan/mempool-tab
that referenced
this pull request
Dec 23, 2021
d6995c1 doc: Document optional result fields in validateaddress (MarcoFalke) 807414e doc: Document optional result fields in getpeerinfo (MarcoFalke) Pull request description: ACKs for top commit: shaavan: ACK d6995c1 Tree-SHA512: 78458d0c4deb9253fbfe37fa5736a7db14eb0478bcc4adeba10ba6945e83d8eac92048293f50c054ea612609939151b4a2e1226c06f6067901f3d58c127c7e18
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
No description provided.