spesmilo / electrum

Electrum Bitcoin Wallet
https://electrum.org
MIT License
7.23k stars 3.03k forks source link

qml: QETxDetails: defer to wallet.get_tx_info() for rbf/cpfp #8894

Closed SomberNight closed 3 months ago

SomberNight commented 4 months ago

I think this logic should be encapsulated in the wallet.

Looks like the extra guards were added in https://github.com/spesmilo/electrum/commit/84cb210e7eed547f28bbd4e948500547fd01213e (except for can_save_as_local -- added in https://github.com/spesmilo/electrum/commit/9cb8dea3435e6a5cdb16a08fc6523198d027031c)

I don't really see a reason why this logic should be GUI-specific. Also, note that it can make sense to allow bump_fee on a local tx, and so wallet.get_tx_info() allows it (and hence so does the qt gui). For example, if your tx falls out of the mempool, you might want to bump it. (though it could also make sense to allow dscancel in the same scenario but we don't allow that) For cpfp, as long as there is no package relay, it does not make sense to allow it for a local tx.

I don't really understand the guard on can_save_as_local - maybe @accumulator can comment if he remembers why it was added.

(My direct motivation is simply that I sometimes manually test rbf/cpfp and it would be easier to only have to patch a single localised thing.)

accumulator commented 4 months ago

I don't really understand the guard on can_save_as_local - maybe @accumulator can comment if he remembers why it was added.

I believe the extra and not txinfo.can_remove is intended to only enable the save button if the transaction is not recorded in the wallet yet (e.g. a scanned raw tx -> can_remove==False). Not sure why txinfo.can_save_as_local was not sufficient, but it could be that it is not updated immediately after saving a TX. I don't remember exactly (it's been a while), but at the time of the commit I was very careful not to mess with the semantics of the backend wallet :)

ecdsa commented 3 months ago

ACK. this should not be GUI specific