feat(core): Allow decreasing output amount in RBF transactions.

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

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

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()

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))
 

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,

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: