Wallet should reject long chains by default

jnewbery commented at 7:00 pm on March 16, 2017: member

#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 -walletrejectlongchains to 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 -walletrejectlongchains set 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.

Make Coin Selection text more general

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.

97cb164f54

Turn -walletrejectlongchains on by default

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

2d825c8582

in qa/rpc-tests/fundrawtransaction.py: in 97cb164f54 outdated

315@@ -316,7 +316,7 @@ def run_test(self):
316         rawtx   = self.nodes[2].createrawtransaction(inputs, outputs)
317         dec_tx  = self.nodes[2].decoderawtransaction(rawtx)
318 
319-        assert_raises_jsonrpc(-4, "Insufficient funds", self.nodes[2].fundrawtransaction, rawtx)
320+        assert_raises_jsonrpc(-4, "Coin Selection Failed", self.nodes[2].fundrawtransaction, rawtx)

lontivero commented at 7:22 pm on March 16, 2017:

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.

jnewbery commented at 7:35 pm on March 16, 2017:

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.

laanwj commented at 10:38 am on March 17, 2017:

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.

NicolasDorier commented at 5:38 am on March 22, 2017:

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.

fanquake added the label Wallet on Mar 17, 2017

NicolasDorier commented at 5:37 am on March 22, 2017: contributor

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.

jnewbery commented at 12:47 pm on March 22, 2017: member

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

NicolasDorier commented at 2:13 am on March 23, 2017: contributor

@jnewbery well in my case, the Insufficient Fee is interpreted into a proper Enum value in my client code. Making this change mean that my code will throw a UnknownException, interpreted like a bug, instead of a Insufficient Fee, which delay the same action for later. (and if long chains, this is how it should be handled as well)

That said this PR should solve my issue #9874 . At least “Insufficient Fee” so you do not break older code, and maybe add “Too long chain” as a new error message in the case that this PR attempt to fix.

NicolasDorier commented at 2:14 am on March 23, 2017: contributor

I advice “too-long-mempool-chain” as it is the same error message sent by sendrawtransaction, if this transaction was constructed.

jnewbery commented at 6:24 pm on June 30, 2017: member

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.

jnewbery closed this on Jun 30, 2017

MarcoFalke locked this on Sep 8, 2021

Wallet should reject long chains by default #10015