SFT-8098: skip change verification for transactions without change
What the developer wrote
Authored by Jacksper13
SFT-8098: skip change verification for transactions without change
Avoid opening the key store when review does not require change verification. Clarify the spinner label and cover cancellation with and without change.
What changed, and why it matters
This commit changes how the Passport hardware wallet reviews Bitcoin transactions that have no 'change' output. Previously, the wallet would always run a change-verification step that opens the secure key store, even when there was no change to verify. Now it skips that step when there is no change, and updates the on-screen spinner label from 'Validating transaction' to 'Verifying change' so the user knows what is happening. The change is framed by the developer as a user-experience and clarity improvement, not as a fix for a known security vulnerability.
Treat as a routine hardening/UX improvement. Reviewers may want to confirm that skipping change verification for change-less transactions does not bypass any other ownership or fee-safety check performed elsewhere in the sign-PSBT flow, and that the key store is still opened later if signing actually occurs.
Security signals we found
Change reduces unnecessary access to the key store during transaction review
No cryptographic check is removed for transactions that actually contain change
UI label changed to more accurately describe the operation being performed
Tests added for cancellation paths with and without change outputs
Evidence from the diff
In sign_psbt_common_flow.py, the check_multisig_import coroutine now wraps the double_check_psbt_change_task call in a guard: if any(output.is_change for output in self.psbt.outputs). The task is only awaited when at least one PSBT output is marked as change. The spinner label is renamed from ‘Validating transaction’ to ‘Verifying change’. Unit tests are updated to cover (a) transactions with change, (b) transactions without change, and (c) cancellation both with and without change. The commit message says the goal is to ‘Avoid opening the key store when review does not require change verification.’
Changed components
ports/stm32/boards/Passport/modules/flows/sign_psbt_common_flow.pyports/stm32/boards/Passport/modules/tests/unit/psbt_multisig_approval.pyInspect captured patch +36 / −11
### ports/stm32/boards/Passport/modules/flows/sign_psbt_common_flow.py
@@ -54,14 +54,15 @@ async def check_multisig_import(self):
return
# Review hides change destinations, so prove ownership before showing it.
- gc.collect()
- (error_msg, error) = await spinner_task('Validating transaction',
- double_check_psbt_change_task, args=[self.psbt])
- gc.collect()
- if error is not None:
- await ErrorPage(error_msg).show()
- self.set_result(None)
- return
+ if any(output.is_change for output in self.psbt.outputs):
+ gc.collect()
+ (error_msg, error) = await spinner_task('Verifying change',
+ double_check_psbt_change_task, args=[self.psbt])
+ gc.collect()
+ if error is not None:
+ await ErrorPage(error_msg).show()
+ self.set_result(None)
+ return
self.goto(self.show_transaction_details)
### ports/stm32/boards/Passport/modules/tests/unit/psbt_multisig_approval.py
@@ -19,7 +19,7 @@
MY_XFP = 0x12345678
OTHER_KEY = unhexlify('02c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5')
PATHS = ("m/84'/0'/0'/1/7", "m/86'/0'/0'/1/7", "m/48'/0'/0'/2'/1/7")
-# Public keys derived from the deterministic test seed bytes(range(32)).
+# Precomputed fixtures from test seed bytes(range(32)); this test does not derive keys.
OWNED_KEYS = (
unhexlify('02477e5978ac99be533333b444e635fdee1001992c01dec8c41327cdb9a7d7b2a1'),
unhexlify('03bdb8d2c655d9d1fb6ff6533b43660033e5c23b2ca4b290223d813f08407d6bdb'),
@@ -135,12 +135,17 @@ def get(self, key, default=None):
class FakeSignFlow(SignPsbtCommonFlow):
def __init__(self, needs_approval):
+ # Exercise state ordering without rendering pages or accessing device settings.
Flow.__init__(self, initial_state=self.check_multisig_import)
self.psbt = FakePsbt(needs_approval)
+ self.cancel_review = False
async def show_transaction_details(self):
events.append('review')
- self.goto(self.sign_transaction)
+ if self.cancel_review:
+ self.set_result(None)
+ else:
+ self.goto(self.sign_transaction)
async def run_tests():
@@ -172,9 +177,28 @@ async def run_tests():
for needs_approval in (True, False):
events.clear()
flow = FakeSignFlow(needs_approval)
+ flow.psbt.outputs = [FakeOutput(PATHS[0], OWNED_KEYS[0])]
assert await flow.run() is flow.psbt
expected = ['import'] if needs_approval else []
- assert events == expected + ['verify', 'clear', 'review', 'confirm', 'sign']
+ assert events == expected + ['verify', PATHS[0], 'clear', 'review', 'confirm', 'sign']
+
+ # Reviewing and cancelling a transaction without change must not open the key store.
+ payment = FakeOutput(PATHS[0], OTHER_KEY)
+ payment.is_change = False
+ for outputs in ([], [payment]):
+ events.clear()
+ flow = FakeSignFlow(needs_approval=False)
+ flow.psbt.outputs = outputs
+ flow.cancel_review = True
+ assert await flow.run() is None
+ assert events == ['review']
+
+ events.clear()
+ flow = FakeSignFlow(needs_approval=False)
+ flow.psbt.outputs = [FakeOutput(PATHS[0], OWNED_KEYS[0])]
+ flow.cancel_review = True
+ assert await flow.run() is None
+ assert events == ['verify', PATHS[0], 'clear', 'review']
# Exercise the real ownership task and flow, with only the key store and UI replaced.
# Its sensitive-value context must close before review, and run just once.Why this scored 29/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.