-
Notifications
You must be signed in to change notification settings - Fork 619
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
P2TR improvements #646
P2TR improvements #646
Conversation
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'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?
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 pushing this. Some minor comments, and there is a build failure
src/blockdata/script.rs
Outdated
@@ -268,13 +271,21 @@ impl Script { | |||
} | |||
|
|||
/// 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_wpkh(pubkey_hash: &WPubkeyHash) -> Script { |
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.
Is there any special reason to make this breaking change? I really liked the v0
part, but I won't die on that hill :)
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.
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.
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.
Why would a soft fork be a breaking change?
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.
not soft fork, but existing API changes merged into rust-bitcoin after lat release
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.
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...
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.
Ok, returned v0
back :)
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.
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
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 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.
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.
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.
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.
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.
@Kixunil pls see my comment #646 (comment) |
Ready for the review (rebased) |
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 |
tACK 77a4447 Started some experiments using this API for creating P2TR contracts. So far, API is ergonomic. |
Rebased on master with fixed CI and no changes since 77a4447 |
@apoelstra pls review once you will have a time to have this before Taproot release |
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 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.
@@ -272,13 +275,20 @@ impl Script { | |||
} | |||
|
|||
/// 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 { |
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 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.
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 going to fail for many crates, and people would need to look at the source code to fix it.
} | ||
|
||
/// Create a pay to taproot address for a given single key, tweaking the key value with its own | ||
/// tagged hash according to BIP-342. |
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.
nit: It's BIP 341. Here and elsewhere
/// | ||
/// 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 { |
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.
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 .
/// [`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); |
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.
/// | ||
/// 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 { |
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 think there should only be one API. new_p2tr(secp, internal_key, Option<merkle_root>, network)
} | ||
|
||
/// Trait for enabling tweaking methods to BIP-340 keys according to the Taroot rules | ||
pub trait TaprootKey { |
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.
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.
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.
agree to re-base on #677
Superseded by #696 |
No description provided.