Skip to content
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

Change Address::p2tr #687

Closed
sanket1729 opened this issue Nov 3, 2021 · 5 comments
Closed

Change Address::p2tr #687

sanket1729 opened this issue Nov 3, 2021 · 5 comments
Projects
Milestone

Comments

@sanket1729
Copy link
Member

sanket1729 commented Nov 3, 2021

I think we should not be offering any API that operates on tweaked keys (See #563). It is a discouraged practice and is not supported in descriptors. Given that we already shipped this in a release version, I am looking for input on how to fix this.

I commented to fix this before release (#563 (comment)), but we forgot about it :) despite creating an issue about it. #585

Text from BIP-341:
Why should the output key always have a taproot commitment, even if there is no script path? If the taproot output key is an aggregate of keys, there is the possibility for a malicious party to add a script path without being noticed by the other parties. This allows to bypass the multiparty policy and to steal the coin. MuSig key aggregation does not have this issue because it already causes the internal key to be randomized. The attack works as follows: Assume Alice and Mallory want to aggregate their keys into a taproot output key without a script path. In order to prevent key cancellation and related attacks they use MSDL-pop instead of MuSig. The MSDL-pop protocol requires all parties to provide a proof of possession of their corresponding secret key and the aggregated key is just the sum of the individual keys. After Mallory receives Alice's key A, Mallory creates M = M0 + int(t)G where M0 is Mallory's original key and t allows a script path spend with internal key P = A + M0 and a script that only contains Mallory's key. Mallory sends a proof of possession of M to Alice and both parties compute output key Q = A + M = P + int(t)G. Alice will not be able to notice the script path, but Mallory can unilaterally spend any coin with output key Q.

As for rust-bitcoin library, I think all APIs should deal with untweaked keys. There should not be any type of Tweaked keys.

Current function signature.

    /// Create a pay to taproot address
    pub fn p2tr(taptweaked_key: schnorrsig::PublicKey, network: Network) -> Address {
    }

Expect function signature

    pub fn p2tr<C: secp256k1::Verification>(
	secp: Secp256k1<C>, 
	internal_key: schnorrsig::PublicKey, 
	merkle_root: Option<TapBranchHash>, 
	network: Network) 
	-> Address {
    }
@sanket1729
Copy link
Member Author

cc @apoelstra @dr-orlovsky

@Kixunil
Copy link
Collaborator

Kixunil commented Nov 4, 2021

I'm not against providing lower-level APIs, but they should probably be prefixed with dangerous_. If you want to remove it completely I don't care much.

@apoelstra
Copy link
Member

I wouldn't mind just fixing this in a breaking change. Next release is the "taproot release" and I think users wouldn't be too surprised to see Taproot-related functionality get broken.

@dr-orlovsky dr-orlovsky added this to Todo in Taproot Nov 6, 2021
@dr-orlovsky dr-orlovsky added this to the 0.28.0 milestone Nov 6, 2021
@nlanson
Copy link
Contributor

nlanson commented Nov 7, 2021

Is it ok if I had a go at this?

@dr-orlovsky dr-orlovsky linked a pull request Nov 8, 2021 that will close this issue
@dr-orlovsky dr-orlovsky moved this from Todo to Ready for Review in Taproot Nov 8, 2021
@dr-orlovsky
Copy link
Collaborator

Closed in #691

Taproot automation moved this from Ready for Review to Done Nov 12, 2021
@sanket1729 sanket1729 mentioned this issue Nov 17, 2021
dr-orlovsky added a commit that referenced this issue Nov 23, 2021
e4774e7 fixups to taptweaking code (sanket1729)

Pull request description:

  This was my bad for not clearly stating the expected spec #687 . Changed values to references so that we only take ownership where it is required.

  This should simplify the #697

ACKs for top commit:
  Kixunil:
    ACK e4774e7
  dr-orlovsky:
    utACK e4774e7

Tree-SHA512: adacbfa8a77f46b2c85720f3760ed12a437f40d8422731d0207662d7947c95dda79d576923f6056c77f57977a3dcd25afd270f0ee11e9c3be9d067ccdc63371a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

5 participants