From f3acde048f9e33c285dce456e12c7bfdf2719a66 Mon Sep 17 00:00:00 2001 From: PiRK Date: Fri, 19 May 2023 11:43:22 +0200 Subject: [PATCH 1/4] split shuffle_inputs_outputs We want to be able to disable one without necessarily disabling the other. --- electrumabc/transaction.py | 4 +++- electrumabc/wallet.py | 8 +++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/electrumabc/transaction.py b/electrumabc/transaction.py index 7cc0a963a23c..eb312a286ded 100644 --- a/electrumabc/transaction.py +++ b/electrumabc/transaction.py @@ -778,8 +778,10 @@ def serialize_input(self, txin, script, estimate_size=False): s += bitcoin.int_to_hex(txin["value"], 8) return s - def shuffle_inputs_outputs(self): + def shuffle_inputs(self): random.shuffle(self._inputs) + + def shuffle_outputs(self): random.shuffle(self._outputs) def serialize_output(self, output): diff --git a/electrumabc/wallet.py b/electrumabc/wallet.py index 5fbdc6d4cb13..30d8d2d6669e 100644 --- a/electrumabc/wallet.py +++ b/electrumabc/wallet.py @@ -213,7 +213,8 @@ def sweep( tx = Transaction.from_io( inputs, outputs, locktime=locktime, sign_schnorr=sign_schnorr ) - tx.shuffle_inputs_outputs() + tx.shuffle_inputs() + tx.shuffle_outputs() tx.sign(keypairs) return tx @@ -2170,7 +2171,8 @@ def fee_estimator(size): if sats_per_byte > 100: raise ExcessiveFee() - tx.shuffle_inputs_outputs() + tx.shuffle_inputs() + tx.shuffle_outputs() # Timelock tx to current height. locktime = 0 if config.is_current_block_locktime_enabled(): @@ -2449,7 +2451,7 @@ def cpfp(self, tx, fee, sign_schnorr=None, enable_current_block_locktime=True): locktime = 0 if enable_current_block_locktime: locktime = self.get_local_height() - # note: no need to call tx.shuffle_inputs_outputs here - single input/output + # note: no need to shuffle inputs or outputs here - single input/output return Transaction.from_io( inputs, outputs, locktime=locktime, sign_schnorr=sign_schnorr ) From 9d9de274af57ea6ea780da7bb4e368c0d320d6a2 Mon Sep 17 00:00:00 2001 From: PiRK Date: Fri, 19 May 2023 16:03:46 +0200 Subject: [PATCH 2/4] transaction: introduce TxOutput namedtuple Using a namedtuple is an intermediate step before creating a proper class for this. It encapsulate the 3-tuples and makes the code easier to read (attributes rather than indices) backport of https://github.com/spesmilo/electrum/pull/4596 and removal of a bunch of code Transaction methods that are unused. --- electrumabc/address.py | 15 ++++- electrumabc/coinchooser.py | 5 +- electrumabc/commands.py | 6 +- electrumabc/paymentrequest.py | 10 +-- electrumabc/tests/test_transaction.py | 15 +++-- electrumabc/transaction.py | 50 ++++++++------- electrumabc/wallet.py | 63 +++++++++---------- electrumabc_gui/qt/main_window.py | 7 ++- electrumabc_gui/qt/paytoedit.py | 18 +++--- electrumabc_gui/stdio.py | 10 ++- electrumabc_gui/text.py | 10 ++- .../digitalbitbox/digitalbitbox.py | 4 +- electrumabc_plugins/hw_wallet/plugin.py | 21 ++----- electrumabc_plugins/keepkey/keepkey.py | 10 ++- electrumabc_plugins/ledger/ledger.py | 24 ++++--- electrumabc_plugins/trezor/trezor.py | 11 +++- 16 files changed, 154 insertions(+), 125 deletions(-) diff --git a/electrumabc/address.py b/electrumabc/address.py index 7c6f02f2684b..a52e833d0a11 100644 --- a/electrumabc/address.py +++ b/electrumabc/address.py @@ -25,6 +25,7 @@ # Many of the functions in this file are copied from ElectrumX from __future__ import annotations +import abc import hashlib import struct from collections import namedtuple @@ -47,6 +48,14 @@ hex_to_bytes = bytes.fromhex +class DestinationType(abc.ABC): + """Base class for TxOutput destination types""" + + @abc.abstractmethod + def to_ui_string(self) -> str: + pass + + class AddressError(Exception): """Exception used for Address errors.""" @@ -106,7 +115,7 @@ def double_sha256(x): return sha256(sha256(x)) -class UnknownAddress: +class UnknownAddress(DestinationType): def to_ui_string(self): return "" @@ -233,7 +242,7 @@ def __repr__(self): return "".format(self.__str__()) -class ScriptOutput(namedtuple("ScriptAddressTuple", "script")): +class ScriptOutput(namedtuple("ScriptAddressTuple", "script"), DestinationType): @classmethod def from_string(self, string): """Instantiate from a mixture of opcodes and raw data.""" @@ -341,7 +350,7 @@ def protocol_factory(script): # A namedtuple for easy comparison and unique hashing -class Address(namedtuple("AddressTuple", "hash160 kind")): +class Address(namedtuple("AddressTuple", "hash160 kind"), DestinationType): # Address kinds ADDR_P2PKH = 0 ADDR_P2SH = 1 diff --git a/electrumabc/coinchooser.py b/electrumabc/coinchooser.py index 87551786d829..a0ea7b713c9e 100644 --- a/electrumabc/coinchooser.py +++ b/electrumabc/coinchooser.py @@ -28,7 +28,7 @@ from .bitcoin import CASH, TYPE_ADDRESS, sha256 from .printerror import PrintError -from .transaction import Transaction +from .transaction import Transaction, TxOutput from .util import NotEnoughFunds @@ -167,7 +167,8 @@ def change_outputs(self, tx, change_addrs, fee_estimator, dust_threshold): dust = sum(amount for amount in amounts if amount < dust_threshold) amounts = [amount for amount in amounts if amount >= dust_threshold] change = [ - (TYPE_ADDRESS, addr, amount) for addr, amount in zip(change_addrs, amounts) + TxOutput(TYPE_ADDRESS, addr, amount) + for addr, amount in zip(change_addrs, amounts) ] self.print_error("change:", change) if dust: diff --git a/electrumabc/commands.py b/electrumabc/commands.py index f051bb606690..c2221e14103c 100644 --- a/electrumabc/commands.py +++ b/electrumabc/commands.py @@ -45,7 +45,7 @@ from .plugins import run_hook from .printerror import print_error from .simple_config import SimpleConfig -from .transaction import OPReturn, Transaction, multisig_script, tx_from_str +from .transaction import OPReturn, Transaction, TxOutput, multisig_script, tx_from_str from .util import format_satoshis, json_decode, to_bytes from .version import PACKAGE_VERSION from .wallet import create_new_wallet, restore_wallet_from_text @@ -427,7 +427,7 @@ def serialize(self, jsontx): txin["num_sig"] = 1 outputs = [ - (TYPE_ADDRESS, Address.from_string(x["address"]), int(x["value"])) + TxOutput(TYPE_ADDRESS, Address.from_string(x["address"]), int(x["value"])) for x in outputs ] tx = Transaction.from_io( @@ -696,7 +696,7 @@ def _mktx( for address, amount in outputs: address = self._resolver(address) amount = satoshis(amount) - final_outputs.append((TYPE_ADDRESS, address, amount)) + final_outputs.append(TxOutput(TYPE_ADDRESS, address, amount)) coins = self.wallet.get_spendable_coins(domain, self.config) if feerate is not None: diff --git a/electrumabc/paymentrequest.py b/electrumabc/paymentrequest.py index d42686a717aa..c5a35818a9b7 100644 --- a/electrumabc/paymentrequest.py +++ b/electrumabc/paymentrequest.py @@ -51,7 +51,7 @@ from .bitcoin import TYPE_ADDRESS from .constants import PROJECT_NAME, PROJECT_NAME_NO_SPACES, XEC from .printerror import PrintError, print_error -from .transaction import Transaction +from .transaction import Transaction, TxOutput from .util import FileImportFailed, FileImportFailedEncrypted, bfh, bh2u from .version import PACKAGE_VERSION @@ -164,7 +164,7 @@ def parse(self, r): self.outputs = [] for o in self.details.outputs: addr = transaction.get_address_from_output_script(o.script)[1] - self.outputs.append((TYPE_ADDRESS, addr, o.amount)) + self.outputs.append(TxOutput(TYPE_ADDRESS, addr, o.amount)) self.memo = self.details.memo self.payment_url = self.details.payment_url @@ -272,10 +272,10 @@ def get_expiration_date(self): def get_amount(self): return sum(map(lambda x: x[2], self.outputs)) - def get_address(self): + def get_address(self) -> str: o = self.outputs[0] - assert o[0] == TYPE_ADDRESS - return o[1].to_ui_string() + assert o.type == TYPE_ADDRESS + return o.destination.to_ui_string() def get_requestor(self): return self.requestor if self.requestor else self.get_address() diff --git a/electrumabc/tests/test_transaction.py b/electrumabc/tests/test_transaction.py index 41418513ac34..ed9cbbdf844d 100644 --- a/electrumabc/tests/test_transaction.py +++ b/electrumabc/tests/test_transaction.py @@ -113,22 +113,27 @@ def test_tx_unsigned(self): tx.as_dict(), {"hex": unsigned_blob, "complete": False, "final": True} ) self.assertEqual( - tx.get_outputs(), + [(o.destination, o.value) for o in tx.outputs()], [(Address.from_string("1MYXdf4moacvaEKZ57ozerpJ3t9xSeN6LK"), 20112408)], ) self.assertEqual( - tx.get_output_addresses(), + [o.destination for o in tx.outputs()], [Address.from_string("1MYXdf4moacvaEKZ57ozerpJ3t9xSeN6LK")], ) + def tx_has_address(addr: Address) -> bool: + return any(addr == o.destination for o in tx.outputs()) or ( + addr in (inp.get("address") for inp in tx.inputs()) + ) + self.assertTrue( - tx.has_address(Address.from_string("1MYXdf4moacvaEKZ57ozerpJ3t9xSeN6LK")) + tx_has_address(Address.from_string("1MYXdf4moacvaEKZ57ozerpJ3t9xSeN6LK")) ) self.assertTrue( - tx.has_address(Address.from_string("13Vp8Y3hD5Cb6sERfpxePz5vGJizXbWciN")) + tx_has_address(Address.from_string("13Vp8Y3hD5Cb6sERfpxePz5vGJizXbWciN")) ) self.assertFalse( - tx.has_address(Address.from_string("1CQj15y1N7LDHp7wTt28eoD1QhHgFgxECH")) + tx_has_address(Address.from_string("1CQj15y1N7LDHp7wTt28eoD1QhHgFgxECH")) ) self.assertEqual(tx.serialize(), unsigned_blob) diff --git a/electrumabc/transaction.py b/electrumabc/transaction.py index eb312a286ded..307df434821e 100644 --- a/electrumabc/transaction.py +++ b/electrumabc/transaction.py @@ -27,13 +27,14 @@ import random import struct import warnings -from typing import Optional, Tuple, Union +from typing import List, NamedTuple, Optional, Tuple, Union import ecdsa from . import bitcoin, schnorr from .address import ( Address, + DestinationType, P2PKH_prefix, P2PKH_suffix, P2SH_prefix, @@ -68,6 +69,13 @@ class InputValueMissing(ValueError): """thrown when the value of an input is needed but not present""" +class TxOutput(NamedTuple): + type: int + destination: DestinationType + # str when the output is set to max: '!' + value: Union[int, str] + + class BCDataStream(object): def __init__(self): self.input = None @@ -324,7 +332,7 @@ def parse_redeemScript(s): def get_address_from_output_script( _bytes: bytes, -) -> Tuple[int, Union[Address, PublicKey, ScriptOutput]]: +) -> Tuple[int, Union[PublicKey, DestinationType]]: """Return the type of the output and the address""" scriptlen = len(_bytes) @@ -408,7 +416,7 @@ def parse_input(vds): return d -def parse_output(vds, i): +def parse_output(vds: BCDataStream, i: int): d = {} d["value"] = vds.read_int64() scriptPubKey = vds.read_bytes(vds.read_compact_size()) @@ -463,7 +471,7 @@ def __init__(self, raw, sign_schnorr=False): else: raise RuntimeError("cannot initialize transaction", raw) self._inputs = None - self._outputs = None + self._outputs: Optional[List[TxOutput]] = None self.locktime = 0 self.version = 2 self._sign_schnorr = sign_schnorr @@ -511,7 +519,7 @@ def inputs(self): self.deserialize() return self._inputs - def outputs(self): + def outputs(self) -> List[TxOutput]: if self._outputs is None: self.deserialize() return self._outputs @@ -615,7 +623,9 @@ def deserialize(self): d = deserialize(self.raw) self.invalidate_common_sighash_cache() self._inputs = d["inputs"] - self._outputs = [(x["type"], x["address"], x["value"]) for x in d["outputs"]] + self._outputs = [ + TxOutput(x["type"], x["address"], x["value"]) for x in d["outputs"] + ] assert all( isinstance(output[1], (PublicKey, Address, ScriptOutput)) for output in self._outputs @@ -625,7 +635,14 @@ def deserialize(self): return d @classmethod - def from_io(klass, inputs, outputs, locktime=0, sign_schnorr=False, version=None): + def from_io( + klass, + inputs, + outputs: List[TxOutput], + locktime=0, + sign_schnorr=False, + version=None, + ): assert all( isinstance(output[1], (PublicKey, Address, ScriptOutput)) for output in outputs @@ -1143,21 +1160,6 @@ def _sign_txin(self, i, j, sec, compressed, *, use_cache=False): txin["pubkeys"][j] = pubkey # needed for fd keys return txin - def get_outputs(self): - """convert pubkeys to addresses""" - o = [] - for type, addr, v in self.outputs(): - o.append((addr, v)) # consider using yield (addr, v) - return o - - def get_output_addresses(self): - return [addr for addr, val in self.get_outputs()] - - def has_address(self, addr): - return (addr in self.get_output_addresses()) or ( - addr in (tx.get("address") for tx in self.inputs()) - ) - def is_final(self): return not any( [ @@ -1641,7 +1643,7 @@ def output_for_stringdata(op_return): op_return_payload = op_return_encoded.hex() script = op_return_code + op_return_payload amount = 0 - return bitcoin.TYPE_SCRIPT, ScriptOutput.from_string(script), amount + return TxOutput(bitcoin.TYPE_SCRIPT, ScriptOutput.from_string(script), amount) @staticmethod def output_for_rawhex(op_return): @@ -1660,7 +1662,7 @@ def output_for_rawhex(op_return): _("OP_RETURN script too large, needs to be no longer than 223 bytes") ) amount = 0 - return ( + return TxOutput( bitcoin.TYPE_SCRIPT, ScriptOutput.protocol_factory(op_return_script), amount, diff --git a/electrumabc/wallet.py b/electrumabc/wallet.py index 30d8d2d6669e..1d3f6683dc7d 100644 --- a/electrumabc/wallet.py +++ b/electrumabc/wallet.py @@ -89,7 +89,7 @@ from .printerror import PrintError from .storage import STO_EV_PLAINTEXT, STO_EV_USER_PW, STO_EV_XPUB_PW, WalletStorage from .synchronizer import Synchronizer -from .transaction import InputValueMissing, Transaction +from .transaction import InputValueMissing, Transaction, TxOutput from .util import ( ExcessiveFee, InvalidPassword, @@ -191,7 +191,7 @@ def sweep( inputs, keypairs = sweep_preparations(privkeys, network, imax) total = sum(i.get("value") for i in inputs) if fee is None: - outputs = [(bitcoin.TYPE_ADDRESS, recipient, total)] + outputs = [TxOutput(bitcoin.TYPE_ADDRESS, recipient, total)] tx = Transaction.from_io(inputs, outputs, sign_schnorr=sign_schnorr) fee = config.estimate_fee(tx.estimated_size()) if total - fee < 0: @@ -205,7 +205,7 @@ def sweep( + f"\nTotal: {total} satoshis\nFee: {fee}\nDust Threshold: {DUST_THRESHOLD}" ) - outputs = [(bitcoin.TYPE_ADDRESS, recipient, total - fee)] + outputs = [TxOutput(bitcoin.TYPE_ADDRESS, recipient, total - fee)] locktime = 0 if config.is_current_block_locktime_enabled(): locktime = network.get_local_height() @@ -802,10 +802,10 @@ def _get_wallet_delta(self, tx, *, ver=1) -> Union[WalletDelta, WalletDelta2]: is_partial = True if not is_mine: is_partial = False - for _type, addr, value in tx.outputs(): - v_out += value - if self.is_mine(addr): - v_out_mine += value + for txo in tx.outputs(): + v_out += txo.value + if self.is_mine(txo.destination): + v_out_mine += txo.value is_relevant = True if is_pruned: # some inputs are mine: @@ -1483,22 +1483,21 @@ def pop_pruned_txo(ser): self.txo[tx_hash] = d = {} for n, txo in enumerate(tx.outputs()): ser = tx_hash + ":%d" % n - _type, addr, v = txo mine = False - if self.is_mine(addr): + if self.is_mine(txo.destination): # add coin to self.txo since it's mine. mine = True - coins = d.get(addr) + coins = d.get(txo.destination) if coins is None: - d[addr] = coins = [] - coins.append((n, v, is_coinbase)) + d[txo.destination] = coins = [] + coins.append((n, txo.value, is_coinbase)) del coins # invalidate cache entry - self._addr_bal_cache.pop(addr, None) - # give v to txi that spends me + self._addr_bal_cache.pop(txo.destination, None) + # give value to txi that spends me next_tx = pop_pruned_txo(ser) if next_tx is not None and mine: - add_to_self_txi(next_tx, addr, ser, v) + add_to_self_txi(next_tx, txo.destination, ser, txo.value) # don't keep empty entries in self.txo if not d: self.txo.pop(tx_hash, None) @@ -1877,8 +1876,8 @@ def fmt_amt(v, is_diff): if addr is None: continue input_addresses.append(addr.to_ui_string()) - for _type, addr, v in tx.outputs(): - output_addresses.append(addr.to_ui_string()) + for txo in tx.outputs(): + output_addresses.append(txo.destination.to_ui_string()) item["input_addresses"] = input_addresses item["output_addresses"] = output_addresses if fx is not None: @@ -2053,7 +2052,7 @@ def get_default_change_addresses(self, count): def make_unsigned_transaction( self, inputs, - outputs, + outputs: List[TxOutput], config: SimpleConfig, fixed_fee=None, change_addr=None, @@ -2067,9 +2066,8 @@ def make_unsigned_transaction( ) # check outputs i_max = None - for i, o in enumerate(outputs): - _type, data, value = o - if value == "!": + for i, txo in enumerate(outputs): + if txo.value == "!": if i_max is not None: raise RuntimeError("More than one output set to spend max") i_max = i @@ -2156,12 +2154,11 @@ def fee_estimator(size): ) else: sendable = sum(map(lambda x: x["value"], inputs)) - _type, data, value = outputs[i_max] - outputs[i_max] = (_type, data, 0) + outputs[i_max] = outputs[i_max]._replace(value=0) tx = Transaction.from_io(inputs, outputs, sign_schnorr=sign_schnorr) fee = fee_estimator(tx.estimated_size()) amount = max(0, sendable - tx.output_value() - fee) - outputs[i_max] = (_type, data, amount) + outputs[i_max] = outputs[i_max]._replace(value=amount) tx = Transaction.from_io(inputs, outputs, sign_schnorr=sign_schnorr) # If user tries to send too big of a fee (more than 50 sat/byte), stop them from shooting themselves in the foot @@ -2435,19 +2432,18 @@ def cpfp(self, tx, fee, sign_schnorr=None, enable_current_block_locktime=True): self.is_schnorr_enabled() if sign_schnorr is None else bool(sign_schnorr) ) txid = tx.txid() - for i, o in enumerate(tx.outputs()): - otype, address, value = o - if otype == bitcoin.TYPE_ADDRESS and self.is_mine(address): + for i, txo in enumerate(tx.outputs()): + if txo.type == bitcoin.TYPE_ADDRESS and self.is_mine(txo.destination): break else: return - coins = self.get_addr_utxo(address) + coins = self.get_addr_utxo(txo.destination) item = coins.get(txid + ":%d" % i) if not item: return self.add_input_info(item) inputs = [item] - outputs = [(bitcoin.TYPE_ADDRESS, address, value - fee)] + outputs = [TxOutput(bitcoin.TYPE_ADDRESS, txo.destination, txo.value - fee)] locktime = 0 if enable_current_block_locktime: locktime = self.get_local_height() @@ -2526,13 +2522,12 @@ def add_hw_info(self, tx): info = {} xpubs = self.get_master_public_keys() for txout in tx.outputs(): - _type, addr, amount = txout - if self.is_change(addr): - index = self.get_address_index(addr) - pubkeys = self.get_public_keys(addr) + if self.is_change(txout.destination): + index = self.get_address_index(txout.destination) + pubkeys = self.get_public_keys(txout.destination) # sort xpubs using the order of pubkeys sorted_pubkeys, sorted_xpubs = zip(*sorted(zip(pubkeys, xpubs))) - info[addr] = ( + info[txout.destination] = ( index, sorted_xpubs, self.m if isinstance(self, MultisigWallet) else None, diff --git a/electrumabc_gui/qt/main_window.py b/electrumabc_gui/qt/main_window.py index 0fe710b6dc41..4d3590de363d 100644 --- a/electrumabc_gui/qt/main_window.py +++ b/electrumabc_gui/qt/main_window.py @@ -66,6 +66,7 @@ OPReturn, SerializationError, Transaction, + TxOutput, tx_from_str, ) from electrumabc.util import ( @@ -2203,7 +2204,7 @@ def do_update_fee(self): outputs = self.payto_e.get_outputs(self.max_button.isChecked()) if not outputs: _type, addr = self.get_payto_or_dummy() - outputs = [(_type, addr, amount)] + outputs = [TxOutput(_type, addr, amount)] try: opreturn_message = ( self.message_opreturn_e.text() @@ -2446,8 +2447,8 @@ def read_send_tab(self): self.show_error(_("No outputs")) return - for _type, addr, amount in outputs: - if amount is None: + for o in outputs: + if o.value is None: self.show_error(_("Invalid Amount")) return diff --git a/electrumabc_gui/qt/paytoedit.py b/electrumabc_gui/qt/paytoedit.py index e614d4cffb30..7447dc86cc04 100644 --- a/electrumabc_gui/qt/paytoedit.py +++ b/electrumabc_gui/qt/paytoedit.py @@ -27,6 +27,7 @@ import re import sys from decimal import Decimal as PyDecimal # Qt 5.12 also exports Decimal +from typing import List from PyQt5 import QtWidgets from PyQt5.QtCore import Qt @@ -36,6 +37,7 @@ from electrumabc.address import Address, AddressError, ScriptOutput from electrumabc.contacts import Contact from electrumabc.printerror import PrintError +from electrumabc.transaction import TxOutput from . import util from .qrtextedit import ScanQRTextEdit @@ -121,11 +123,11 @@ def setExpired(self): self._original_style_sheet + util.ColorScheme.RED.as_stylesheet(True) ) - def parse_address_and_amount(self, line): + def parse_address_and_amount(self, line) -> TxOutput: x, y = line.split(",") out_type, out = self.parse_output(x) amount = self.parse_amount(y) - return out_type, out, amount + return TxOutput(out_type, out, amount) @classmethod def parse_output(cls, x): @@ -176,16 +178,16 @@ def check_text(self): is_max = False for i, line in enumerate(lines): try: - _type, to_address, amount = self.parse_address_and_amount(line) + output = self.parse_address_and_amount(line) except (AddressError, ArithmeticError, ValueError): self.errors.append((i, line.strip())) continue - outputs.append((_type, to_address, amount)) - if amount == "!": + outputs.append(output) + if output.value == "!": is_max = True else: - total += amount + total += output.value self.win.max_button.setChecked(is_max) self.outputs = outputs @@ -203,7 +205,7 @@ def get_errors(self): def get_recipient(self): return self.payto_address - def get_outputs(self, is_max): + def get_outputs(self, is_max) -> List[TxOutput]: if self.payto_address: if is_max: amount = "!" @@ -211,7 +213,7 @@ def get_outputs(self, is_max): amount = self.amount_edit.get_amount() _type, addr = self.payto_address - self.outputs = [(_type, addr, amount)] + self.outputs = [TxOutput(_type, addr, amount)] return self.outputs[:] diff --git a/electrumabc_gui/stdio.py b/electrumabc_gui/stdio.py index e6b5b3b606c7..ad34b2a6f6bd 100644 --- a/electrumabc_gui/stdio.py +++ b/electrumabc_gui/stdio.py @@ -7,6 +7,7 @@ from electrumabc.constants import SCRIPT_NAME from electrumabc.printerror import set_verbosity from electrumabc.storage import WalletStorage +from electrumabc.transaction import TxOutput from electrumabc.util import format_satoshis from electrumabc.wallet import Wallet @@ -255,7 +256,14 @@ def do_send(self): try: tx = self.wallet.mktx( - [(TYPE_ADDRESS, self.str_recipient, amount)], password, self.config, fee + [ + TxOutput( + TYPE_ADDRESS, Address.from_string(self.str_recipient), amount + ) + ], + password, + self.config, + fee, ) except Exception as e: print(str(e)) diff --git a/electrumabc_gui/text.py b/electrumabc_gui/text.py index 1092b5e3838e..1e3fd9d51a07 100644 --- a/electrumabc_gui/text.py +++ b/electrumabc_gui/text.py @@ -12,6 +12,7 @@ from electrumabc.constants import SCRIPT_NAME from electrumabc.printerror import set_verbosity from electrumabc.storage import WalletStorage +from electrumabc.transaction import TxOutput from electrumabc.util import format_satoshis from electrumabc.wallet import Wallet @@ -436,7 +437,14 @@ def do_send(self): password = None try: tx = self.wallet.mktx( - [(TYPE_ADDRESS, self.str_recipient, amount)], password, self.config, fee + [ + TxOutput( + TYPE_ADDRESS, Address.from_string(self.str_recipient), amount + ) + ], + password, + self.config, + fee, ) except Exception as e: self.show_message(str(e)) diff --git a/electrumabc_plugins/digitalbitbox/digitalbitbox.py b/electrumabc_plugins/digitalbitbox/digitalbitbox.py index eeb9069390b3..87e0a19e8185 100644 --- a/electrumabc_plugins/digitalbitbox/digitalbitbox.py +++ b/electrumabc_plugins/digitalbitbox/digitalbitbox.py @@ -655,8 +655,8 @@ def sign_transaction(self, tx, password, *, use_cache=False): ) # should never happen # Build pubkeyarray from outputs - for _type, address, amount in tx.outputs(): - info = tx.output_info.get(address) + for o in tx.outputs(): + info = tx.output_info.get(o.destination) if info is not None: index, xpubs, m, script_type = info changePath = self.get_derivation() + "/%d/%d" % index diff --git a/electrumabc_plugins/hw_wallet/plugin.py b/electrumabc_plugins/hw_wallet/plugin.py index 75d1af48e4ab..fc6655fba785 100644 --- a/electrumabc_plugins/hw_wallet/plugin.py +++ b/electrumabc_plugins/hw_wallet/plugin.py @@ -28,8 +28,7 @@ from typing import TYPE_CHECKING, Any, Iterable, Optional, Sequence, Type -from electrumabc.address import OpCodes, Script -from electrumabc.bitcoin import TYPE_SCRIPT +from electrumabc.address import OpCodes, Script, ScriptOutput from electrumabc.i18n import _, ngettext from electrumabc.plugins import BasePlugin, Device, DeviceInfo, DeviceMgr, hook from electrumabc.transaction import Transaction @@ -262,16 +261,13 @@ def is_any_tx_output_on_change_branch(tx: Transaction) -> bool: # will return address.script[2:] (everyting after the first OP_RETURN & PUSH bytes) def validate_op_return_output_and_get_data( - # tuple(typ, 'address', amount) - output: tuple, + script_output: ScriptOutput, # in bytes max_size: int = 220, # number of pushes supported after the OP_RETURN, most HW wallets support only 1 # push, some more than 1. Specify None to omit the number-of-pushes check. max_pushes: int = 1, ) -> bytes: - _type, address, _amount = output - if max_pushes is None: # Caller says "no limit", so just to keep the below code simple, we # do this and effectively sets the limit on pushes to "unlimited", @@ -280,10 +276,7 @@ def validate_op_return_output_and_get_data( assert max_pushes >= 1 - if _type != TYPE_SCRIPT: - raise Exception("Unexpected output type: {}".format(_type)) - - ops = Script.get_ops(address.script) + ops = Script.get_ops(script_output.script) num_pushes = len(ops) - 1 @@ -303,18 +296,14 @@ def validate_op_return_output_and_get_data( ).format(max_pushes=max_pushes) ) - data = address.script[ - 2: - ] # caller expects everything after the OP_RETURN and PUSHDATA op + # caller expects everything after the OP_RETURN and PUSHDATA op + data = script_output.script[2:] if len(data) > max_size: raise RuntimeError( _("OP_RETURN data size exceeds the maximum of {} bytes.".format(max_size)) ) - if _amount != 0: - raise RuntimeError(_("Amount for OP_RETURN output must be zero.")) - return data diff --git a/electrumabc_plugins/keepkey/keepkey.py b/electrumabc_plugins/keepkey/keepkey.py index 82d7591f6d20..4869be584ed6 100644 --- a/electrumabc_plugins/keepkey/keepkey.py +++ b/electrumabc_plugins/keepkey/keepkey.py @@ -1,4 +1,5 @@ from binascii import unhexlify +from typing import TYPE_CHECKING from electrumabc import networks from electrumabc.address import Address @@ -21,6 +22,9 @@ validate_op_return_output_and_get_data, ) +if TYPE_CHECKING: + from electrumabc.transaction import Transaction + # TREZOR initialization methods TIM_NEW, TIM_RECOVER, TIM_MNEMONIC, TIM_PRIVKEY = range(0, 4) @@ -491,7 +495,7 @@ def f(x_pubkey): return inputs - def tx_outputs(self, derivation, tx): + def tx_outputs(self, derivation, tx: Transaction): def create_output_by_derivation(): keepkey_script_type = self.get_keepkey_output_script_type(script_type) if len(xpubs) == 1: @@ -523,7 +527,7 @@ def create_output_by_address(): if _type == TYPE_SCRIPT: txoutputtype.script_type = self.types.PAYTOOPRETURN txoutputtype.op_return_data = validate_op_return_output_and_get_data( - o, max_pushes=1 + o.destination, max_pushes=1 ) elif _type == TYPE_ADDRESS: txoutputtype.script_type = self.types.PAYTOADDRESS @@ -537,7 +541,7 @@ def create_output_by_address(): any_output_on_change_branch = is_any_tx_output_on_change_branch(tx) for o in tx.outputs(): - _type, address, amount = o + _type, address, amount = o.type, o.destination, o.value use_create_by_derivation = False info = tx.output_info.get(address) diff --git a/electrumabc_plugins/ledger/ledger.py b/electrumabc_plugins/ledger/ledger.py index 2ea84c777bb1..6bc16ffda772 100644 --- a/electrumabc_plugins/ledger/ledger.py +++ b/electrumabc_plugins/ledger/ledger.py @@ -448,7 +448,7 @@ def sign_message(self, sequence, message, password, sigtype=SignatureType.BITCOI @test_pin_unlocked @set_and_unset_signing - def sign_transaction(self, tx, password, *, use_cache=False): + def sign_transaction(self, tx: Transaction, password, *, use_cache=False): if tx.is_complete(): return inputs = [] @@ -516,9 +516,8 @@ def sign_transaction(self, tx, password, *, use_cache=False): txOutput = var_int(len(tx.outputs())) for txout in tx.outputs(): - output_type, addr, amount = txout - txOutput += int_to_hex(amount, 8) - script = tx.pay_script(addr) + txOutput += int_to_hex(txout.value, 8) + script = tx.pay_script(txout.destination) txOutput += var_int(len(script) // 2) txOutput += script txOutput = bfh(txOutput) @@ -531,22 +530,21 @@ def sign_transaction(self, tx, password, *, use_cache=False): ).format(self.device) ) for o in tx.outputs(): - _type, address, amount = o if client_electrum.is_hw1(): - if not _type == TYPE_ADDRESS: + if not o.type == TYPE_ADDRESS: self.give_error( _("Only address outputs are supported by {}").format( self.device ) ) else: - if _type not in [TYPE_ADDRESS, TYPE_SCRIPT]: + if o.type not in [TYPE_ADDRESS, TYPE_SCRIPT]: self.give_error( _("Only address and script outputs are supported by {}").format( self.device ) ) - if _type == TYPE_SCRIPT: + if o.type == TYPE_SCRIPT: try: # Ledger has a maximum output size of 200 bytes: # https://github.com/LedgerHQ/ledger-app-btc/commit/3a78dee9c0484821df58975803e40d58fbfc2c38#diff-c61ccd96a6d8b54d48f54a3bc4dfa7e2R26 @@ -555,7 +553,7 @@ def sign_transaction(self, tx, password, *, use_cache=False): # max_pushes, so we specify max_pushes=None so as # to bypass that check. validate_op_return_output_and_get_data( - o, max_size=187, max_pushes=None + o.destination, max_size=187, max_pushes=None ) except RuntimeError as e: self.give_error("{}: {}".format(self.device, str(e))) @@ -572,8 +570,8 @@ def sign_transaction(self, tx, password, *, use_cache=False): ) has_change = False any_output_on_change_branch = is_any_tx_output_on_change_branch(tx) - for _type, address, amount in tx.outputs(): - info = tx.output_info.get(address) + for o in tx.outputs(): + info = tx.output_info.get(o.destination) if (info is not None) and len(tx.outputs()) > 1 and not has_change: index, xpubs, m, script_type = info on_change_branch = index[0] == 1 @@ -585,9 +583,9 @@ def sign_transaction(self, tx, password, *, use_cache=False): ) has_change = True else: - output = address + output = o.destination else: - output = address + output = o.destination try: # Get trusted inputs from the original transactions diff --git a/electrumabc_plugins/trezor/trezor.py b/electrumabc_plugins/trezor/trezor.py index 38cddc3fe5dd..54798317fa19 100644 --- a/electrumabc_plugins/trezor/trezor.py +++ b/electrumabc_plugins/trezor/trezor.py @@ -1,6 +1,9 @@ +from __future__ import annotations + import sys import traceback from binascii import unhexlify +from typing import TYPE_CHECKING from electrumabc.base_wizard import HWD_SETUP_NEW_WALLET from electrumabc.bitcoin import ( @@ -19,6 +22,9 @@ from ..hw_wallet import HWPluginBase +if TYPE_CHECKING: + from electrumabc.transaction import Transaction + try: import trezorlib import trezorlib.transport @@ -492,7 +498,7 @@ def _make_multisig(self, m, xpubs, signatures=None): return MultisigRedeemScriptType(pubkeys=pubkeys, signatures=signatures, m=m) - def tx_outputs(self, derivation, tx, client): + def tx_outputs(self, derivation, tx: Transaction, client): def create_output_by_derivation(): deriv = parse_path("/%d/%d" % index) multisig = self._make_multisig(m, [(xpub, deriv) for xpub in xpubs]) @@ -552,7 +558,8 @@ def create_output_by_address(): has_change = False any_output_on_change_branch = self.is_any_tx_output_on_change_branch(tx) - for _type, address, amount in tx.outputs(): + for o in tx.outputs(): + _type, address, amount = o.type, o.destination, o.value use_create_by_derivation = False info = tx.output_info.get(address) if info is not None and not has_change: From 3c1ed6a3c26ed27f673bed0569268f3d0f80af11 Mon Sep 17 00:00:00 2001 From: PiRK Date: Fri, 19 May 2023 16:22:34 +0200 Subject: [PATCH 3/4] alway put op_return as the first output There are use cases that require the op_return to be the first output in a transaction (e.g. SLP), and I'm not aware of any use case that requires it to not be in that position. --- electrumabc/transaction.py | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/electrumabc/transaction.py b/electrumabc/transaction.py index 307df434821e..c5d6f5ab7a01 100644 --- a/electrumabc/transaction.py +++ b/electrumabc/transaction.py @@ -44,6 +44,7 @@ ScriptOutput, UnknownAddress, ) +from .bitcoin import TYPE_SCRIPT from .bitcoin import OpCodes as opcodes from .caches import ExpiringCache from .constants import DEFAULT_TXIN_SEQUENCE @@ -75,6 +76,15 @@ class TxOutput(NamedTuple): # str when the output is set to max: '!' value: Union[int, str] + def is_opreturn(self) -> bool: + if not self.type == TYPE_SCRIPT or not isinstance( + self.destination, ScriptOutput + ): + return False + + ops = Script.get_ops(self.destination.script) + return len(ops) >= 1 and ops[0][0] == opcodes.OP_RETURN + class BCDataStream(object): def __init__(self): @@ -799,7 +809,16 @@ def shuffle_inputs(self): random.shuffle(self._inputs) def shuffle_outputs(self): - random.shuffle(self._outputs) + op_returns = [] + other_outputs = [] + for txo in self._outputs: + if txo.is_opreturn(): + op_returns.append(txo) + else: + other_outputs.append(txo) + + random.shuffle(other_outputs) + self._outputs = op_returns + other_outputs def serialize_output(self, output): output_type, addr, amount = output From 22b5d084d3f691f7dc56b6c5fc61783e17cb15f4 Mon Sep 17 00:00:00 2001 From: PiRK Date: Fri, 19 May 2023 16:36:33 +0200 Subject: [PATCH 4/4] add a checkbox to disable shuffling the outputs when using op_return The intended use case for this is sending SLP token using a hex encoded op_return payload. Test plan: Check that the new checkbox is only enabled if something is typed into the op_return payload field, and disabled when this payload is deleted. Add multiple outputs in the send tab, leave the op_return field blank. Click "Preview" multiple times and make sure the outputs are shuffled (no matter if the new checkbox is checked or not). Type something in the op_return field, and repeat the above test. Now the shuffling behavior must depend on the checkbox. The op_return output should always be first. The other outputs should be shuffled if the checkbox is checked. If it is unchecked, then the change output must be last, and preceded by the other outputs in the specified order. Click send and check the order in a block explorer. --- electrumabc/transaction.py | 8 +++++--- electrumabc/wallet.py | 6 ++++-- electrumabc_gui/qt/main_window.py | 30 +++++++++++++++++++++++++++++- 3 files changed, 38 insertions(+), 6 deletions(-) diff --git a/electrumabc/transaction.py b/electrumabc/transaction.py index c5d6f5ab7a01..5f0285c5972e 100644 --- a/electrumabc/transaction.py +++ b/electrumabc/transaction.py @@ -808,7 +808,9 @@ def serialize_input(self, txin, script, estimate_size=False): def shuffle_inputs(self): random.shuffle(self._inputs) - def shuffle_outputs(self): + def sort_outputs(self, shuffle: bool = True): + """Put the op_return output first, and then shuffle the other outputs unless + this behavior is explicitely disabled.""" op_returns = [] other_outputs = [] for txo in self._outputs: @@ -816,8 +818,8 @@ def shuffle_outputs(self): op_returns.append(txo) else: other_outputs.append(txo) - - random.shuffle(other_outputs) + if shuffle: + random.shuffle(other_outputs) self._outputs = op_returns + other_outputs def serialize_output(self, output): diff --git a/electrumabc/wallet.py b/electrumabc/wallet.py index 1d3f6683dc7d..6a62d0c9e3d5 100644 --- a/electrumabc/wallet.py +++ b/electrumabc/wallet.py @@ -214,7 +214,7 @@ def sweep( inputs, outputs, locktime=locktime, sign_schnorr=sign_schnorr ) tx.shuffle_inputs() - tx.shuffle_outputs() + tx.sort_outputs() tx.sign(keypairs) return tx @@ -2057,6 +2057,7 @@ def make_unsigned_transaction( fixed_fee=None, change_addr=None, sign_schnorr=None, + shuffle_outputs=True, ): """sign_schnorr flag controls whether to mark the tx as signing with schnorr or not. Specify either a bool, or set the flag to 'None' to use @@ -2169,7 +2170,8 @@ def fee_estimator(size): raise ExcessiveFee() tx.shuffle_inputs() - tx.shuffle_outputs() + tx.sort_outputs(shuffle=shuffle_outputs) + # Timelock tx to current height. locktime = 0 if config.is_current_block_locktime_enabled(): diff --git a/electrumabc_gui/qt/main_window.py b/electrumabc_gui/qt/main_window.py index 4d3590de363d..2a152bac4432 100644 --- a/electrumabc_gui/qt/main_window.py +++ b/electrumabc_gui/qt/main_window.py @@ -1944,11 +1944,30 @@ def create_send_tab(self): ) ) hbox.addWidget(self.opreturn_rawhex_cb) + self.opreturn_shuffle_outputs_cb = QtWidgets.QCheckBox(_("Shuffle outputs")) + self.opreturn_shuffle_outputs_cb.setChecked(True) + self.opreturn_shuffle_outputs_cb.setEnabled( + self.message_opreturn_e.text() != "" + ) + self.opreturn_shuffle_outputs_cb.setToolTip( + _( + "

For some OP_RETURN use cases such as SLP, the order of the outputs" + " in the transaction matters, so you might want to uncheck this. By" + " default, outputs are shuffled for privacy reasons. This setting is " + "ignored if the OP_RETURN data is empty.

" + ) + ) + hbox.addWidget(self.opreturn_shuffle_outputs_cb) grid.addLayout(hbox, 3, 1, 1, -1) + self.message_opreturn_e.textChanged.connect( + lambda text: self.opreturn_shuffle_outputs_cb.setEnabled(bool(text)) + ) + self.send_tab_opreturn_widgets = [ self.message_opreturn_e, self.opreturn_rawhex_cb, + self.opreturn_shuffle_outputs_cb, self.opreturn_label, ] @@ -2573,8 +2592,17 @@ def do_send(self, preview=False): if not r: return outputs, fee, tx_desc, coins = r + shuffle_outputs = True + if ( + self.message_opreturn_e.isVisible() + and self.message_opreturn_e.text() + and not self.opreturn_shuffle_outputs_cb.isChecked() + ): + shuffle_outputs = False try: - tx = self.wallet.make_unsigned_transaction(coins, outputs, self.config, fee) + tx = self.wallet.make_unsigned_transaction( + coins, outputs, self.config, fee, shuffle_outputs=shuffle_outputs + ) except NotEnoughFunds: self.show_message(_("Insufficient funds")) return