Taproot tweaks generalization & KeyPair support by dr-orlovsky · Pull Request #696 · rust-bitcoin/rust-bitcoin

dr-orlovsky · 2021-11-12T22:58:03Z

Adds taproot-related methods to Script
Fixes API for existing taproot methods
Generalizes TapTweak trait to work with both public keys and key pairs

~~UPD: PR is pending rust-bitcoin/rust-secp256k1#342~~

src/blockdata/script.rs

dr-orlovsky · 2021-11-23T16:55:00Z

Rebased and addressed nits by @tcharding

tcharding · 2021-11-24T00:18:04Z

Thanks

dr-orlovsky · 2021-11-24T11:22:53Z

@sanket1729 @Kixunil can you pls review this PR since it is quite important for completion of Taproot support - we need to tweak not only public keys, but also private ones, if we are going to even spent taproot key-path :)

Kixunil · 2021-11-24T15:39:26Z

I wasn't able to do deep, high-focus reviews lately but I will try to find some time for it today/tomorrow.

sanket1729

Before proceeding with the above review, we should form consensus on the one comment.

src/util/schnorr.rs

Kixunil · 2021-11-25T15:31:05Z

Sadly today wasn't great and I don't expect I'll be able to. If I get lucky it'll be tomorrow morning. Otherwise not sooner than Tuesday :(

dr-orlovsky · 2021-11-25T19:03:42Z

@Kixunil don't worry; this PR got blocked by the required upstream changes (see my discussion with @sanket1729 above). The rest of stuff has moved to #721 which should be simpler to review.

dr-orlovsky · 2021-12-02T19:12:45Z

#721 (comment)

d0a87be Add slice 'serialize' method for TweakedPublicKey (Dr. Maxim Orlovsky) 37352d1 Add Display and LowerHex to TweakedPublicKey (Dr. Maxim Orlovsky) Pull request description: Extraction of a portion from #696 which can be done without changes in `rust-secp256k1` ACKs for top commit: Kixunil: ACK d0a87be sanket1729: ACK d0a87be Tree-SHA512: d439ea1a4c4235bea9867e5d87514f928ad481f7a32403922654c33e101cfaba444eec8b61899f2aaaf1dcf5236bb618b9e14674736d3798effd56ca7097dc78

dr-orlovsky · 2022-01-06T09:34:47Z

Will re-base and use the new secp library once the pending issue with it will be fixed in a new point release (I am talking about #360)

dr-orlovsky · 2022-01-07T20:46:00Z

Fixed, rebased, ready for review

sanket1729

Looks good, just left some comments. CI is also failing on no_std.

sanket1729 · 2022-01-08T02:25:12Z

src/util/schnorr.rs

In 8b4c5b9d5d0c11b13a2f0cfbe26d20cd7cf24a86
Is it a good idea to have this? I would rather have users parse UnTweakedPubKey and call assume_dangerous.

FromStr is required by serde impl. Also, this brings us back to the question raised in #737.

I suggest don't impl (De)Serialize until we are not confident what the representation should be. We can always add impl later without breaking anything.

If I am not wrong TweakedPublicKey should be used in PSBT, and removing serde there will be a regression

I think it's okay to parse as TweakedPublicKey. I agree that there's a safety loophole here because we can't actually distinguish tweaked keys from untweaked keys, but I think it would be too unergonomic (and as @dr-orlovsky says, possibly incompatible with the PSBT API) to make the user do the assume_dangerous dance every time they parse a key.

Especially because untweeaked keys will not exactly be common in the wild .. so we are asking the user to think about an undetectable rare situation that they may not even understand.

I don't see why we need TweakedPubkeys in PSBT APIs? I think partial sigs should be XOnlyPublicKey -> SchnorrSig.

In the most common case, those XOnlyKeys would be tweaked, but I don't see any benefit for adding that type in psbt API. In my view, the only purpose of TweakedPublicKey is to guard users using the address API.

The keys in other places in the codebase might also be tweaked public keys, but we don't gain anything by having those being tweaked public keys (for example psbt API should only operate on XOnlyKeys). IMO, we should just remove this commit.

We will still need this for rawtr processing

We can parse XOnlyPubKey, and use the assume_dangerous_tweaked API? I think that is preciously how rawtr should work (if it is added in spec).
I feel strongly about this because we did put so much into Tweaking API, and added type safety with TweakedPublicKey. It would be a shame if we can break it this easily.

src/util/schnorr.rs

src/blockdata/script.rs

dr-orlovsky · 2022-01-08T13:45:09Z

@sanket1729 addressed your requests except the first one, where I gave my explanation why FromStr is still required.

src/util/schnorr.rs

apoelstra · 2022-01-08T15:53:09Z

src/blockdata/script.rs

In e49a6d98f5b1bdf7bafe130777f9d8ce094d4760:

I think this name should be reserved for a conversion from a TweakedKey to a script, which is the cheapest and (maybe) most common case. Then users with other key types can either obtain one by tweaking (or assume_dangerous :P).

We probably also should have a method like this, which takes an xonly key and a merkle root, but it should be called something longer, new_v1_p2tr_full or something.

Implemented

which is the cheapest and (maybe) most common case.

I think the most common case would be p2tr(UntweakedPublicKey). I don't want to go into API design debates :P , but I think this saves the user from importing a trait TapTweak and learning about the trait API in the most common case. For descriptors, tr() descriptor has the internal untweaked key as the argument and the uncommon rawtr() descriptor would have the tweaked key.

I would prefer, new_v1_p2tr(secp, untweaked_key, Option<MerkleRoot>) and new_v1_p2tr_raw(tweaked_key)

Although this also has the benefit that the users know that the tweaking is being done internally in the API and they are expected to use the tweaked keypairs for signing.

Not going to hold against this because we are offered the same functionality.

apoelstra · 2022-01-08T15:54:24Z

Done reviewing a2e9d2eae63919c9c5988708b932d7adbece0e60

sanket1729

I think that the previous API for p2tr that took in UntweakedPublicKey was better, but not going to stop this PR on it. As long as we are offered the same functionality, I don't care too much about it.

sanket1729 · 2022-01-09T00:32:48Z

src/util/schnorr.rs

In the most common case, those XOnlyKeys would be tweaked, but I don't see any benefit for adding that type in psbt API. In my view, the only purpose of TweakedPublicKey is to guard users using the address API.

The keys in other places in the codebase might also be tweaked public keys, but we don't gain anything by having those being tweaked public keys (for example psbt API should only operate on XOnlyKeys). IMO, we should just remove this commit.

sanket1729 · 2022-01-09T00:38:05Z

src/blockdata/script.rs

In affd45408adc858af9a9cbc03a61ca526a5d41ad
nit: Would prefer the name be new_v1_p2tr_script_spend

sanket1729 · 2022-01-09T00:42:25Z

src/blockdata/script.rs

In 7dc5554dcbbb8f3275fa78db2a5e100fca9e3706

Why add a new API that is being deprecated? This should be removed.

Sorry, my bad. Will remove when will be updating the PR according to the results of other discussions here.

dr-orlovsky · 2022-01-11T05:12:35Z

I am fine with removing FromStr, if other feels that this is more a foot-gun rather than helper to situations like rawtr. I rather have no strong opinion about that (feels like this is not a footgun since we distinguished types in the API and it is easier to call dangerous_assume_tweaked, then convert through Display/FromStr pair).

The only thing I am asking is to do that post-merge in a separate PR. I will open one to discuss that specific matter not to force re-reviwing this one once again.

sanket1729 · 2022-01-11T07:14:51Z

sanket1729 dismissed apoelstra’s stale review via 79874ea 41 seconds ago

I did not do that. I don't know how that happened :( . If it is because of merging #591, I don't know why my review was not dismissed.

sanket1729 · 2022-01-11T07:18:15Z

@dr-orlovsky, looks there are conflicts here now :( . Can you please separate the first commit into another PR? I will ACK the remaining PR asap

dr-orlovsky · 2022-01-11T07:46:30Z

@sanket1729 done: #772

Will rebase this one w/o that commit after #762 merge

nlanson

Left a small documentation suggestion.

nlanson · 2022-01-10T23:59:26Z

src/util/schnorr.rs

Small doc suggestion:

Since both implementations of the TapTweak trait describes what operations are done to the inputted key, I think the second half of the description here should be removed and have just the first two sentences left as a short overview. It could make the three descriptions not conflict/duplicate and make it less confusing.

sanket1729 · 2022-01-11T10:16:40Z

@dr-orlovsky , #762 merged

dr-orlovsky · 2022-01-11T12:12:04Z

@sanket1729 done

src/util/schnorr.rs

sanket1729

ACK 7405836

apoelstra

ACK 7405836

Kixunil

Some style comments otherwise looks good. Not rejecting but would like to see some answer to the comments before I ack.

Kixunil · 2022-01-11T15:18:25Z

src/blockdata/script.rs

+    /// Generates P2TR for key spending path for a known [`TweakedPublicKey`].
+    ///
+    /// NB: Make sure that the used key is indeed tweaked (for instance, it comes from `rawtr`
+    /// descriptor content); otherwise please use [`Script::new_v1_p2tr`] method.


I think this comment here is not needed since tweaking is tracked in the type. Such comment is more appropriate for assume_tweaked.

The commends are different between types, at least should be

I mean you don't have a "make sure this is dereferencable" comment on &T documentation because the type already guarantees it and you have that comment on the "constructor".

Kixunil · 2022-01-11T15:22:23Z

src/blockdata/script.rs

+
+    #[inline]
+    fn witness_version(&self) -> Option<WitnessVersion> {
+        WitnessVersion::from_opcode(self.0[0].into()).ok()


I think self.0.get(0)? would be better to pass the responsibility for checking to the caller.

src/util/schnorr.rs

Kixunil · 2022-01-11T15:36:10Z

src/util/schnorr.rs

+    fn tap_tweak<C: Verification>(self, secp: &Secp256k1<C>, merkle_root: Option<TapBranchHash>) -> TweakedKeyPair {
+        let pubkey = XOnlyPublicKey::from_keypair(&self);
+        let tweak_value = TapTweakHash::from_key_and_tweak(pubkey, merkle_root).into_inner();
+        let mut output_key = self.clone();


This clone is confusing, why not use mut self or let mut output_key = self?

Kixunil · 2022-01-11T15:39:35Z

src/util/schnorr.rs

+    }
+
+    fn dangerous_assume_tweaked(self) -> TweakedKeyPair {
+        TweakedKeyPair(self)


I think it'd be cleaner to implement these in terms of public API of TweakedPublicKey/TweakedKeyPair. Maybe even put them into separate modules to enforce this. Not very important but though.

Not sure I understood

I mean calling TweakedPublicKey::dangerous_assume_tweaked(self).

Still not get it. We can't call TweakedPublicKey::dangerous_assume_tweaked(self) since we need key pair, not a public key

Kixunil · 2022-01-11T15:41:31Z

src/util/schnorr.rs

+    #[inline]
+    pub fn into_inner(self) -> KeyPair {
+        self.0
+    }


Should we also impl From<TweakedKeyPair> for KeyPair?
Also maybe into_raw is a clearer name?

I think into_inner is fine. But yeah, having a From impl would be nice.

Will do a From impl in the follow-up PR

dr-orlovsky · 2022-01-11T15:55:06Z

@Kixunil @apoelstra I propose to merge this PR and address @Kixunil comments right away in a follow-up PR - otherwise I am afraid to have an endless cycle of re-ACKing due to a style/naming changes and API design play.

apoelstra · 2022-01-11T15:55:59Z

Ok by me.

dr-orlovsky · 2022-01-13T16:42:37Z

@Kixunil addressed all your comments in #778

@Kixunil

62a27a5 Document that serde impl of LeafVersion uses u8 in consensus encoding (Dr Maxim Orlovsky) 73e6ce4 Re-export Witness at crate level. Closes #770 (Dr Maxim Orlovsky) 6364ebd Code style fixups to taproot key functions (Dr Maxim Orlovsky) 7514f2c Tweaked -> untweaked keys conversions (Dr Maxim Orlovsky) Pull request description: This addresses @Kixunil review comments in #696 post-merge Update: also closes nits from issues #764 and #770 ACKs for top commit: Kixunil: ACK 62a27a5 sanket1729: utACK 62a27a5 Tree-SHA512: 2f10393abab41d1c82f4733d83bf85bd82e268a2891c156eb89744c0fc444fdfec4d60deec2dda6fde2e5881989625c18b9c5ca21d360018edba69b6d2a85eae

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

dr-orlovsky added this to the 0.28.0 milestone Nov 12, 2021

This was referenced Nov 12, 2021

BIP341 test vectors #695

Merged

P2TR improvements #646

Closed

dr-orlovsky changed the title ~~Taproot/improvements~~ Taproot improvements Nov 12, 2021

dr-orlovsky mentioned this pull request Nov 12, 2021

Tracking issue for Taproot #503

Closed

20 tasks

tcharding reviewed Nov 15, 2021

View reviewed changes

src/blockdata/script.rs Outdated Show resolved Hide resolved

src/blockdata/script.rs Outdated Show resolved Hide resolved

sanket1729 reviewed Nov 24, 2021

View reviewed changes

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

dr-orlovsky marked this pull request as draft November 25, 2021 10:04

dr-orlovsky mentioned this pull request Nov 25, 2021

Improvements to script methods related to Taproot #721

Merged

dr-orlovsky changed the title ~~Taproot improvements~~ Taproot tweaks generalization & KeyPair support Nov 25, 2021

dr-orlovsky requested a review from sanket1729 January 7, 2022 20:45

dr-orlovsky marked this pull request as ready for review January 7, 2022 20:45

sanket1729 reviewed Jan 8, 2022

View reviewed changes

dr-orlovsky requested a review from sanket1729 January 8, 2022 13:47

apoelstra reviewed Jan 8, 2022

View reviewed changes

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

apoelstra reviewed Jan 8, 2022

View reviewed changes

sanket1729 reviewed Jan 9, 2022

View reviewed changes

sanket1729 mentioned this pull request Jan 11, 2022

Implement taproot specific script helpers #604

Closed

sanket1729 dismissed apoelstra’s stale review via 79874ea January 11, 2022 07:13

This was referenced Jan 11, 2022

Rename inner key field in PrivateKey and PublicKey #762

Merged

Add FromStr implementation for a TweakedPublicKey #772

Closed

nlanson reviewed Jan 11, 2022

View reviewed changes

dr-orlovsky dismissed sanket1729’s stale review via 77dec44 January 11, 2022 12:11

dr-orlovsky requested review from apoelstra and sanket1729 January 11, 2022 12:12

apoelstra reviewed Jan 11, 2022

View reviewed changes

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

dr-orlovsky added 5 commits January 11, 2022 16:09

Generalizing taproot key tweaking for KeyPairs

599c5f9

Taproot-related methods for Script type

91b68a4

Making Script method new_* names more consistent

f77c571

CI: do not fail fast

f39b130

Fix warning about deprecated method use

7405836

sanket1729 approved these changes Jan 11, 2022

View reviewed changes

apoelstra approved these changes Jan 11, 2022

View reviewed changes

Kixunil reviewed Jan 11, 2022

View reviewed changes

sanket1729 merged commit 7d62277 into rust-bitcoin:master Jan 13, 2022

dr-orlovsky mentioned this pull request Jan 13, 2022

Fixups for taproot improvements #778

Merged

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

Conversation

dr-orlovsky commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dr-orlovsky commented Nov 23, 2021

Uh oh!

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

Uh oh!

Uh oh!

dr-orlovsky commented Nov 24, 2021

Uh oh!

Kixunil commented Nov 24, 2021

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kixunil commented Nov 25, 2021

Uh oh!

dr-orlovsky commented Nov 25, 2021

Uh oh!

dr-orlovsky commented Dec 2, 2021

Uh oh!

dr-orlovsky commented Jan 6, 2022

Uh oh!

dr-orlovsky commented Jan 7, 2022

Uh oh!

sanket1729 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dr-orlovsky commented Jan 8, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Jan 8, 2022

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dr-orlovsky commented Nov 12, 2021 •

edited

Loading

tcharding commented Nov 24, 2021 •

edited

Loading

sanket1729 left a comment •

edited

Loading

dr-orlovsky commented Jan 11, 2022 •

edited

Loading

sanket1729 commented Jan 11, 2022 •

edited

Loading