Use un/tweaked public key types #728

tcharding · 2021-11-29T02:23:04Z

We have two types for tweaked/untweaked schnorr public keys to help users of the taproot API not mix these two keys up. Currently the taproot module uses 'raw' schnoor::PublicKeys.

Use the schnoor module's tweak/untweaked public key types for the taproot API.

Fixes: #725

Please note, I saw this was labeled 'good-first-issue' but I ignored that and greedily implemented a solution because of two reasons

We want to get taproot stuff done post haste.
I'm struggling to follow what is going on with all the taproot work so this seemed like a way to get my hands dirty.

sanket1729

ACK 3351927, left a small nit

src/util/taproot.rs

Kixunil · 2021-12-01T23:02:27Z

src/util/taproot.rs

@@ -268,7 +268,7 @@ impl TaprootSpendInfo {
            internal_key: internal_key,
            merkle_root: merkle_root,
            output_key_parity: parity,
-            output_key: output_key,
+            output_key: TweakedPublicKey::new(output_key),


This constructor should've been called dangerous_assume_tweaked :(

Yup agreed. I was going to bring this in this review. Don't know how it skipped my review :( during the original one.

Along with changing the new API to dangerous_assume_tweaked, we can also the logic here to that output_key is computed using the taptweak method instead of manually tweaking as implemented here. That way output_key would be of the required type directly.

To implement these suggestions I added 4 additional patches to this PR. The first two of which are cleanup

Remove comments: d84e359

Use debug_assert! instead of unreachable!: 8424aa1

Implement @Kixunil suggestion, rename constructor: 1f71b75

Implement @sanket1729 suggestion, use tap_tweak 6f25d0c but to do so I return the parity from tap_tweak also. This changes the taproot API, please review accordingly.

sanket1729

ACK ec24418. Super clean, no comments :)

Kixunil

ACK ec24418

The only issue I have can (and probably should) be addressed in a separate PR.

Kixunil · 2021-12-02T11:17:03Z

src/util/schnorr.rs

+    ///
+    /// # Returns
+    /// The tweaked key and its parity.
+    fn tap_tweak<C: Verification>(&self, secp: &Secp256k1<C>, merkle_root: Option<&TapBranchHash>) -> (TweakedPublicKey, bool);


bool is not very clear value for parity. Which is which? I believe it'd be much better to have:

enum KeyParity { Even, Odd, }

However, I think this can go to a separate PR given that parity: bool was used already.

I'll do the PR, thanks.

lol, I started doing this over in rust-secp256k1 and I do not know which is which. Is true == Even or Odd?

rust-bitcoin/rust-secp256k1#344

dr-orlovsky · 2021-12-02T11:55:10Z

Needs rebase :(

dr-orlovsky

Can you pls squash the commits into one? This is usual requirement for trivial commits by @apoelstra and I do not think that we should diverge from it. Why this is an important? Since we need to test each of the commits to compile and pass all tests, and having commits changing just one line of code makes this really computationally expansive. Another thing is that these all commits introduce the same change, so it probably best in terms of history and blame to have these change done in just a one commit.

src/util/schnorr.rs

src/util/taproot.rs

sanket1729 · 2021-12-03T10:57:41Z

I disagree over here. I like the structure of commits because they are different and do things in an atomic way. Maybe Add newline to end of file and Remove 'what' comments can go into one refactor commit, but apart from that the I think other commits are atomic changes that do one thing.

Having atomic commits really does aid in my review, and I think it would have been (slightly) more difficult to review if all were in one commit.

Since we need to test each of the commits to compile and pass all tests, and having commits changing just one line of code makes this really computationally expansive.

We do want this, but IMO this should never come at the cost of atomic commits. Lines of code have nothing to do with commits, and bitcoin core is full of commits with 10 line descriptions and a few line diffs.

Infact, I think we should include the text from https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches in #587.

Kixunil · 2021-12-03T11:46:00Z

Actually an additional commit increases the complexity of bisect using formula log2(n + 1) where n is number of previous commits. So by very small amount. One would have to double the number of all commits to require one more compilation.

tcharding · 2021-12-08T20:23:54Z

Can you pls squash the commits into one? This is usual requirement for trivial commits by @apoelstra and I do not think that we should diverge from it.

I'm happy to argue this one over a beer with @apoelstra but I feel pretty strongly that trivial changes should not be done as part of other work, its jarring for reviewers to have to stop and think 'why is that whitespace added there'.

Why this is an important? Since we need to test each of the commits to compile and pass all tests, and having commits changing just one line of code makes this really computationally expansive.

I rekon we should optimize for reviewers not for machine running time on CI, reviewers are expensive machines are cheap.

Another thing is that these all commits introduce the same change, so it probably best in terms of history and blame to have these change done in just a one commit.

I guess this is subjective to a certain extent, all PRs can be described, at some level, as 'improve xyz'. But like I say above I think it makes review easier if there are many small commits, when folks can trust that my commits don't do strange things its easier for them to catch my strange and silly mistakes.

Idiomatic UNIX file handling leaves files with a newline at the end. Add newline to end of `schnorr` module.

All new types in `rust-bitcoin` should use our standard set of derives. Add said standard derives to `TweakedPublickKey`.

We have two types for tweaked/untweaked schnorr public keys to help users of the taproot API not mix these two keys up. Currently the `taproot` module uses 'raw' `schnoor::PublicKey`s. Use the `schnoor` module's tweak/untweaked public key types for the `taproot` API.

When used, code comments should say _why_ we do something not _what_ we do, the code already says what we do. Remove 'what we do' style comments.

We currently run `tweak_add_check` and use the result as a conditional branch, the error path of which uses `unreachable`. This usage of `unreachable` is non-typical. An 'unreachable' statement is by definition supposed to be unreachable, it is not clear why we would need to have a conditional branch to check an unreachable statement. Use `debug_assert!` so programmer errors get caught in un-optimised builds but in optimised builds the call to `tweak_add_check` is not even done.

Keeping inline with the method on `UntweakedPublicKey` that outputs a `TweakedPublicKey` we can use the same name, for the same reasons. Use `dangerous_assume_tweaked` as the constructor name to highlight the fact that this constructor should probably not be being used.

Currently we calculate the parity during `tap_tweak` but do not return it, this means others must re-do work done inside `tap_tweak` in order to calculate the parity. We can just return the parity along with the tweaked key.

Improve the docs by doing: - Use [`Foo`] for types - Use third person tense - Add trailing periods

dr-orlovsky · 2021-12-11T20:28:07Z

utACK b5bf6d7

I just told about my experience in working with this repo for several years: my multi-commit PRs were frequently required to be squashed. I also prefer to have atomic/simple commits.

sanket1729 · 2021-12-12T05:35:06Z

I would be really surprised if Andrew is against this. What specifically Andrew mentioned (quite a few times on different PRs from different authors) is one should squash changes in a PR that you have already changed in the same PR. I think most people(including me) who review commit by commit and find it extra work when something is implemented that is only changed again. I don't mean any of this as any critique of your work, but am just trying to arrive at a consensus on atomic commits.
As an example: https://github.com/rust-bitcoin/rust-bitcoin/pull/499/commits

PSBT: adding global types (version, xpub)
PSBT: merging new global keys
PSBT: Key pair serialization for new global keys 
Improving PSBT merge with dedicated 1.29 rustc borrow scope
Nits in new PSBT global types reviews
BSPT: Improving global xpub merging algorithm  
PSBT: Improved global keys version and xpub handling
PSBT global xpub merging algorithm reworked

The first three commits are only what is required for the PR. The other commits make changes to the code introduced in the same PR and belong to the previous commits. I understand that sometimes it might be annoying to fix something in a specific commit. On a similar rationale, I had requested to squash only the fixups in the contributing.md PR (that were correctly addressed in the same PR). This is not a hard rule, but based on the complexity of the PR, already existing ACKs from reviewers, subjective decisions can be made to merge fixup commits.

I am sorry for pointing to specific examples, but if you were asked to squash commits it should have been for this reason. If not, it is likely an incorrect recommendation that we should not follow in the future.

sanket1729

ACK b5bf6d7

dr-orlovsky · 2021-12-12T06:27:37Z

@sanket1729 ok, I see, thank you for the explanation.

I do not know why github does not show my ACK above as a review approval. Trying once again...

dr-orlovsky

utACK b5bf6d7

sanket1729 previously approved these changes Nov 30, 2021

View reviewed changes

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

Kixunil reviewed Dec 1, 2021

View reviewed changes

tcharding dismissed sanket1729’s stale review via 6f25d0c December 2, 2021 03:47

tcharding force-pushed the use-TweakedPublicKey branch 2 times, most recently from 6f25d0c to ec24418 Compare December 2, 2021 03:54

tcharding requested review from sanket1729 and Kixunil December 2, 2021 03:54

sanket1729 previously approved these changes Dec 2, 2021

View reviewed changes

Kixunil previously approved these changes Dec 2, 2021

View reviewed changes

Kixunil added the waiting for author This can only progress if the author responds to a request. label Dec 2, 2021

tcharding dismissed stale reviews from Kixunil and sanket1729 via 57ae7da December 3, 2021 03:06

tcharding force-pushed the use-TweakedPublicKey branch from ec24418 to 57ae7da Compare December 3, 2021 03:06

dr-orlovsky reviewed Dec 3, 2021

View reviewed changes

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

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

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

tcharding added 8 commits December 10, 2021 11:37

Add newline to end of file

9c015d9

Idiomatic UNIX file handling leaves files with a newline at the end. Add newline to end of `schnorr` module.

Add standard derives to TweakedPublickKey

402bd99

All new types in `rust-bitcoin` should use our standard set of derives. Add said standard derives to `TweakedPublickKey`.

Remove 'what' comments

d8e42d1

When used, code comments should say _why_ we do something not _what_ we do, the code already says what we do. Remove 'what we do' style comments.

Return parity when doing tap_tweak

a6d3514

Currently we calculate the parity during `tap_tweak` but do not return it, this means others must re-do work done inside `tap_tweak` in order to calculate the parity. We can just return the parity along with the tweaked key.

Improve rustdocs on schnorr module

b5bf6d7

Improve the docs by doing: - Use [`Foo`] for types - Use third person tense - Add trailing periods

tcharding force-pushed the use-TweakedPublicKey branch from 57ae7da to b5bf6d7 Compare December 10, 2021 01:08

sanket1729 approved these changes Dec 12, 2021

View reviewed changes

dr-orlovsky approved these changes Dec 12, 2021

View reviewed changes

dr-orlovsky merged commit ed40f3d into rust-bitcoin:master Dec 12, 2021

tcharding mentioned this pull request Jan 10, 2022

Fix the mess around Parity rust-bitcoin/rust-secp256k1#370

Closed

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

tcharding deleted the use-TweakedPublicKey branch August 17, 2023 02:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use un/tweaked public key types #728

Use un/tweaked public key types #728

tcharding commented Nov 29, 2021

sanket1729 left a comment

Kixunil Dec 1, 2021

sanket1729 Dec 1, 2021

tcharding Dec 2, 2021

sanket1729 left a comment

Kixunil left a comment

Kixunil Dec 2, 2021

tcharding Dec 2, 2021

tcharding Dec 3, 2021

tcharding Dec 3, 2021

dr-orlovsky commented Dec 2, 2021

dr-orlovsky left a comment

sanket1729 commented Dec 3, 2021 •

edited

Loading

Kixunil commented Dec 3, 2021

tcharding commented Dec 8, 2021

dr-orlovsky commented Dec 11, 2021

sanket1729 commented Dec 12, 2021

sanket1729 left a comment

dr-orlovsky commented Dec 12, 2021

dr-orlovsky left a comment

Use un/tweaked public key types #728

Use un/tweaked public key types #728

Conversation

tcharding commented Nov 29, 2021

sanket1729 left a comment

Choose a reason for hiding this comment

Kixunil Dec 1, 2021

Choose a reason for hiding this comment

sanket1729 Dec 1, 2021

Choose a reason for hiding this comment

tcharding Dec 2, 2021

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

Kixunil Dec 2, 2021

Choose a reason for hiding this comment

tcharding Dec 2, 2021

Choose a reason for hiding this comment

tcharding Dec 3, 2021

Choose a reason for hiding this comment

tcharding Dec 3, 2021

Choose a reason for hiding this comment

dr-orlovsky commented Dec 2, 2021

dr-orlovsky left a comment

Choose a reason for hiding this comment

sanket1729 commented Dec 3, 2021 • edited Loading

Kixunil commented Dec 3, 2021

tcharding commented Dec 8, 2021

dr-orlovsky commented Dec 11, 2021

sanket1729 commented Dec 12, 2021

sanket1729 left a comment

Choose a reason for hiding this comment

dr-orlovsky commented Dec 12, 2021

dr-orlovsky left a comment

Choose a reason for hiding this comment

sanket1729 commented Dec 3, 2021 •

edited

Loading