[rpc] fix verbose argument for getblock in bitcoin-cli #10747

Merged
merged 1 commit into from Jul 10, 2017

Conversation

Projects
None yet
7 participants
Member

jnewbery commented Jul 5, 2017

Using the verbose option with getblock in bitcoin-cli has been broken since #8704:

→ bitcoin-cli -named getblock blockhash=0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206 verbose=true
error code: -1
error message:
JSON value is not a boolean as expected

→ bitcoin-cli -named getblock blockhash=0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206 verbosity=true
{
  "hash": "0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206",
...

In general, I think that adding aliases because some people find argument names distasteful is a bad idea since it leads to subtle bugs like this.

However, that functionality has already been merged in so I'm not going to try to undo it. This is the simplest fix for restoring the previous behavior.

@achow101 @luke-jr

Member

jonasschnelli commented Jul 5, 2017

I agree, we should not allow aliases (If possible, something API compatibility could require it) . Should we then get completely rid of the alias here: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/blockchain.cpp#L1542

The alias in general works, it's just the bitcoin-cli's JSON string-to-bool conversion that failed.

Contributor

achow101 commented Jul 5, 2017

utACK 58e9864

Contributor

TheBlueMatt commented Jul 5, 2017

Not to blow a small fix into a big change, but is it possible to more easily ensure we dont break this stuff in the future (also generally issues in having typos between the server registration and client registration here scares me cause most of it is untested). Maybe the easiest way is to just add a python or bash script that checks for consistency ala our documentation-required-for-options checks?

Member

jnewbery commented Jul 5, 2017

is it possible to more easily ensure we dont break this stuff in the future

Yes, absolutely agree that there should be better testing (bitcoin-cli seems to be entirely untested). Can we open an issue to track that and treat this as a quick fix for the recent regression?

Contributor

TheBlueMatt commented Jul 5, 2017

@jnewbery fair enough. utACK 58e9864

Member

luke-jr commented Jul 5, 2017

The alias isn't about taste, but compatibility. bitcoin-cli is essentially just a testing tool, so I don't think it needs to itself be compatible (but I don't mind either).

Owner

laanwj commented Jul 10, 2017

utACK 58e9864

@laanwj laanwj merged commit 58e9864 into bitcoin:master Jul 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@laanwj laanwj added a commit that referenced this pull request Jul 10, 2017

@laanwj laanwj Merge #10747: [rpc] fix verbose argument for getblock in bitcoin-cli
58e9864 [rpc] fix verbose argument for getblock in bitcoin-cli (John Newbery)

Tree-SHA512: 7f176e1ddd9f3f7722ea0d268549629567ecf0c30bebf368824997566df0bfa01d31cf761abc9ca355e48c0bf0cb06d49d15a02b858999fcb7472dc7df2fbbf2
9edda0c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment