doc: Document optional RPC result fields #23652

MarcoFalke commented at 4:39 PM on December 2, 2021: member

No description provided.

doc: Document optional result fields in getpeerinfo

Can be reviewed with --ignore-all-space --word-diff-regex=.

faee2656a8

MarcoFalke commented at 4:39 PM on December 2, 2021: member

Needed by itself and for #23083

laanwj added the label Docs on Dec 2, 2021

MarcoFalke renamed this:
~~doc: Document optional result fields in getpeerinfo~~
doc: Document optional RPC result fields
on Dec 2, 2021

doc: Document optional result fields in validateaddress

Can be reviewed with --ignore-all-space --word-diff-regex=.

fab6c43b40

in src/rpc/misc.cpp:56 in fab6c43b40

  58 | +                {RPCResult::Type::BOOL, "isscript", /* optional */ true, "If the key is a script"},
  59 | +                {RPCResult::Type::BOOL, "iswitness", /* optional */ true, "If the address is a witness address"},
  60 | +                {RPCResult::Type::NUM, "witness_version", /* optional */ true, "The version number of the witness program"},
  61 | +                {RPCResult::Type::STR_HEX, "witness_program", /* optional */ true, "The hex value of the witness program"},
  62 | +                {RPCResult::Type::STR, "error", /* optional */ true, "Error message, if any"},
  63 | +                {RPCResult::Type::ARR, "error_locations", /*optional=*/true, "Indices of likely error locations in address, if known (e.g. Bech32 errors)",

shaavan commented at 8:00 AM on December 3, 2021:

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.

MarcoFalke commented at 8:36 AM on December 3, 2021:

Yeah, this is being done in #23545

shaavan commented at 8:48 AM on December 3, 2021:

this is being done in #23545

That's great! Just have to wait for a little to resolve PR conflicts with #23545

shaavan approved

shaavan commented at 8:01 AM on December 3, 2021: contributor

ACK fab6c43b40773555b3f919c1403b8f3f48e92d5c

MarcoFalke commented at 9:13 AM on December 3, 2021: member

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

MarcoFalke referenced this in commit 0ee9a00f90 on Dec 3, 2021

fanquake commented at 9:18 AM on December 3, 2021: member

This was merged.

fanquake closed this on Dec 3, 2021

MarcoFalke deleted the branch on Dec 3, 2021

sidhujag referenced this in commit 5f64f29c74 on Dec 3, 2021

in src/rpc/net.cpp:154 in fab6c43b40

 221 | +                    {RPCResult::Type::NUM, "synced_headers", /*optional=*/true, "The last header we have in common with this peer"},
 222 | +                    {RPCResult::Type::NUM, "synced_blocks", /*optional=*/true, "The last block we have in common with this peer"},
 223 | +                    {RPCResult::Type::ARR, "inflight", /*optional=*/true, "",
 224 | +                    {
 225 | +                        {RPCResult::Type::NUM, "n", "The heights of blocks we're currently asking from this peer"},
 226 | +                    }},

luke-jr commented at 12:57 AM on December 10, 2021:

When are these not provided? Looks impossible to actually happen? Maybe a rare threading race (should we even document those?)?

MarcoFalke commented at 8:17 AM on December 10, 2021:

I am happy to review a pull request that always provides them.

luke-jr commented at 5:08 PM on December 10, 2021:

My point is that the current code appears to already always provide them.

MarcoFalke commented at 8:29 AM on December 11, 2021:

They are not if fStateStats is false.

Again, if you find a way to remove the fStateStats bool, please ping me for review.

luke-jr commented at 3:21 PM on December 11, 2021:

When can it be false?

MarcoFalke commented at 3:30 PM on December 11, 2021:

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

alirezaayande commented at 1:06 AM on December 10, 2021: none

.

fanquake deleted a comment on Dec 10, 2021

RandyMcMillan referenced this in commit bf0ef6150f on Dec 23, 2021

DrahtBot locked this on Dec 11, 2022