tfix #6674: raise exceptions if dscancel or cpfp create dust or negative output. - electrum - Electrum Bitcoin wallet (HTM) git clone https://git.parazyd.org/electrum (DIR) Log (DIR) Files (DIR) Refs (DIR) Submodules --- (DIR) commit 7619949b2fd335cd63b4c88d793fbe59d28f3f40 (DIR) parent be438bdd60529dc1ad131c9f45325f082082e33f (HTM) Author: ThomasV <thomasv@electrum.org> Date: Thu, 14 Jan 2021 19:44:15 +0100 fix #6674: raise exceptions if dscancel or cpfp create dust or negative output. Diffstat: M electrum/gui/qt/history_list.py | 5 ++--- M electrum/gui/qt/main_window.py | 11 ++++++++--- M electrum/wallet.py | 83 +++++++++++++++++-------------- 3 files changed, 57 insertions(+), 42 deletions(-) --- (DIR) diff --git a/electrum/gui/qt/history_list.py b/electrum/gui/qt/history_list.py t@@ -694,9 +694,8 @@ class HistoryList(MyTreeView, AcceptFileDragDrop): if tx_details.can_bump: menu.addAction(_("Increase fee"), lambda: self.parent.bump_fee_dialog(tx)) else: - child_tx = self.wallet.cpfp(tx, 0) - if child_tx: - menu.addAction(_("Child pays for parent"), lambda: self.parent.cpfp(tx, child_tx)) + if tx_details.can_cpfp: + menu.addAction(_("Child pays for parent"), lambda: self.parent.cpfp_dialog(tx)) if tx_details.can_dscancel: menu.addAction(_("Cancel (double-spend)"), lambda: self.parent.dscancel_dialog(tx)) invoices = self.wallet.get_relevant_invoices_for_tx(tx) (DIR) diff --git a/electrum/gui/qt/main_window.py b/electrum/gui/qt/main_window.py t@@ -69,7 +69,7 @@ from electrum.transaction import (Transaction, PartialTxInput, PartialTransaction, PartialTxOutput) from electrum.wallet import (Multisig_Wallet, CannotBumpFee, Abstract_Wallet, sweep_preparations, InternalAddressCorruption, - CannotDoubleSpendTx) + CannotDoubleSpendTx, CannotCPFP) from electrum.version import ELECTRUM_VERSION from electrum.network import (Network, TxBroadcastError, BestEffortRequestFailed, UntrustedServerReturnedError, NetworkException) t@@ -3127,7 +3127,8 @@ class ElectrumWindow(QMainWindow, MessageBoxMixin, Logger): vbox.addLayout(Buttons(CloseButton(d))) d.exec_() - def cpfp(self, parent_tx: Transaction, new_tx: PartialTransaction) -> None: + def cpfp_dialog(self, parent_tx: Transaction) -> None: + new_tx = self.wallet.cpfp(parent_tx, 0) total_size = parent_tx.estimated_size() + new_tx.estimated_size() parent_txid = parent_tx.txid() assert parent_txid t@@ -3210,7 +3211,11 @@ class ElectrumWindow(QMainWindow, MessageBoxMixin, Logger): if fee > max_fee: self.show_error(_('Max fee exceeded')) return - new_tx = self.wallet.cpfp(parent_tx, fee) + try: + new_tx = self.wallet.cpfp(parent_tx, fee) + except CannotCPFP as e: + self.show_error(str(e)) + return new_tx.set_rbf(True) self.show_transaction(new_tx) (DIR) diff --git a/electrum/wallet.py b/electrum/wallet.py t@@ -218,11 +218,17 @@ def get_locktime_for_new_transaction(network: 'Network') -> int: -class CannotBumpFee(Exception): pass - +class CannotBumpFee(Exception): + def __str__(self): + return _('Cannot bump fee') + ':\n\n' + Exception.__str__(self) -class CannotDoubleSpendTx(Exception): pass +class CannotDoubleSpendTx(Exception): + def __str__(self): + return _('Cannot cancel transaction') + ':\n\n' + Exception.__str__(self) +class CannotCPFP(Exception): + def __str__(self): + return _('Cannot create child transaction') + ':\n\n' + Exception.__str__(self) class InternalAddressCorruption(Exception): def __str__(self): t@@ -236,6 +242,7 @@ class TxWalletDetails(NamedTuple): label: str can_broadcast: bool can_bump: bool + can_cpfp: bool can_dscancel: bool # whether user can double-spend to self can_save_as_local: bool amount: Optional[int] t@@ -567,6 +574,7 @@ class Abstract_Wallet(AddressSynchronizer, ABC): exp_n = None can_broadcast = False can_bump = False + can_cpfp = False tx_hash = tx.txid() # note: txid can be None! e.g. when called from GUI tx dialog is_lightning_funding_tx = False if self.has_lightning() and tx_hash is not None: t@@ -602,6 +610,11 @@ class Abstract_Wallet(AddressSynchronizer, ABC): can_bump = is_any_input_ismine and not tx.is_final() can_dscancel = (is_any_input_ismine and not tx.is_final() and not all([self.is_mine(txout.address) for txout in tx.outputs()])) + try: + self.cpfp(tx, 0) + can_cpfp = True + except: + can_cpfp = False else: status = _('Local') can_broadcast = self.network is not None t@@ -632,6 +645,7 @@ class Abstract_Wallet(AddressSynchronizer, ABC): label=label, can_broadcast=can_broadcast, can_bump=can_bump, + can_cpfp=can_cpfp, can_dscancel=can_dscancel, can_save_as_local=can_save_as_local, amount=amount, t@@ -1377,24 +1391,20 @@ class Abstract_Wallet(AddressSynchronizer, ABC): if not isinstance(tx, PartialTransaction): tx = PartialTransaction.from_tx(tx) assert isinstance(tx, PartialTransaction) - if tx.is_final(): - raise CannotBumpFee(_('Cannot bump fee') + ': ' + _('transaction is final')) + raise CannotBumpFee(_('Transaction is final')) new_fee_rate = quantize_feerate(new_fee_rate) # strip excess precision old_tx_size = tx.estimated_size() - try: # note: this might download input utxos over network tx.add_info_from_wallet(self, ignore_network_issues=False) except NetworkException as e: - raise CannotBumpFee(_('Cannot bump fee') + ': ' + repr(e)) - + raise CannotBumpFee(repr(e)) old_fee = tx.get_fee() assert old_fee is not None old_fee_rate = old_fee / old_tx_size # sat/vbyte if new_fee_rate <= old_fee_rate: - raise CannotBumpFee(_('Cannot bump fee') + ': ' + _("The new fee rate needs to be higher than the old fee rate.")) - + raise CannotBumpFee(_("The new fee rate needs to be higher than the old fee rate.")) try: # method 1: keep all inputs, keep all not is_mine outputs, # allow adding new inputs t@@ -1413,14 +1423,13 @@ class Abstract_Wallet(AddressSynchronizer, ABC): tx_new = self._bump_fee_through_decreasing_outputs( tx=tx, new_fee_rate=new_fee_rate) method_used = 2 - target_min_fee = new_fee_rate * tx_new.estimated_size() actual_fee = tx_new.get_fee() if actual_fee + 1 < target_min_fee: - raise Exception(f"bump_fee fee target was not met (method: {method_used}). " - f"got {actual_fee}, expected >={target_min_fee}. " - f"target rate was {new_fee_rate}") - + raise CannotBumpFee( + f"bump_fee fee target was not met (method: {method_used}). " + f"got {actual_fee}, expected >={target_min_fee}. " + f"target rate was {new_fee_rate}") tx_new.locktime = get_locktime_for_new_transaction(self.network) tx_new.add_info_from_wallet(self) return tx_new t@@ -1450,14 +1459,14 @@ class Abstract_Wallet(AddressSynchronizer, ABC): # all outputs are is_mine and none of them are change. # we bail out as it's unclear what the user would want! # the coinchooser bump fee method is probably not a good idea in this case - raise CannotBumpFee(_('Cannot bump fee') + ': all outputs are non-change is_mine') + raise CannotBumpFee(_('All outputs are non-change is_mine')) old_not_is_mine = list(filter(lambda o: not self.is_mine(o.address), old_outputs)) if old_not_is_mine: fixed_outputs = old_not_is_mine else: fixed_outputs = old_outputs if not fixed_outputs: - raise CannotBumpFee(_('Cannot bump fee') + ': could not figure out which outputs to keep') + raise CannotBumpFee(_('Could not figure out which outputs to keep')) if coins is None: coins = self.get_spendable_coins(None) t@@ -1469,12 +1478,13 @@ class Abstract_Wallet(AddressSynchronizer, ABC): return self.config.estimate_fee_for_feerate(fee_per_kb=new_fee_rate*1000, size=size) coin_chooser = coinchooser.get_coin_chooser(self.config) try: - return coin_chooser.make_tx(coins=coins, - inputs=old_inputs, - outputs=fixed_outputs, - change_addrs=change_addrs, - fee_estimator_vb=fee_estimator, - dust_threshold=self.dust_threshold()) + return coin_chooser.make_tx( + coins=coins, + inputs=old_inputs, + outputs=fixed_outputs, + change_addrs=change_addrs, + fee_estimator_vb=fee_estimator, + dust_threshold=self.dust_threshold()) except NotEnoughFunds as e: raise CannotBumpFee(e) t@@ -1500,7 +1510,7 @@ class Abstract_Wallet(AddressSynchronizer, ABC): x_fee_address, x_fee_amount = x_fee s = filter(lambda o: o.address != x_fee_address, s) if not s: - raise CannotBumpFee(_('Cannot bump fee') + ': no outputs at all??') + raise CannotBumpFee('No outputs at all??') # prioritize low value outputs, to get rid of dust s = sorted(s, key=lambda o: o.value) t@@ -1520,7 +1530,7 @@ class Abstract_Wallet(AddressSynchronizer, ABC): # note: delta might be negative now, in which case # the value of the next output will be increased if delta > 0: - raise CannotBumpFee(_('Cannot bump fee') + ': ' + _('could not find suitable outputs')) + raise CannotBumpFee(_('Could not find suitable outputs')) return PartialTransaction.from_io(inputs, outputs) t@@ -1531,16 +1541,19 @@ class Abstract_Wallet(AddressSynchronizer, ABC): if self.is_mine(address): break else: - return + raise CannotCPFP(_("Could not find suitable output")) coins = self.get_addr_utxo(address) item = coins.get(TxOutpoint.from_str(txid+':%d'%i)) if not item: - return + raise CannotCPFP(_("Could not find coins for output")) inputs = [item] out_address = (self.get_single_change_address_for_new_transaction(allow_reuse=False) or self.get_unused_address() or address) - outputs = [PartialTxOutput.from_address_and_value(out_address, value - fee)] + output_value = value - fee + if output_value < self.dust_threshold(): + raise CannotCPFP(_("The output value remaining after fee is too low.")) + outputs = [PartialTxOutput.from_address_and_value(out_address, output_value)] locktime = get_locktime_for_new_transaction(self.network) tx_new = PartialTransaction.from_io(inputs, outputs, locktime=locktime) tx_new.add_info_from_wallet(self) t@@ -1558,22 +1571,19 @@ class Abstract_Wallet(AddressSynchronizer, ABC): assert isinstance(tx, PartialTransaction) if tx.is_final(): - raise CannotDoubleSpendTx(_('Cannot cancel transaction') + ': ' + _('transaction is final')) + raise CannotDoubleSpendTx(_('Transaction is final')) new_fee_rate = quantize_feerate(new_fee_rate) # strip excess precision old_tx_size = tx.estimated_size() - try: # note: this might download input utxos over network tx.add_info_from_wallet(self, ignore_network_issues=False) except NetworkException as e: - raise CannotDoubleSpendTx(_('Cannot cancel transaction') + ': ' + repr(e)) - + raise CannotDoubleSpendTx(repr(e)) old_fee = tx.get_fee() assert old_fee is not None old_fee_rate = old_fee / old_tx_size # sat/vbyte if new_fee_rate <= old_fee_rate: - raise CannotDoubleSpendTx(_('Cannot cancel transaction') + ': ' + _("The new fee rate needs to be higher than the old fee rate.")) - + raise CannotDoubleSpendTx(_("The new fee rate needs to be higher than the old fee rate.")) # grab all ismine inputs inputs = [txin for txin in tx.inputs() if self.is_mine(self.get_txin_address(txin))] t@@ -1582,9 +1592,7 @@ class Abstract_Wallet(AddressSynchronizer, ABC): old_change_addrs = [o.address for o in tx.outputs() if self.is_mine(o.address)] out_address = (self.get_single_change_address_for_new_transaction(old_change_addrs) or self.get_receiving_address()) - locktime = get_locktime_for_new_transaction(self.network) - outputs = [PartialTxOutput.from_address_and_value(out_address, value)] tx_new = PartialTransaction.from_io(inputs, outputs, locktime=locktime) new_tx_size = tx_new.estimated_size() t@@ -1593,6 +1601,9 @@ class Abstract_Wallet(AddressSynchronizer, ABC): old_fee + self.relayfee() * new_tx_size / Decimal(1000), # BIP-125 rules 3 and 4 ) new_fee = int(math.ceil(new_fee)) + output_value = value - new_fee + if output_value < self.dust_threshold(): + raise CannotDoubleSpendTx(_("The output value remaining after fee is too low.")) outputs = [PartialTxOutput.from_address_and_value(out_address, value - new_fee)] tx_new = PartialTransaction.from_io(inputs, outputs, locktime=locktime) tx_new.add_info_from_wallet(self)