Wallet should reject long chains by default #10015
Conversation
Currently, if Coin Selection fails, then the error returned to the user is "Insufficient Funds". This is misleading as Coin Selection can fail for other reasons, such as if the transaction would form a chain that is too long to enter the mempool. This commit changes the error message returned from "Insufficient Funds" to "Coin Selection Failed" and updates the test scripts to expect the correct error message.
This commit turns the -walletrejectlongchains option on by default and makes the wallet reject any transaction which would form a chain up to (min(limitancestorcount, limitdescendantcount) - 1) long. The wallet will reject the transaction before the maximum chain length is reached so that the user has a final change to add a transaction to the chain (for example to add a CPFP transaction if the entire transaction package is too low fee).
| @@ -316,7 +316,7 @@ def run_test(self): | |||
| rawtx = self.nodes[2].createrawtransaction(inputs, outputs) | |||
| dec_tx = self.nodes[2].decoderawtransaction(rawtx) | |||
|
|
|||
| assert_raises_jsonrpc(-4, "Insufficient funds", self.nodes[2].fundrawtransaction, rawtx) | |||
| assert_raises_jsonrpc(-4, "Coin Selection Failed", self.nodes[2].fundrawtransaction, rawtx) | |||
There was a problem hiding this comment.
The new error message is a bit confussing to me because it doesn't provides a reason or error code. IMO if something can fail for two different reasons, it should let the caller know the reason in order to let him take the appropiate actions, show the best error message, etc.
There was a problem hiding this comment.
I agree it'd be better to have more precise error codes/messages returned. However, I think this is an improvement since we're returning "Insufficient funds" when the problem may be something different.
There was a problem hiding this comment.
Agree that if there are really insufficient funds it'd be good to reply that. A specific error is better than a non-specific one if the message is correct. Better an aspecific error than an incorrect one though. An incorrect "insufficient funds" error could set the caller on a wild goose chase or get them to panic. In that case it's better to return something general which prompts the caller to do their own research.
There was a problem hiding this comment.
I think "Insufficient funds" is correct. If the user does not have enough liquidity to not do a long chain, then he does not have enough funds.
|
NACK on "Coin Selection Failed". It would break existing code (I rely in some project on the Insufficient Funds message), and I think "Insufficient funds" is correct. If you can't create a transaction because the chain is too long, then you really do not have enough liquidity. |
|
@NicolasDorier I strongly disagree with your definition of 'liquidity' here. In the too-long-chains case, the coin selection is being rejected by local policy. If the user was able to get the transaction to a miner who was prepared to mine a long chain, then it would be a perfectly valid transaction. I also think the most obvious reading of "insufficient funds" is that there are not enough funds in the wallet, not that the funds are temporarily unusable for technical reasons. That said, I do have sympathy for client applications which expect the interface to bitcoind to remain stable. It's just that in this case the interface is wrong and should be corrected. If your client application is propagating the error message to the end user that there are insufficient funds, then it's likely to cause alarm and anxiety for users who may think that their funds have been lost. Ideally, we'd have error messages that were specific and correct, and that's where we should aim to get to. The choice here is between log messages that are (specific and potentially incorrect) and log messages that are (general and correct). Log messages that are general and correct are preferable. |
|
@jnewbery well in my case, the Insufficient Fee is interpreted into a proper Enum value in my client code. That said this PR should solve my issue #9874 . |
|
I advice "too-long-mempool-chain" as it is the same error message sent by |
|
Doesn't seem to be much appetite for this. There was a bit of disagreement on IRC here: https://botbot.me/freenode/bitcoin-core-dev/2017-03-16/?msg=82546050&page=3 Closing for now. |
#9262 introduced
-walletrejectlongchains, which prevents the wallet from adding transactions which would form a chain which is too long to be accepted by the mempool. However, it's set to false by default.There are legitimate reasons for wanting the wallet to accept longer chains than the mempool, and #9290 does ensure that wallet transactions will be resubmitted to the mempool eventually. However, in the mainline case the behaviour is confusing for the user and the wallet should reject the long chains. See for instance #10004 and #9752.
This PR sets
-walletrejectlongchainsto true by default, and makes the wallet reject transactions that form a chain up to (maxlength - 1). This means that the user has one final chance to add a transaction to the chain by restarting with-walletrejectlongchainsset to false. This could be helpful if a user wants to use CPFP to unstick a transaction package with too low fees.Tests also updated to verify new behaviour.
This PR also changes the error message returned if Coin Selection fails. Currently, if Coin Selection fails, then the error returned to the user is "Insufficient Funds". This is misleading as Coin Selection can fail for other reasons, such as if the transaction would form a chain that is too long to enter the mempool. This commit changes the error message returned from "Insufficient Funds" to "Coin Selection Failed" and updates the test scripts to expect the correct error message.