Don't directly delete abandoned txes from GUI

john-moffett commented at 3:34 pm on November 28, 2022: contributor

This fully closes bitcoin/bitcoin#12179. Currently, when a user abandons a transaction by clicking “Abandon Transaction” in the context menu, a call is made to remove it from the GUI view:

model->getTransactionTableModel()->updateTransaction(hashQStr, CT_UPDATED, false);

(The false parameter is for bool showTransaction)

This behavior is probably unwanted, as the transaction is not actually removed from the wallet and would show up again if the node is restarted.

However, the previous line, model->wallet().abandonTransaction(hash);, changes the underlying model and calls NotifyTransactionChanged(wtx.GetHash(), CT_UPDATED);, which queues a signal that eventually calls back to updateTransaction, this time with showTransaction set to true. This runs on a separate thread, so it gets called after the ‘subsequent’ updateTransaction. The transaction gets removed from the GUI and immediately added back.

In a nutshell, updateTransaction gets called twice. The first (direct) call deletes the transaction from the GUI. The second (sent via a queued signal) brings it back to the GUI. The first direct call is redundant and unwanted. Worse, if the abandonTransaction call fails for any reason, the transaction still gets removed from the GUI. (This is what caused bitcoin#12179. It can still be triggered if, eg., a user clicks “Abandon Transaction” the moment after a new block is found.)

There are no conditions (to my knowledge) where an abandoned transaction should be directly removed from the GUI. If the underlying model changes, the deletion should be reflected anyway by the queued signal to updateTransaction.

The behavior is borne out by the QT logs. To reproduce, send a transaction with RBF enabled, then bump the fee, then ‘abandon transaction’ on the first transaction. The logs will show something like this:

02022-11-28T14:48:00Z [qt] GUI: "NotifyTransactionChanged: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 status= 1"
12022-11-28T14:48:00Z [qt] GUI: "TransactionTablePriv::updateWallet: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 1"
22022-11-28T14:48:00Z [qt] GUI: "    inModel=1 Index=381-382 showTransaction=0 derivedStatus=2"
32022-11-28T14:48:00Z [qt] GUI: "TransactionTablePriv::updateWallet: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 1"
42022-11-28T14:48:00Z [qt] GUI: "    inModel=0 Index=381-381 showTransaction=1 derivedStatus=0"

Notice the duplicate updateWallet calls with different showTransaction values.

Minor fix: Don't directly delete abandoned txes

This fully closes bitcoin#12179. Currently, in the GUI, when a user
abandons a transaction, a call is made to remove it from the list,
and another signal fires (eventually) that adds it back to the GUI
with a trash can icon.

There are no conditions where the abandoned transaction should be
directly removed from the GUI. If the underlying model changes, the
deletion will be reflected anyway.

e75d227632

DrahtBot commented at 3:34 pm on November 28, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, jarolrod

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

hebasto added the label Wallet on Nov 28, 2022

hebasto renamed this:
~~Minor fix: Don't directly delete abandoned txes from GUI~~
Don't directly delete abandoned txes from GUI
on Nov 28, 2022

hebasto requested review from achow101 on Nov 28, 2022

hebasto commented at 11:50 am on December 5, 2022: member

@john-moffett Thank you for your contribution!

This fully closes bitcoin/bitcoin#12179.

That issue looks outdated as it describes behavior no longer possible on the master branch, for example, a user’s ability to abandon of a conflicted transaction (see https://github.com/bitcoin/bitcoin/pull/12282).

Currently, when a user abandons a transaction by clicking “Abandon Transaction” in the context menu, a call is made to remove it from the GUI view

Is there a specific wrong or unexpected behavior from a user’s point of view?

john-moffett commented at 2:07 pm on December 5, 2022: contributor

@hebasto Happy to help.

That issue looks outdated as it describes behavior no longer possible on the master branch, for example, a user’s ability to abandon of a conflicted transaction (see https://github.com/bitcoin/bitcoin/pull/12282).

Yes, the main issue has already been fixed – that a conflicted tx could still be “abandoned”. However, an issue that still isn’t fully fixed in master is that a row may be removed from the GUI when it shouldn’t be. This is what was referred to in this comment. It typically won’t occur, but still can in edge cases. For example, the following steps will reproduce the issue:

Send an RBF-enabled transaction.
Bump the fee.
Right click the original transaction in the GUI. Notice “Abandon Transaction” is clickable (as it should be).
A new block is added before you actually click “Abandon Transaction”. (To simulate this, I started Qt in regtest with -server and issued sleep 10 && src/bitcoin-cli generatetoaddress 1 bcrt....)
Click “Abandon Transaction” (at this point it should be a no-op since it’s now conflicted). However, the transaction is removed from the GUI:

Restart Bitcoin-Qt and the transaction reappears:

hebasto approved

hebasto commented at 5:18 pm on December 6, 2022: member

ACK e75d2276324d54a01971afdf531df91748275bd5

I’m curios whether another TransactionTableModel::updateTransaction() call is still required: https://github.com/bitcoin-core/gui/blob/e75d2276324d54a01971afdf531df91748275bd5/src/qt/transactionview.cpp#L442 ?

hebasto requested review from jarolrod on Dec 6, 2022

john-moffett commented at 8:48 pm on December 6, 2022: contributor

I’m curious whether another TransactionTableModel::updateTransaction() call is still required:

I believe it is unnecessary, as bumpfee will (on success) have committed a new transaction and the old transaction(s) get updated here:

https://github.com/bitcoin-core/gui/blob/0596aa40f77a630d8a21035856fa5fd6838b292e/src/wallet/wallet.cpp#L2301

Indeed, bumping the fee via bitcoin-cli results in an updated GUI.

john-moffett commented at 6:44 pm on December 12, 2022: contributor

This may also close #635. I haven’t been able to replicate that, but given the behavior of delete-then-add-back in master, it would make sense.

jarolrod approved

jarolrod commented at 8:46 pm on December 13, 2022: member

tACK e75d2276324d54a01971afdf531df91748275bd5

I have reviewed and tested the code and agree it can be merged. Also agree with the ideas presented in the PR description.

hebasto merged this on Dec 13, 2022

hebasto closed this on Dec 13, 2022

sidhujag referenced this in commit 0f66e32853 on Dec 14, 2022

bitcoin-core locked this on Dec 13, 2023

Don’t directly delete abandoned txes from GUI #682

Reviews