Skip to content

Return correct error codes from bumpfee()#9714

Closed
jnewbery wants to merge 1 commit intobitcoin:masterfrom
jnewbery:bumpfeeerrormessages
Closed

Return correct error codes from bumpfee()#9714
jnewbery wants to merge 1 commit intobitcoin:masterfrom
jnewbery:bumpfeeerrormessages

Conversation

@jnewbery
Copy link
Contributor

@jnewbery jnewbery commented Feb 7, 2017 •
edited
Loading

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]

@jnewbery jnewbery force-pushed the bumpfeeerrormessages branch from 6acc6eb to ab19e90 Compare February 7, 2017 19:42
@laanwj
Copy link
Member

laanwj commented Feb 8, 2017 •
edited
Loading

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)

@jnewbery
Copy link
Contributor Author

jnewbery commented Feb 9, 2017

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

    //! Standard JSON-RPC 2.0 errors
    RPC_INVALID_REQUEST  = -32600,
    RPC_METHOD_NOT_FOUND = -32601,
    RPC_INVALID_PARAMS   = -32602,
    RPC_INTERNAL_ERROR   = -32603,
    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).

@jnewbery jnewbery force-pushed the bumpfeeerrormessages branch from ab19e90 to 55083b6 Compare February 9, 2017 20:21
@jnewbery
Copy link
Contributor Author

jnewbery commented Feb 9, 2017

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 #9707

@laanwj
Copy link
Member

laanwj commented Feb 10, 2017

@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.

@jnewbery jnewbery force-pushed the bumpfeeerrormessages branch from 55083b6 to 17f6ffb Compare February 10, 2017 18:31
@jnewbery
Copy link
Contributor Author

jnewbery commented Feb 10, 2017

#9707 is now merged so this PR only contains the single commit which needs to be reviewed.

@morcos
Copy link
Contributor

morcos commented Feb 10, 2017

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?

@jnewbery
Return correct error codes from bumpfee()
4ac1c33 
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.
@jnewbery jnewbery force-pushed the bumpfeeerrormessages branch from 17f6ffb to 4ac1c33 Compare February 15, 2017 21:44
@jnewbery
Copy link
Contributor Author

jnewbery commented Feb 15, 2017

@morcos I've updated the error codes in the original PR notes (and updated the commit message).

@jnewbery jnewbery mentioned this pull request Feb 23, 2017
Merged
@jnewbery
Copy link
Contributor Author

jnewbery commented Feb 24, 2017

Closing in favour of #9853.

@jnewbery jnewbery closed this Feb 24, 2017
@jnewbery jnewbery deleted the bumpfeeerrormessages branch February 24, 2017 16:29
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jnewbery @laanwj @morcos @fanquake