Merge pull request #722 from Chaitanya-Keyal/psbt-huge-fee-warning

[Feature] Warning Screen for High Tx Fees

src/seedsigner/gui/screens/psbt_screens.py

@@ -25,6 +25,10 @@ class PSBTOverviewScreen(ButtonListScreen):
     num_change_outputs: int = 0
     destination_addresses: list[str] = None
     has_op_return: bool = False
+    is_high_fee_tx: bool = False
+
+    # Appended to a row that needs the user's attention, drawn in the dire warning color
+    WARNING_MARK = " (!)"
     
 
     def __post_init__(self):
@@ -147,7 +151,13 @@ class PSBTOverviewScreen(ButtonListScreen):
                 # TRANSLATOR_NOTE: Inserts the recipient number (e.g. the fifth one is: "recipient 5")
                 destination_column.append(_("recipient {}").format(len(self.destination_addresses) + self.num_self_transfer_outputs))
 
-            destination_column.append(_("fee"))
+            fee_label = _("fee")
+            if self.is_high_fee_tx:
+                # Part of the label, not something appended at render time: the column is
+                # measured from these strings, so a mark added later would be drawn
+                # outside the width that was reserved for the row.
+                fee_label += PSBTOverviewScreen.WARNING_MARK
+            destination_column.append(fee_label)
 
             if self.has_op_return:
                 # TRANSLATOR_NOTE: Technical term, should probably NOT be translated in most languages
@@ -308,11 +318,15 @@ class PSBTOverviewScreen(ButtonListScreen):
 
         output_curves = []
         for destination in destination_column:
+            text_color = chart_font_color
+            if destination.endswith(PSBTOverviewScreen.WARNING_MARK):
+                text_color = GUIConstants.DIRE_WARNING_COLOR
+
             draw.text(
                 (recipients_text_x, destination_y),
                 text=destination,
                 font=font,
-                fill=chart_font_color,
+                fill=text_color,
                 anchor="lt"
             )
 
@@ -471,6 +485,7 @@ class PSBTMathScreen(ButtonListScreen):
     num_recipients: int = 0
     fee_amount: int = 0
     change_amount: int = 0
+    is_high_fee_tx: bool = False
 
 
     def __post_init__(self):
@@ -569,10 +584,17 @@ class PSBTMathScreen(ButtonListScreen):
             )
 
         cur_y += digits_height + GUIConstants.BODY_LINE_SPACING * ssf
+
+        info_text = _("fee")
+        info_text_color = GUIConstants.BODY_FONT_COLOR
+        if self.is_high_fee_tx:
+            info_text += PSBTOverviewScreen.WARNING_MARK
+            info_text_color = GUIConstants.DIRE_WARNING_COLOR
         render_amount(
             cur_y,
             f"-{self.fee_amount}",
-            info_text=_("fee"),
+            info_text=info_text,
+            info_text_color=info_text_color,
         )
 
         cur_y += digits_height + GUIConstants.BODY_LINE_SPACING * ssf

src/seedsigner/models/psbt_parser.py

@@ -156,6 +156,10 @@ class PSBTParser():
     # just stops getting cache hits once the cache is full.
     MAX_CACHED_DERIVATIONS = 1000
 
+    # Warn when the fee exceeds this percentage of what is being sent (outputs other than
+    # change). TODO: Possibly make this configurable via settings.
+    HIGH_FEES_WARNING_THRESHOLD = 25
+
 
     def __init__(self, p: PSBT, seed: Seed, network: str = SettingsConstants.MAINNET):
         self.psbt: PSBT = p
@@ -173,6 +177,11 @@ class PSBTParser():
         self.destination_amounts = []
         self.op_return_data: bytes = None
 
+        # Whether the fee is high relative to what is being sent; see has_high_fee().
+        # Computed once at the end of parse() so the views can read it without each
+        # re-walking the outputs.
+        self.is_high_fee: bool = False
+
         # Contains one entry per input in psbt.inputs and per output in psbt.outputs. Each
         # entry is either the derivation path the seed genuinely owns there, or it is set
         # to `None`.
@@ -299,6 +308,9 @@ class PSBTParser():
         if rt == False:
             return False
 
+        # Every total is known now, so settle this once rather than per view.
+        self.is_high_fee = self.has_high_fee()
+
         return True
 
 
@@ -1181,4 +1193,48 @@ class PSBTParser():
             _fill_scope(inp)
 
         for out in self.psbt.outputs:
-            _fill_scope(out)
+            _fill_scope(out)
+
+
+    def get_total_output_value(self, include_change: bool = False):
+        """
+            Returns the sum of all outputs (fee not included).
+
+            `change_data` holds every output that comes back to this seed, which is two
+            different things: change, and self-transfers to one of our own receive
+            addresses. With `include_change=False` only the change is subtracted;
+            self-transfers stay in the total, since the user chose to send funds there
+            just as they did for any external recipient. The two are told apart the way
+            the views do it, by `is_change_branch` on the derivation path the parse
+            proved this seed owns.
+
+            Used to decide whether the fee is high relative to what is actually being
+            sent, and whether to warn.
+        """
+        total = sum(out.value for out in self.psbt.tx.vout)
+
+        if include_change:
+            return total
+
+        # Subtract the change; keep self-transfers, they count as recipients.
+        true_change = sum(
+            entry["amount"]
+            for entry in self.change_data
+            if PSBTParser.is_change_branch(entry["verified_derivation_path"])
+        )
+        return total - true_change
+
+
+    def has_high_fee(self):
+        """
+            Returns True if the fee is high.
+            i.e. fee amount > <HIGH_FEES_WARNING_THRESHOLD>% of total outputs excluding change
+        """
+        total_output_value_excluding_change = self.get_total_output_value()
+
+        # If there are no outputs other than change, then it can't be a high fee
+        if total_output_value_excluding_change <= 0:
+            return False
+
+        else:
+            return self.fee_amount > ((self.HIGH_FEES_WARNING_THRESHOLD / 100) * total_output_value_excluding_change)

src/seedsigner/views/psbt_views.py

@@ -174,6 +174,7 @@ class PSBTOverviewView(View):
             num_change_outputs=num_change_outputs,
             destination_addresses=psbt_parser.destination_addresses,
             has_op_return=psbt_parser.op_return_data is not None,
+            is_high_fee_tx=psbt_parser.is_high_fee,
         )
 
         if selected_menu_num == RET_CODE__BACK_BUTTON:
@@ -184,6 +185,9 @@ class PSBTOverviewView(View):
         # skip change warning and psbt math view
         if psbt_parser.policy == None:
             return Destination(PSBTUnsupportedScriptTypeWarningView)
+
+        elif psbt_parser.is_high_fee:
+            return Destination(PSBTHighFeeWarningView, view_args={"warning_threshold_percent": psbt_parser.HIGH_FEES_WARNING_THRESHOLD})
         
         elif psbt_parser.change_amount == 0:
             return Destination(PSBTNoChangeWarningView)
@@ -235,6 +239,39 @@ class PSBTNoChangeWarningView(View):
 
 
 
+class PSBTHighFeeWarningView(View):
+    def __init__(self, warning_threshold_percent: int):
+        super().__init__()
+        
+        self.warning_threshold_percent = warning_threshold_percent
+    
+    def run(self):
+        selected_menu_num = self.run_screen(
+            DireWarningScreen,
+            status_headline=_("High Fee!"),
+            # TRANSLATOR_NOTE: Variable is the percentage of the total output value (excluding change) that the fee exceeds. (e.g. "This transaction has a fee higher than 25% of the total output value (excluding change).")
+            text=_("This transaction has a fee higher than {}% of the total output value (excluding change).").format(self.warning_threshold_percent),
+            button_data=[ButtonOption("Continue")],
+        )
+
+        if selected_menu_num == RET_CODE__BACK_BUTTON:
+            return Destination(BackStackView)
+
+        # PSBT may have high fee + no change
+        if self.controller.psbt_parser.change_amount == 0:
+            return Destination(
+                PSBTNoChangeWarningView,
+                skip_current_view=True,  # Prevent going BACK to WarningViews
+            )
+
+        else:
+            return Destination(
+                PSBTMathView,
+                skip_current_view=True,  # Prevent going BACK to WarningViews
+            )
+
+
+
 class PSBTMathView(View):
     """
         Follows the Overview pictogram. Shows:
@@ -259,6 +296,7 @@ class PSBTMathView(View):
             num_recipients=psbt_parser.num_destinations,
             fee_amount=psbt_parser.fee_amount,
             change_amount=psbt_parser.change_amount,
+            is_high_fee_tx=psbt_parser.is_high_fee,
         )
 
         if selected_menu_num == RET_CODE__BACK_BUTTON:

tests/screenshot_generator/generator.py

@@ -439,6 +439,7 @@ def generate_screenshots(locale):
                 ScreenshotConfig(psbt_views.PSBTOverviewView,   mock_context_manager=mock_multisig_psbt_loaded),
                 ScreenshotConfig(psbt_views.PSBTUnsupportedScriptTypeWarningView),
                 ScreenshotConfig(psbt_views.PSBTNoChangeWarningView),
+                ScreenshotConfig(psbt_views.PSBTHighFeeWarningView, dict(warning_threshold_percent=25)),
                 ScreenshotConfig(psbt_views.PSBTMathView, mock_context_manager=mock_multisig_psbt_loaded),
                 ScreenshotConfig(psbt_views.PSBTAddressDetailsView, dict(address_num=0), mock_context_manager=mock_multisig_psbt_loaded),
 

tests/test_psbt_parser.py

@@ -3,6 +3,7 @@ import random
 
 from binascii import a2b_base64
 from copy import deepcopy
+from types import SimpleNamespace
 from unittest.mock import patch
 from embit import bip32, script
 from embit.ec import PublicKey
@@ -59,7 +60,11 @@ class TestPSBTParser:
             assert psbt_parser.change_amount == input_amount - recipient_amount - fee_amount
             assert psbt_parser.fee_amount == fee_amount
             assert psbt_parser.input_amount == psbt_parser.spend_amount + psbt_parser.change_amount + psbt_parser.fee_amount
-        
+
+            # No self-transfer here, so all change is true change
+            assert psbt_parser.get_total_output_value() == psbt_parser.spend_amount
+            assert psbt_parser.get_total_output_value(include_change=True) == psbt_parser.spend_amount + psbt_parser.change_amount
+
         # Internally cycle the input(s) back to sender via the `self_transfer_data`
         psbt.outputs.clear()
         psbt.outputs.append(create_output(self_transfer_data, input_amount - fee_amount))
@@ -75,6 +80,11 @@ class TestPSBTParser:
         assert psbt_parser.fee_amount == fee_amount
         assert psbt_parser.input_amount == psbt_parser.spend_amount + psbt_parser.change_amount + psbt_parser.fee_amount
 
+        # Only self-transfer, and no "true" change, so `change_amount` is included in total output
+        # Both calls should return the same value
+        assert psbt_parser.get_total_output_value() == psbt_parser.spend_amount + psbt_parser.change_amount
+        assert psbt_parser.get_total_output_value(include_change=True) == psbt_parser.spend_amount + psbt_parser.change_amount
+
         # Now do full spends with no change
         fee_amount = random.randint(5_000, 100_000)
         recipient_amount = input_amount - fee_amount
@@ -94,6 +104,10 @@ class TestPSBTParser:
             assert psbt_parser.fee_amount == fee_amount
             assert psbt_parser.input_amount == psbt_parser.spend_amount + psbt_parser.change_amount + psbt_parser.fee_amount
 
+            # No self-transfer here, so all change is true change
+            assert psbt_parser.get_total_output_value() == psbt_parser.spend_amount
+            assert psbt_parser.get_total_output_value(include_change=True) == psbt_parser.spend_amount + psbt_parser.change_amount
+
         # Now try a single mega psbt with ALL the outputs at once
         psbt.outputs.clear()
         change_amount = input_amount - fee_amount
@@ -116,6 +130,10 @@ class TestPSBTParser:
         assert psbt_parser.fee_amount == fee_amount
         assert psbt_parser.input_amount == psbt_parser.spend_amount + psbt_parser.change_amount + psbt_parser.fee_amount
 
+        # No self-transfer here, so all change is true change
+        assert psbt_parser.get_total_output_value() == psbt_parser.spend_amount
+        assert psbt_parser.get_total_output_value(include_change=True) == psbt_parser.spend_amount + psbt_parser.change_amount
+
 
     def test_singlesig_native_segwit(self):
         self.run_basic_test(PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_CHANGE, PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_SELF_TRANSFER)
@@ -348,6 +366,104 @@ class TestPSBTParser:
         """
         assert PSBTParser.is_change_branch(bip32.parse_path("m/84h/1h/0h/1/0")) is True
         assert PSBTParser.is_change_branch(bip32.parse_path("m/84h/1h/0h/0/0")) is False
+    @pytest.mark.parametrize("vout_values, change_data, expected", [
+        # single destination + single change
+        ([100, 200], [{"verified_derivation_path": bip32.parse_path("m/84h/0h/0h/1/0"), "amount": 200}], 100),
+        # multiple destinations + single change
+        ([50, 75, 25], [{"verified_derivation_path": bip32.parse_path("m/84h/0h/0h/1/0"), "amount": 25}], 50 + 75),
+        # no change outputs at all
+        ([10, 20, 30], [], 10 + 20 + 30),
+        # only change outputs
+        ([123], [{"verified_derivation_path": bip32.parse_path("m/84h/0h/0h/1/0"), "amount": 123}], 0),
+        # mix of true change and self-transfer
+        (
+            [100, 200, 300], 
+            [
+                {"verified_derivation_path": bip32.parse_path("m/84h/0h/0h/0/0"), "amount": 200},  # self-transfer
+                {"verified_derivation_path": bip32.parse_path("m/84h/0h/0h/1/0"), "amount": 300},  # true change
+            ], 
+            100 + 200  # Only subtract true change (300)
+        ),
+    ])
+    def test_get_total_output_value(self, vout_values, change_data, expected):
+        """
+            get_total_output_value() should return
+            sum(vout_values) - sum(true_change),
+            where true change is determined by derivation path having chain index 1.
+        """
+        # Build a dummy parser without running .parse()
+        parser = PSBTParser.__new__(PSBTParser)
+
+        # Stub out parser.psbt.tx.vout as list of objects with a .value attribute
+        parser.psbt = SimpleNamespace(
+            tx=SimpleNamespace(
+                vout=[SimpleNamespace(value=v) for v in vout_values]
+            )
+        )
+        parser.change_data = change_data
+
+        assert parser.get_total_output_value() == expected
+
+
+    @pytest.mark.parametrize("vin, vout_values, change_data, expected", [
+        # fee=30 (24%) -> NOT high
+        (180, [50, 75, 25], [{"verified_derivation_path": bip32.parse_path("m/84h/0h/0h/1/0"), "amount": 25}], False),
+        # fee=40 (32%) -> HIGH
+        (190, [50, 75, 25], [{"verified_derivation_path": bip32.parse_path("m/84h/0h/0h/1/0"), "amount": 25}], True),
+        # fee=15 (exactly 25%) -> NOT high
+        (75, [10, 20, 30], [], False),
+        # only change outputs: excluding change=0 -> never high by definition
+        (130, [123], [{"verified_derivation_path": bip32.parse_path("m/84h/0h/0h/1/5"), "amount": 123}], False),
+        # fee=60 (20%) -> NOT high
+        (660, [100, 200, 300], [
+            {"verified_derivation_path": bip32.parse_path("m/84h/0h/0h/0/0"), "amount": 200},  # self-transfer
+            {"verified_derivation_path": bip32.parse_path("m/84h/0h/0h/1/0"), "amount": 300},  # true change
+        ], False),
+        # same mix but high fee: fee=100 (33%) -> HIGH
+        (700, [100, 200, 300], [
+            {"verified_derivation_path": bip32.parse_path("m/84h/0h/0h/0/0"), "amount": 200},
+            {"verified_derivation_path": bip32.parse_path("m/84h/0h/0h/1/0"), "amount": 300},
+        ], True),
+    ])
+    def test_has_high_fee(self, vin, vout_values, change_data, expected):
+        """
+            Should correctly identify if a PSBT has a high fee.
+        """
+        # Build a dummy parser without running .parse()
+        parser = PSBTParser.__new__(PSBTParser)
+        parser.HIGH_FEES_WARNING_THRESHOLD = 25
+
+        # Stub out parser.psbt.tx.vout as list of objects with a .value attribute
+        parser.psbt = SimpleNamespace(
+            tx=SimpleNamespace(
+                vout=[SimpleNamespace(value=v) for v in vout_values]
+            )
+        )
+        parser.change_data = change_data
+        parser.fee_amount = vin - sum(vout_values)
+
+        assert parser.has_high_fee() is expected
+
+
+    def test_parse_sets_is_high_fee(self):
+        """
+            parse() should settle is_high_fee once, from the real totals, so the views
+            can read it without recomputing. Checked in both directions: a realistic fee
+            leaves it False, a fee dwarfing the spend sets it True.
+        """
+        # 272 sat fee on a 2 BTC spend: nowhere near the threshold
+        psbt = PSBT.parse(a2b_base64(PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_2_INPUTS))
+        psbt_parser = PSBTParser(p=psbt, seed=PSBTTestData.two_input_seed, network=SettingsConstants.REGTEST)
+        assert psbt_parser.is_high_fee is False
+        assert psbt_parser.is_high_fee == psbt_parser.has_high_fee()
+
+        # 1 BTC input paying a 50,000 sat recipient and 10,000 sats change: almost all fee
+        psbt = PSBT.parse(a2b_base64(PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_1_INPUT))
+        psbt.outputs.append(create_output(PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_RECEIVE, 50_000))
+        psbt.outputs.append(create_output(PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_CHANGE, 10_000))
+        psbt_parser = PSBTParser(p=psbt, seed=PSBTTestData.seed, network=SettingsConstants.REGTEST)
+        assert psbt_parser.is_high_fee is True
+        assert psbt_parser.is_high_fee == psbt_parser.has_high_fee()