From d872be7f6bc19689bbff4392e35e31a491119c65 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Mon, 4 Nov 2019 02:17:31 +0100 Subject: [PATCH] psbt: don't put xpubs and full paths into tx by def; only while signing --- electrum/gui/qt/transaction_dialog.py | 2 +- electrum/keystore.py | 19 ++++++----- electrum/plugins/coldcard/coldcard.py | 4 +-- electrum/transaction.py | 45 +++++++++++++++++++++------ electrum/wallet.py | 37 +++++++++++++--------- 5 files changed, 73 insertions(+), 34 deletions(-) diff --git a/electrum/gui/qt/transaction_dialog.py b/electrum/gui/qt/transaction_dialog.py index 066c8b204..487309558 100644 --- a/electrum/gui/qt/transaction_dialog.py +++ b/electrum/gui/qt/transaction_dialog.py @@ -96,7 +96,7 @@ class TxDialog(QDialog, MessageBoxMixin): # if the wallet can populate the inputs with more info, do it now. # as a result, e.g. we might learn an imported address tx is segwit, - # in which case it's ok to display txid + # or that a beyond-gap-limit address is is_mine tx.add_info_from_wallet(self.wallet) self.setMinimumWidth(950) diff --git a/electrum/keystore.py b/electrum/keystore.py index eadf893e5..28de738bd 100644 --- a/electrum/keystore.py +++ b/electrum/keystore.py @@ -349,14 +349,15 @@ class Xpub: """ return self._root_fingerprint - def get_fp_and_derivation_to_be_used_in_partial_tx(self, der_suffix: Sequence[int]) -> Tuple[bytes, Sequence[int]]: - """Returns fingerprint and 'full' derivation path corresponding to a derivation suffix. - The fingerprint is either the root fp or the intermediate fp, depending on what is available, - and the 'full' derivation path is adjusted accordingly. + def get_fp_and_derivation_to_be_used_in_partial_tx(self, der_suffix: Sequence[int], *, + only_der_suffix: bool = True) -> Tuple[bytes, Sequence[int]]: + """Returns fingerprint and derivation path corresponding to a derivation suffix. + The fingerprint is either the root fp or the intermediate fp, depending on what is available + and 'only_der_suffix', and the derivation path is adjusted accordingly. """ fingerprint_hex = self.get_root_fingerprint() der_prefix_str = self.get_derivation_prefix() - if fingerprint_hex is not None and der_prefix_str is not None: + if not only_der_suffix and fingerprint_hex is not None and der_prefix_str is not None: # use root fp, and true full path fingerprint_bytes = bfh(fingerprint_hex) der_prefix_ints = convert_bip32_path_to_list_of_uint32(der_prefix_str) @@ -367,9 +368,10 @@ class Xpub: der_full = der_prefix_ints + list(der_suffix) return fingerprint_bytes, der_full - def get_xpub_to_be_used_in_partial_tx(self) -> str: + def get_xpub_to_be_used_in_partial_tx(self, *, only_der_suffix: bool) -> str: assert self.xpub - fp_bytes, der_full = self.get_fp_and_derivation_to_be_used_in_partial_tx(der_suffix=[]) + fp_bytes, der_full = self.get_fp_and_derivation_to_be_used_in_partial_tx(der_suffix=[], + only_der_suffix=only_der_suffix) bip32node = BIP32Node.from_xkey(self.xpub) depth = len(der_full) child_number_int = der_full[-1] if len(der_full) >= 1 else 0 @@ -603,7 +605,8 @@ class Old_KeyStore(Deterministic_KeyStore): return self._root_fingerprint # TODO Old_KeyStore and Xpub could share a common baseclass? - def get_fp_and_derivation_to_be_used_in_partial_tx(self, der_suffix: Sequence[int]) -> Tuple[bytes, Sequence[int]]: + def get_fp_and_derivation_to_be_used_in_partial_tx(self, der_suffix: Sequence[int], *, + only_der_suffix: bool = True) -> Tuple[bytes, Sequence[int]]: fingerprint_hex = self.get_root_fingerprint() der_prefix_str = self.get_derivation_prefix() fingerprint_bytes = bfh(fingerprint_hex) diff --git a/electrum/plugins/coldcard/coldcard.py b/electrum/plugins/coldcard/coldcard.py index ceae02e8e..bcb9fd63d 100644 --- a/electrum/plugins/coldcard/coldcard.py +++ b/electrum/plugins/coldcard/coldcard.py @@ -575,7 +575,7 @@ class ColdcardPlugin(HW_PluginBase): xpubs = [] derivs = set() for xpub, ks in zip(wallet.get_master_public_keys(), wallet.get_keystores()): - fp_bytes, der_full = ks.get_fp_and_derivation_to_be_used_in_partial_tx(der_suffix=[]) + fp_bytes, der_full = ks.get_fp_and_derivation_to_be_used_in_partial_tx(der_suffix=[], only_der_suffix=False) fp_hex = fp_bytes.hex().upper() der_prefix_str = bip32.convert_bip32_intpath_to_strpath(der_full) xpubs.append( (fp_hex, xpub, der_prefix_str) ) @@ -620,7 +620,7 @@ class ColdcardPlugin(HW_PluginBase): xfp_paths = [] for pubkey_hex in pubkey_deriv_info: ks, der_suffix = pubkey_deriv_info[pubkey_hex] - fp_bytes, der_full = ks.get_fp_and_derivation_to_be_used_in_partial_tx(der_suffix) + fp_bytes, der_full = ks.get_fp_and_derivation_to_be_used_in_partial_tx(der_suffix, only_der_suffix=False) xfp_int = xfp_int_from_xfp_bytes(fp_bytes) xfp_paths.append([xfp_int] + list(der_full)) diff --git a/electrum/transaction.py b/electrum/transaction.py index a9452a272..c2a6745dc 100644 --- a/electrum/transaction.py +++ b/electrum/transaction.py @@ -1731,19 +1731,46 @@ class PartialTransaction(Transaction): txin.witness = None self.invalidate_ser_cache() - def add_info_from_wallet(self, wallet: 'Abstract_Wallet') -> None: + def add_info_from_wallet(self, wallet: 'Abstract_Wallet', *, + include_xpubs_and_full_paths: bool = False) -> None: if self.is_complete(): return - from .keystore import Xpub - for ks in wallet.get_keystores(): - if isinstance(ks, Xpub): - fp_bytes, der_full = ks.get_fp_and_derivation_to_be_used_in_partial_tx(der_suffix=[]) - bip32node = BIP32Node.from_xkey(ks.get_xpub_to_be_used_in_partial_tx()) - self.xpubs[bip32node] = (fp_bytes, der_full) + only_der_suffix = not include_xpubs_and_full_paths + if include_xpubs_and_full_paths: + from .keystore import Xpub + for ks in wallet.get_keystores(): + if isinstance(ks, Xpub): + fp_bytes, der_full = ks.get_fp_and_derivation_to_be_used_in_partial_tx(der_suffix=[], + only_der_suffix=only_der_suffix) + xpub = ks.get_xpub_to_be_used_in_partial_tx(only_der_suffix=only_der_suffix) + bip32node = BIP32Node.from_xkey(xpub) + self.xpubs[bip32node] = (fp_bytes, der_full) for txin in self.inputs(): - wallet.add_input_info(txin) + wallet.add_input_info(txin, only_der_suffix=only_der_suffix) for txout in self.outputs(): - wallet.add_output_info(txout) + wallet.add_output_info(txout, only_der_suffix=only_der_suffix) + + def remove_xpubs_and_bip32_paths(self) -> None: + self.xpubs.clear() + for txin in self.inputs(): + txin.bip32_paths.clear() + for txout in self.outputs(): + txout.bip32_paths.clear() + + def prepare_for_export_for_coinjoin(self) -> None: + """Removes all sensitive details.""" + # globals + self.xpubs.clear() + self._unknown.clear() + # inputs + for txin in self.inputs(): + txin.bip32_paths.clear() + # outputs + for txout in self.outputs(): + txout.redeem_script = None + txout.witness_script = None + txout.bip32_paths.clear() + txout._unknown.clear() def remove_signatures(self): for txin in self.inputs(): diff --git a/electrum/wallet.py b/electrum/wallet.py index 10a3f4454..85ed53a3a 100644 --- a/electrum/wallet.py +++ b/electrum/wallet.py @@ -1226,10 +1226,11 @@ class Abstract_Wallet(AddressSynchronizer): locktime = get_locktime_for_new_transaction(self.network) return PartialTransaction.from_io(inputs, outputs, locktime=locktime) - def _add_input_sig_info(self, txin: PartialTxInput, address: str) -> None: + def _add_input_sig_info(self, txin: PartialTxInput, address: str, *, only_der_suffix: bool = True) -> None: raise NotImplementedError() # implemented by subclasses - def _add_txinout_derivation_info(self, txinout: Union[PartialTxInput, PartialTxOutput], address: str) -> None: + def _add_txinout_derivation_info(self, txinout: Union[PartialTxInput, PartialTxOutput], + address: str, *, only_der_suffix: bool = True) -> None: pass # implemented by subclasses def _add_input_utxo_info(self, txin: PartialTxInput, address: str) -> None: @@ -1255,7 +1256,7 @@ class Abstract_Wallet(AddressSynchronizer): """ return False # implemented by subclasses - def add_input_info(self, txin: PartialTxInput) -> None: + def add_input_info(self, txin: PartialTxInput, *, only_der_suffix: bool = True) -> None: address = self.get_txin_address(txin) if not self.is_mine(address): is_mine = self._learn_derivation_path_for_address_from_txinout(txin, address) @@ -1277,7 +1278,7 @@ class Abstract_Wallet(AddressSynchronizer): txin.witness_script = bfh(witness_script_hex) if witness_script_hex else None except UnknownTxinType: pass - self._add_input_sig_info(txin, address) + self._add_input_sig_info(txin, address, only_der_suffix=only_der_suffix) def can_sign(self, tx: Transaction) -> bool: if not isinstance(tx, PartialTransaction): @@ -1309,7 +1310,7 @@ class Abstract_Wallet(AddressSynchronizer): tx = Transaction(raw_tx) return tx - def add_output_info(self, txout: PartialTxOutput) -> None: + def add_output_info(self, txout: PartialTxOutput, *, only_der_suffix: bool = True) -> None: address = txout.address if not self.is_mine(address): is_mine = self._learn_derivation_path_for_address_from_txinout(txout, address) @@ -1320,7 +1321,7 @@ class Abstract_Wallet(AddressSynchronizer): txout.is_change = self.is_change(address) if isinstance(self, Multisig_Wallet): txout.num_sig = self.m - self._add_txinout_derivation_info(txout, address) + self._add_txinout_derivation_info(txout, address, only_der_suffix=only_der_suffix) if txout.redeem_script is None: try: redeem_script_hex = self.get_redeem_script(address) @@ -1339,14 +1340,21 @@ class Abstract_Wallet(AddressSynchronizer): return if not isinstance(tx, PartialTransaction): return - tx.add_info_from_wallet(self) + # add info to a temporary tx copy; including xpubs + # and full derivation paths as hw keystores might want them + tmp_tx = copy.deepcopy(tx) + tmp_tx.add_info_from_wallet(self, include_xpubs_and_full_paths=True) # sign. start with ready keystores. for k in sorted(self.get_keystores(), key=lambda ks: ks.ready_to_sign(), reverse=True): try: - if k.can_sign(tx): - k.sign_transaction(tx, password) + if k.can_sign(tmp_tx): + k.sign_transaction(tmp_tx, password) except UserCancelled: continue + # remove sensitive info; then copy back details from temporary tx + tmp_tx.remove_xpubs_and_bip32_paths() + tx.combine_with_other_psbt(tmp_tx) + tx.add_info_from_wallet(self, include_xpubs_and_full_paths=False) return tx def try_detecting_internal_addresses_corruption(self): @@ -1901,7 +1909,7 @@ class Imported_Wallet(Simple_Wallet): def get_txin_type(self, address): return self.db.get_imported_address(address).get('type', 'address') - def _add_input_sig_info(self, txin, address): + def _add_input_sig_info(self, txin, address, *, only_der_suffix=True): if not self.is_mine(address): return if txin.script_type in ('unknown', 'address'): @@ -2013,17 +2021,18 @@ class Deterministic_Wallet(Abstract_Wallet): return {k.derive_pubkey(*der_suffix): (k, der_suffix) for k in self.get_keystores()} - def _add_input_sig_info(self, txin, address): - self._add_txinout_derivation_info(txin, address) + def _add_input_sig_info(self, txin, address, *, only_der_suffix=True): + self._add_txinout_derivation_info(txin, address, only_der_suffix=only_der_suffix) - def _add_txinout_derivation_info(self, txinout, address): + def _add_txinout_derivation_info(self, txinout, address, *, only_der_suffix=True): if not self.is_mine(address): return pubkey_deriv_info = self.get_public_keys_with_deriv_info(address) txinout.pubkeys = sorted([bfh(pk) for pk in list(pubkey_deriv_info)]) for pubkey_hex in pubkey_deriv_info: ks, der_suffix = pubkey_deriv_info[pubkey_hex] - fp_bytes, der_full = ks.get_fp_and_derivation_to_be_used_in_partial_tx(der_suffix) + fp_bytes, der_full = ks.get_fp_and_derivation_to_be_used_in_partial_tx(der_suffix, + only_der_suffix=only_der_suffix) txinout.bip32_paths[bfh(pubkey_hex)] = (fp_bytes, der_full) def create_new_address(self, for_change=False):