8000 Try to fix up sighash export mess by tcharding · Pull Request #1277 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Try to fix up sighash export mess #1277

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 24, 2022
Merged

Try to fix up sighash export mess #1277

merged 1 commit into from
Sep 24, 2022

Conversation

tcharding
Copy link
Member

Recently we moved a few types from transaction to sighash, while doing so I erroneously annotated code with the deprecated attribute hoping it would give downstream users a gentle upgrade experience. It turns out deprecated only works on functions.

During that same work, we re-exported from the crate root a bunch of types from the sighash module that probably should not have been re-exported. We are currently trying to create a nice clean API surface, in an effort to move in the right direction we should remove the re-exports and just re-export the sighash module.

Try to clean up the sighash export mess by doing:

  • Remove the re-exports from the transaction module
  • Remove crate level re-exports of sighash module types
  • Re-export sighash module

Note, this patch is a breaking API change, justified by the fact that there is no good way to gently lead downstream when moving types since types cannot be deprecated with the deprecated attribute.

@tcharding tcharding force-pushed the 09-15-sighash-exports branch 2 times, most recently from 7d5176c to eddceae Compare September 15, 2022 03:27
@tcharding tcharding added API break This PR requires a version bump for the next release release notes mention labels Sep 15, 2022
@tcharding
Copy link
Member Author

I think sighash should go in crypto/, all the uses of it in transaction are in deprecated code, the use in witness is a function that will go behind "crypto" feature flag if we add one (if crypto crate is optional). That leaves just psbt and ecdsa, which will depend on crypto and is in crypto respectively. Ok I convinced myself :)

@tcharding
Copy link
Member Author

ooo, I might have been wrong about witness, its using ecdsa module from secp256k1 not from crypto/.

bitcoin/src/blockdata/witness.rs:221: pub fn push_bitcoin_signature(&mut self, signature: &ecdsa::SerializedSignature, hash_type: EcdsaSighashType) {

@apoelstra
Copy link
Member

I don't think sighash can go in crypto because it depends on having access to the Transaction primitive.

AFAIK everything else in crypto is standalone and only needs hashing+libsecp.

@apoelstra
Copy link
Member

But ACK this PR -- pretty frustrating that we can't usefully deprecate types.

apoelstra
apoelstra previously approved these changes Sep 15, 2022
View reviewed changes
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 eddceae0fad0caa8a788e7ee331b9ab842c0ccc5

@tcharding tcharding force-pushed the 09-15-sighash-exports branch from eddceae to 1ca4607 Compare September 15, 2022 22:48
@tcharding
Copy link
Member Author

Rebase only, no other changes.

apoelstra
apoelstra previously approved these changes Sep 16, 2022
View reviewed changes
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 1ca4607f0f344da1bc3c94f10ec7fdd475a5dd0c

@tcharding tcharding force-pushed the 09-15-sighash-exports branch from 1ca4607 to a3347e3 Compare September 19, 2022 06:41
@tcharding
Copy link
Member Author

Rebase, also use crate::sighash for lines that are already touched by this PR (i.e., do not change util::sighash for lines outside of this PR).

@tcharding
Try to fix up sighash export mess
2001f44 
Recently we moved a few types from `transaction` to `sighash`, while
doing so I erroneously annotated code with the `deprecated` attribute
hoping it would give downstream users a gentle upgrade experience. It
turns out `deprecated` only works on functions.

During that same work, we re-exported from the crate root a bunch of
types from the `sighash` module that probably should not have been
re-exported. We are currently trying to create a nice clean API surface,
in an effort to move in the right direction we should remove the
re-exports and just re-export the `sighash` module.

Try to clean up the sighash export mess by doing:

- Remove the re-exports from the `transaction` module
- Remove crate level re-exports of `sighash` module types
- Re-export `sighash` module

Note, this patch is a breaking API change, justified by the fact that
there is no good way to gently lead downstream when moving types since
types cannot be deprecated with the `deprecated` attribute.
@tcharding tcharding force-pushed the 09-15-sighash-exports branch from a3347e3 to 2001f44 Compare September 19, 2022 06:47
@tcharding
Copy link
Member Author

Note please this PR removes the types from transaction without re-exporting them. I know it was suggested to comment and mention in release-notes but this release is going to be such a massive overhaul of the directory tree I think its not worth the extra trouble. Mentioning it explicitly to give you a chance to tell me to stop being lazy and do it @Kixunil :)

@Kixunil
Copy link
Collaborator
Kixunil commented Sep 19, 2022

OK, reexporting them doesn't even help that much because people could miss them anyway and various weird hacks I tried involving type aliases to dummy types don't work, so I don't mind just removing them. Especially since there's already a ton of breaking changes.

apoelstra
apoelstra approved these changes Sep 20, 2022
View reviewed changes
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 2001f44

Kixunil
Kixunil approved these changes Sep 22, 2022
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 2001f44

@apoelstra apoelstra merged commit 6101d8d into rust-bitcoin:master Sep 24, 2022
@tcharding tcharding deleted the 09-15-sighash-exports branch September 30, 2022 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

Assig 4FA9 nees
No one assigned
Labels
API break This PR requires a version bump for the next release release notes mention
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tcharding @apoelstra @Kixunil
0