getrawtransaction should take a bool for verbose #9025

jnewbery commented at 12:11 PM on October 26, 2016: member

As suggested by @gmaxwell in #9024 .

getrawtransaction now takes a bool for verbose. It will still take an int for verbose for back-compatibility, but issue a warning to the user.

laanwj commented at 12:13 PM on October 26, 2016: member

utACK, but please remove the deprecation warning. We may want to repurpose the ints as 'verbosity level' in the future, if an even more verbose mode is added. See #8704 (comment) . There's zero overhead in simply accepting both without warnings or complaints.

Maybe add a GetVerbosityLevel function that accepts UniValue and returns int. It would map bool false to 0, bool true to 1, as well as accepts integers (which are returned as-is). This could be shared between various RPC calls that have a verbosity level (yay consistency).

luke-jr commented at 12:15 PM on October 26, 2016: member

Seems if we're going to do a verbosity level for getblock, we should just make that the "preferred" form here too?

jnewbery commented at 12:29 PM on October 26, 2016: member

Seems if we're going to do a verbosity level for getblock, we should just make that the "preferred" form here too?

NACK. There are several other RPCs which take a bool for a more verbose output: getrawmempool, getmempoolancestors, getmempooldescendants, getblockheader, getblock, gettxout (where the option is includemempool). As far as I can tell, getrawtransaction is the only call which currently takes a num. It's less disruptive to change one RPC to use a bool than change all the others to use nums.

jnewbery force-pushed on Oct 26, 2016

jnewbery commented at 12:39 PM on October 26, 2016: member

@laanwj - warning removed and commits squashed.

laanwj added the label RPC/REST/ZMQ on Oct 26, 2016

in src/rpc/rawtransaction.cpp:None in 837c06fe22 outdated

 143 |              "\nArguments:\n"
 144 |              "1. \"txid\"      (string, required) The transaction id\n"
 145 | -            "2. verbose       (numeric, optional, default=0) If 0, return a string, other return a json object\n"
 146 | +            "2. verbose       (bool, optional, default=false) If true, return a string, other return a json object\n"
 147 |  
 148 |              "\nResult (if verbose is not set or set to 0):\n"

jonasschnelli commented at 8:47 AM on October 28, 2016:

maybe s/or set to 0/or set to false/?

in src/rpc/rawtransaction.cpp:None in 837c06fe22 outdated

 206 |      bool fVerbose = false;
 207 | -    if (request.params.size() > 1)
 208 | -        fVerbose = (request.params[1].get_int() != 0);
 209 | +    if ((request.params.size() > 1) &&
 210 | +        ((request.params[1].isNum() && request.params[1].get_int() != 0) ||
 211 | +         (request.params[1].isBool() && request.params[1].get_bool())))

jonasschnelli commented at 8:50 AM on October 28, 2016:

maybe simplify with s/(request.params[1].isBool() && request.params[1].get_bool())/request.params[1].isTrue()/

jonasschnelli commented at 8:50 AM on October 28, 2016: contributor

utACK 837c06fe22a08d49f480ae629767c1ed8b7b9246, maybe consider nits/simplifications.

jnewbery force-pushed on Nov 2, 2016

jnewbery commented at 5:45 PM on November 2, 2016: member

Thanks @jonasschnelli . Nits fixed and squashed.

jnewbery force-pushed on Nov 2, 2016

in src/rpc/rawtransaction.cpp:None in 719f7e36f0 outdated

 206 |      bool fVerbose = false;
 207 | -    if (request.params.size() > 1)
 208 | -        fVerbose = (request.params[1].get_int() != 0);
 209 | +    if ((request.params.size() > 1) &&
 210 | +        ((request.params[1].isNum() && request.params[1].get_int() != 0) ||
 211 | +         (request.params[1].isTrue())))

laanwj commented at 8:28 PM on November 2, 2016:

Note that isTrue doesn't fail if the value is not a bool, so this also accepts non-bool and non-int values. E.g.:

src/bitcoin-cli -regtest getrawtransaction 0000000000000000000000000000000000000000000000000000000000000000 '"b"'
error code: -5
error message:
No information available about transaction

Same for {} [] etc. Should probably raise an error in that case.

laanwj commented at 8:13 AM on November 21, 2016: member

@jnewbery this just needs my above comment fixed and it can be merged.

jnewbery commented at 11:10 AM on November 21, 2016: member

Thanks @laanwj . I've made the suggested change. I want to write some additional test cases to cover the new code and then I'll push the new commits.

getrawtransaction should take a bool for verbose ce2bb23aa5

add testcases for getrawtransaction 240189b2c1

jnewbery force-pushed on Nov 22, 2016

jnewbery commented at 2:40 PM on November 22, 2016: member

@laanwj - as requested, getrawtransaction now throws an error whenever an argument is passed in which isn't either a bool or an int. I've also added test cases to rawtransaction.py to cover cover the RPC.

laanwj commented at 6:12 AM on November 23, 2016: member

utACK https://github.com/bitcoin/bitcoin/pull/9025/commits/240189b2c14244c3058e80b40bd4dacbd200c4c7

laanwj merged this on Nov 23, 2016

laanwj closed this on Nov 23, 2016

laanwj referenced this in commit 4d8558a287 on Nov 23, 2016

jnewbery deleted the branch on Nov 23, 2016

luke-jr referenced this in commit b97900676e on Dec 21, 2016

luke-jr referenced this in commit 5072fd1e84 on Dec 21, 2016

codablock referenced this in commit ce3f38d6ca on Jan 15, 2018

andvgal referenced this in commit c4d9f504b0 on Jan 6, 2019

CryptoCentric referenced this in commit 9ae893925a on Feb 24, 2019

MarcoFalke locked this on Sep 8, 2021