diff --git a/core/.changelog.d/4351.added b/core/.changelog.d/4351.added new file mode 100644 index 0000000000..72b264b78e --- /dev/null +++ b/core/.changelog.d/4351.added @@ -0,0 +1 @@ +Added support for lexicographic sorting of pubkeys in multisig. diff --git a/core/src/apps/bitcoin/multisig.py b/core/src/apps/bitcoin/multisig.py index 8ac1b7a1e5..205fccc392 100644 --- a/core/src/apps/bitcoin/multisig.py +++ b/core/src/apps/bitcoin/multisig.py @@ -1,5 +1,6 @@ from typing import TYPE_CHECKING +from trezor.enums import MultisigPubkeysOrder from trezor.wire import DataError if TYPE_CHECKING: @@ -28,11 +29,14 @@ def multisig_fingerprint(multisig: MultisigRedeemScriptType) -> bytes: if len(d.public_key) != 33 or len(d.chain_code) != 32: raise DataError("Invalid multisig parameters") - pubnodes = sorted(pubnodes, key=lambda n: n.public_key + n.chain_code) + if multisig.pubkeys_order == MultisigPubkeysOrder.LEXICOGRAPHIC: + # If the order of pubkeys is lexicographic, we don't want the fingerprint to depend on the order of the pubnodes, so we sort the pubnodes before hashing. + pubnodes.sort(key=lambda n: n.public_key + n.chain_code) h = HashWriter(sha256()) write_uint32(h, m) write_uint32(h, n) + write_uint32(h, multisig.pubkeys_order) for d in pubnodes: write_uint32(h, d.depth) write_uint32(h, d.fingerprint) @@ -84,9 +88,14 @@ def multisig_get_pubkey(n: HDNodeType, p: paths.Bip32Path) -> bytes: def multisig_get_pubkeys(multisig: MultisigRedeemScriptType) -> list[bytes]: validate_multisig(multisig) if multisig.nodes: - return [multisig_get_pubkey(hd, multisig.address_n) for hd in multisig.nodes] + pubkeys = [multisig_get_pubkey(hd, multisig.address_n) for hd in multisig.nodes] else: - return [multisig_get_pubkey(hd.node, hd.address_n) for hd in multisig.pubkeys] + pubkeys = [ + multisig_get_pubkey(hd.node, hd.address_n) for hd in multisig.pubkeys + ] + if multisig.pubkeys_order == MultisigPubkeysOrder.LEXICOGRAPHIC: + pubkeys.sort() + return pubkeys def multisig_get_pubkey_count(multisig: MultisigRedeemScriptType) -> int: diff --git a/core/tests/test_apps.bitcoin.change_detector.py b/core/tests/test_apps.bitcoin.change_detector.py index d262d146b5..7eec98f791 100644 --- a/core/tests/test_apps.bitcoin.change_detector.py +++ b/core/tests/test_apps.bitcoin.change_detector.py @@ -1,7 +1,8 @@ from common import H_, await_result, unittest # isort:skip from ubinascii import hexlify, unhexlify -from trezor.enums import InputScriptType, OutputScriptType +from trezor.enums import InputScriptType, OutputScriptType, MultisigPubkeysOrder +from trezor.enums.MultisigPubkeysOrder import LEXICOGRAPHIC, PRESERVED from trezor.messages import ( TxInput, TxOutput, @@ -39,12 +40,13 @@ xpub3 = HDNodeType( ) -def get_multisig(path: list[int], xpubs: list[HDNodeType]) -> MultisigRedeemScriptType: +def get_multisig(path: list[int], xpubs: list[HDNodeType], pubkeys_order: MultisigPubkeysOrder) -> MultisigRedeemScriptType: return MultisigRedeemScriptType( nodes=xpubs, signatures=b"" * len(xpubs), address_n=path[-2:], m=2, + pubkeys_order=pubkeys_order, ) @@ -58,23 +60,23 @@ def get_singlesig_input(path: list[int]) -> TxInput: ) -def get_multisig_input(path: list[int], xpubs: list[HDNodeType]) -> TxInput: +def get_multisig_input(path: list[int], xpubs: list[HDNodeType], pubkeys_order: MultisigPubkeysOrder) -> TxInput: return TxInput( address_n=path, amount=1_000_000, prev_hash=bytes(32), prev_index=0, script_type=InputScriptType.SPENDMULTISIG, - multisig=get_multisig(path, xpubs), + multisig=get_multisig(path, xpubs, pubkeys_order), ) -def get_internal_multisig_output(path: list[int], xpubs: list[HDNodeType]) -> TxOutput: +def get_internal_multisig_output(path: list[int], xpubs: list[HDNodeType], pubkeys_order: MultisigPubkeysOrder) -> TxOutput: return TxOutput( address_n=path, amount=1_000_000, script_type=OutputScriptType.PAYTOMULTISIG, - multisig=get_multisig(path, xpubs), + multisig=get_multisig(path, xpubs, pubkeys_order), ) @@ -115,7 +117,8 @@ class TestChangeDetector(unittest.TestCase): assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 1, 0, 0])) == False # Multisig instead of singlesig - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1])) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1], PRESERVED)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1], LEXICOGRAPHIC)) == False # External output assert self.d.output_is_change(get_external_singlesig_output()) == False @@ -130,23 +133,24 @@ class TestChangeDetector(unittest.TestCase): assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 1, 0, 0])) == False # Multisig instead of singlesig - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1])) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 1, 0, 0], [xpub1], PRESERVED)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 1, 0, 0], [xpub1], LEXICOGRAPHIC)) == False # External output assert self.d.output_is_change(get_external_singlesig_output()) == False - def test_multisig(self): + def test_unsorted_multisig(self): # Different change and account index - self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2])) - self.d.add_input(get_multisig_input([H_(45), 0, 0, 1], [xpub1, xpub2])) - self.d.add_input(get_multisig_input([H_(45), 0, 1, 0], [xpub1, xpub2])) - self.d.add_input(get_multisig_input([H_(45), 0, 1, 1], [xpub1, xpub2])) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 1], [xpub1, xpub2], PRESERVED)) + self.d.add_input(get_multisig_input([H_(45), 0, 1, 0], [xpub1, xpub2], PRESERVED)) + self.d.add_input(get_multisig_input([H_(45), 0, 1, 1], [xpub1, xpub2], PRESERVED)) # Same outputs as inputs - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2])) == True - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 1], [xpub1, xpub2])) == True - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 1, 0], [xpub1, xpub2])) == True - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 1, 1], [xpub1, xpub2])) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 1], [xpub1, xpub2], PRESERVED)) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 1, 0], [xpub1, xpub2], PRESERVED)) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 1, 1], [xpub1, xpub2], PRESERVED)) == True # Singlesig instead of multisig assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False @@ -155,22 +159,60 @@ class TestChangeDetector(unittest.TestCase): assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 1, 0, 0])) == False # Different order of xpubs - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1])) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1], PRESERVED)) == False + + # Sorted instead of unsorted + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) == False # Different xpubs - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3])) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3], PRESERVED)) == False # External output assert self.d.output_is_change(get_external_singlesig_output()) == False - def test_multisig_different_xpubs_order(self): + def test_sorted_multisig(self): + # Different change and account index + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 1], [xpub1, xpub2], LEXICOGRAPHIC)) + self.d.add_input(get_multisig_input([H_(45), 0, 1, 0], [xpub1, xpub2], LEXICOGRAPHIC)) + self.d.add_input(get_multisig_input([H_(45), 0, 1, 1], [xpub1, xpub2], LEXICOGRAPHIC)) + + # Same outputs as inputs + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 1], [xpub1, xpub2], LEXICOGRAPHIC)) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 1, 0], [xpub1, xpub2], LEXICOGRAPHIC)) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 1, 1], [xpub1, xpub2], LEXICOGRAPHIC)) == True + + # Singlesig instead of multisig + assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False + + # Different account index + assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 1, 0, 0])) == False + # Different order of xpubs - self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2])) - self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub2, xpub1])) + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1], LEXICOGRAPHIC)) == True + + # Unsorted instead of sorted + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) == False + + # Different xpubs + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3], LEXICOGRAPHIC)) == False + + # External output + assert self.d.output_is_change(get_external_singlesig_output()) == False + + def test_unsorted_multisig_different_xpubs_order(self): + # Different order of xpubs + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub2, xpub1], PRESERVED)) # Same ouputs as inputs - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2])) == True - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1])) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1], PRESERVED)) == False + + # Sorted instead of unsorted + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1], LEXICOGRAPHIC)) == False # Singlesig instead of multisig assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False @@ -178,14 +220,70 @@ class TestChangeDetector(unittest.TestCase): # External output assert self.d.output_is_change(get_external_singlesig_output()) == False - def test_multisig_different_xpubs(self): - # Different xpubs - self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2])) - self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub3])) + def test_sorted_multisig_different_xpubs_order(self): + # Different order of xpubs + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub2, xpub1], LEXICOGRAPHIC)) # Same ouputs as inputs - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2])) == False - assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3])) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) == True + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1], LEXICOGRAPHIC)) == True + + # Sorted instead of unsorted + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub2, xpub1], PRESERVED)) == False + + # Singlesig instead of multisig + assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False + + # External output + assert self.d.output_is_change(get_external_singlesig_output()) == False + + def test_unsorted_multisig_different_xpubs(self): + # Different xpubs + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub3], PRESERVED)) + + # Same ouputs as inputs + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3], PRESERVED)) == False + + # Sorted instead of unsorted + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3], LEXICOGRAPHIC)) == False + + # Singlesig instead of multisig + assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False + + # External output + assert self.d.output_is_change(get_external_singlesig_output()) == False + + def test_sorted_multisig_different_xpubs(self): + # Different xpubs + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1, xpub3], LEXICOGRAPHIC)) + + # Same ouputs as inputs + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], LEXICOGRAPHIC)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3], LEXICOGRAPHIC)) == False + + # Sorted instead of unsorted + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub2], PRESERVED)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1, xpub3], PRESERVED)) == False + + # Singlesig instead of multisig + assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False + + # External output + assert self.d.output_is_change(get_external_singlesig_output()) == False + + def test_mixed_sorted_and_unsorted_multisig_1(self): + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1], PRESERVED)) + self.d.add_input(get_multisig_input([H_(45), 0, 0, 0], [xpub1], LEXICOGRAPHIC)) + + # Same ouputs as inputs + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1], PRESERVED)) == False + assert self.d.output_is_change(get_internal_multisig_output([H_(45), 0, 0, 0], [xpub1], LEXICOGRAPHIC)) == False # Singlesig instead of multisig assert self.d.output_is_change(get_internal_singlesig_output([H_(45), 0, 0, 0])) == False diff --git a/tests/device_tests/bitcoin/test_multisig_change.py b/tests/device_tests/bitcoin/test_multisig_change.py index 55caaf9342..990c92c82e 100644 --- a/tests/device_tests/bitcoin/test_multisig_change.py +++ b/tests/device_tests/bitcoin/test_multisig_change.py @@ -63,7 +63,7 @@ NODE_INT = bip32.deserialize( # m/2 => 038caebd6f753bbbd2bb1f3346a43cd32140648583673a31d62f2dfb56ad0ab9e3 multisig_in1 = messages.MultisigRedeemScriptType( - nodes=[NODE_EXT2, NODE_EXT1, NODE_INT], + nodes=[NODE_EXT1, NODE_EXT2, NODE_INT], address_n=[0, 0], signatures=[b"", b"", b""], m=2, @@ -84,7 +84,7 @@ multisig_in3 = messages.MultisigRedeemScriptType( ) prev_hash_1, prev_tx_1 = forge_prevtx( - [("3HwrvQEfYw4wUvGHpGmixWB15HPgqrvTh1", 50_000_000)] + [("3Ltgk5WPUMLcT2QvwRXKj9CWsYuAKqeHJ8", 50_000_000)] ) INP1 = messages.TxInputType( address_n=[H_(45), 0, 0, 0],