diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md index deb37db8b2..5133ee87ef 100644 --- a/core/CHANGELOG.md +++ b/core/CHANGELOG.md @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Decred staking. [#1249] ### Changed +- Allow decreasing the output value in RBF transactions. [#1491] ### Deprecated @@ -376,3 +377,4 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). [#1415]: https://github.com/trezor/trezor-firmware/pull/1415 [#1467]: https://github.com/trezor/trezor-firmware/issues/1467 [#1518]: https://github.com/trezor/trezor-firmware/pull/1518 +[#1491]: https://github.com/trezor/trezor-firmware/issues/1491 diff --git a/core/src/apps/bitcoin/sign_tx/approvers.py b/core/src/apps/bitcoin/sign_tx/approvers.py index 79b6dbb2c8..07d742175a 100644 --- a/core/src/apps/bitcoin/sign_tx/approvers.py +++ b/core/src/apps/bitcoin/sign_tx/approvers.py @@ -44,6 +44,12 @@ class Approver: self.amount_unit = tx.amount_unit + def is_payjoin(self) -> bool: + # A PayJoin is a replacement transaction which manipulates the external inputs of the + # original transaction. A replacement transaction is not allowed to remove any inputs from + # the original, so the condition below is equivalent to external_in > orig_external_in. + return self.external_in != self.orig_external_in + async def add_internal_input(self, txi: TxInput) -> None: self.weight.add_input(txi) self.total_in += txi.amount @@ -79,6 +85,11 @@ class Approver: def add_orig_external_output(self, txo: TxOutput) -> None: self.orig_total_out += txo.amount + async def approve_orig_txids( + self, tx_info: TxInfo, orig_txs: List[OriginalTxInfo] + ) -> None: + raise NotImplementedError + async def approve_tx(self, tx_info: TxInfo, orig_txs: List[OriginalTxInfo]) -> None: raise NotImplementedError @@ -109,11 +120,28 @@ class BasicApprover(Approver): ) -> None: await super().add_external_output(txo, script_pubkey, orig_txo) - # Replacement transactions must not decrease the value of any external outputs. - if orig_txo and txo.amount < orig_txo.amount: - raise wire.ProcessError( - "Reducing original output amounts is not supported." - ) + if orig_txo: + if txo.amount < orig_txo.amount: + # Replacement transactions may need to decrease the value of external outputs to + # bump the fee. This is needed if the original transaction transfers the entire + # account balance ("Send Max"). + if self.is_payjoin(): + # In case of PayJoin the above could be used to increase other external + # outputs, which would create too much UI complexity. + raise wire.ProcessError( + "Reducing original output amounts is not supported." + ) + await helpers.confirm_modify_output( + txo, orig_txo, self.coin, self.amount_unit + ) + elif txo.amount > orig_txo.amount: + # PayJoin transactions may increase the value of external outputs without + # confirmation, because approve_tx() together with the branch above ensures that + # the increase is paid by external inputs. + if not self.is_payjoin(): + raise wire.ProcessError( + "Increasing original output amounts is not supported." + ) if self.orig_total_in: # Skip output confirmation for replacement transactions, @@ -125,6 +153,26 @@ class BasicApprover(Approver): else: await helpers.confirm_output(txo, self.coin, self.amount_unit) + async def approve_orig_txids( + self, tx_info: TxInfo, orig_txs: List[OriginalTxInfo] + ) -> None: + if not orig_txs: + return + + if self.is_payjoin(): + description = "PayJoin" + elif tx_info.rbf_disabled() and any( + not orig.rbf_disabled() for orig in orig_txs + ): + description = "Finalize transaction" + elif len(orig_txs) > 1: + description = "Meld transactions" + else: + description = "Update transaction" + + for orig in orig_txs: + await helpers.confirm_replacement(description, orig.orig_hash) + async def approve_tx(self, tx_info: TxInfo, orig_txs: List[OriginalTxInfo]) -> None: fee = self.total_in - self.total_out @@ -175,30 +223,25 @@ class BasicApprover(Approver): "Original transactions must have same effective nLockTime as replacement transaction." ) - if self.external_in > self.orig_external_in: - description = "PayJoin" - elif tx_info.rbf_disabled() and any( - not orig.rbf_disabled() for orig in orig_txs - ): - description = "Finalize transaction" - elif len(orig_txs) > 1: - description = "Transaction meld" - else: - description = "Fee modification" - - for orig in orig_txs: - await helpers.confirm_replacement(description, orig.orig_hash) - - # Always ask the user to confirm when they are paying more towards the fee. - # If they are not spending more, then ask for confirmation only if it's not - # a PayJoin. In complex scenarios where the user is not spending more and - # there are new external inputs the scenario can be open to multiple - # interpretations and the dialog would likely cause more confusion than - # what it's worth, see PR #1292. - if spending > orig_spending or self.external_in == self.orig_external_in: + if not self.is_payjoin(): + # Not a PayJoin: Show the actual fee difference, since any difference in the fee is + # coming entirely from the user's own funds and from decreases of external outputs. + # We consider the decreases as belonging to the user. + await helpers.confirm_modify_fee( + fee - orig_fee, fee, self.coin, self.amount_unit + ) + elif spending > orig_spending: + # PayJoin and user is spending more: Show the increase in the user's contribution + # to the fee, ignoring any contribution from external inputs. Decreasing of + # external outputs is not allowed in PayJoin, so there is no need to handle those. await helpers.confirm_modify_fee( spending - orig_spending, fee, self.coin, self.amount_unit ) + else: + # PayJoin and user is not spending more: When new external inputs are involved and + # the user is paying less, the scenario can be open to multiple interpretations and + # the dialog would likely cause more confusion than what it's worth, see PR #1292. + pass else: # Standard transaction. if tx_info.tx.lock_time > 0: @@ -263,6 +306,11 @@ class CoinJoinApprover(Approver): await super().add_external_output(txo, script_pubkey, orig_txo) self._add_output(txo, script_pubkey) + async def approve_orig_txids( + self, tx_info: TxInfo, orig_txs: List[OriginalTxInfo] + ) -> None: + pass + async def approve_tx(self, tx_info: TxInfo, orig_txs: List[OriginalTxInfo]) -> None: # The mining fee of the transaction as a whole. mining_fee = self.total_in - self.total_out diff --git a/core/src/apps/bitcoin/sign_tx/bitcoin.py b/core/src/apps/bitcoin/sign_tx/bitcoin.py index f22ba6f600..39d3379086 100644 --- a/core/src/apps/bitcoin/sign_tx/bitcoin.py +++ b/core/src/apps/bitcoin/sign_tx/bitcoin.py @@ -44,6 +44,9 @@ class Bitcoin: # Add inputs to hash143 and h_tx_check and compute the sum of input amounts. await self.step1_process_inputs() + # Approve the original TXIDs in case of a replacement transaction. + await self.approver.approve_orig_txids(self.tx_info, self.orig_txs) + # Add outputs to hash143 and h_tx_check, approve outputs and compute # sum of output amounts. await self.step2_approve_outputs() diff --git a/core/src/apps/bitcoin/sign_tx/helpers.py b/core/src/apps/bitcoin/sign_tx/helpers.py index 0108ce19b8..a56257a46d 100644 --- a/core/src/apps/bitcoin/sign_tx/helpers.py +++ b/core/src/apps/bitcoin/sign_tx/helpers.py @@ -85,6 +85,27 @@ class UiConfirmReplacement(UiConfirm): __eq__ = utils.obj_eq +class UiConfirmModifyOutput(UiConfirm): + def __init__( + self, + txo: TxOutput, + orig_txo: TxOutput, + coin: CoinInfo, + amount_unit: EnumTypeAmountUnit, + ): + self.txo = txo + self.orig_txo = orig_txo + self.coin = coin + self.amount_unit = amount_unit + + def confirm_dialog(self, ctx: wire.Context) -> Awaitable[Any]: + return layout.confirm_modify_output( + ctx, self.txo, self.orig_txo, self.coin, self.amount_unit + ) + + __eq__ = utils.obj_eq + + class UiConfirmModifyFee(UiConfirm): def __init__( self, @@ -199,6 +220,10 @@ def confirm_replacement(description: str, txid: bytes) -> Awaitable[Any]: # typ return (yield UiConfirmReplacement(description, txid)) +def confirm_modify_output(txo: TxOutput, orig_txo: TxOutput, coin: CoinInfo, amount_unit: EnumTypeAmountUnit) -> Awaitable[Any]: # type: ignore + return (yield UiConfirmModifyOutput(txo, orig_txo, coin, amount_unit)) + + def confirm_modify_fee(user_fee_change: int, total_fee_new: int, coin: CoinInfo, amount_unit: EnumTypeAmountUnit) -> Awaitable[Any]: # type: ignore return (yield UiConfirmModifyFee(user_fee_change, total_fee_new, coin, amount_unit)) diff --git a/core/src/apps/bitcoin/sign_tx/layout.py b/core/src/apps/bitcoin/sign_tx/layout.py index 568aab47a4..98043df800 100644 --- a/core/src/apps/bitcoin/sign_tx/layout.py +++ b/core/src/apps/bitcoin/sign_tx/layout.py @@ -96,6 +96,27 @@ async def confirm_replacement(ctx: wire.Context, description: str, txid: bytes) ) +async def confirm_modify_output( + ctx: wire.Context, + txo: TxOutput, + orig_txo: TxOutput, + coin: CoinInfo, + amount_unit: EnumTypeAmountUnit, +) -> None: + assert txo.address is not None + address_short = addresses.address_short(coin, txo.address) + amount_change = txo.amount - orig_txo.amount + await require( + layouts.confirm_modify_output( + ctx, + address_short, + amount_change, + format_coin_amount(abs(amount_change), coin, amount_unit), + format_coin_amount(txo.amount, coin, amount_unit), + ) + ) + + async def confirm_modify_fee( ctx: wire.Context, user_fee_change: int, diff --git a/core/src/trezor/ui/layouts/tt.py b/core/src/trezor/ui/layouts/tt.py index c76463d023..5ecdc68967 100644 --- a/core/src/trezor/ui/layouts/tt.py +++ b/core/src/trezor/ui/layouts/tt.py @@ -50,6 +50,7 @@ __all__ = ( "confirm_joint_total", "confirm_metadata", "confirm_replacement", + "confirm_modify_output", "confirm_modify_fee", ) @@ -407,13 +408,43 @@ def confirm_replacement( return interact(ctx, Confirm(text), "confirm_replacement", ButtonRequestType.SignTx) +def confirm_modify_output( + ctx: wire.GenericContext, + address: str, + sign: int, + amount_change: str, + amount_new: str, +) -> LayoutType: + page1 = Text("Modify amount", ui.ICON_SEND, ui.GREEN) + page1.normal("Address:") + page1.br_half() + page1.mono(*_split_address(address)) + + page2 = Text("Modify amount", ui.ICON_SEND, ui.GREEN) + if sign < 0: + page2.normal("Decrease amount by:") + else: + page2.normal("Increase amount by:") + page2.bold(amount_change) + page2.br_half() + page2.normal("New amount:") + page2.bold(amount_new) + + return interact( + ctx, + Paginated([page1, Confirm(page2)]), + "modify_output", + ButtonRequestType.ConfirmOutput, + ) + + def confirm_modify_fee( ctx: wire.GenericContext, sign: int, user_fee_change: str, total_fee_new: str, ) -> LayoutType: - text = Text("Fee modification", ui.ICON_SEND, ui.GREEN) + text = Text("Modify fee", ui.ICON_SEND, ui.GREEN) if sign == 0: text.normal("Your fee did not change.") else: