From 5aa9e644be38d39f319ebe92f4ea3e64ad654f16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Vejpustek?= Date: Tue, 12 Nov 2024 12:52:40 +0100 Subject: [PATCH] refactor(change): factor out ChangeDetector --- core/src/all_modules.py | 2 + core/src/apps/bitcoin/sign_tx/approvers.py | 8 ++- .../apps/bitcoin/sign_tx/change_detector.py | 65 +++++++++++++++++++ core/src/apps/bitcoin/sign_tx/tx_info.py | 55 ++-------------- 4 files changed, 78 insertions(+), 52 deletions(-) create mode 100644 core/src/apps/bitcoin/sign_tx/change_detector.py diff --git a/core/src/all_modules.py b/core/src/all_modules.py index 0d643fcbfb..12233665d1 100644 --- a/core/src/all_modules.py +++ b/core/src/all_modules.py @@ -271,6 +271,8 @@ apps.bitcoin.sign_tx.bitcoin import apps.bitcoin.sign_tx.bitcoin apps.bitcoin.sign_tx.bitcoinlike import apps.bitcoin.sign_tx.bitcoinlike +apps.bitcoin.sign_tx.change_detector +import apps.bitcoin.sign_tx.change_detector apps.bitcoin.sign_tx.decred import apps.bitcoin.sign_tx.decred apps.bitcoin.sign_tx.helpers diff --git a/core/src/apps/bitcoin/sign_tx/approvers.py b/core/src/apps/bitcoin/sign_tx/approvers.py index f7b37607ba..a803a22d0c 100644 --- a/core/src/apps/bitcoin/sign_tx/approvers.py +++ b/core/src/apps/bitcoin/sign_tx/approvers.py @@ -227,7 +227,9 @@ class BasicApprover(Approver): "Adding new OP_RETURN outputs in replacement transactions is not supported." ) elif txo.payment_req_index is None or self.show_payment_req_details: - source_path = tx_info.wallet_path.get_path() if tx_info else None + source_path = ( + tx_info.change_detector.wallet_path.get_path() if tx_info else None + ) # Ask user to confirm output, unless it is part of a payment # request, which gets confirmed separately. await helpers.confirm_output( @@ -294,7 +296,7 @@ class BasicApprover(Approver): if self.has_unverified_external_input: await helpers.confirm_unverified_external_input() - if tx_info.wallet_path.get_path() is None: + if tx_info.change_detector.wallet_path.get_path() is None: await helpers.confirm_multiple_accounts() fee = self.total_in - self.total_out @@ -390,7 +392,7 @@ class BasicApprover(Approver): fee_rate, coin, amount_unit, - tx_info.wallet_path.get_path(), + tx_info.change_detector.wallet_path.get_path(), ) else: await helpers.confirm_joint_total(spending, total, coin, amount_unit) diff --git a/core/src/apps/bitcoin/sign_tx/change_detector.py b/core/src/apps/bitcoin/sign_tx/change_detector.py new file mode 100644 index 0000000000..39630aa6ed --- /dev/null +++ b/core/src/apps/bitcoin/sign_tx/change_detector.py @@ -0,0 +1,65 @@ +from micropython import const +from typing import TYPE_CHECKING + +from .. import common + +if TYPE_CHECKING: + from trezor.messages import TxInput, TxOutput + +# The chain id used for change. +_BIP32_CHANGE_CHAIN = const(1) + +# The maximum allowed change address. This should be large enough for normal +# use and still allow to quickly brute-force the correct BIP32 path. +_BIP32_MAX_LAST_ELEMENT = const(1_000_000) + + +class ChangeDetector: + def __init__(self) -> None: + from .matchcheck import ( + MultisigFingerprintChecker, + ScriptTypeChecker, + WalletPathChecker, + ) + + # Checksum of multisig inputs, used to validate change-output. + self.multisig_fingerprint = MultisigFingerprintChecker() + + # Common prefix of input paths, used to validate change-output. + self.wallet_path = WalletPathChecker() + + # Common script type, used to validate change-output. + self.script_type = ScriptTypeChecker() + + def add_input(self, txi: TxInput) -> None: + if not common.input_is_external(txi): + self.wallet_path.add_input(txi) + self.script_type.add_input(txi) + self.multisig_fingerprint.add_input(txi) + + def check_input(self, txi: TxInput) -> None: + self.wallet_path.check_input(txi) + self.script_type.check_input(txi) + self.multisig_fingerprint.check_input(txi) + + def output_is_change(self, txo: TxOutput) -> bool: + if txo.script_type not in common.CHANGE_OUTPUT_SCRIPT_TYPES: + return False + + # Check the multisig fingerprint only for multisig outputs. This means + # that a transfer from a multisig account to a singlesig account is + # treated as a change-output as long as all other change-output + # conditions are satisfied. This goes a bit against the concept of a + # multisig account but the other cosigners will notice that they are + # relinquishing control of the funds, so there is no security risk. + if txo.multisig and not self.multisig_fingerprint.output_matches(txo): + return False + + return ( + self.wallet_path.output_matches(txo) + and self.script_type.output_matches(txo) + and len(txo.address_n) >= common.BIP32_WALLET_DEPTH + and txo.address_n[-2] <= _BIP32_CHANGE_CHAIN + and txo.address_n[-1] <= _BIP32_MAX_LAST_ELEMENT + and txo.amount > 0 + ) diff --git a/core/src/apps/bitcoin/sign_tx/tx_info.py b/core/src/apps/bitcoin/sign_tx/tx_info.py index deb10dfd04..8ffe9bddc0 100644 --- a/core/src/apps/bitcoin/sign_tx/tx_info.py +++ b/core/src/apps/bitcoin/sign_tx/tx_info.py @@ -1,7 +1,7 @@ from micropython import const from typing import TYPE_CHECKING -from .. import common, writers +from .. import writers if TYPE_CHECKING: from typing import Protocol @@ -32,13 +32,6 @@ if TYPE_CHECKING: ) -> None: ... -# The chain id used for change. -_BIP32_CHANGE_CHAIN = const(1) - -# The maximum allowed change address. This should be large enough for normal -# use and still allow to quickly brute-force the correct BIP32 path. -_BIP32_MAX_LAST_ELEMENT = const(1_000_000) - # Setting nSequence to this value for every input in a transaction disables nLockTime. _SEQUENCE_FINAL = const(0xFFFF_FFFF) @@ -52,20 +45,9 @@ class TxInfoBase: from trezor.crypto.hashlib import sha256 from trezor.utils import HashWriter - from .matchcheck import ( - MultisigFingerprintChecker, - ScriptTypeChecker, - WalletPathChecker, - ) + from .change_detector import ChangeDetector - # Checksum of multisig inputs, used to validate change-output. - self.multisig_fingerprint = MultisigFingerprintChecker() - - # Common prefix of input paths, used to validate change-output. - self.wallet_path = WalletPathChecker() - - # Common script type, used to validate change-output. - self.script_type = ScriptTypeChecker() + self.change_detector = ChangeDetector() # h_tx_check is used to make sure that the inputs and outputs streamed in # different steps are the same every time, e.g. the ones streamed for approval @@ -88,42 +70,17 @@ class TxInfoBase: self.sig_hasher.add_input(txi, script_pubkey) writers.write_tx_input_check(self.h_tx_check, txi) self.min_sequence = min(self.min_sequence, txi.sequence) - - if not common.input_is_external(txi): - self.wallet_path.add_input(txi) - self.script_type.add_input(txi) - self.multisig_fingerprint.add_input(txi) + self.change_detector.add_input(txi) def add_output(self, txo: TxOutput, script_pubkey: bytes) -> None: self.sig_hasher.add_output(txo, script_pubkey) writers.write_tx_output(self.h_tx_check, txo, script_pubkey) def check_input(self, txi: TxInput) -> None: - self.wallet_path.check_input(txi) - self.script_type.check_input(txi) - self.multisig_fingerprint.check_input(txi) + self.change_detector.check_input(txi) def output_is_change(self, txo: TxOutput) -> bool: - if txo.script_type not in common.CHANGE_OUTPUT_SCRIPT_TYPES: - return False - - # Check the multisig fingerprint only for multisig outputs. This means - # that a transfer from a multisig account to a singlesig account is - # treated as a change-output as long as all other change-output - # conditions are satisfied. This goes a bit against the concept of a - # multisig account but the other cosigners will notice that they are - # relinquishing control of the funds, so there is no security risk. - if txo.multisig and not self.multisig_fingerprint.output_matches(txo): - return False - - return ( - self.wallet_path.output_matches(txo) - and self.script_type.output_matches(txo) - and len(txo.address_n) >= common.BIP32_WALLET_DEPTH - and txo.address_n[-2] <= _BIP32_CHANGE_CHAIN - and txo.address_n[-1] <= _BIP32_MAX_LAST_ELEMENT - and txo.amount > 0 - ) + return self.change_detector.output_is_change(txo) def lock_time_disabled(self) -> bool: return self.min_sequence == _SEQUENCE_FINAL