[wallet] Abandon transactions that fail to go into the mempool #11019

This PR addresses the case where a wallet transaction fails to be accepted to the mempool when committed. This would keep the transaction in the wallet, to be rebroadcast at some undetermined time in the future, with the consequence that the change output would be cloaked. Even adding the mempool transactions into a new block will not immediately resolve the problem, due to the wallet not rebroadcasting the transaction immediately (even to itself) for (I think) privacy reasons.

Although there is a walletrejectlongchain option, the behavior when this is turned off is confusing and unpredictable, as noted above, and the CommitTransaction code will do the exact same thing for other mempool add failures, whichever those may be.

Edit: ~~~open question and currently investigating: will the coin selection algorithm inadvertently pick UTXOs that would result in a rejection of the transaction even if the user has other UTXOs available that wouldn't result in a too long chain? I have tried to trigger this case but the coin select is always picking the "right" UTXO that won't result in failure.~~~

Edit 2: In https://github.com/bitcoin/bitcoin/blame/master/src/wallet/wallet.cpp#L2457 the very last attempt to select coins increases the ancestor cap to num max, so above concern about coin select sometimes inadvertently picking an unspendable-due-to-chain-length tx is not a problem.

immediately resolve the problem, due to the wallet not rebroadcasting the transaction immediately (even to itself) for (I think) privacy reasons.

It resolves it shortly. Why do you think immediate is required?

You change makes it impossible to have the wallet simply queue transactions in long chains for sending when it becomes possible.

Shortly is several minutes, which isn't really acceptable in some cases.

The code does indeed prevent you from adding one additional transaction to an existing chain, but that's all you get from the current system. You don't get those UTXOs for further chaining as they are cloaked, as noted above. You would get the exact same results by adding 1 to the max chain length.

@kallewoof trying to reword, correct me if wrong.

Currently situation: Imagine that the unconf chain limit is 3 transactions and you have 1 UTXO

sendtoaddress abc 10 #TX1 OK
sendtoaddress abc 10 #TX2 OK
sendtoaddress abc 10 #TX3 OK

sendtoaddress abc 10 #TX4 OK  (but mempool rejected by chain-too-long)

TX4 is still added to the wallet and will be rebroadcasted later. Note that now, getbalance will returns 0, whatever the amount of the initial UTXO.

sendtoaddress abc 10 #TX5 BOOM insufficient funds

TX5 will not be rebroadcasted later.

With @kallewoof PR we have:

sendtoaddress abc 10 #TX1 OK
sendtoaddress abc 10 #TX2 OK
sendtoaddress abc 10 #TX3 OK

sendtoaddress abc 10 #TX4 BOOM chain-too-long

With TX4 not rebroadcasted by the wallet later, and getbalance does not returns 0.

I think it makes lots of sense. The current situation only save into the wallet one transaction on top of the longest chain, and the downside is that the user see his balance to 0 when he still have funds. The current situation is also counter intuitive: why TX4 get queue by the wallet for later broadcast, but not TX5 ?

Concept ACK for me.

Yes, the core issue is that a transaction that did not make it into the mempool but stayed in the wallet is invisible to the wallet code until it is put into the mempool at some undetermined later time.

Perhaps the better approach here is to make an additional coin view on top of the mempool called wallet view or something, which would include these to-be-mempooled transactions.

This would allow for arbitrary length tx chains, presuming the wallet code for rebroadcasting is able to deal with ordering, and presuming users will not be thrown off by transactions taking a lot longer to get into the blockchain (100 txs would at minimum take ~5 blocks, as only 20 or so txs in one chain would be accepted at a time).

I am also wondering how very-long-chaining will affect existing UTXOs. ~~~Presuming the coin select picks indiscriminately (which does NOT seem to be the case, but I can't pinpoint why), having a long chain and sending more would "lock up" more and more UTXOs into that chain, locking more and more of your funds into an ever growing chain of transactions that have to go into the blockchain.~~~

Edited: coin select tries to avoid long-chain outputs if possible. Users will still lock up additional UTXOs if they have many smaller UTXOs and the locked-up one is large enough to be required to fund a new transaction.

The more I think about it, the more I lean towards not allowing chains that are longer than what would go into the mempool, period. I.e. what this PR does.

change makes it impossible to have the wallet simply queue transactions in long chains for sending when it becomes possible.

I agree with @gmaxwell; I believe there are use cases which prompted supporting the current behavior of allowing transactions to be queued up (for instance, if you have lots and lots of utxos, and your transactions are not in a hurry, then it may be fine to have some of them fail to get added to the mempool and become unavailable for further spending).

I also think that in the future we may want to build more wallet features around the idea of, say, starting with a low fee (which may not make it into the mempool) and then auto-fee-bumping according to some schedule until it gets confirmed. So I wouldn't want to preclude these kinds of strategies.

As an aside -- with this patch, don't we risk clogging up a user's wallet if they are trying to send in a loop and bump into this behavior? Like if they get an error and just try again, hoping to get different inputs -- but if they don't have enough utxo's they get the same result, a transaction eating up space in their wallet which has been abandoned. That seems like a bad outcome as well.... I'm not sure if a problem like this is unique to this approach but I'm generally wary of promoting more use of AbandonTransaction(); in general I feel like that should be for emergencies only. I think to implement something like this behavior it would make more sense to add a way to remove a transaction from the wallet.

@sdaftuar

I believe there are use cases which prompted supporting the current behavior of allowing transactions to be queued up (for instance, if you have lots and lots of utxos, and your transactions are not in a hurry, then it may be fine to have some of them fail to get added to the mempool and become unavailable for further spending).

I'm (still) confused about this statement, as the current behavior will only let you queue up one extra transaction, as if you increased mempool chain limit by 1. Beyond that, the UTXOs are hidden from the user when selecting coins, and the user will simply not be able to add to the chain anymore.

I also think that in the future we may want to build more wallet features around the idea of, say, starting with a low fee (which may not make it into the mempool) and then auto-fee-bumping according to some schedule until it gets confirmed. So I wouldn't want to preclude these kinds of strategies.

If it's so low it doesn't make it into the mempool, what is the purpose of the transaction existing at all? It will never make it into the mempool as is, so you could just as well abandon it in favor of the higher-fee variant that comes along later. Again, I may be missing some subtlety here.

Edit: I had missed one subtlety: that abandoned transactions are kept in the wallet. It would make more sense to discard completely in these cases, but not sure if that's possible.