From cd0b30db2ff365b4bb6a294b587fd6381de82595 Mon Sep 17 00:00:00 2001 From: Chaitanya-Keyal Date: Wed, 9 Apr 2025 17:41:51 +0000 Subject: [PATCH 1/3] feat: add a warning for high tx fee --- src/seedsigner/models/psbt_parser.py | 50 ++++++++++++- src/seedsigner/views/psbt_views.py | 38 ++++++++++ tests/screenshot_generator/generator.py | 1 + tests/test_psbt_parser.py | 97 ++++++++++++++++++++++++- 4 files changed, 184 insertions(+), 2 deletions(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index 441c1e1d..f7300eff 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 @@ -1181,4 +1185,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..96191a0e 100644 --- a/src/seedsigner/views/psbt_views.py +++ b/src/seedsigner/views/psbt_views.py @@ -163,6 +163,8 @@ class PSBTOverviewView(View): else: num_self_transfer_outputs += 1 + is_high_fee_tx = psbt_parser.has_high_fee() + # Run the overview screen selected_menu_num = self.run_screen( PSBTOverviewScreen, @@ -184,6 +186,9 @@ class PSBTOverviewView(View): # skip change warning and psbt math view if psbt_parser.policy == None: return Destination(PSBTUnsupportedScriptTypeWarningView) + + elif is_high_fee_tx: + 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 +240,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: 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..c055b921 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,83 @@ 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 From 5b15804a7a7d784ae13585290d8b7531a733a226 Mon Sep 17 00:00:00 2001 From: okaybro <66475772+Chaitanya-Keyal@users.noreply.github.com> Date: Thu, 14 Aug 2025 23:58:43 +0000 Subject: [PATCH 2/3] feat: highlight high fees in PSBTOverview and PSBTMath Screen Co-authored-by: alvroble <50918598+alvroble@users.noreply.github.com> --- src/seedsigner/gui/screens/psbt_screens.py | 28 +++++++++++++++++++--- src/seedsigner/views/psbt_views.py | 2 ++ 2 files changed, 27 insertions(+), 3 deletions(-) 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/views/psbt_views.py b/src/seedsigner/views/psbt_views.py index 96191a0e..213b8ae6 100644 --- a/src/seedsigner/views/psbt_views.py +++ b/src/seedsigner/views/psbt_views.py @@ -176,6 +176,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=is_high_fee_tx, ) if selected_menu_num == RET_CODE__BACK_BUTTON: @@ -297,6 +298,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.has_high_fee(), ) if selected_menu_num == RET_CODE__BACK_BUTTON: From 2195adfdbdfb7dda49f3852f5381b5cfb93a9533 Mon Sep 17 00:00:00 2001 From: okaybro <66475772+Chaitanya-Keyal@users.noreply.github.com> Date: Sun, 20 Sep 2026 16:13:13 +0530 Subject: [PATCH 3/3] Settle is_high_fee once in parse() Both PSBTOverviewView and PSBTMathView asked the parser whether the fee was high, and the overview asked twice (once for the screen, once for routing). Each call re-summed the outputs through psbt.tx, which rebuilds the whole transaction on every access. The totals it depends on are all fixed by the end of parse(), so compute it there once and store it as is_high_fee, the same way the other totals are exposed. has_high_fee() stays as the computation itself. --- src/seedsigner/models/psbt_parser.py | 8 ++++++++ src/seedsigner/views/psbt_views.py | 8 +++----- tests/test_psbt_parser.py | 21 +++++++++++++++++++++ 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index f7300eff..73293dc3 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -177,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`. @@ -303,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 diff --git a/src/seedsigner/views/psbt_views.py b/src/seedsigner/views/psbt_views.py index 213b8ae6..7ac58062 100644 --- a/src/seedsigner/views/psbt_views.py +++ b/src/seedsigner/views/psbt_views.py @@ -163,8 +163,6 @@ class PSBTOverviewView(View): else: num_self_transfer_outputs += 1 - is_high_fee_tx = psbt_parser.has_high_fee() - # Run the overview screen selected_menu_num = self.run_screen( PSBTOverviewScreen, @@ -176,7 +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=is_high_fee_tx, + is_high_fee_tx=psbt_parser.is_high_fee, ) if selected_menu_num == RET_CODE__BACK_BUTTON: @@ -188,7 +186,7 @@ class PSBTOverviewView(View): if psbt_parser.policy == None: return Destination(PSBTUnsupportedScriptTypeWarningView) - elif is_high_fee_tx: + 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: @@ -298,7 +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.has_high_fee(), + is_high_fee_tx=psbt_parser.is_high_fee, ) if selected_menu_num == RET_CODE__BACK_BUTTON: diff --git a/tests/test_psbt_parser.py b/tests/test_psbt_parser.py index c055b921..ee8506fe 100644 --- a/tests/test_psbt_parser.py +++ b/tests/test_psbt_parser.py @@ -445,6 +445,27 @@ class TestPSBTParser: 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() + + # TODO: Refactor all tests to be in the TestPSBTParser class(?) def test_p2tr_change_detection():