bitcoin-cli returns no error on unsuccessful gettxout call #18476

issue adiabat openend this issue on March 30, 2020
  1. adiabat commented at 10:01 pm on March 30, 2020: none

    When using bitcoin-cli command gettxout, the program does not return an error if the utxo queried doesn’t exist. I initially found this behavior a bit confusing, as in command line contexts a command not returning anything usually means “it worked”. Similar calls like getmempoolentry will return Transaction not in mempool errors, so I expected something like that from gettxout.

    Expected behavior

    (maybe this is more like “desired” behavior) How about something like:

    0user@computer:~$ bitcoin-cli gettxout 0e3e2357e806b6cdb1f70b54c3a3a17b6714ee1f0e68bebb44a74b1efd512098 1
    1error code: -5
    2error message:
    3Transaction output not found
    4user@computer:~$
    

    Actual behavior

     0user@computer:~$ bitcoin-cli gettxout 0e3e2357e806b6cdb1f70b54c3a3a17b6714ee1f0e68bebb44a74b1efd512098 0
     1{
     2  "bestblock": "00000000000000000000b2320e348d60a5eb0532a1fe67bddd197077f7782637",
     3  "confirmations": 623664,
     4  "value": 50.00000000,
     5  "scriptPubKey": {
     6    "asm": "0496b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52da7589379515d4e0a604f8141781e62294721166bf621e73a82cbf2342c858ee OP_CHECKSIG",
     7    "hex": "410496b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52da7589379515d4e0a604f8141781e62294721166bf621e73a82cbf2342c858eeac",
     8    "type": "pubkey"
     9  },
    10  "coinbase": true
    11}
    12user@computer:~$ bitcoin-cli gettxout 0e3e2357e806b6cdb1f70b54c3a3a17b6714ee1f0e68bebb44a74b1efd512098 1
    13user@computer:~$
    

    To reproduce

    Query any non-exsistent utxo with gettxout. Nothing happens, and the program returns with no error.

    System information

    Ubuntu 18.04.3 LTS, Bitcoin Core 0.19.01

    Fix? Is there a reason why either the RPC call itself or bitcoin-cli doesn’t return an error in this case? If so, or if it’s complicated to fix, then the lack of an error message isn’t not a big deal, and I can close this or leave it around as a “yeah maybe should fix that someday”. If it’s just an omission however, I’d be happy to open a PR changing it to return some kind of error message.

  2. adiabat added the label Bug on Mar 30, 2020
  3. sipa commented at 10:08 pm on March 30, 2020: member

    I disagree this is a bug. The RPC user cannot know that the UTXO doesn’t exist (its existence is part of the RPC answer), so it shouldn’t be an error when this occurs. The response is a JSON null, which is printed by bitcoin-cli as the empty string.

    In retrospect it would probably have been better to add a field “exists” or so, and only emit "exits": false in case of a non-existing UTXO. That’d be a pretty big breaking change though.

  4. adiabat commented at 10:12 pm on March 30, 2020: none

    Yeah I understand that from an RPC point of view, a null response is correct, but from the bitcoin-cli point of view it seems strange. How about something like what getzmqnotifications does:

    0user@computer:~$ bitcoin-cli getzmqnotifications
    1[
    2]
    3user@computer:~$ 
    

    Maybe not quite the same, because the brackets indicate an empty array, but some kind of indication that an empty response was received?

  5. sipa commented at 10:18 pm on March 30, 2020: member
    Yes, I agree that a better response is possible. I don’t see how to introduce it in a backward-compatible way, except adding an extra argument to the input to tell it to produce new-style output - which seems like overkill. Thoughts?
  6. adiabat commented at 10:22 pm on March 30, 2020: none
    Hm, if users are currently depending on bitcoin-cli returning nothing in this case, then returning an error message is a breaking change. Are there any strings that will still parse as empty, like "" or {} ? While somewhat ambiguous, either of those would be a good indication that the command is working as expected, but the utxo queried isn’t there.
  7. sipa commented at 10:25 pm on March 30, 2020: member
    I doubt anyone is relying on bitcoin-cli’s response, but they may be relying on the JSON-RPC interface.
  8. adiabat commented at 10:52 pm on March 30, 2020: none

    Do you think it’s then OK to leave the RPC as-is, and change bitcoin-cli to say either an error or {}?

    Also if other people think it’s worth changing or best to leave as-is.

  9. maflcko removed the label Bug on Mar 30, 2020
  10. maflcko added the label Brainstorming on Mar 30, 2020
  11. maflcko added the label RPC/REST/ZMQ on Mar 30, 2020
  12. sipa commented at 11:06 pm on March 30, 2020: member
    @adiabat How would you do that? Add a special exception in bitcoin-cli just for that RPC?
  13. adiabat commented at 11:38 pm on March 30, 2020: none

    OK now that I’m looking more, this seems tricky. A bunch of RPC calls return NullUniValue, and bitcoin-cli silently exits whenever that happens.

    But returning NullUniValue usually means success, like with RPC calls like ping, addnode, reconsiderblock, and most other places. In contrast, with gettransaction, getrawtransaction, getblock, getmempoolentry etc…, RPC errors are returned instead of NullUniValue when it can’t find the transaction or block requested. The JSON error then carries over into the bitcoin-cli response.

    In fact, looking through the “get*” RPC calls, they all seem to return -5 errors when they can’t find the thing specified…

    So I guess the options are: change the RPC return to give back a “not found” error, to make it more in line with other “get*” RPC calls. This is a breaking change though if people are expecting null responses for non-existent utxos.

    Or, there would have to be gettxout specific code in bitcoin-cli, since it does seem like most of the other NullUniValue feel “correct” in that they’re traditional “OK I did the thing you asked, no data to return” situations. This is certainly a bit of a hack / ugly to have RPC-specific code there.

    Or, 3rd option is to just leave it :) Not a big deal. I certainly know what to expect from this RPC now, it just confused me for a few minutes when I was querying an invalid outpoint.

  14. jonasschnelli commented at 8:00 am on March 31, 2020: contributor

    I think it’s not worth changing the current behaviour (even if it is inconsistent with other get* calls). I see @sipa point of not returning an error because the user likes to probe an UTXO existence,… though, I guess we return errors for similar “checks” via RPC and REST.

    bitcoin-cli is currently more or less a dumb RPC client,.. but recently got more and more logic (like the getinfo call or the rpcwallet argument). Adding too much logic to the client side should be avoided IMO for the sake of maintainability and reduced (spaghetti-)complexity. Applications or users needing more client side feature should implement that in their own stack.

  15. darosior commented at 8:27 am on March 31, 2020: member

    I doubt anyone is relying on bitcoin-cli’s response, but they may be relying on the JSON-RPC interface.

    C-lightning is, and fwiw we rely on this specific behaviour..

  16. brakmic commented at 12:35 pm on April 12, 2020: contributor

    Maybe we could offer extended variants of original API calls? Something similar to how Windows treats its APIs.

    For example, you have RegisterClass and RegisterClassEx functions. Both of them can register window objects but the Ex-variant offers some additional options.

    Sure, comparing low-level APIs with RPC calls is maybe not the best engineering approach, but I found no better example.


    In our case, we could leave the original RPC untouched, thus maintaining compatibility, while at the same time offering new data to those clients who are interested in it (and who know what they’re doing).

    Internally, the logic of those RPC calls could be reused without sacrificing the DRY principle. So, gettxoutex would internally call gettxout and only then add "exists": boolean field when there’s an error.

    Here’s my implementation of this hypothetical gettxoutex RPC.

    For now, I have just copied the whole RPCHelpMan stuff, but later we could maybe make it a static variable to reuse it in both functions.

    Example

    success

     0./src/bitcoin-cli -regtest gettxoutex 99a4e8132eac92e69a00845c0fa22013ecfd576a2d2
     1111868881a412dea9bc0f 0
     2{
     3  "bestblock": "471cb2312a77239dc39ae4cdd37e0394ba99788f4838af84f68479efc89b0996",
     4  "confirmations": 1,
     5  "value": 1.56250000,
     6  "scriptPubKey": {
     7    "asm": "0 77c449e951fb3fb3763ec521b6da3f847f86d19b",
     8    "hex": "001477c449e951fb3fb3763ec521b6da3f847f86d19b",
     9    "reqSigs": 1,
    10    "type": "witness_v0_keyhash",
    11    "addresses": [
    12      "bcrt1qwlzyn623lvlmxa37c5smdk3ls3lcd5vmwxdx50"
    13    ]
    14  },
    15  "coinbase": true
    16}
    

    failed

    0./src/bitcoin-cli -regtest gettxoutex 99a4e8132eac92e69a00845c0fa22013ecfd576a2d2111868881a412dea9baaa 0
    1{
    2  "exists": false
    3}
    
  17. fanquake referenced this in commit 16209b1b19 on Mar 15, 2021
  18. maflcko referenced this in commit ad4bf8a945 on Apr 3, 2021
  19. Fabcien referenced this in commit f71cb0fc4a on Dec 3, 2022
  20. adamjonas commented at 4:09 pm on March 17, 2023: member
    This appears to be a won’t fix given the lack of momentum and the above-cited concerns.
  21. adamjonas closed this on Mar 17, 2023

  22. bitcoin locked this on Mar 16, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me