Re-work sighash type conversion methods by tcharding · Pull Request #796 · rust-bitcoin/rust-bitcoin

tcharding · 2022-01-18T01:39:31Z

This PR has evolved into a full blown clean up of the conversion methods for all three sighash types based on the discussion below.

Everything is split up into very small patches to aid review (and bikeshedding, this PR is almost totally just naming things).

EDIT: I'm not convinced this should be an RC-fix, the changes are too wide spread now. It started as just a single method rename. Leaving label as is for others to consider.

sanket1729 · 2022-01-18T01:40:59Z

This is also good, I also wouldn't mind nuking the inner method. Your preference.

sanket1729

utACK 5efa0067344cbc9b03cc6c5f70729254e82a04c5

tcharding · 2022-01-18T01:42:24Z

Let's nuke it :)

sanket1729

ACK bec898e055d3b4024aa5221396dd5153b426557c

Kixunil

I think this deserves at least a comment but wouldn't block it.

src/util/psbt/map/input.rs

src/util/psbt/serialize.rs

apoelstra

ACK f627f67626f3640af01c368094043a83e7d95c5c

tcharding · 2022-01-27T01:49:44Z

@apoelstra please re-ack, if you agree, PR now uses to_u32_standard. Thanks.

Kixunil

ACK bb17582862146bdb83611e54c917e845f7a4241c

apoelstra

ACK bb17582862146bdb83611e54c917e845f7a4241c

I think the new name is a good idea, given our current crappy handling of non-standard flags.

tcharding · 2022-01-31T02:15:14Z

ouch, I was on the wrong branch and rebased on master. Sorry fellas, no changes made. Please re-ack and give me a virtual slap.

Kixunil

ACK 9de614e6d9914194020861765c3739ce8e1cfcbc

apoelstra

ACK 9de614e6d9914194020861765c3739ce8e1cfcbc

tcharding · 2022-02-16T08:47:57Z

I believe this one should go in before we do the taproot release, friendly bump :)

sanket1729 · 2022-02-17T07:55:24Z

Thanks for the bump, I have been exclusively looking at taproot release PRs. Feel free to tag me for review.

I will look at your amazing PRs soon :) , but so far I was only tunnel visioning on 0.28 milestone. I missed the RC fix label.

Edit: Phone autocorrect "amazing PRs" to ~~amazing places~~ . Hopefully you did not see that :P

sanket1729 · 2022-02-17T08:06:08Z

I think the new name is a good idea, given our current crappy handling of non-standard flags.

This is not true. It is possible to have non-standard values if they are explicitly constructed from serialize::Deserialize with non-standard u32. I would rather it be called to_u32.

The purpose of PsbtSigHashType is to capture non-standard sighash types, but users would need their own parsing code to deal with it. All high level APIs like ecdsa_hash_ty and schnorr_hash_ty will fail and the user would need to parse the sighash byte separately and deal with it. No information should be lost in ser/de using psbt serialize methods.

apoelstra · 2022-02-17T14:20:23Z

Ok, given @sanket1729's comments I rescind my ACK

Kixunil · 2022-02-17T15:16:21Z

It is possible to have non-standard values if they are explicitly constructed from serialize::Deserialize with non-standard u32.

This sounds massively broken. Either we should enforce it's standard in all constructors or in neither. I believe we agreed standard is better?

tcharding · 2022-03-23T02:02:02Z

Changes in force push:

I added back ticks to a u32 in the docs change in 06e7b09 Rename as_u32 -> to_u32.
rebased

While checking over the diff before force pushing I noticed that this PR adds a From<&str> method that immediately calls to_string() on the argument. Do you want me to change this to a From<String> method @Kixunil and then call to_string at the call site? (Noticed while reviewing: #898)

tcharding · 2022-03-24T00:13:05Z

This PR had a bunch of unrelated changes to the SchnorrSigHashType, these were conflicting with work being done by @dr-orlovsky in #898. I've pulled all those changes out and put them in a separate PR so we can more easily handle the conflicts.

sanket1729

ACK 51ac67356d6b581a99f4775546962a964c18b77b. In the interest of moving things forward, I think we should merge this after it meets the requirements(2 ACKs and no Nacks/comments for other reviewers).

We can address @Kixunil new comments in follow-up PR after the next RC or if required in the next breaking release.

dr-orlovsky · 2022-03-24T06:13:38Z

ACK 51ac67356d6b581a99f4775546962a964c18b77b

Not merging yet to give @apoelstra and @Kixunil another day to comment.

dr-orlovsky · 2022-03-27T11:25:49Z

ouh, this now requires rebase :( :( :(

sanket1729 · 2022-03-27T23:26:17Z

I think we should focus on getting this in before any other merge now :)

We have conversion functions that include suffixes `_consensus` and `_standard` to make it explicit what guarantees are provided by the returned `u32` value. The `From` implementation reduces the clarity of the API.

Rust naming conventions stipulate that conversion methods from owned -> owned for `Copy` types use the naming convention `to_`. This change makes the function name objectively better, however it makes no claims of being the 'best' name. We have had much discussion on using `to_standard` vs `to_u32` but are unable to reach consensus.

`EcdsaSigHashType::from_u32` was deprecated in v0.26, since we are working on the v0.28 release we can drop this method.

The functions `from_u32_standard` and `from_u32_consensus` smell a bit like hungarian notation. We can look at the method definition to see that the methods accept `u32` arguments without mentioning that in the method names. Remove `_u32_` from the method names. This brings the `from_*` methods in line with the `to_standard` method also.

Improve the `PsbtSigHashType` conversion methods by doing: - Re-name `inner` -> `to_u32` as per Rust convention - Add `from_u32` method Note, we explicitly do _not_ use suffix 'consensus' because these conversion methods make no guarantees about the validity of the underlying `u32`.

It is possible, although not immediately obvious, that it is possible to create a `PsbtSigHashType` with a non-standard value. Add a unit test to show this and also catch any regressions if we accidental change this logic.

tcharding · 2022-03-27T23:46:21Z

Force push is rebase only, no other changes.

tcharding · 2022-03-27T23:47:47Z

@Kixunil where have you gone man, are you ok with this one? Its not perfect but is it acceptable?

sanket1729

utACK 8e2422f.

dr-orlovsky

ACK 8e2422f

dr-orlovsky · 2022-03-28T07:47:41Z

Merging now: if there will be major comments from other reviewers will do a follow-up PR. We waited for more than a week for the comments already.

Post-merge #796 follow-up. Feel free to add other changes/nits which hadn't get into #796.

tcharding · 2022-03-28T08:44:43Z

BOOM!

c3d30d5 Remove deprecated method use for sighash conversion (Dr. Maxim Orlovsky) Pull request description: Post-merge #796 follow-up. Feel free to add other changes/nits which hadn't get into #796. ACKs for top commit: tcharding: ACK c3d30d5 sanket1729: ACK c3d30d5 apoelstra: ACK c3d30d5 Tree-SHA512: 7418f90bad9ce43b5504a3e45f06fecd636f62f2f7bd75bfc27e29faa181202595ed9b5175866e0fce01a301ea34c2b07afb16e658757215823965e7e1440c2e

sanket1729 previously approved these changes Jan 18, 2022

View reviewed changes

tcharding dismissed sanket1729’s stale review via bec898e January 18, 2022 01:44

tcharding force-pushed the as_u32 branch from 5efa006 to bec898e Compare January 18, 2022 01:44

sanket1729 previously approved these changes Jan 18, 2022

View reviewed changes

Kixunil reviewed Jan 18, 2022

View reviewed changes

src/util/psbt/map/input.rs Outdated Show resolved Hide resolved

src/util/psbt/serialize.rs Outdated Show resolved Hide resolved

tcharding dismissed sanket1729’s stale review via f627f67 January 19, 2022 00:55

tcharding force-pushed the as_u32 branch from bec898e to f627f67 Compare January 19, 2022 00:55

tcharding changed the title ~~Use function name as_32 instead of inner~~ Use function name to_32 instead of inner Jan 19, 2022

RCasatta changed the title ~~Use function name to_32 instead of inner~~ Use function name to_u32 instead of inner Jan 19, 2022

apoelstra previously approved these changes Jan 19, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via bb17582 January 27, 2022 01:48

tcharding force-pushed the as_u32 branch from f627f67 to bb17582 Compare January 27, 2022 01:48

Kixunil previously approved these changes Jan 27, 2022

View reviewed changes

apoelstra previously approved these changes Jan 27, 2022

View reviewed changes

tcharding dismissed stale reviews from apoelstra and Kixunil via 9de614e January 31, 2022 02:12

tcharding force-pushed the as_u32 branch from bb17582 to 9de614e Compare January 31, 2022 02:12

Kixunil previously approved these changes Jan 31, 2022

View reviewed changes

apoelstra previously approved these changes Jan 31, 2022

View reviewed changes

tcharding added the RC fix label Feb 16, 2022

sanket1729 added this to the 0.28.0 milestone Feb 17, 2022

tcharding dismissed stale reviews from dr-orlovsky and sanket1729 via ff811c5 March 23, 2022 01:52

tcharding force-pushed the as_u32 branch 2 times, most recently from ff811c5 to 9b1ee35 Compare March 23, 2022 01:59

tcharding force-pushed the as_u32 branch from 9b1ee35 to 51ac673 Compare March 23, 2022 23:59

sanket1729 previously approved these changes Mar 24, 2022

View reviewed changes

tcharding added 6 commits March 28, 2022 10:43

Remove From<EcdsaSigHashType> for u32

2bd71c3

We have conversion functions that include suffixes `_consensus` and `_standard` to make it explicit what guarantees are provided by the returned `u32` value. The `From` implementation reduces the clarity of the API.

Remove deprecated conversion method

5646826

`EcdsaSigHashType::from_u32` was deprecated in v0.26, since we are working on the v0.28 release we can drop this method.

Add unit test for deserialize non-standard sighash

8e2422f

It is possible, although not immediately obvious, that it is possible to create a `PsbtSigHashType` with a non-standard value. Add a unit test to show this and also catch any regressions if we accidental change this logic.

tcharding dismissed sanket1729’s stale review via 8e2422f March 27, 2022 23:43

tcharding force-pushed the as_u32 branch from 51ac673 to 8e2422f Compare March 27, 2022 23:43

sanket1729 approved these changes Mar 28, 2022

View reviewed changes

dr-orlovsky approved these changes Mar 28, 2022

View reviewed changes

dr-orlovsky merged commit 6417c37 into rust-bitcoin:master Mar 28, 2022

dr-orlovsky added a commit that referenced this pull request Mar 28, 2022

Remove deprecated method use for sighash conversion

c3d30d5

Post-merge #796 follow-up. Feel free to add other changes/nits which hadn't get into #796.

dr-orlovsky mentioned this pull request Mar 28, 2022

Remove deprecated method use for sighash conversion #913

Merged

tcharding deleted the as_u32 branch April 4, 2022 22:29

Conversation

tcharding commented Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanket1729 commented Jan 18, 2022

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Jan 18, 2022

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Jan 27, 2022

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Jan 31, 2022

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Feb 16, 2022

Uh oh!

sanket1729 commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanket1729 commented Feb 17, 2022

Uh oh!

apoelstra commented Feb 17, 2022

Uh oh!

Kixunil commented Feb 17, 2022

Uh oh!

tcharding commented Mar 23, 2022

Uh oh!

tcharding commented Mar 24, 2022

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Mar 24, 2022

Uh oh!

dr-orlovsky commented Mar 27, 2022

Uh oh!

sanket1729 commented Mar 27, 2022

Uh oh!

tcharding commented Mar 27, 2022

Uh oh!

tcharding commented Mar 27, 2022

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Mar 28, 2022

Uh oh!

Reviewers

Assignees

Labels

tcharding commented Jan 18, 2022 •

edited

Loading

sanket1729 commented Feb 17, 2022 •

edited

Loading

dr-orlovsky commented Mar 28, 2022 •

edited

Loading