rpc: Properly document return values (submitblock, gettxout, getblocktemplate, scantxoutset)

MarcoFalke commented at 9:19 am on December 3, 2020: member

Currently a few return values are undocumented. This is causing confusion at the least. See for example #18476

MarcoFalke added the label Docs on Dec 3, 2020

MarcoFalke added the label RPC/REST/ZMQ on Dec 3, 2020

DrahtBot commented at 1:30 pm on December 3, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21426 (rpc: remove scantxoutset EXPERIMENTAL warning by jonatack)
#21401 (Refactor versionbits deployments to avoid potential uninitialized variables by achow101)
#21399 (Genericide BIP9 in variable/type names and comments by luke-jr)
#21393 (BIP 341: Add Speedy Trial activation parameters by achow101)
#21392 (Implement BIP 8 based Speedy Trial activation by achow101)
#20286 (rpc: deprecate addresses and reqSigs from rpc outputs by mjdietzx)
#16795 (rpc: have raw transaction decoding infer output descriptors by instagibbs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jarolrod commented at 4:25 pm on December 4, 2020: member

Testing fa3cbac Calling gettxout with an invalid utxo returns no output using bitcoin-cli, but returns null in the gui. I think the better behavior would be to return an empty json block like we do with other rpc commands such as listtransactions

MarcoFalke commented at 4:39 pm on December 4, 2020: member

This can be discussed in #18476 further. The goal of this pull is to simply document the return values as they are and always have been.

jonasschnelli commented at 8:09 am on December 7, 2020: contributor

Looks good. What is the rational in renaming STR_AMOUNT to NUM_AMOUNT? It creates a fairly large diff and IMO it still is represented as a string on the JSON layer.

MarcoFalke commented at 8:18 am on December 7, 2020: member

The (scripted) diff is only 50 lines. If there are any conflicts, it should be trivial to resolve. That the internal representation is a string could be a coincidence. The type is really VNUM (numeric) and not VSTR (string).

I checked that the only conflict due to this scripted diff is #19002.

laanwj commented at 1:06 pm on December 18, 2020: member

Just an aside: there used to be talk of, at some point, of making ValueFromAmount return a decimal string instead of a number, or at least having an option to do so, because it’s easier to parse exactly in some languages that interpret javascript numbers as floating point number (which are unwise to use for monetary amounts). See e.g. #3759.

This bled out but seeing them as separate types an abstraction level above JSON string/value distinction makes sense.

MarcoFalke force-pushed on Dec 21, 2020

MarcoFalke commented at 2:11 pm on December 21, 2020: member

Ok, dropped the scripted diff because it seemed controversial

MarcoFalke force-pushed on Jan 29, 2021

MarcoFalke commented at 9:12 am on January 29, 2021: member

Rebased

laanwj added this to the "Blockers" column in a project

in src/rpc/blockchain.cpp:1114 in fa84858cc0 outdated

1133+            {"txid", RPCArg::Type::STR, RPCArg::Optional::NO, "The transaction id"},
1134+            {"n", RPCArg::Type::NUM, RPCArg::Optional::NO, "vout number"},
1135+            {"include_mempool", RPCArg::Type::BOOL, /* default */ "true", "Whether to include the mempool. Note that an unspent output that is spent in the mempool won't appear."},
1136+        },
1137+        {
1138+            RPCResult{"When the UTXO couldn't be found", RPCResult::Type::NONE, "", ""},

amitiuttarwar commented at 9:58 pm on February 24, 2021:

I’d find it clearer to say:

0            RPCResult{"If the UTXO was not found", RPCResult::Type::NONE, "", ""},

in src/rpc/mining.cpp:533 in fa84858cc0 outdated

553-                RPCResult{
554-                    RPCResult::Type::OBJ, "", "",
555+        },
556+        {
557+            RPCResult{"When mode=='proposal'", RPCResult::Type::STR, "", ""},
558+            RPCResult{"When mode=='proposal'", RPCResult::Type::NONE, "", ""},

amitiuttarwar commented at 10:11 pm on February 24, 2021:

as an RPC user, I’m confused about how to interpret this output.. when the mode is proposal, there might be a string returned (of what?) or a json null object? what conditions lead to one or the other?

don’t need to go into detail, but since the goal here is to document return types & help usability, I think this is worth clarifying.

MarcoFalke commented at 7:21 am on February 25, 2021:

Thanks, fixed

amitiuttarwar commented at 10:20 pm on February 24, 2021: contributor

concept ACK. one improvement requested, otherwise generally looks good. I checked most of the documented results actually match up.

rpc: Properly document gettxout return value

Can be reviewed with --ignore-all-space

faa2059547

rpc: Properly document scantxoutset return value

Can be reviewed with --ignore-all-space

fabaccf031

MarcoFalke force-pushed on Feb 25, 2021

rpc: Properly document getblocktemplate return value

Can be reviewed with --ignore-all-space

fae542c28b

rpc: Properly document submitblock return value

Can be reviewed with --ignore-all-space

fa7ff0790e

MarcoFalke force-pushed on Feb 25, 2021

MarcoFalke commented at 7:22 am on February 25, 2021: member

Force pushed to address feedback. Should be easy to re-ACK with git range-diff

in src/rpc/mining.cpp:519 in fa7ff0790e

521+        "    https://github.com/bitcoin/bips/blob/master/bip-0145.mediawiki\n",
522+        {
523+            {"template_request", RPCArg::Type::OBJ, "{}", "Format of the template",
524+            {
525+                {"mode", RPCArg::Type::STR, /* treat as named arg */ RPCArg::Optional::OMITTED_NAMED_ARG, "This must be set to \"template\", \"proposal\" (see BIP 23), or omitted"},
526+                {"capabilities", RPCArg::Type::ARR, /* treat as named arg */ RPCArg::Optional::OMITTED_NAMED_ARG, "A list of strings",

amitiuttarwar commented at 7:57 pm on February 25, 2021:

tangential since this PR just aims to document return types, but I think a data param is missing from these help docs?

MarcoFalke commented at 9:04 am on February 26, 2021:

I think this is intentionally omitted (via template_request, which is described in the BIPs)

amitiuttarwar approved

amitiuttarwar commented at 10:01 pm on February 25, 2021: contributor

tACK fa7ff0790e

gettxout -> checked we get an empty result if UTXO is not found
scantxoutset -> checked abort returns bool, status returns empty or progress based on if scan is occurring, the existing docs apply to results from start command.
getblocktemplate -> checked we get string result if proposal is rejected, didn’t test an accepted proposal, but saw in the code that we would return null, saw the existing docs apply to other modes.
submitblock -> same, saw string for failure, looked at code for success

made sure all the help docs make sense & look good when called from command line

the changes from }, \n }, -> }}, kind of confuse me, but I assume its fine considering code compiles & help docs are showing up correctly.

fjahr commented at 8:55 pm on March 14, 2021: member

utACK fa7ff0790ec21d187f1a32310918b0c8d66e5266

Reviewed changes ignoring whitespace.

fanquake merged this on Mar 15, 2021

fanquake closed this on Mar 15, 2021

MarcoFalke deleted the branch on Mar 15, 2021

sidhujag referenced this in commit 08592b0bba on Mar 15, 2021

jnewbery removed this from the "Blockers" column in a project

luke-jr referenced this in commit 1efe3b8b8d on Dec 14, 2021

luke-jr referenced this in commit e9aca6156e on Dec 14, 2021

luke-jr referenced this in commit 175336e44e on Dec 14, 2021

luke-jr referenced this in commit b83129bb8a on Dec 14, 2021

Fabcien referenced this in commit ae11b360f0 on Apr 13, 2022

DrahtBot locked this on Aug 16, 2022

rpc: Properly document return values (submitblock, gettxout, getblocktemplate, scantxoutset) #20556

Conflicts