-
-
Notifications
You must be signed in to change notification settings - Fork 691
Taproot multisig #4159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
andrewtoth
wants to merge
14
commits into
trezor:main
Choose a base branch
from
andrewtoth:taproot-multisig
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Taproot multisig #4159
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
2ffb0d3
crypto: return parity when tweaking public key
andrewtoth ca128b1
feat(BTC): add taproot multisig address generation
andrewtoth 91a4f83
feat(BTC): add taproot multisig input signing
andrewtoth 268d479
tests: test taproot multisig getaddress and signtx
andrewtoth cd5a650
fix: taproot multisig
prusnak fe7b32b
fix: type errors
andrewtoth 26c6b3f
style: import sort order
andrewtoth cf58fa7
fix: reword changelog
prusnak dce7fd0
style: move parity param to same line
andrewtoth 0bddc02
tests: skip testing multisig taproot with legacy
andrewtoth d066232
multisig: compute dummy chaincode based on pubkeys
andrewtoth 2965484
fix weight calculation
andrewtoth 3f6b1ff
add test for pubkey tweak parity
andrewtoth f6cb74d
tests: update address tests for taproot multisig
andrewtoth File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Sending to and spending Taproot Multisig |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 nodeHD1
and the other a nodeHD2
. Suppose we have a multisig address created from public keysHD1/0/0
andHD1/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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But
address_n
must be the same for the entireMultisigRedeemScriptType
, so how can the address be created byHD1/0/0
andHD1/0/1
? That would imply anaddress_n
of[0,0]
for the first key and[0,1]
for the second. Am I misunderstanding?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was under the impression that you could specify
address_n
for each pubkey separately inmultisig.pubkeys[*].address_n
.trezor-firmware/core/src/apps/bitcoin/multisig.py
Line 111 in 549d592
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Unless @onvej-sl has any objection to the proposed method, I encourage you to go ahead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no objections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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