P2TR address from untweaked key by nlanson · Pull Request #691 · rust-bitcoin/rust-bitcoin

nlanson · 2021-11-07T06:33:39Z

Addressing issue #687.

Changes:

New method Address::dangerous_p2tr that creates an address given an untweaked internal key and optional merkle root of a script tree. The method utilizes existing hash methods from within the library and key tweaking from the rust-secp256k1 library.
Added a test to create a P2TR address from an internal key without a script tree. Test was taken from BIP-86.

Questions:

Should key tweaking be a seperate function in it self? Since the secp256k1 key tweaking method does not hash the data, there could be a method that does the tweaking and commitment part of the Address::dangerous_p2tr method.
What error should Address::dangerous_p2tr return if an error occurs while tweaking the internal key?

I would love feedback as this is my first PR to this repository.

See updated changes below

src/util/address.rs

Kixunil

Alternative API idea:

Have TweakedPublicKey and UntweakedPublicKey
UntweakedPublicKey can be created from shnorrsig::PublicKey or is a type alias of shnorrsig::PublicKey
Allow conversion between them by calling tweak() method.
Allow direct conversion with dangerous_assume_tweaked() method

src/util/address.rs

nlanson · 2021-11-07T13:35:51Z

Having a tweaked and untweaked type alias sounds like a good idea since it is ambiguous.

Something like this might work? It is my first time using type aliases so not sure what the convention is surrounding them.

type UntweakedPublicKey = PublicKey;
type TweakedPublicKey = PublicKey;

trait KeyTweaking {
    /// Tweaks an untweaked public key.
    fn tweak<C: Verification>(&self, secp: Secp256k1<C>, merkle_root: Option<TapBranchHash>) -> TweakedPublicKey;

    /// Directly convert an UntweakedPublicKey to a TweakedPublicKey 
    fn dangerous_assume_tweaked(&self) -> TweakedPublicKey;
}

impl KeyTweaking for UntweakedPublicKey {
    fn tweak<C: Verification>(&self, secp: Secp256k1<C>, merkle_root: Option<TapBranchHash>) -> TweakedPublicKey {
        // Applies the tweak to self
    }

    fn dangerous_assume_tweaked(&self) -> TweakedPublicKey {
        // Direct converstion
    }
}

and Address::p2tr can be update to take in an untweaked key:

/// Tweaks the internal key
pub fn p2tr<C: Verification>(
    secp: Secp256k1<C>, 
    internal_key: UntweakedPublicKey, 
    merkle_root: Option<TapBranchHash>, 
    network: Network
) -> Address {
    Address {
        network: network,
        payload: Payload::WitnessProgram {
            version: WitnessVersion::V1,
            program: internal_key.tweak(secp, merkle_root).serialize().to_vec()
        }
    }
}

/// Does not tweak the given key assuming it has already been tweaked
pub fn dangerous_p2tr<C: Verification>(
    internal_key: TweakedPublicKey, 
    network: Network
 ) -> Address {
    Address {
        network: network,
        payload: Payload::WitnessProgram {
            version: WitnessVersion::V1,
            program: internal_key.serialize().to_vec()
        }
    }
}

Kixunil · 2021-11-07T16:47:12Z

TweakedPublicKey should not be a type alias but a newtype: struct TweakedPublicKey(PublicKey);

Also I don't see why trait is useful and consuming self seems to make more sense.

pub type UntweakedPublicKey = PublicKey;
pub struct TweakedPublicKey(PublicKey);

impl UntweakedPublicKey {
    /// Tweaks an untweaked public key.
    pub fn tweak<C: Verification>(self, secp: Secp256k1<C>, merkle_root: Option<TapBranchHash>) -> TweakedPublicKey {
        // Applies the tweak to self
    }

    /// Directly convert an UntweakedPublicKey to a TweakedPublicKey 
    fn dangerous_assume_tweaked(self) -> TweakedPublicKey {
        TweakedPublicKey(self)
    }
}

impl TweakedPublicKey {
    fn into_raw(self) -> PublicKey {
        self.0
    }
}

sanket1729 · 2021-11-07T18:16:45Z

+1 for the current suggestion from @Kixunil .

src/util/schnorr.rs

nlanson · 2021-11-08T09:42:39Z

Most recent commits include changes to reflect suggestions from @Kixunil and @sanket1729.

Changes

-Added two new structs in src/util/schnorr.rs to represent Tweaked and Untweaked public keys. The UntweakedPublicKey struct has methods to convert to a TweakedPublicKey with or without tweaking the underlying key.
-The dangerous API in Address is now the method that operates on tweaked keys. The non dangerous api is the method that operates on untweaked keys.

Please let me know if there is anything is wrong or missing.

Kixunil · 2021-11-08T09:46:27Z

The dangerous API in Address is now the method that operates on tweaked keys. The non dangerous api is the method that operates on untweaked keys.

I believe it should be the other way around? Or rather, only have non-dangerous API?

nlanson · 2021-11-08T10:20:41Z

The dangerous API in Address is now the method that operates on tweaked keys. The non dangerous api is the method that operates on untweaked keys.

I believe it should be the other way around? Or rather, only have non-dangerous API?

#687 points out that operating on tweaked keys is discouraged so I believe it is correct that Address::dangerous_p2tr() operates on tweaked keys and the regular Address::p2tr() operates on untweaked keys.

Kixunil · 2021-11-08T14:55:57Z

The API I suggested should remove the danger but maybe I'm missing something?

dr-orlovsky · 2021-11-08T14:58:41Z

Just to note that we still need API working on keys assumed to be tweaked (dangerous) for supporting pay-to-contract schemes with taproot, which will not follow exact BIP-341 recommendation and will tweak internal key not with its hash, but with some contract (non-script) hash.

src/util/address.rs

src/util/schnorr.rs

dr-orlovsky · 2021-11-08T15:05:37Z

src/util/schnorr.rs

I think this is not needed

Better to have internal panic than incorrect results?

How we can have an incorrect result if it is checked in line 58 already and this check adds nothing to that?

https://doc.rust-lang.org/std/macro.unreachable.html ?

src/util/schnorr.rs

sanket1729 · 2021-11-08T15:59:51Z

The API I suggested should remove the danger but maybe I'm missing something?

Yes, it should. my bad! I would still prefer the regular p2tr() to operate on untweaked keys because that is how it is defined in the bitcoin descriptor spec. And maybe we can have p2tr_tweaked() as an additional API?

nlanson · 2021-11-09T03:57:06Z

Most recent commits include changes requested by @dr-orlovsky

Changes

Rename tweak() method to tap_tweak() for ambiguity.
Rename into_raw() method to into_inner().
Renamed internal_key parameter to output_key to conform with BIP-341.
Removes unnecessary allocation when computing tap tweak.

nlanson · 2021-11-09T04:10:32Z

I am still stuck on deciding whether UntweakedPublicKey should it's own new type or just a type alias. At the moment, it is it is a new type.

If it is going to be a type alias it will need to implement a trait as I cannot directly implement to PublicKey outside of the secp256k1 library.

I am leaning towards using a type alias though as it will make using the Address::p2tr() API easier.

dr-orlovsky · 2021-11-09T11:46:10Z

@nlanson I am up to have a type alias since it will simplify API usage overall (it's easier to use trait than to wrap/extract newtype inner value each time)

dr-orlovsky

LGTM. Will ACK once we will have a final code with type alias (if everyone agree upon that) and old code comments removed

src/util/address.rs

Kixunil · 2021-11-09T12:06:08Z

We could also add TweakedPublicKey::new(...) so that the extension trait doesn't have to be used (but methods are nicer so the trait is still useful).

nlanson · 2021-11-09T20:26:14Z

Most recent commits now use a type alias instead of a new type for UntweakedPublicKey.

Changes

Type alias and trait for UntweakedPublicKey
Rename dangerous_p2tr(...) to p2tr_tweaked(...)
Add TweakedPublicKey::new(...) method

dr-orlovsky

utACK 1a990b9b126ce8cf0b1f26b419cabc1d8621979c modulo several nets

But we need commits to be squashed before merging

src/util/address.rs

src/util/schnorr.rs

sanket1729

This looks great, left a couple of nits. I agree with the overall API, I think commits can be squashed and we are good to go.

src/util/address.rs

src/util/schnorr.rs

Ambiguous TweakedPublicKey and UntweakedPublicKey type aliases and methods to convert Use structs for Untweaked and Tweaked key type swap dangerous api to work on tweaked keys remove unecessary allocations and rename methods Use type alias for UntweakedPublicKey TweakedPublicKey::new(...) method added minor naming and doc changes

nlanson · 2021-11-11T03:38:38Z

Managed to squash the commits after battling with git for a good hour.

dr-orlovsky

utACK

sanket1729

Ack 803b5fe. Thanks @nlanson, this is an excellent firs time contribution.

…g tweak for UntweakedPublicKey 5b21a9c Use TapTweakHash method for computing tweak (Noah) Pull request description: Quick follow up PR to #691 using a method from #677. ### Changes - Updated `UntweakedPublicKey::tap_tweak(...)` to use `TapTweakHash::from_key_and_tweak(...)` ACKs for top commit: Kixunil: ACK 5b21a9c dr-orlovsky: utACK 5b21a9c Tree-SHA512: d00455bba51981e9ec942a6cf69672666e227850d073b1fdcd92d2eb6ad553659fb2967aec2ce12d3ed109cee5fa125cdda649cddb25404f08adae2bfd3e19bb

7aacc37 Add tests from BIP341 (sanket1729) 61629cc Make taproot hashes forward display (sanket1729) Pull request description: Add tests for taproot. - ~Also fixes one bug in #677, namely, I was returning `LeafVersion::default()` instead of given version~ - ~ Fixes a bug in #691 about taking secp context as a reference instead of consuming it. This should have not passed my review, but this is easy to miss. ~ - Makes the display on taproot hashes forward instead of the reverse (because the BIP prints in a forward way, I think we should too and it is more natural. ) ACKs for top commit: RCasatta: ACK 7aacc37 apoelstra: ACK 7aacc37 Tree-SHA512: 2e0442131fc036ffa10f88c91c8fc02d9b67ff6c16c592aa6f4e6a220c26a00fc6ca95a288f14aa40667a289fb0446219fd6c76c0196ead766252356592b9941

rebeccahermanreedroe-cpu

What is this ?Is this done

nlanson · 2026-02-04T11:04:09Z

What is this ?Is this done

This PR was merged years ago.

tcharding · 2026-02-04T23:34:05Z

@nlanson they have internet in the mountains huh?

nlanson · 2026-02-05T11:49:49Z

Yep. And it's fast.

tcharding · 2026-02-05T23:52:38Z

Now you winding me up.

nlanson commented Nov 7, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

Kixunil reviewed Nov 7, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

nlanson commented Nov 7, 2021

View reviewed changes

src/util/schnorr.rs Outdated Show resolved Hide resolved

nlanson commented Nov 7, 2021

View reviewed changes

src/util/schnorr.rs Outdated Show resolved Hide resolved

dr-orlovsky suggested changes Nov 8, 2021

View reviewed changes

dr-orlovsky previously approved these changes Nov 9, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

dr-orlovsky added the API break This PR requires a version bump for the next release label Nov 9, 2021

dr-orlovsky mentioned this pull request Nov 9, 2021

Add taproot data structures #677

Merged

2 tasks

nlanson dismissed dr-orlovsky’s stale review via 1a990b9 November 9, 2021 20:23

dr-orlovsky reviewed Nov 10, 2021

View reviewed changes

sanket1729 reviewed Nov 10, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

Kixunil requested changes Nov 10, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

src/util/schnorr.rs Outdated Show resolved Hide resolved

dr-orlovsky approved these changes Nov 11, 2021

View reviewed changes

sanket1729 approved these changes Nov 12, 2021

View reviewed changes

dr-orlovsky merged commit 5631ec5 into rust-bitcoin:master Nov 12, 2021

This was referenced Nov 12, 2021

Change Address::p2tr #687

Closed

API for taproot tweaked keys #585

Closed

Tracking issue for Taproot #503

Closed

sanket1729 mentioned this pull request Nov 12, 2021

BIP341 test vectors #695

Merged

nlanson mentioned this pull request Nov 13, 2021

Use TapTweakHash::from_key_and_tweak() method in computing tweak for UntweakedPublicKey #697

Merged

sanket1729 mentioned this pull request Nov 15, 2021

tests: improve coverage for P2tr and AddressType #683

Merged

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

rebeccahermanreedroe-cpu reviewed Feb 4, 2026

View reviewed changes

Conversation

nlanson commented Nov 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

See updated changes below

Uh oh!

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nlanson commented Nov 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil commented Nov 7, 2021

Uh oh!

sanket1729 commented Nov 7, 2021

Uh oh!

Uh oh!

Uh oh!

nlanson commented Nov 8, 2021

Changes

Uh oh!

Kixunil commented Nov 8, 2021

Uh oh!

nlanson commented Nov 8, 2021

Uh oh!

Kixunil commented Nov 8, 2021

Uh oh!

dr-orlovsky commented Nov 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dr-orlovsky Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

sanket1729 Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

dpc Nov 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sanket1729 commented Nov 8, 2021

Uh oh!

nlanson commented Nov 9, 2021

Changes

Uh oh!

nlanson commented Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-orlovsky commented Nov 9, 2021

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Kixunil commented Nov 9, 2021

Uh oh!

nlanson commented Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

nlanson commented Nov 7, 2021 •

edited

Loading

nlanson commented Nov 7, 2021 •

edited

Loading

nlanson commented Nov 9, 2021 •

edited

Loading

nlanson commented Nov 9, 2021 •

edited

Loading

nlanson commented Feb 4, 2026 •

edited

Loading

tcharding commented Feb 4, 2026 •

edited

Loading

nlanson commented Feb 5, 2026 •

edited

Loading