rpc: Improve RPC help by explicitly mentioning output types #24339

pull kiminuo wants to merge 2 commits into bitcoin:master from kiminuo:feature/2022-02-output-types changing 4 files +18 −11

kiminuo commented at 1:39 PM on February 14, 2022: contributor

This PR attempts to replicate https://github.com/bitcoin/bitcoin/blob/0ccf9b2e5594581deef2f60174c3651a57f93b64/src/rpc/rawtransaction.cpp#L547 to one other place (at the moment) so that users have better idea what RPC methods can actually return.

I created this PR as a follow-up to the idea mentioned here #23320 (review) (resolved).
Move `GetAllOutputTypes` function from `rpc/rawtransaction.cpp` to `rpc/util.{h|cpp}` d970a85d33
kiminuo renamed this:
~~Improve RPC help by explicitly mentioning output types~~
rpc: Improve RPC help by explicitly mentioning output types
on Feb 14, 2022
kiminuo commented at 1:55 PM on February 14, 2022: contributor

cc @laanwj given he said #23320 (review).
DrahtBot added the label RPC/REST/ZMQ on Feb 14, 2022
kiminuo marked this as ready for review on Feb 15, 2022

laanwj commented at 12:18 PM on February 15, 2022: member

Concept ACK

Edit: diff:

--- /tmp/getblock.1     2022-02-15 13:33:21.436527762 +0100
+++ /tmp/getblock.2     2022-02-15 13:48:38.017920912 +0100
@@ -66,7 +66,7 @@
               "asm" : "str",             (string) The asm
               "hex" : "str",             (string) The hex
               "address" : "str",         (string, optional) The Bitcoin address (only if a well-defined address exists)
-              "type" : "str"             (string) The type, eg 'pubkeyhash'
+              "type" : "str"             (string) The type (nonstandard, pubkey, pubkeyhash, scripthash, multisig, nulldata, witness_v0_scripthash, witness_v0_keyhash, witness_v1_taproot, witness_unknown)
             }
           }
         },

michaelfolkson commented at 1:22 PM on February 15, 2022: contributor

Concept ACK

Seems like an improvement to display all the output types in the RPC help rather than just the pubkeyhash example.
kiminuo commented at 1:27 PM on February 15, 2022: contributor

Does it make sense to you to use GetAllOutputTypes where possible. I mean in RPC help sections?

Is the output OK? Or are possibly any output types that you would NOT show?

in src/rpc/blockchain.cpp:1040 in 78d9227ced outdated

1036 | @@ -1037,7 +1037,7 @@ static RPCHelpMan getblock()
1037 |                                              {RPCResult::Type::STR, "asm", "The asm"},
1038 |                                              {RPCResult::Type::STR, "hex", "The hex"},
1039 |                                              {RPCResult::Type::STR, "address", /* optional */ true, "The Bitcoin address (only if a well-defined address exists)"},
1040 | -                                            {RPCResult::Type::STR, "type", "The type, eg 'pubkeyhash'"},
1041 | +                                            {RPCResult::Type::STR, "type", "The type (" + GetAllOutputTypes() + ")"},

luke-jr commented at 10:52 PM on February 18, 2022:

nit

Existing other case says "(e.g. " ...

Maybe "(one of: " would be better.

prusnak commented at 10:31 AM on February 20, 2022:

I agree that one of: sounds better if we are enumerating all possible options.

Let's also change src/rpc/rawtransaction.cpp:568

kiminuo commented at 9:35 PM on February 20, 2022:

Thank you. Modified.

luke-jr approved
luke-jr commented at 10:52 PM on February 18, 2022: member

utACK 78d9227ced0442c10249a08ba86386f7b3d2755a
theStack commented at 6:46 PM on February 20, 2022: member

Concept ACK
kiminuo force-pushed on Feb 20, 2022
kristapsk approved
kristapsk commented at 5:40 AM on February 21, 2022: contributor

ACK c9ca3e8bab76da8ab20c362c8cd541443e2e8d77
prusnak approved
prusnak commented at 8:15 AM on February 21, 2022: contributor

Approach ACK c9ca3e8bab76da8ab20c362c8cd541443e2e8d77
Use `GetAllOutputTypes` in `getblock` RPC function c821ab8be8

in src/rpc/rawtransaction.cpp:567 in c9ca3e8bab outdated

 563 | @@ -574,7 +564,7 @@ static RPCHelpMan decodescript()
 564 |                   {
 565 |                       {RPCResult::Type::STR, "asm", "String representation of the script public key"},
 566 |                       {RPCResult::Type::STR_HEX, "hex", "Hex string of the script public key"},
 567 | -                     {RPCResult::Type::STR, "type", "The type of the script public key (e.g. witness_v0_keyhash or witness_v0_scripthash)"},
 568 | +                     {RPCResult::Type::STR, "type", "The type of the script public key (one of: " + GetAllOutputTypes() + ")"},

MarcoFalke commented at 8:28 AM on February 21, 2022:

pretty sure this is wrong. Only v0 can be wrapped, no?

kiminuo commented at 12:00 PM on February 21, 2022:

Reverted. Thanks.

kiminuo force-pushed on Feb 21, 2022
kristapsk approved
kristapsk commented at 12:29 PM on February 21, 2022: contributor

re-ACK c821ab8be8dffb749853c05e05cb515c11e6328a
MarcoFalke merged this on Feb 21, 2022
MarcoFalke closed this on Feb 21, 2022
kiminuo deleted the branch on Feb 21, 2022
sidhujag referenced this in commit ae8ff8541c on Feb 22, 2022
DrahtBot locked this on Feb 21, 2023

Contributors

Labels

Linked (view graph)

#23320 rpc: Add RPC help for getblock verbosity level 3