From d6ee542deb54372e9c3e940f64951505e972fdbf Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Fri, 17 Jul 2020 17:50:02 +0200 Subject: [PATCH] core/bitcoin: Move transaction confirmation logic from Bitcoin to BasicApprover class. --- core/src/apps/bitcoin/sign_tx/__init__.py | 19 ++- core/src/apps/bitcoin/sign_tx/approvers.py | 98 +++++++++++++++ core/src/apps/bitcoin/sign_tx/bitcoin.py | 124 +++++++------------ core/src/apps/bitcoin/sign_tx/bitcoinlike.py | 5 - core/src/apps/bitcoin/sign_tx/decred.py | 26 ++-- core/src/apps/bitcoin/sign_tx/zcash.py | 14 ++- 6 files changed, 175 insertions(+), 111 deletions(-) create mode 100644 core/src/apps/bitcoin/sign_tx/approvers.py diff --git a/core/src/apps/bitcoin/sign_tx/__init__.py b/core/src/apps/bitcoin/sign_tx/__init__.py index bb731d620..8a57c1090 100644 --- a/core/src/apps/bitcoin/sign_tx/__init__.py +++ b/core/src/apps/bitcoin/sign_tx/__init__.py @@ -8,7 +8,7 @@ from apps.common import coininfo, paths, seed from ..common import BITCOIN_NAMES from ..keychain import with_keychain -from . import bitcoin, helpers, layout, progress +from . import approvers, bitcoin, helpers, layout, progress if not utils.BITCOIN_ONLY: from . import bitcoinlike, decred, zcash @@ -21,20 +21,19 @@ if False: async def sign_tx( ctx: wire.Context, msg: SignTx, keychain: seed.Keychain, coin: coininfo.CoinInfo ) -> TxRequest: - if not utils.BITCOIN_ONLY: + approver = approvers.BasicApprover(msg, coin) + + if utils.BITCOIN_ONLY or coin.coin_name in BITCOIN_NAMES: + signer_class = bitcoin.Bitcoin + else: if coin.decred: - signer_class = decred.Decred # type: Type[bitcoin.Bitcoin] + signer_class = decred.Decred elif coin.overwintered: signer_class = zcash.Zcashlike - elif coin.coin_name not in BITCOIN_NAMES: - signer_class = bitcoinlike.Bitcoinlike else: - signer_class = bitcoin.Bitcoin - - else: - signer_class = bitcoin.Bitcoin + signer_class = bitcoinlike.Bitcoinlike - signer = signer_class(msg, keychain, coin).signer() + signer = signer_class(msg, keychain, coin, approver).signer() res = None # type: Union[TxAck, bool, None] field_cache = {} diff --git a/core/src/apps/bitcoin/sign_tx/approvers.py b/core/src/apps/bitcoin/sign_tx/approvers.py new file mode 100644 index 000000000..ab6b72bcd --- /dev/null +++ b/core/src/apps/bitcoin/sign_tx/approvers.py @@ -0,0 +1,98 @@ +from micropython import const + +from trezor import wire +from trezor.messages.SignTx import SignTx +from trezor.messages.TxInputType import TxInputType +from trezor.messages.TxOutputType import TxOutputType + +from apps.common import coininfo + +from .. import addresses +from . import helpers, tx_weight + + +# An Approver object computes the transaction totals and either prompts the user +# to confirm transaction parameters (output addresses, amounts and fees) or uses +# an Authorization object to verify that the user authorized a transaction with +# these parameters to be executed. +class Approver: + def __init__(self, tx: SignTx, coin: coininfo.CoinInfo) -> None: + self.tx = tx + self.coin = coin + self.weight = tx_weight.TxWeightCalculator(tx.inputs_count, tx.outputs_count) + + # amounts + self.total_in = 0 # sum of input amounts + self.external_in = 0 # sum of external input amounts + self.total_out = 0 # sum of output amounts + self.change_out = 0 # change output amount + + async def add_internal_input(self, txi: TxInputType, amount: int) -> None: + self.weight.add_input(txi) + self.total_in += amount + + def add_external_input(self, txi: TxInputType) -> None: + self.weight.add_input(txi) + self.total_in += txi.amount + self.external_in += txi.amount + + def add_change_output(self, txo: TxOutputType, script_pubkey: bytes) -> None: + self.weight.add_output(script_pubkey) + self.total_out += txo.amount + self.change_out += txo.amount + + async def add_external_output( + self, txo: TxOutputType, script_pubkey: bytes + ) -> None: + self.weight.add_output(script_pubkey) + self.total_out += txo.amount + + async def approve_tx(self) -> None: + raise NotImplementedError + + +class BasicApprover(Approver): + # the maximum number of change-outputs allowed without user confirmation + MAX_SILENT_CHANGE_COUNT = const(2) + + def __init__(self, tx: SignTx, coin: coininfo.CoinInfo) -> None: + super().__init__(tx, coin) + self.change_count = 0 # the number of change-outputs + + async def add_internal_input(self, txi: TxInputType, amount: int) -> None: + if not addresses.validate_full_path(txi.address_n, self.coin, txi.script_type): + await helpers.confirm_foreign_address(txi.address_n) + + await super().add_internal_input(txi, amount) + + def add_change_output(self, txo: TxOutputType, script_pubkey: bytes) -> None: + super().add_change_output(txo, script_pubkey) + self.change_count += 1 + + async def add_external_output( + self, txo: TxOutputType, script_pubkey: bytes + ) -> None: + await super().add_external_output(txo, script_pubkey) + await helpers.confirm_output(txo, self.coin) + + async def approve_tx(self) -> None: + fee = self.total_in - self.total_out + + # some coins require negative fees for reward TX + if fee < 0 and not self.coin.negative_fee: + raise wire.NotEnoughFunds("Not enough funds") + + total = self.total_in - self.change_out + spending = total - self.external_in + + # fee > (coin.maxfee per byte * tx size) + if fee > (self.coin.maxfee_kb / 1000) * (self.weight.get_total() / 4): + await helpers.confirm_feeoverthreshold(fee, self.coin) + if self.change_count > self.MAX_SILENT_CHANGE_COUNT: + await helpers.confirm_change_count_over_threshold(self.change_count) + if self.tx.lock_time > 0: + await helpers.confirm_nondefault_locktime(self.tx.lock_time) + if not self.external_in: + await helpers.confirm_total(total, fee, self.coin) + else: + await helpers.confirm_joint_total(spending, total, self.coin) diff --git a/core/src/apps/bitcoin/sign_tx/bitcoin.py b/core/src/apps/bitcoin/sign_tx/bitcoin.py index 4f6aae0b3..f02731da9 100644 --- a/core/src/apps/bitcoin/sign_tx/bitcoin.py +++ b/core/src/apps/bitcoin/sign_tx/bitcoin.py @@ -20,7 +20,7 @@ from .. import addresses, common, multisig, scripts, writers from ..common import SIGHASH_ALL, ecdsa_sign from ..ownership import verify_nonownership from ..verification import SignatureVerifier -from . import helpers, progress, tx_weight +from . import approvers, helpers, progress from .matchcheck import MultisigFingerprintChecker, WalletPathChecker if False: @@ -37,45 +37,47 @@ _BIP32_MAX_LAST_ELEMENT = const(1000000) # the number of bytes to preallocate for serialized transaction chunks _MAX_SERIALIZED_CHUNK_SIZE = const(2048) -# the maximum number of change-outputs allowed without user confirmation -_MAX_SILENT_CHANGE_COUNT = const(2) - class Bitcoin: async def signer(self) -> None: - # Add inputs to hash143 and h_confirmed and compute the sum of input amounts + # Add inputs to hash143 and h_approved and compute the sum of input amounts # by requesting each previous transaction and checking its output amounts. await self.step1_process_inputs() - # Add outputs to hash143 and h_confirmed, confirm outputs and compute + # Add outputs to hash143 and h_approved, approve outputs and compute # sum of output amounts. - await self.step2_confirm_outputs() + await self.step2_approve_outputs() - # Check fee, confirm lock_time and total. - await self.step3_confirm_tx() + # Check fee, approve lock_time and total. + await self.approver.approve_tx() # Verify external inputs which have already been signed or which come with # a proof of non-ownership. - await self.step4_verify_external_inputs() + await self.step3_verify_external_inputs() # Check that inputs are unchanged. Serialize inputs and sign the non-segwit ones. - await self.step5_serialize_inputs() + await self.step4_serialize_inputs() # Serialize outputs. - await self.step6_serialize_outputs() + await self.step5_serialize_outputs() # Sign segwit inputs and serialize witness data. - await self.step7_sign_segwit_inputs() + await self.step6_sign_segwit_inputs() # Write footer and send remaining data. - await self.step8_finish() + await self.step7_finish() def __init__( - self, tx: SignTx, keychain: seed.Keychain, coin: coininfo.CoinInfo + self, + tx: SignTx, + keychain: seed.Keychain, + coin: coininfo.CoinInfo, + approver: approvers.Approver, ) -> None: - self.coin = coin self.tx = helpers.sanitize_sign_tx(tx, coin) self.keychain = keychain + self.coin = coin + self.approver = approver # checksum of multisig inputs, used to validate change-output self.multisig_fingerprint = MultisigFingerprintChecker() @@ -89,14 +91,6 @@ class Bitcoin: # set of indices of inputs which are external self.external = set() # type: Set[int] - # amounts - self.total_in = 0 # sum of input amounts - self.external_in = 0 # sum of external input amounts - self.total_out = 0 # sum of output amounts - self.change_out = 0 # change output amount - self.change_count = 0 # the number of change-outputs - self.weight = tx_weight.TxWeightCalculator(tx.inputs_count, tx.outputs_count) - # transaction and signature serialization self.serialized_tx = writers.empty_bytearray(_MAX_SERIALIZED_CHUNK_SIZE) self.tx_req = TxRequest() @@ -104,13 +98,13 @@ class Bitcoin: self.tx_req.serialized = TxRequestSerializedType() self.tx_req.serialized.serialized_tx = self.serialized_tx - # h_confirmed is used to make sure that the inputs and outputs streamed for - # confirmation in Steps 1 and 2 are the same as the ones streamed for signing - # legacy inputs in Step 5. - self.h_confirmed = self.create_hash_writer() # not a real tx hash + # h_approved is used to make sure that the inputs and outputs streamed for + # approval in Steps 1 and 2 are the same as the ones streamed for signing + # legacy inputs in Step 4. + self.h_approved = self.create_hash_writer() # not a real tx hash # h_external is used to make sure that the signed external inputs streamed for - # confirmation in Step 1 are the same as the ones streamed for verification + # approval in Step 1 are the same as the ones streamed for verification # in Step 3. self.h_external = self.create_hash_writer() @@ -126,7 +120,6 @@ class Bitcoin: for i in range(self.tx.inputs_count): # STAGE_REQUEST_1_INPUT in legacy txi = await helpers.request_tx_input(self.tx_req, i, self.coin) - self.weight.add_input(txi) if input_is_segwit(txi): self.segwit.add(i) @@ -137,36 +130,14 @@ class Bitcoin: progress.advance() await self.process_internal_input(txi) - async def step2_confirm_outputs(self) -> None: + async def step2_approve_outputs(self) -> None: for i in range(self.tx.outputs_count): # STAGE_REQUEST_3_OUTPUT in legacy txo = await helpers.request_tx_output(self.tx_req, i, self.coin) script_pubkey = self.output_derive_script(txo) - self.weight.add_output(script_pubkey) - await self.confirm_output(txo, script_pubkey) - - async def step3_confirm_tx(self) -> None: - fee = self.total_in - self.total_out - - if fee < 0: - self.on_negative_fee() - - total = self.total_in - self.change_out - spending = total - self.external_in - - # fee > (coin.maxfee per byte * tx size) - if fee > (self.coin.maxfee_kb / 1000) * (self.weight.get_total() / 4): - await helpers.confirm_feeoverthreshold(fee, self.coin) - if self.change_count > _MAX_SILENT_CHANGE_COUNT: - await helpers.confirm_change_count_over_threshold(self.change_count) - if self.tx.lock_time > 0: - await helpers.confirm_nondefault_locktime(self.tx.lock_time) - if not self.external: - await helpers.confirm_total(total, fee, self.coin) - else: - await helpers.confirm_joint_total(spending, total, self.coin) + await self.approve_output(txo, script_pubkey) - async def step4_verify_external_inputs(self) -> None: + async def step3_verify_external_inputs(self) -> None: # should come out the same as h_external, checked before continuing h_check = self.create_hash_writer() @@ -197,11 +168,11 @@ class Bitcoin: ) verifier.verify(tx_digest) - # check that the inputs were the same as those streamed for confirmation + # check that the inputs were the same as those streamed for approval if self.h_external.get_digest() != h_check.get_digest(): raise wire.ProcessError("Transaction has changed during signing") - async def step5_serialize_inputs(self) -> None: + async def step4_serialize_inputs(self) -> None: self.write_tx_header(self.serialized_tx, self.tx, bool(self.segwit)) write_bitcoin_varint(self.serialized_tx, self.tx.inputs_count) @@ -214,13 +185,13 @@ class Bitcoin: else: await self.sign_nonsegwit_input(i) - async def step6_serialize_outputs(self) -> None: + async def step5_serialize_outputs(self) -> None: write_bitcoin_varint(self.serialized_tx, self.tx.outputs_count) for i in range(self.tx.outputs_count): progress.advance() await self.serialize_output(i) - async def step7_sign_segwit_inputs(self) -> None: + async def step6_sign_segwit_inputs(self) -> None: if not self.segwit: progress.advance(self.tx.inputs_count) return @@ -237,19 +208,16 @@ class Bitcoin: # add empty witness for non-segwit inputs self.serialized_tx.append(0) - async def step8_finish(self) -> None: + async def step7_finish(self) -> None: self.write_tx_footer(self.serialized_tx, self.tx) await helpers.request_tx_finish(self.tx_req) async def process_internal_input(self, txi: TxInputType) -> None: self.wallet_path.add_input(txi) self.multisig_fingerprint.add_input(txi) - writers.write_tx_input_check(self.h_confirmed, txi) + writers.write_tx_input_check(self.h_approved, txi) self.hash143_add_input(txi) # all inputs are included (non-segwit as well) - if not addresses.validate_full_path(txi.address_n, self.coin, txi.script_type): - await helpers.confirm_foreign_address(txi.address_n) - if txi.script_type not in common.INTERNAL_INPUT_SCRIPT_TYPES: raise wire.DataError("Wrong input script type") @@ -260,29 +228,26 @@ class Bitcoin: if txi.amount is not None and prev_amount != txi.amount: raise wire.DataError("Invalid amount specified") - self.total_in += prev_amount + await self.approver.add_internal_input(txi, prev_amount) async def process_external_input(self, txi: TxInputType) -> None: if txi.amount is None: raise wire.DataError("Expected input with amount") writers.write_tx_input_check(self.h_external, txi) - writers.write_tx_input_check(self.h_confirmed, txi) + writers.write_tx_input_check(self.h_approved, txi) self.hash143_add_input(txi) # all inputs are included (non-segwit as well) - self.total_in += txi.amount - self.external_in += txi.amount + self.approver.add_external_input(txi) - async def confirm_output(self, txo: TxOutputType, script_pubkey: bytes) -> None: + async def approve_output(self, txo: TxOutputType, script_pubkey: bytes) -> None: if self.output_is_change(txo): - # output is change and does not need confirmation - self.change_out += txo.amount - self.change_count += 1 + # output is change and does not need approval + self.approver.add_change_output(txo, script_pubkey) else: - await helpers.confirm_output(txo, self.coin) + await self.approver.add_external_output(txo, script_pubkey) - self.write_tx_output(self.h_confirmed, txo, script_pubkey) + self.write_tx_output(self.h_approved, txo, script_pubkey) self.hash143_add_output(txo, script_pubkey) - self.total_out += txo.amount async def get_tx_digest( self, @@ -298,9 +263,6 @@ class Bitcoin: digest, _, _ = await self.get_legacy_tx_digest(i, script_pubkey) return digest - def on_negative_fee(self) -> None: - raise wire.NotEnoughFunds("Not enough funds") - async def serialize_external_input(self, i: int) -> None: txi = await helpers.request_tx_input(self.tx_req, i, self.coin) if not input_is_external(txi): @@ -373,7 +335,7 @@ class Bitcoin: ) -> Tuple[bytes, TxInputType, Optional[HDNode]]: # the transaction digest which gets signed for this input h_sign = self.create_hash_writer() - # should come out the same as h_confirmed, checked before signing the digest + # should come out the same as h_approved, checked before signing the digest h_check = self.create_hash_writer() self.write_tx_header(h_sign, self.tx, witness_marker=False) @@ -422,8 +384,8 @@ class Bitcoin: writers.write_uint32(h_sign, self.tx.lock_time) writers.write_uint32(h_sign, self.get_sighash_type(txi_sign)) - # check that the inputs were the same as those streamed for confirmation - if self.h_confirmed.get_digest() != h_check.get_digest(): + # check that the inputs were the same as those streamed for approval + if self.h_approved.get_digest() != h_check.get_digest(): raise wire.ProcessError("Transaction has changed during signing") tx_digest = writers.get_tx_hash(h_sign, double=self.coin.sign_hash_double) diff --git a/core/src/apps/bitcoin/sign_tx/bitcoinlike.py b/core/src/apps/bitcoin/sign_tx/bitcoinlike.py index 726810ec4..d6f897db0 100644 --- a/core/src/apps/bitcoin/sign_tx/bitcoinlike.py +++ b/core/src/apps/bitcoin/sign_tx/bitcoinlike.py @@ -55,11 +55,6 @@ class Bitcoinlike(Bitcoin): i, txi, public_keys, threshold, script_pubkey ) - def on_negative_fee(self) -> None: - # some coins require negative fees for reward TX - if not self.coin.negative_fee: - super().on_negative_fee() - def get_sighash_type(self, txi: TxInputType) -> int: hashtype = super().get_sighash_type(txi) if self.coin.fork_id is not None: diff --git a/core/src/apps/bitcoin/sign_tx/decred.py b/core/src/apps/bitcoin/sign_tx/decred.py index 5ac27ee94..13512a79f 100644 --- a/core/src/apps/bitcoin/sign_tx/decred.py +++ b/core/src/apps/bitcoin/sign_tx/decred.py @@ -15,7 +15,7 @@ from apps.common.writers import write_bitcoin_varint from .. import multisig, scripts, writers from ..common import ecdsa_hash_pubkey, ecdsa_sign -from . import helpers, progress +from . import approvers, helpers, progress from .bitcoin import Bitcoin DECRED_SERIALIZE_FULL = const(0 << 16) @@ -31,10 +31,14 @@ if False: class Decred(Bitcoin): def __init__( - self, tx: SignTx, keychain: seed.Keychain, coin: coininfo.CoinInfo + self, + tx: SignTx, + keychain: seed.Keychain, + coin: coininfo.CoinInfo, + approver: approvers.Approver, ) -> None: ensure(coin.decred) - super().__init__(tx, keychain, coin) + super().__init__(tx, keychain, coin, approver) self.write_tx_header(self.serialized_tx, self.tx, witness_marker=True) write_bitcoin_varint(self.serialized_tx, self.tx.inputs_count) @@ -49,10 +53,10 @@ class Decred(Bitcoin): def create_hash_writer(self) -> HashWriter: return HashWriter(blake256()) - async def step2_confirm_outputs(self) -> None: + async def step2_approve_outputs(self) -> None: write_bitcoin_varint(self.serialized_tx, self.tx.outputs_count) write_bitcoin_varint(self.h_prefix, self.tx.outputs_count) - await super().step2_confirm_outputs() + await super().step2_approve_outputs() self.write_tx_footer(self.serialized_tx, self.tx) self.write_tx_footer(self.h_prefix, self.tx) @@ -65,11 +69,11 @@ class Decred(Bitcoin): async def process_external_input(self, txi: TxInputType) -> None: raise wire.DataError("External inputs not supported") - async def confirm_output(self, txo: TxOutputType, script_pubkey: bytes) -> None: - await super().confirm_output(txo, script_pubkey) + async def approve_output(self, txo: TxOutputType, script_pubkey: bytes) -> None: + await super().approve_output(txo, script_pubkey) self.write_tx_output(self.serialized_tx, txo, script_pubkey) - async def step5_serialize_inputs(self) -> None: + async def step4_serialize_inputs(self) -> None: write_bitcoin_varint(self.serialized_tx, self.tx.inputs_count) prefix_hash = self.h_prefix.get_digest() @@ -126,13 +130,13 @@ class Decred(Bitcoin): self.write_tx_input_witness(self.serialized_tx, txi_sign, script_sig) self.set_serialized_signature(i_sign, signature) - async def step6_serialize_outputs(self) -> None: + async def step5_serialize_outputs(self) -> None: pass - async def step7_sign_segwit_inputs(self) -> None: + async def step6_sign_segwit_inputs(self) -> None: pass - async def step8_finish(self) -> None: + async def step7_finish(self) -> None: await helpers.request_tx_finish(self.tx_req) def check_prevtx_output(self, txo_bin: TxOutputBinType) -> None: diff --git a/core/src/apps/bitcoin/sign_tx/zcash.py b/core/src/apps/bitcoin/sign_tx/zcash.py index 13fb8f18b..5398303b5 100644 --- a/core/src/apps/bitcoin/sign_tx/zcash.py +++ b/core/src/apps/bitcoin/sign_tx/zcash.py @@ -24,7 +24,7 @@ from ..writers import ( write_uint32, write_uint64, ) -from . import helpers +from . import approvers, helpers from .bitcoinlike import Bitcoinlike if False: @@ -35,14 +35,20 @@ OVERWINTERED = const(0x80000000) class Zcashlike(Bitcoinlike): - def __init__(self, tx: SignTx, keychain: Keychain, coin: CoinInfo) -> None: + def __init__( + self, + tx: SignTx, + keychain: Keychain, + coin: CoinInfo, + approver: approvers.Approver, + ) -> None: ensure(coin.overwintered) - super().__init__(tx, keychain, coin) + super().__init__(tx, keychain, coin, approver) if self.tx.version != 4: raise wire.DataError("Unsupported transaction version.") - async def step8_finish(self) -> None: + async def step7_finish(self) -> None: self.write_tx_footer(self.serialized_tx, self.tx) write_uint64(self.serialized_tx, 0) # valueBalance