Taproot multisig #4159

andrewtoth · 2024-09-05T17:53:59Z

Adds basic taproot multisig support.

Uses a single OP_CHECKSIGADD-based script at the root level. Uses the secp256k1 generator as the basis for an xpub that derives an unspendable internal pubkey.

Resolves #1946

andrewtoth · 2024-09-09T14:13:33Z

@prusnak @andrewkozlik @onvej-sl @matejcik Any feedback on this PR?

matejcik · 2024-09-10T08:29:00Z

any feedback on this PR?

sure!
in general, we are interested in taproot multisig functionality.
we don't have it on the roadmap at the moment, so before anything happens on the PR, you'll need to wait for us to prioritize it.
personally I don't know the status of the various taproot multisig proposals, so i can't comment on whether your solution implements the one (or one of) that we want

in the future, please open an issue first where you outline your proposed solution. that way you can avoid doing all the work in case we say it's not something we are interested in 🤷‍♀️

prusnak · 2024-09-10T08:34:18Z

The change is not so big and honestly it is worth merging in. Of course, we really need to have the new code properly tested.

prusnak

Can we also add some tests that test the construction of a Taproot multisig transaction?

core/src/apps/bitcoin/common.py

andrewtoth · 2024-09-10T13:45:31Z

Thank you for the feedback @matejcik and @prusnak

in the future, please open an issue first

There is in fact an open issue, #1946, which I linked. The consensus in that issue is to implement the approach in this PR.

Also, this approach will be compatible with BIP387 descriptors tr(key, multi_a(k, key1, ... keyn)) and also the BIP388 wallet policy tr(@0/**, multi_a(k, @1/**, ..., @n/**)).

Can we also add some tests that test the construction of a Taproot multisig transaction?

Yes, I'd be happy to add tests and make any changes needed to have this feature merged and released. But first, I would like some assurance that this will in fact be merged. Would we be able to approve the workflows here so I can make sure I didn't miss anything else?

andrewtoth · 2024-09-12T23:02:34Z

@matejcik @prusnak thank you - I have updated the PR to use a deterministic HDNode for the unspendable internal pubkey, and added tests for getting addresses and signing and serializing a tx.

prusnak · 2024-09-13T07:28:19Z

I have updated the PR to use a deterministic HDNode for the unspendable internal pubkey

Why is this preferred to using a static dummy key from previously used code?

andrewtoth · 2024-09-13T11:42:00Z

I have updated the PR to use a deterministic HDNode for the unspendable internal pubkey

Why is this preferred to using a static dummy key from previously used code?

This makes it compatible with BIP388 wallet policies. I cannot make a multisig wallet using a Trezor as one key and another hardware wallet that uses wallet policies as another key with just a static dummy key.

prusnak · 2024-09-16T13:28:35Z

added tests for getting addresses and signing and serializing a tx.

Amazing!

prusnak · 2024-09-16T13:47:31Z

I fixed smaller issues detected by the CI in commit e8cc7c4, but there is still bigger fish to fry:

$ cd core
$ make pyright                
python ../tools/pyright_tool.py

Ignored 88 custom-defined errors from 17 files.

ERROR: We have issues!

/Users/stick/work/trezor/trezor-firmware/core/src/apps/bitcoin/sign_tx/bitcoin.py:661: - error: Expression of type "tuple[bytes, bytes]" is incompatible with return type "bytes"
  "tuple[bytes, bytes]" is incompatible with "bytes" (reportReturnType)

/Users/stick/work/trezor/trezor-firmware/core/src/apps/bitcoin/sign_tx/bitcoin.py:677: - error: Argument of type "int" cannot be assigned to parameter "pubkey" of type "bytes" in function "multisig_pubkey_index"
  "int" is incompatible with "bytes" (reportArgumentType)

/Users/stick/work/trezor/trezor-firmware/core/src/apps/bitcoin/sign_tx/bitcoin.py:682: - error: Argument of type "int" cannot be assigned to parameter "signature" of type "bytes" in function "write_witness_multisig_taproot"
  "int" is incompatible with "bytes" (reportArgumentType)

/Users/stick/work/trezor/trezor-firmware/core/src/apps/bitcoin/sign_tx/bitcoin.py:688: - error: Argument of type "int" cannot be assigned to parameter "signature" of type "bytes" in function "write_witness_p2tr"
  "int" is incompatible with "bytes" (reportArgumentType)

/Users/stick/work/trezor/trezor-firmware/core/src/apps/bitcoin/sign_tx/bitcoin.py:713: - error: Argument of type "int | bytes" cannot be assigned to parameter "signature" of type "bytes" in function "set_serialized_signature"
  Type "int | bytes" is incompatible with type "bytes"
    "int" is incompatible with "bytes" (reportArgumentType)

/Users/stick/work/trezor/trezor-firmware/core/src/apps/bitcoin/sign_tx/decred.py:168: - error: Expression of type "DecredSigHasher" is incompatible with return type "SigHasher"
  "DecredSigHasher" is incompatible with protocol "SigHasher"
    "hash341" is an incompatible type
      Type "(i: int, tx: SignTx | PrevTx, sighash_type: SigHashType) -> bytes" is incompatible with type "(i: int, tx: SignTx | PrevTx, sighash_type: SigHashType, leaf_hash: bytes | None) -> bytes"
        Function accepts too many positional parameters; expected 3 but received 4 (reportReturnType)

/Users/stick/work/trezor/trezor-firmware/core/src/trezor/utils.py:282: - error: Expression of type "memoryview[_I@memoryview] | memoryview[int]" is incompatible with return type "memoryview[int]"
  Type "memoryview[_I@memoryview] | memoryview[int]" is incompatible with type "memoryview[int]"
    "memoryview[_I@memoryview]" is incompatible with "memoryview[int]"
      Type parameter "_I@memoryview" is invariant, but "_I@memoryview" is not the same as "int" (reportReturnType)

/Users/stick/work/trezor/trezor-firmware/core/src/trezor/utils.py:292: - error: Expression of type "_I@memoryview | int" is incompatible with return type "int"
  Type "object* | int" is incompatible with type "int"
    "object*" is incompatible with "int" (reportReturnType)

/Users/stick/work/trezor/trezor-firmware/core/src/trezor/utils.py:300: - error: Expression of type "_I@memoryview | int" is incompatible with return type "int"
  Type "object* | int" is incompatible with type "int"
    "object*" is incompatible with "int" (reportReturnType)

Found 9 issues above
make: *** [pyright] Error 1

andrewtoth · 2024-09-16T14:01:31Z

For me

$ cd core
$ make pyright

returns

python ../tools/pyright_tool.py
Error: [Errno 2] No such file or directory: 'pyright'
make: *** [Makefile:211: pyright] Error 1

I couldn't find much docs on setting up pyright. Any hints?

prusnak · 2024-09-16T14:04:50Z

I couldn't find much docs on setting up pyright. Any hints?

Hm. Not sure - maybe npm install -g [email protected]?

Or you can use pyright from brew, if you are no macOS.

andrewtoth · 2024-09-16T14:09:56Z

Thanks! After my last commit I get:

python ../tools/pyright_tool.py

Ignored 88 custom-defined errors from 17 files.

SUCCESS: Everything is fine!

andrewtoth · 2024-09-16T14:12:15Z

I get an error that I need to add a changelog as well. Should I do that or will you?

prusnak · 2024-09-16T14:28:04Z

I get an error that I need to add a changelog as well. Should I do that or will you?

I added it in my commit already

andrewtoth · 2024-09-16T15:01:51Z

Ah I see legacy tests are failing. I only tested this on Model T with core. Can this feature be disabled on legacy devices, or is legacy support required for this?

prusnak · 2024-09-16T15:08:20Z

Can this feature be disabled on legacy devices, or is legacy support required for this?

We can disable the tests with

@pytest.mark.models(skip=["legacy"])

andrewtoth · 2024-09-16T21:28:52Z

For the device tests, it appears I need to add hashes for each device and each language. Could that be done in an automated step by CI?

andrewtoth · 2024-09-20T14:05:42Z

@prusnak I'm unsure how to proceed here. I don't have any context about what the Post comment with UI diff URLs jobs are and why they are failing. There's also a Changelog check job failing that seems to be looking for a legacy changelog but there hasn't been anything touched in legacy.

Is there anything else I need to do here?

prusnak · 2024-09-20T15:53:04Z

@prusnak I'm unsure how to proceed here.

Let's leave that to @matejcik or someone else from the firmware team. I am sure the fix is trivial, but for me or you it would take hours to figure out.

Also legacy needs a changelog entry because we touched the code in the legacy/ folder (added extra parameter to some functions). Again, let's have the firmware team to either add a dummy changelog entry or find a way how to suppress the change.

Again, thank you for this massive contribution and we'll take it from here.

core/src/apps/bitcoin/sign_tx/tx_weight.py

crypto/tests/test_check.c

andrewtoth · 2024-09-30T16:42:14Z

core/src/apps/bitcoin/multisig.py

+        depth=0,
+        fingerprint=2084970077,
+        child_num=0,
+        chain_code=b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00",


I think we also want to allow passing in the chain code to create a randomized xpub that still uses the NUMS point public key. That way the derived public key is provably unspendable, but cannot be fingerprinted on chain.
We will need to define a new parameter for this in the MultisigRedeemScriptType.

onvej-sl · 2024-09-30T16:43:36Z

core/src/apps/bitcoin/multisig.py

+    for i in multisig.address_n:
+        node.derive(i, True)
+    return node.public_key()


This is something I don't understand: The dummy pubkey is derived using multisig.address_n. Suppose we have two wallets. The first one uses a node HD1 and the other a node HD2. Suppose we have a multisig address created from public keys HD1/0/0 and HD1/0/1. Since the wallets use different paths to derive the dummy pubkey, they will generate different addresses. Consequently, if the first wallet creates a transaction with the multisig address as an output, the second wallet will create a valid witness with a probability of 1/2, because the parity of the tweaked dummy key is used in the witness. Is this correct?

But address_n must be the same for the entire MultisigRedeemScriptType, so how can the address be created by HD1/0/0 and HD1/0/1? That would imply an address_n of [0,0] for the first key and [0,1] for the second. Am I misunderstanding?

I was under the impression that you could specify address_n for each pubkey separately in multisig.pubkeys[*].address_n.

trezor-firmware/core/src/apps/bitcoin/multisig.py

Line 111 in 549d592

return [multisig_get_pubkey(hd.node, hd.address_n) for hd in multisig.pubkeys]

Oh I see you can have pubkeys or nodes, and pubkeys have their own address_n individually. I haven't tested this with that. I suppose we can disable using pubkeys list for taproot multisig?
Is pubkeys list legacy or is it still widely used?

By the way, I'm going to do a quick research to see what internal keys other wallets that support Taproot multisig use. If we want other wallets to be interoperable with trezor, we should follow the same approach.

I don't think it makes a difference in terms of security and privacy for the first 3 options.
Only the chain code and public key are used to derive the actual public key that will be used in the script though, so the last option would introduce malleability. For example, two xpubs where one lies about the depth would produce the same redeem script but would result in a different chaincode for the dummy xpub.

Yes, I didn't realize but if using just the 33 bytes compressed public key like I have here it does make it compatible with Liana, if Liana has their xpubs sorted, no duplicates, and max depth of 2.

I can proceed with a BIP and mailing list post if this is acceptable.

Thanks. Unless @onvej-sl has any objection to the proposed method, I encourage you to go ahead.

I have no objections.

Thanks for your patience. I have created a BIP draft specifying this behavior. Once it gets assigned a number I can rebase this and hopefully we can see it merged 🤞

bitcoin/bips#1746

Compute a dummy xpub to use as the internal pubkey for taproot multisig. Sort and remove duplicates of all xpub pubkeys, concatenate and compute sha256 hash. The result is the chaincode, and the pubkey is the NUMS point.

andrewtoth requested review from prusnak, andrewkozlik and onvej-sl as code owners September 5, 2024 17:54

andrewtoth force-pushed the taproot-multisig branch 2 times, most recently from 675c45c to 7adcabd Compare September 5, 2024 18:10

andrewtoth mentioned this pull request Sep 5, 2024

Implement multisig for Taproot #1946

Open

andrewtoth force-pushed the taproot-multisig branch from 7adcabd to 20ad7e0 Compare September 5, 2024 18:53

Hannsek requested a review from matejcik September 6, 2024 12:14

andrewtoth force-pushed the taproot-multisig branch from 20ad7e0 to 14e9116 Compare September 6, 2024 19:19

prusnak reviewed Sep 10, 2024

View reviewed changes

core/src/apps/bitcoin/common.py Outdated Show resolved Hide resolved

andrewtoth force-pushed the taproot-multisig branch from 14e9116 to c89ace9 Compare September 12, 2024 23:00

prusnak force-pushed the taproot-multisig branch from 385baa6 to a9ff3de Compare September 16, 2024 15:07

andrewtoth force-pushed the taproot-multisig branch from eebf5ad to 69e4e74 Compare September 19, 2024 14:45

andrewtoth force-pushed the taproot-multisig branch from 69e4e74 to 549d592 Compare September 20, 2024 01:23

onvej-sl reviewed Sep 30, 2024

View reviewed changes

core/src/apps/bitcoin/sign_tx/tx_weight.py Outdated Show resolved Hide resolved

onvej-sl reviewed Sep 30, 2024

View reviewed changes

crypto/tests/test_check.c Outdated Show resolved Hide resolved

andrewtoth commented Sep 30, 2024

View reviewed changes

onvej-sl reviewed Sep 30, 2024

View reviewed changes

andrewtoth and others added 14 commits December 10, 2024 09:42

crypto: return parity when tweaking public key

2ffb0d3

feat(BTC): add taproot multisig address generation

ca128b1

feat(BTC): add taproot multisig input signing

91a4f83

tests: test taproot multisig getaddress and signtx

268d479

fix: taproot multisig

cd5a650

fix: type errors

fe7b32b

style: import sort order

26c6b3f

fix: reword changelog

cf58fa7

style: move parity param to same line

dce7fd0

tests: skip testing multisig taproot with legacy

0bddc02

multisig: compute dummy chaincode based on pubkeys

d066232

Compute a dummy xpub to use as the internal pubkey for taproot multisig. Sort and remove duplicates of all xpub pubkeys, concatenate and compute sha256 hash. The result is the chaincode, and the pubkey is the NUMS point.

fix weight calculation

2965484

add test for pubkey tweak parity

3f6b1ff

tests: update address tests for taproot multisig

f6cb74d

andrewtoth force-pushed the taproot-multisig branch from 11982dc to f6cb74d Compare December 11, 2024 00:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Taproot multisig #4159

Taproot multisig #4159

andrewtoth commented Sep 5, 2024 •

edited

Loading

andrewtoth commented Sep 9, 2024

matejcik commented Sep 10, 2024

prusnak commented Sep 10, 2024

prusnak left a comment

andrewtoth commented Sep 10, 2024

andrewtoth commented Sep 12, 2024

prusnak commented Sep 13, 2024

andrewtoth commented Sep 13, 2024

prusnak commented Sep 16, 2024

prusnak commented Sep 16, 2024 •

edited

Loading

andrewtoth commented Sep 16, 2024

prusnak commented Sep 16, 2024 •

edited

Loading

andrewtoth commented Sep 16, 2024

andrewtoth commented Sep 16, 2024

prusnak commented Sep 16, 2024

andrewtoth commented Sep 16, 2024

prusnak commented Sep 16, 2024 •

edited

Loading

andrewtoth commented Sep 16, 2024

andrewtoth commented Sep 20, 2024

prusnak commented Sep 20, 2024

andrewtoth Sep 30, 2024

onvej-sl Sep 30, 2024

andrewtoth Sep 30, 2024

onvej-sl Oct 1, 2024

andrewtoth Oct 1, 2024

onvej-sl Oct 1, 2024

andrewtoth Dec 11, 2024

andrewtoth Dec 11, 2024

andrewkozlik Dec 11, 2024

onvej-sl Dec 12, 2024

andrewtoth Jan 17, 2025

Taproot multisig #4159

Are you sure you want to change the base?

Taproot multisig #4159

Conversation

andrewtoth commented Sep 5, 2024 • edited Loading

andrewtoth commented Sep 9, 2024

matejcik commented Sep 10, 2024

prusnak commented Sep 10, 2024

prusnak left a comment

Choose a reason for hiding this comment

andrewtoth commented Sep 10, 2024

andrewtoth commented Sep 12, 2024

prusnak commented Sep 13, 2024

andrewtoth commented Sep 13, 2024

prusnak commented Sep 16, 2024

prusnak commented Sep 16, 2024 • edited Loading

andrewtoth commented Sep 16, 2024

prusnak commented Sep 16, 2024 • edited Loading

andrewtoth commented Sep 16, 2024

andrewtoth commented Sep 16, 2024

prusnak commented Sep 16, 2024

andrewtoth commented Sep 16, 2024

prusnak commented Sep 16, 2024 • edited Loading

andrewtoth commented Sep 16, 2024

andrewtoth commented Sep 20, 2024

prusnak commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewtoth commented Sep 5, 2024 •

edited

Loading

prusnak commented Sep 16, 2024 •

edited

Loading

prusnak commented Sep 16, 2024 •

edited

Loading

prusnak commented Sep 16, 2024 •

edited

Loading