Fix bitcoin-cli exit status code

[cozz]

bitcoin-cli returns 1 on success and 0 on error. By convention it should be the other way round.

It is not possible to distinguish between different error codes, all errors return 0.
But for an AppInitRPC error, we would return 1...

[laanwj]

Maybe we should also return 1 in the case of an exception (so don't default ret to 0)

[cozz]

Yes, I was also thinking about that, but I was not sure.
So I will update the pull then.

[cozz]

update:

• return 1 in the case of an exception

[laanwj]

ACK on code changes (didn't test yet)

[laanwj]

$ src/bitcoin-cli help; echo $?
...
0

$ src/bitcoin-cli; echo $?
... [help message] ...
1

$ src/bitcoin-cli sendtoaddress "1234" 1.0; echo $?
error: {"code":-5,"message":"Invalid Bitcoin address"}
5

src/bitcoin-cli -rpcport=6667 help; echo $?
error: couldn't connect to server
87

ACK

[laanwj]

Nothing new in this pull, but I wonder why this returns 87. IMO it should simply be 1 (=-RPC_MISC_ERROR): https://github.com/bitcoin/bitcoin/blob/master/src/rpcclient.cpp#L236

[mcassano]

cozz: Can you refactor the 1 and 0 into RET_SUCCESS and RET_FAIL variables?

[laanwj]

RET_SUCCESS / RET_FAIL is not any POSIX or C standard is it? We're also not using them in any other place at the moment.

If you want to use constants, using abs(constant_from_RPCErrorCode) from src/rpcprotocol.h would make more sense as that's where the error code that are returned by CommandLineRPC (and thus by the -cli now) come from.

[cozz]

update:

• replace 1 with abs(RPC_MISC_ERROR)
• replace 87 with abs(RPC_MISC_ERROR)

We might break a script by changing this error code, but should be very unlikely.

[BitcoinPullTester]

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/875acdf78693332a1500557ee728a4341dbe2d4c for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

[laanwj]

Execution will never get here. PrintException does a re-throw unlike PrintExceptionContinue. I'd never code it that way and would put the throw in the catch() statement itself for clarity, but this is part of our legacy.

[cozz]

I see. Removing this then.

[laanwj]

ACK on the abs(RPC_MISC_ERROR) change.