getrawtransaction should take a bool for verbose #9025

Merged
merged 2 commits into from Nov 23, 2016

Projects

None yet

4 participants

@jnewbery
Contributor

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.

@jnewbery jnewbery referenced this pull request Oct 26, 2016
Closed

Rpc boolean consistency #9024

@laanwj
Member
laanwj commented Oct 26, 2016 edited

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
Member
luke-jr commented Oct 26, 2016

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

@jnewbery
Contributor

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
Contributor

@laanwj - warning removed and commits squashed.

@laanwj laanwj added the RPC/REST/ZMQ label Oct 26, 2016
src/rpc/rawtransaction.cpp
"\nArguments:\n"
"1. \"txid\" (string, required) The transaction id\n"
- "2. verbose (numeric, optional, default=0) If 0, return a string, other return a json object\n"
+ "2. verbose (bool, optional, default=false) If true, return a string, other return a json object\n"
"\nResult (if verbose is not set or set to 0):\n"
@jonasschnelli
jonasschnelli Oct 28, 2016 edited Member

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

src/rpc/rawtransaction.cpp
- fVerbose = (request.params[1].get_int() != 0);
+ if ((request.params.size() > 1) &&
+ ((request.params[1].isNum() && request.params[1].get_int() != 0) ||
+ (request.params[1].isBool() && request.params[1].get_bool())))
@jonasschnelli
jonasschnelli Oct 28, 2016 Member

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

@jonasschnelli
Member

utACK 837c06f, maybe consider nits/simplifications.

@jnewbery
Contributor
jnewbery commented Nov 2, 2016

Thanks @jonasschnelli . Nits fixed and squashed.

src/rpc/rawtransaction.cpp
- fVerbose = (request.params[1].get_int() != 0);
+ if ((request.params.size() > 1) &&
+ ((request.params[1].isNum() && request.params[1].get_int() != 0) ||
+ (request.params[1].isTrue())))
@laanwj
laanwj Nov 2, 2016 Member

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
Member
laanwj commented Nov 21, 2016

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

@jnewbery
Contributor

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.

@jnewbery
Contributor

@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
Member
laanwj commented Nov 23, 2016 edited

utACK 240189b

@laanwj laanwj merged commit 240189b into bitcoin:master Nov 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laanwj laanwj added a commit that referenced this pull request Nov 23, 2016
@laanwj laanwj Merge #9025: getrawtransaction should take a bool for verbose
240189b add testcases for getrawtransaction (John Newbery)
ce2bb23 getrawtransaction should take a bool for verbose (jnewbery)
4d8558a
@jnewbery jnewbery deleted the jnewbery:getrawtransbool branch Nov 23, 2016
@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016
@jnewbery @luke-jr jnewbery + luke-jr getrawtransaction should take a bool for verbose
Github-Pull: #9025
Rebased-From: ce2bb23
b979006
@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016
@jnewbery @luke-jr jnewbery + luke-jr add testcases for getrawtransaction
Github-Pull: #9025
Rebased-From: 240189b
5072fd1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment