Skip to content

Conversation

@uncomputable
Copy link
Contributor

Updates TapTweak to latest rust-bitcoin and adds TweakedKeyPair.

@uncomputable
Copy link
Contributor Author

We could also update the methods of TweakedPublicKey if we want.

@uncomputable
Copy link
Contributor Author

The syn-2.0.13 dependency seems to use the 2021 edition, which is not understood by rust 1.41.1.

@sanket1729
Copy link
Contributor

This can be rebased to fix CI


/// A trait for tweaking Schnorr public keys
/// A trait for tweaking Schnorr key types (x-only public keys and key pairs).
pub trait TapTweak {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 063a8f5:

Why do we even have this trait? Why not use the one from Bitcoin?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with this entire module, really.

@apoelstra
Copy link
Member

In general, I'm confused about the value of this PR, which seems to just copy types and traits from rust-bitcoin 0.30. We can just reexport these.

@uncomputable
Copy link
Contributor Author

uncomputable commented Apr 13, 2023

The difference between this trait and the one from rust-bitcoin is that tweaking is done with elements::taproot::TapBranchHash. This little detail is really annoying when working with elements::taproot::TaprootSpendInfo.

@apoelstra
Copy link
Member

Gotcha, ok.

Lemme spend a bit more time thinking if there's a better way to keep these crates in sync. But maybe not, and this PR should go in.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 7b3f081

@uncomputable
Copy link
Contributor Author

The code duplication is annoying, and we could avoid it by casting between hash types. But then again the only reason these newtypes exist is to prevent casting! One could cast between "equivalent" hashes, like TapBranchHash, but this equivalence might be hard to determine.

@apoelstra
Copy link
Member

Casting hashtypes would not be sufficient to eliminate the code duplication. In general, we need to do an overhaul of this library to bring it closer to rust-bitcoin, but Sanket and I decided to wait for rust-bitcoin 1.0 (or at least, for it to quiet down a lot) before trying this.

@apoelstra apoelstra merged commit 6606a0d into ElementsProject:master Apr 14, 2023
@uncomputable uncomputable deleted the tap_tweak branch May 11, 2023 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants