P2TR improvements by dr-orlovsky · Pull Request #646 · rust-bitcoin/rust-bitcoin

dr-orlovsky · 2021-09-09T09:25:53Z

No description provided.

Kixunil

I'm not that sure about removing _v0. It's a breaking change for cosmetic reasons and can we say there will never be e.g. _v5 version with both variants?

src/blockdata/script.rs

sanket1729

Thanks for pushing this. Some minor comments, and there is a build failure

sanket1729 · 2021-09-09T21:20:48Z

src/blockdata/script.rs

Is there any special reason to make this breaking change? I really liked the v0 part, but I won't die on that hill :)

The PR is breaking anyway at least because it is based on WitnessVersion PR which is highly intrusive. And even if we will have v5_ one day, it still will be a breaking change (caused by bitcoin softfork).

As of today, keeping v0_ seems to be pointless. If one day with v5_wpkh we will need it once more, again, that will be a breaking change and we will introduce v0_ once again.

Why would a soft fork be a breaking change?

not soft fork, but existing API changes merged into rust-bitcoin after lat release

Do we want to cause breaking changes every time there's a soft fork though? I'd like this crate to be stabilized at some point...

Ok, returned v0 back :)

Actually we have API inconsistency, when methods in Address do not have v0/v1 prefix, while methods in Script do have. I think we need to stick to a single way of naming @apoelstra @Kixunil

I personally like the explicitness of including version number but don't consider it a huge issue. If someone feels strongly about it go ahead and ignore me.

We can add v1_wpkh as an alias for tr if it makes people happy. The reason to have v0 in one case is that "wpkh" does not imply a version, while tr does.

To be clear, I'm fine with the current solution (keeping v0 and also having v1_tr). I just don't see the old situation as inconsistent.

src/blockdata/script.rs

dr-orlovsky · 2021-09-09T23:24:56Z

@Kixunil pls see my comment #646 (comment)

dr-orlovsky · 2021-09-26T08:46:39Z

Ready for the review (rebased)

dr-orlovsky · 2021-09-27T06:26:32Z

CI fails on unrelated bug in fuzztests, which I can't understand: https://github.com/rust-bitcoin/rust-bitcoin/pull/646/checks?check_run_id=3717024631#step:5:2501

GeneFerneau · 2021-09-29T19:56:41Z

tACK 77a4447

Started some experiments using this API for creating P2TR contracts. So far, API is ergonomic.

dr-orlovsky · 2021-09-30T12:59:09Z

Rebased on master with fixed CI and no changes since 77a44476806f05cab56ffd4d0da163a4364fe5ae

dr-orlovsky · 2021-11-02T10:45:53Z

@apoelstra pls review once you will have a time to have this before Taproot release

sanket1729

Thanks for this. I think if you have #677 (or provide your feedback there), this PR can be made much simpler because it already has support for tweaking.
Also left some comments on API naming and design.

sanket1729 · 2021-11-03T16:37:19Z

src/blockdata/script.rs

    /// Generates P2WPKH-type of scriptPubkey
-    pub fn new_v0_wpkh(pubkey_hash: &WPubkeyHash) -> Script {
-        Script::new_witness_program(WitnessVersion::V0, &pubkey_hash.to_vec())
+    pub fn new_v0_p2wpkh(pubkey_hash: &WPubkeyHash) -> Script {


I don't know what others think, but we can deprecate the old API instead of removing it? This will break things for me downstream, but I am okay with it.

This is going to fail for many crates, and people would need to look at the source code to fix it.

sanket1729 · 2021-11-03T16:39:35Z

src/util/address.rs

+    }
+
+    /// Create a pay to taproot address for a given single key, tweaking the key value with its own
+    /// tagged hash according to BIP-342.


nit: It's BIP 341. Here and elsewhere

sanket1729 · 2021-11-03T16:44:11Z

src/util/address.rs

+    ///
+    /// For generating P2TR addresses with tapscript please use [`Address::p2tr_script`] method.
+    #[inline]
+    pub fn p2tr_tweaked_key(tweaked_key: schnorrsig::PublicKey, network: Network) -> Address {


We should not offer this API at all. It is a discouraged practice and might be misused as mentioned in BIP-341. https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#cite_ref-22-0 .

sanket1729 · 2021-11-03T17:13:26Z

src/util/address.rs

+    /// [`Address::p2tr_internal_key`] and [`Address::p2tr_tweaked_key`] methods.
+    #[inline]
+    pub fn p2tr_script<C: Verification>(secp: &Secp256k1<C>, mut internal_key: schnorrsig::PublicKey, merkle_root: TapBranchHash, network: Network) -> Address {
+        internal_key.script_tweak(&secp, merkle_root);


This API is incorrect and can panic on user-crafted inputs. This also does not follow the BIP341 correctly. This should be H_taptweak(P||Hbranch(S)) .

I have implemented similar things in #677 . I think this PR can wait on #677 where you can directly use some of the functions there.

sanket1729 · 2021-11-03T17:15:15Z

src/util/address.rs

+    ///
+    /// For generating P2TR addresses with tapscript please use [`Address::p2tr_script`] method.
+    #[inline]
+    pub fn p2tr_internal_key<C: Verification + Signing>(secp: &Secp256k1<C>, mut internal_key: schnorrsig::PublicKey, network: Network) -> Address {


I think there should only be one API. new_p2tr(secp, internal_key, Option<merkle_root>, network)

sanket1729 · 2021-11-03T17:16:53Z

src/util/taproot.rs

+}
+
+/// Trait for enabling tweaking methods to BIP-340 keys according to the Taroot rules
+pub trait TaprootKey {


Is there any reason for implementing this for Keypair? I think if we wait for #677 to be merged, this PR can be made simpler.

agree to re-base on #677

dr-orlovsky · 2021-11-12T23:00:02Z

Superseded by #696

dr-orlovsky added help wanted API break This PR requires a version bump for the next release labels Sep 9, 2021

dr-orlovsky added this to the 0.28.0 milestone Sep 9, 2021

dr-orlovsky requested a review from apoelstra September 9, 2021 09:25

dr-orlovsky mentioned this pull request Sep 9, 2021

Tracking issue for Taproot #503

Closed

20 tasks

Kixunil reviewed Sep 9, 2021

View reviewed changes

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

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

sanket1729 reviewed Sep 9, 2021

View reviewed changes

dr-orlovsky marked this pull request as draft September 10, 2021 22:12

dr-orlovsky mentioned this pull request Sep 19, 2021

Taproot P2TR address #563

Merged

dr-orlovsky marked this pull request as ready for review September 26, 2021 08:44

dr-orlovsky requested a review from sanket1729 September 26, 2021 08:46

dr-orlovsky removed the help wanted label Sep 26, 2021

dr-orlovsky changed the title ~~Script handling of P2TR single-key addresses~~ P2TR improvements Sep 27, 2021

dr-orlovsky mentioned this pull request Sep 27, 2021

Taproot-related data types #666

Closed

Script handling of P2TR single-key addresses

9ef5b5b

sanket1729 reviewed Nov 3, 2021

View reviewed changes

dr-orlovsky linked an issue Nov 8, 2021 that may be closed by this pull request

Change Address::p2tr #687

Closed

dr-orlovsky closed this Nov 12, 2021

Conversation

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

Uh oh!

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

dr-orlovsky commented Sep 9, 2021

Uh oh!

dr-orlovsky commented Sep 26, 2021

Uh oh!

dr-orlovsky commented Sep 27, 2021

Uh oh!

GeneFerneau commented Sep 29, 2021

Uh oh!

dr-orlovsky commented Sep 30, 2021

Uh oh!

dr-orlovsky commented Nov 2, 2021

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!

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 Nov 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dr-orlovsky commented Sep 9, 2021 •

edited

Loading