Skip to content
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.

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

Derive Hash on a bunch of types #990

Merged
merged 1 commit into from Jul 17, 2022

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented May 6, 2022

In preparation for being able to derive Hash on all types in miniscript, derive Hash on all of the required types.

This PR includes all the changes in https://github.com/rust-bitcoin/rust-bitcoin/pull/933/files and hence supersedes it.

ref: rust-bitcoin/rust-miniscript#226

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.

Concept ACK, but hashes of secret data need to be removed or protected.

src/util/key.rs Outdated
@@ -235,7 +235,7 @@ impl FromStr for PublicKey {
}

/// A Bitcoin ECDSA private key
#[derive(Copy, Clone, PartialEq, Eq)]
#[derive(Copy, Clone, PartialEq, Eq, Hash)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is dangerous, see rust-bitcoin/rust-secp256k1#471

Copy link
Member Author

Choose a reason for hiding this comment

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

Will remove, cheers.

@@ -45,7 +45,7 @@ impl_array_newtype!(Fingerprint, u8, 4);
impl_bytes_newtype!(Fingerprint, 4);

/// Extended private key
#[derive(Copy, Clone, PartialEq, Eq)]
#[derive(Copy, Clone, PartialEq, Eq, Hash)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also dangerous.

In preparation for being able to derive `Hash` on all types in
`miniscript`, derive `Hash` on all of the required types.
@Kixunil
Copy link
Collaborator

Kixunil commented Jul 15, 2022

I'd ACK this, why is it a draft?

@tcharding tcharding marked this pull request as ready for review July 16, 2022 00:11
@tcharding
Copy link
Member Author

Was waiting for CI run to finish before taking off draft and forgot about it.

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 ef7fef0

IMO no need to wait for CI before undrafting next time.

@Kixunil Kixunil added enhancement trivial Obvious, easy and quick to review (few lines or doc-only...) one ack PRs that have one ACK, so one more can progress them labels Jul 16, 2022
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 ef7fef0

@apoelstra apoelstra merged commit 06a777d into rust-bitcoin:master Jul 17, 2022
@tcharding tcharding deleted the derive-hash branch July 18, 2022 05:39
ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
ef7fef0 Derive Hash on a bunch of types (Tobin C. Harding)

Pull request description:

  In preparation for being able to derive `Hash` on all types in `miniscript`, derive `Hash` on all of the required types.

  This PR includes all the changes in https://github.com/rust-bitcoin/rust-bitcoin/pull/933/files and hence supersedes it.

  ref: rust-bitcoin/rust-miniscript#226

ACKs for top commit:
  Kixunil:
    ACK ef7fef0
  apoelstra:
    ACK ef7fef0

Tree-SHA512: 1a1db8b4df2ea8f9e176434bb6fdee5b96f47dcdc6395ebc59e5f5ac5eb13a66fb61e1d90cdbbf12a027f7685fdff21060338c5f27b9d9bf5e9fee452c7c7e83
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement one ack PRs that have one ACK, so one more can progress them trivial Obvious, easy and quick to review (few lines or doc-only...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants