diff --git a/src/seedsigner/gui/screens/psbt_screens.py b/src/seedsigner/gui/screens/psbt_screens.py index 973d3ecb..e97da724 100644 --- a/src/seedsigner/gui/screens/psbt_screens.py +++ b/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 diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index 441c1e1d..73293dc3 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/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) \ No newline at end of file + _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 > % 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) diff --git a/src/seedsigner/views/psbt_views.py b/src/seedsigner/views/psbt_views.py index dba57fb4..7ac58062 100644 --- a/src/seedsigner/views/psbt_views.py +++ b/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: diff --git a/tests/screenshot_generator/generator.py b/tests/screenshot_generator/generator.py index 5fa52a2c..c938ba3f 100644 --- a/tests/screenshot_generator/generator.py +++ b/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), diff --git a/tests/test_psbt_parser.py b/tests/test_psbt_parser.py index 4d28c2ff..ee8506fe 100644 --- a/tests/test_psbt_parser.py +++ b/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()