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 pyright@1.1.361?

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.

@darosior already implemented a deterministic derivation for the chaincode in Liana; it would be great if new implementations of the deterministic derivations are kept compatible.

I don't think it's great that it depends on the order of keys provided. If we want to sort the pubkeys beforehand and order the xpubs in the order of the derived pubkeys, that would make a different chaincode for each order.

I think we should sort the xpubs before hashing.

Well, it's great to see that someone else came to essentially the same solution. Seems like we are on the right path. To make sure that we are all on the same page, let me summarize the current plan:

The dummy XPUB that is used to derive the internal public key will use the NUMS public key specified in BIP 341 and a chain code deterministically derived from the multisig XPUBs.

MultisigRedeemScriptType doesn't change. Trezor derives the dummy XPUB internally.

All the public keys in the multisig, including the internal public key will use the same derivation path.

To keep things simple, I would disable MultisigRedeemScriptType.pubkeys for taproot multisig, i.e. enforce the use of MultisigRedeemScriptType.nodes. There is nothing stopping us from loosening this restriction in the future if necessary.

The question remains how to derive the chain code. This is tricky. Trezor doesn't deal with policies or descriptors directly at the moment, at least not for multisig. Right now, distinguishing between sortedmulti_a() and multi_a() would be up to the host application, and it's up to that application to fill in the MultisigRedeemScriptType in the correct order. So if sortedmulti_a() is specified, it needs to derive the public keys and sort the XPUBs accordingly in MultisigRedeemScriptType.

It seems to me that we can't just take the order of the XPUBs given in MultisigRedeemScriptType, because with sortedmulti_a() it would make the dummy XPUB depend on the path, since the host changes the XPUB order for each path. That would presumably be incompatible with Ledger, not to mention confusing. So unless we overhaul MultisigRedeemScriptType to be able to handle descriptors, we need the dummy XPUB derivation to be independent of the XPUB order. Meaning they need to be sorted somehow, as you proposed, or the derivation operation needs to be order-agnostic, e.g. dummy chain code = XOR of the multisig chain codes. The XOR is not great, because it allows a malicious participant to set the chain code to a desired value, but come to think of it, perhaps that not an issue.

All that being said, I'd very much hope for the solution to be compatible with existing implementations, and ideally with whatever will be the future specification, because sooner or later this will need to be standardized to ensure smooth interoperability amongst wallets. Hopefully your comment in delvingbitcoin.org will spark some discussion about that.

It seems to me that we can't just take the order of the XPUBs given in MultisigRedeemScriptType, because with sortedmulti_a() it would make the dummy XPUB depend on the path, since the host changes the XPUB order for each path. That would presumably be incompatible with Ledger, not to mention confusing.

Ledger's implementation doesn't care how the chaincode of the unspendable key is computed, as it's explicitly passed on by the client together with all the other xpubs. It does recognize it as unspendable as long as the pubkey is the standard NUMS point, and that's important for security - or the client could lie and put an actual spendable pubkey.

Using the deterministic derivation only matters for UX (the device could in principle skip showing it altogether in that case); this is currently left for future improvements to the wallet policies / miniscript UX.

I am somewhat disappointed that our comments in delvingbitcoin.org did not spark any feedback. The way I see it, we are practically facing these options:

The host will supply the chaincode for the unspendable XPUB and the user will confirm it on Trezor. This has poor UX. As if multisig signing wasn't hard enough to use for laymen, we add yet another layer of complexity for them to understand.

Design our own scheme that derives the chaincode for the unspendable XPUB from the multisig XPUBs independently of their order. Poor interoperability, e.g. with Liana. All we can do is hope that our design is appealing enough that others adopt it.

Adopt the Liana scheme, which in my opinion is not the best choice.

If a standard derivation scheme arises in the future, that is not identical with our choice, we will need to maintain the old scheme somewhere in our code, which I would like to avoid if we can. We currently don't have the capacity to push the standardization process forward. @andrewtoth would you be willing to do that? Even starting a discussion on the Bitcoin mailing list could be a step forward.

@andrewkozlik thanks for that summary. I agree, we should ideally have a BIP for this before merging here. #1 would be safest option in that sense since we can guarantee we will be forward compatible with any standard that emerges. But, the UX drawback is indeed unfortunate.

I do think there is already a risk with Trezor not showing all xpubs being used in a multisig address, for the same reason the dummy xpub needs to be shown. A single trezor user must take for granted all the other xpubs used in creating the multisig address shown. Ledger solves this by storing this information as a wallet policy that only has to be confirmed once.

I will be meeting with some of the other developers who have commented in that delving thread next week. I will be sure to discuss this with them.

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

crypto: return parity when tweaking public key

f2cc8ee

andrewtoth and others added 9 commits September 19, 2024 10:41

feat(BTC): add taproot multisig address generation

28f51e6

feat(BTC): add taproot multisig input signing

b6e0bb9

tests: test taproot multisig getaddress and signtx

63295ae

fix: taproot multisig

fcaa334

fix: type errors

c353997

style: import sort order

32b0706

fix: reword changelog

43c6764

style: move parity param to same line

09b2a35

tests: skip testing multisig taproot with legacy

f413b4f

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

tests: add ui device test fixture hashes

549d592

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 added 2 commits October 1, 2024 16:49

fix weight calculation

e9c3689

add test for pubkey tweak parity

11982dc

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 Oct 2, 2024

andrewkozlik Oct 2, 2024

bigspider Oct 3, 2024

andrewkozlik Oct 11, 2024

andrewtoth Oct 11, 2024

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