Skip to content

getrawtransaction should take a bool for verbose #9025

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
laanwj merged 2 commits into from
Nov 23, 2016
Merged

getrawtransaction should take a bool for verbose #9025

laanwj merged 2 commits into from
Nov 23, 2016

Conversation

@jnewbery
Copy link
Contributor

@jnewbery jnewbery commented Oct 26, 2016

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 mentioned this pull request Oct 26, 2016
Closed
@laanwj
Copy link
Member

laanwj commented Oct 26, 2016 •
edited
Loading

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
Copy link
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
Copy link
Contributor Author

jnewbery 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?

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
Copy link
Contributor Author

jnewbery commented Oct 26, 2016

@laanwj - warning removed and commits squashed.

jonasschnelli
jonasschnelli reviewed Oct 28, 2016
View reviewed changes
src/rpc/rawtransaction.cpp Outdated
Copy link
Contributor

@jonasschnelli jonasschnelli Oct 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jonasschnelli
jonasschnelli reviewed Oct 28, 2016
View reviewed changes
src/rpc/rawtransaction.cpp Outdated
Copy link
Contributor

@jonasschnelli jonasschnelli Oct 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Oct 28, 2016

utACK 837c06fe22a08d49f480ae629767c1ed8b7b9246, maybe consider nits/simplifications.

@jnewbery
Copy link
Contributor Author

jnewbery commented Nov 2, 2016

Thanks @jonasschnelli . Nits fixed and squashed.

laanwj
laanwj reviewed Nov 2, 2016
View reviewed changes
src/rpc/rawtransaction.cpp Outdated
Copy link
Member

@laanwj laanwj Nov 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

laanwj commented Nov 21, 2016

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

@jnewbery
Copy link
Contributor Author

jnewbery commented Nov 21, 2016

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
Copy link
Contributor Author

jnewbery commented Nov 22, 2016

@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
Copy link
Member

laanwj commented Nov 23, 2016 •
edited
Loading

utACK 240189b

@laanwj laanwj merged commit 240189b into bitcoin:master Nov 23, 2016
laanwj added a commit that referenced this pull request Nov 23, 2016
@laanwj
Merge #9025: getrawtransaction should take a bool for verbose
4d8558a 
240189b add testcases for getrawtransaction (John Newbery)
ce2bb23 getrawtransaction should take a bool for verbose (jnewbery)
@jnewbery jnewbery deleted the getrawtransbool branch November 23, 2016 10:19
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016
@jnewbery @luke-jr
getrawtransaction should take a bool for verbose
b979006 
Github-Pull: bitcoin#9025
Rebased-From: ce2bb23
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016
@jnewbery @luke-jr
add testcases for getrawtransaction
5072fd1 
Github-Pull: bitcoin#9025
Rebased-From: 240189b
@laanwj laanwj mentioned this pull request Feb 21, 2017
Closed
codablock pushed a commit to codablock/dash that referenced this pull request Jan 15, 2018
@laanwj @codablock
Merge bitcoin#9025: getrawtransaction should take a bool for verbose
ce3f38d 
240189b add testcases for getrawtransaction (John Newbery)
ce2bb23 getrawtransaction should take a bool for verbose (jnewbery)
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
@laanwj @andvgal
Merge bitcoin#9025: getrawtransaction should take a bool for verbose
c4d9f50 
240189b add testcases for getrawtransaction (John Newbery)
ce2bb23 getrawtransaction should take a bool for verbose (jnewbery)
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 24, 2019
@laanwj @CryptoCentric
Merge bitcoin#9025: getrawtransaction should take a bool for verbose
9ae8939 
240189b add testcases for getrawtransaction (John Newbery)
ce2bb23 getrawtransaction should take a bool for verbose (jnewbery)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

+1 more reviewer

@jonasschnelli jonasschnelli jonasschnelli left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jnewbery @laanwj @luke-jr @jonasschnelli