getblockhash: Return RPC_NOT_FOUND when nHeight

dermoth commented at 6:18 AM on June 18, 2015: none

Based on IRC talks, renamed RPC_INVALID_ADDRESS_OR_KEY to RPC_NOT_FOUND and made getblockhash return NOT_FOUND when block is above the current height.

Some chat excerpts, which started earlier on the fact the error code changed in 10.x for out of range getblockhash requests:

<wumpus> RPC_INVALID_PARAMETER           = -8,  //! Invalid, missing or duplicate parameter
<wumpus> that's a strange error code to return for a non-existing block
* splix has quit (Remote host closed the connection)
<dermoth> https://github.com/bitcoin/bitcoin.git
<wumpus> are you providing a valid hash?
<dermoth> oops
<dermoth> 2ffdf21ce39fc3133fc
<dermoth> that's the commit
* Aido (~Aido@178.167.130.193.threembb.ie) has joined #bitcoin-dev
<dermoth> parameter is a block number, app used to rely on error -1 to know when it reached the most recent block
<wumpus> -1 is the general error, -8 is a more specialized one
<dermoth> I see
* hsmiths has quit (Quit: END OF LINE)
<gmaxwell> error: {"code":-8,"message":"Block height out of range"}
<wumpus> I think a NOT_FOUND code would be more appropriate than invalid parameter (which hints at a parse error),but don't be afraid I won't change it again...
<dermoth> agreed
<wumpus> at least the message is clear

and later on...

<dermoth> closest error IMHO, in existing code, is RPC_INVALID_ADDRESS_OR_KEY
<wumpus> indeed. We've been using that as a NOT_FOUND.
<wumpus> maybe renaming it would be an idea
<wumpus> as said, those error names are back-projected by me, the original codebase used bare numbers without any explanation
<dermoth> would you keep the current one as an alias? I can submit a pr
<wumpus> yes, good idea
<dermoth> if (nHeight < 0... should be NOT FOUND too?
<dermoth> I see where the invalid param comes from, maybe split it up then
<wumpus> bleh.
<wumpus> see why this was never addressed? you lift it in one place, and notice the whole fabric is coming apart :-)
<wumpus> I mean I'm sure RPC_INVALID_ADDRESS_OR_KEY is also used in many cases where it really means RPC_INVALID_PARAMETER
<wumpus> before you know it, you're breaking the entire API
* abossard (~andre@46.140.123.2) has joined #bitcoin-dev
<wumpus> (and maybe that's for the best, but then it will be a fully fledged RPC return values redesign, and you would have accomplished exactly what you were coming here to complain about in the first place - hehe)
<wumpus> but maybe there's a middle way, I don't know, thanks for looking into it.

The change has been tested with the backward-compatible alias commented out:

<dermoth> wumpus, tested ok
<dermoth> $ ~/bitcoin/bitcoin/bin/bitcoin-cli getblockhash -1
<dermoth> error: {"code":-8,"message":"Negative block height"}
<wumpus> great, time to submit a PR then
<dermoth> $ ~/bitcoin/bitcoin/bin/bitcoin-cli getblockhash 361428
<dermoth> error: {"code":-5,"message":"Block height out of range"}
<dermoth> $ ~/bitcoin/bitcoin/bin/bitcoin-cli getblockhash 361424
<dermoth> 000000000000000004fd86600f6a12e5186ec18f11d7502afba3b7f7e00cae1a
<dermoth> used the 10.2 client though, but was runnign the 0.11-blarhs-dirty one (dirty bc of the commented out alias)
<dermoth> v0.11.99.0-fa19502-dirty (64-bit)

Rename RPC_INVALID_ADDRESS_OR_KEY to RPC_NOT_FOUND e800440818

getblockhash: Return RPC_NOT_FOUND when nHeight > chainActive.Height() 989f61e418

Replace RPC_NOT_FOUND with RPC_INVALID_PARAMETER where appropriate c7b85ae25d

in src/wallet/rpcdump.cpp:None in fa19502a79 outdated

 182 | @@ -183,7 +183,7 @@ UniValue importaddress(const UniValue& params, bool fHelp)
 183 |          std::vector<unsigned char> data(ParseHex(params[0].get_str()));
 184 |          script = CScript(data.begin(), data.end());
 185 |      } else {
 186 | -        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address or script");
 187 | +        throw JSONRPCError(RPC_NOT_FOUND, "Invalid Bitcoin address or script");

jonasschnelli commented at 6:52 AM on June 18, 2015:

Not sure about that. This example (rpcdum.cppL186): there RPC_INVALID_ADDRESS_OR_KEY is totally fine IMO.

dermoth commented at 8:26 AM on June 18, 2015:

You're right; this one should be RPC_INVALID_PARAMETER. Same for a couple other cases, but many of them are really not found errors.

dermoth force-pushed on Jun 18, 2015

dermoth commented at 9:03 AM on June 18, 2015: none

I corrected a typo in first commit, missed since I tested with the line commented out.

The 3rd commit changes the obviously invalid errors to RPC_INVALID_PARAMETER.. The least obvious case was:

src/wallet/rpcwallet.cpp: throw JSONRPCError(RPC_NOT_FOUND, "Invalid or non-wallet transaction id");

I left it at NOT_FOUND as it's what it actually means: transaction not found in wallet.

dermoth commented at 3:39 PM on June 27, 2015: none

@jonasschnelli can you please check out my updated PR?

I really don't think we need INVALID_ADDRESS and INVALID_PARAMETER; addresses are request parameters after all. Since a lot of the INVALID_ADDRESS were basically meaning "address not found" I changed the error code for the validity checks to INVALID_PARAMETER, so INVALID ADDRESS can be cleanly renamed to NOT_FOUND with a broader use case (such as looking up blocks that doesn't exist).

Regards,

in src/wallet/rpcwallet.cpp:None in c7b85ae25d

 537 | @@ -538,7 +538,7 @@ UniValue signmessage(const UniValue& params, bool fHelp)
 538 |  
 539 |      vector<unsigned char> vchSig;
 540 |      if (!key.SignCompact(ss.GetHash(), vchSig))
 541 | -        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Sign failed");
 542 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, "Sign failed");

jonasschnelli commented at 4:59 PM on June 27, 2015:

This should be something different than RPC_INVALID_PARAMETER (maybe a general "internal error" or introduce a new RPC_SIGN_FAILED).

in src/wallet/rpcwallet.cpp:None in c7b85ae25d

 572 | @@ -573,7 +573,7 @@ UniValue getreceivedbyaddress(const UniValue& params, bool fHelp)
 573 |      // Bitcoin address
 574 |      CBitcoinAddress address = CBitcoinAddress(params[0].get_str());
 575 |      if (!address.IsValid())
 576 | -        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address");
 577 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid Bitcoin address");

jonasschnelli commented at 5:01 PM on June 27, 2015:

Why not keeping RPC_INVALID_ADDRESS_OR_KEY and adding RPC_INVALID_PARAMETER (not renaming from RPC_INVALID_PARAMETER)? In this case (and the ones below) RPC_INVALID_ADDRESS_OR_KEY is more precise than RPC_INVALID_PARAMETER.

jgarzik commented at 9:47 PM on June 27, 2015: contributor

mostly ACK. @jonasschnelli makes good comments. if we take my own thoughts and his comments into account, the following results:

add RPC_NOT_FOUND, stealing current API integer value used for RPC_INVALID_ADDRESS_OR_KEY
update RPC_INVALID_ADDRESS_OR_KEY with new API integer value, in the rare cases where it is actually applicable
keep the rest of the changes in this PR (well done)
add release note; these are API changes in RPC which are not backwards compatible

laanwj added the label RPC on Jul 2, 2015

laanwj commented at 5:59 AM on July 27, 2015: member

add release note; these are API changes in RPC which are not backwards compatible

Agree, please add something to doc/release-notes.md mentioning this interface change. It's a minor change so it can be very short.

laanwj commented at 6:18 AM on July 27, 2015: member

utACK apart from that, this makes the return values more consistent.

jgarzik commented at 1:49 PM on September 16, 2015: contributor

Seems merge-ready after rebase and release note update as @laanwj requested

dcousens commented at 3:35 PM on September 16, 2015: contributor

concept ACK

dermoth commented at 12:02 PM on September 18, 2015: none

@jgarzik, @laanwj, sorry I haven't yet made the changes suggested as I've been swamped with other things. I'll try to take a stab at it this week-end.

Regards

laanwj commented at 8:12 AM on October 27, 2015: member

@dermoth Ping

laanwj commented at 5:21 PM on November 11, 2015: member

Closing this for now. Let me know when you start working on this again, then we'll reopen.

laanwj closed this on Nov 11, 2015

DrahtBot locked this on Sep 8, 2021

getblockhash: Return RPC_NOT_FOUND when nHeight > chainActive.Height() #6300