Return correct error codes from bumpfee() #9714

1. 
   jnewbery commented at 7:38 pm on February 7, 2017: member

   The bumpfee() RPC was returning misleading or incorrect error codes (for example RPC_INVALID_ADDRESS_OR_KEY when the transaction was not BIP125 replacable). This PR changes those error codes to what I think are more sensible:

   [EDITED]

   • RPC_INVALID_ADDRESS_OR_KEY if an invalid address was provided:
     • Invalid change address given
   • RPC_INVALID_PARAMETER if a single (non-address/key) parameter is incorrect
     • confTarget and totalFee options should not both be set.
     • Invalid confTarget
     • Insufficient totalFee (cannot be less than required fee)
   • RPC_WALLET_ERROR for any other error
     • Transaction has descendants in the wallet
     • Transaction has descendants in the mempool
     • Transaction has been mined, or is conflicted with a mined transaction
     • Transaction is not BIP 125 replaceable
     • Transaction has already been bumped
     • Transaction contains inputs that don’t belong to the wallet
     • Transaction has multiple change outputs
     • Transaction does not have a change output
     • Fee is higher than maxTxFee
     • New fee rate is less than the minimum fee rate
     • Change output is too small.

   [/EDITED]

   This PR also updates the test cases to explicitly test the error code.

   This PR builds on top of #9707

   [EDIT: updated error codes/classes of error above]

2. jnewbery force-pushed on Feb 7, 2017
3. 
   laanwj commented at 10:25 am on February 8, 2017: member

   Thanks for trying to improve the error codes, however please don’t use these two for application-level errors, they are reserved for errors in low-level protocol handling:

   • RPC_INVALID_REQUEST is mapped to HTTP_BAD_REQUEST (400)
   • RPC_METHOD_NOT_FOUND is mapped to HTTP_NOT_FOUND (404)
4. fanquake added the label RPC/REST/ZMQ on Feb 8, 2017
5. 
   jnewbery commented at 7:43 pm on February 9, 2017: member

   Thanks @laanwj - can you confirm that none of the error codes in this block should be used for application-level errors:

   0    //! Standard JSON-RPC 2.0 errors
   1    RPC_INVALID_REQUEST  = -32600,
   2    RPC_METHOD_NOT_FOUND = -32601,
   3    RPC_INVALID_PARAMS   = -32602,
   4    RPC_INTERNAL_ERROR   = -32603,
   5    RPC_PARSE_ERROR      = -32700,
   

   I’ve grepped through the RPC code and it appears that those are mostly not being used in the application-level code except for:

   • RPC_INTERNAL_ERROR - used (correctly I think) in rpc/blockchain.cpp and rawtransaction.cpp when the internal datastructures are corrupt.
   • RPC_INTERNAL_ERROR - abused in mining.cpp for application-level errors
   • RPC_INVALID_REQUEST, RPC_METHOD_NOT_FOUND, and RPC_INTERNAL_ERROR abused in rpcwallet.cpp for application-level errors.

   If you confirm my understanding is correct, I’ll fix those up in future commits (and add a comment to protocol.h to warn people not to use the -32xxx codes for application-level errors).

6. jnewbery force-pushed on Feb 9, 2017
7. 
   jnewbery commented at 8:22 pm on February 9, 2017: member

   I’ve removed all uses of RPC_INTERNAL_ERROR from bumpfee()

   Reviewers: please only review the second commit in this PR. The first commit is https://github.com/bitcoin/bitcoin/pull/9707

8. 
   laanwj commented at 12:32 pm on February 10, 2017: member

   @jnewbery Not so sure about avoiding all the standard JSON-RPC errors. But propagating the two I mention above to the HTTP layer is a historical accident, which we can’t undo without breaking the interface, but it means it’s better to avoid them.
9. jnewbery force-pushed on Feb 10, 2017
10. 
    jnewbery commented at 6:31 pm on February 10, 2017: member

    #9707 is now merged so this PR only contains the single commit which needs to be reviewed.
11. 
    morcos commented at 7:37 pm on February 10, 2017: member

    can you update the PR text at the top to what you think now as to which error codes should be used for which class of errors?
12. Return correct error codes from bumpfee()

    The bumpfee() RPC was returning misleading or incorrect error codes
    (for example RPC_INVALID_ADDRESS_OR_KEY when the transaction was not
    BIP125 replacable). This commit fixes those error codes:
    
    - RPC_INVALID_ADDRESS_OR_KEY if an invalid address was provided:
        - Invalid change address given
    - RPC_INVALID_PARAMETER if a single (non-address/key) parameter is incorrect
        - confTarget and totalFee options should not both be set.
        - Invalid confTarget
        - Insufficient totalFee (cannot be less than required fee)
    - RPC_WALLET_ERROR for any other error
        - Transaction has descendants in the wallet
        - Transaction has descendants in the mempool
        - Transaction has been mined, or is conflicted with a mined transaction
        - Transaction is not BIP 125 replaceable
        - Transaction has already been bumped
        - Transaction contains inputs that don't belong to the wallet
        - Transaction has multiple change outputs
        - Transaction does not have a change output
        - Fee is higher than maxTxFee
        - New fee rate is less than the minimum fee rate
        - Change output is too small.
    
    This commit also updates the test cases to explicitly test the error code.

    4ac1c330ad
13. jnewbery force-pushed on Feb 15, 2017
14. 
    jnewbery commented at 9:45 pm on February 15, 2017: member

    @morcos I’ve updated the error codes in the original PR notes (and updated the commit message).
15. 
    jnewbery commented at 4:29 pm on February 24, 2017: member

    Closing in favour of #9853.
16. jnewbery closed this on Feb 24, 2017

    ---

17. jnewbery deleted the branch on Feb 24, 2017
18. DrahtBot locked this on Sep 8, 2021

Contributors
jnewbery laanwj morcos

Labels
RPC/REST/ZMQ