Skip to content

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

Merged
dr-orlovsky merged 1 commit intorust-bitcoin:masterfrom
nlanson:master
Dec 2, 2021
Merged

Use TapTweakHash::from_key_and_tweak() method in computing tweak for UntweakedPublicKey#697
dr-orlovsky merged 1 commit intorust-bitcoin:masterfrom
nlanson:master

Conversation

@nlanson
Copy link
Copy Markdown
Contributor

@nlanson nlanson commented Nov 13, 2021

Quick follow up PR to #691 using a method from #677.

Changes

  • Updated UntweakedPublicKey::tap_tweak(...) to use TapTweakHash::from_key_and_tweak(...)

dr-orlovsky
dr-orlovsky previously approved these changes Nov 13, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

utACK 9e2bad304c36cd2d314db11b31d11be3b8f703c7

@dr-orlovsky dr-orlovsky added this to the 0.28.0 milestone Nov 13, 2021
sanket1729
sanket1729 previously approved these changes Nov 16, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK 9e2bad304c36cd2d314db11b31d11be3b8f703c7

Kixunil
Kixunil reviewed Nov 16, 2021
View reviewed changes
src/util/schnorr.rs Outdated
Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil Nov 16, 2021

Choose a reason for hiding this comment

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

Isn't clone() expensive?

Copy link
Copy Markdown
Member

@sanket1729 sanket1729 Nov 16, 2021

Choose a reason for hiding this comment

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

schnorr::PublicKey is Copy, I think it is the same.

Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil Nov 16, 2021

Choose a reason for hiding this comment

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

In that case .clone() is redundant and I don't think it should be used.

Copy link
Copy Markdown
Contributor Author

@nlanson nlanson Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

TapTweakHash::from_key_and_tweak(...) takes in schnorrsig::PublicKey where as the function it is being called in has &UntweakedPublicKey.

I used the .clone() to get around the borrow checker.

Should I instead pass in *self to the method or should I modify TapTweakHash::from_key_and_tweak(...) to take in &schnorrsig::PublicKey as a parameter? Or maybe I can change tap_tweak(...) to consume self?

Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil Nov 17, 2021

Choose a reason for hiding this comment

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

I think taking self by value everywhere is the correct approach. The compiler should insert references for optimization if needed.

Copy link
Copy Markdown
Contributor Author

@nlanson nlanson Nov 17, 2021

Choose a reason for hiding this comment

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

Fixed in the latest commit and squashed.

@sanket1729 sanket1729 dismissed their stale review November 16, 2021 16:13

Requesting to remove the clone as KixUnil suggested

@sanket1729 sanket1729 mentioned this pull request Nov 17, 2021
Merged
dr-orlovsky added a commit that referenced this pull request Nov 23, 2021
@dr-orlovsky
Merge #707: P2tr fixes
435298c 
e4774e7 fixups to taptweaking code (sanket1729)

Pull request description:

  This was my bad for not clearly stating the expected spec #687 . Changed values to references so that we only take ownership where it is required.

  This should simplify the #697

ACKs for top commit:
  Kixunil:
    ACK e4774e7
  dr-orlovsky:
    utACK e4774e7

Tree-SHA512: adacbfa8a77f46b2c85720f3760ed12a437f40d8422731d0207662d7947c95dda79d576923f6056c77f57977a3dcd25afd270f0ee11e9c3be9d067ccdc63371a
@dr-orlovsky
Copy link
Copy Markdown
Collaborator

dr-orlovsky commented Nov 23, 2021

Needs rebase

@nlanson
Copy link
Copy Markdown
Contributor Author

nlanson commented Nov 24, 2021

I have tried to rebase but noticed that in #707 , the merkle_root parameter for Address::p2tr has been updated to take in Option<&TapBranchHash> but in TapTweakHash::from_key_and_tweak() it is Option<TapBranchHash> without the reference as it's parameter.

Should TapTweakHash::from_key_and_tweak() be updated and changes reflected accross taproot.rs (a couple of spots need to be updated to use reference) or is this out of the scope of the PR?

@dr-orlovsky
Copy link
Copy Markdown
Collaborator

dr-orlovsky commented Nov 24, 2021

Generally I think we should take Option<TapBranchHash> and pass it by value. First, it's small enough and meets our agreement on pass-by-value we discussed recently; secondly it will be passed as Some(hash) and not variable to this API, so it does not make sense to force users to write Some(&hash) instead

@dr-orlovsky
Copy link
Copy Markdown
Collaborator

dr-orlovsky commented Nov 24, 2021

@nlanson: #695 (review)

@nlanson
Copy link
Copy Markdown
Contributor Author

nlanson commented Nov 24, 2021

Updated the following methods to take Option<TapBranchHash> as value instead of reference:

  • TapTweak::tap_tweak(...)
  • Address::p2tr(...)

dr-orlovsky
dr-orlovsky approved these changes Nov 25, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

utACK 5b21a9c

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Nov 30, 2021

ACK 5b21a9c

(sorry for forgetting to add this into approve message - IDK if relevant)

@dr-orlovsky dr-orlovsky merged commit 95cf9b0 into rust-bitcoin:master Dec 2, 2021
@dr-orlovsky dr-orlovsky mentioned this pull request Jan 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sanket1729 sanket1729 sanket1729 left review comments

@Kixunil Kixunil Kixunil approved these changes

+1 more reviewer

@dr-orlovsky dr-orlovsky dr-orlovsky approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.28.0

Development

Successfully merging this pull request may close these issues.

4 participants

@nlanson @dr-orlovsky @Kixunil @sanket1729