Skip to content

Use secp256k1::{SecretKey, PublicKey} instead of own types#221

Closed
stevenroose wants to merge 3 commits intorust-bitcoin:masterfrom
stevenroose:no-key
Closed

Use secp256k1::{SecretKey, PublicKey} instead of own types#221
stevenroose wants to merge 3 commits intorust-bitcoin:masterfrom
stevenroose:no-key

Conversation

@stevenroose
Copy link
Copy Markdown
Collaborator

Another attempt to rewrite #183

I know some don't like the _uncompressed variants all that much, but I think it makes more sense like this than to have them stored in a separate struct.

This comes with #220 for missing WIF stuff.

@stevenroose stevenroose mentioned this pull request Jan 21, 2019
@stevenroose
Copy link
Copy Markdown
Collaborator Author

I based this off Carl's PR, so I didn't see the new Address::p2upkh constructor. I used p2pkh_uncompressed instead. If either name is preferred, I can change it back.

Also apologies for adding the commas, that's my editor doing that for me :) I can remove them if they clutter the diff.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

See #222.

@tamasblummer
Copy link
Copy Markdown
Contributor

It is disrespectful of reviewers time if you introduce lots of diffs because of your editor conf.

@stevenroose
Copy link
Copy Markdown
Collaborator Author

stevenroose commented Jan 22, 2019

It is disrespectful of reviewers time if you introduce lots of diffs because of your editor conf.

That's why I apologized beforehand and offered to remove them if they were bothering. I'll remove them.

EDIT: done :)

@tamasblummer
Copy link
Copy Markdown
Contributor

If you know you'd have to apologize then just do not do it.

/// therefore only adds ambiguity
#[inline]
pub fn p2upkh(pk: &PublicKey, network: Network) -> Address {
pub fn p2pkh_uncompressed(pk: &PublicKey, network: Network) -> Address {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The name was fine as is.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah like I said, this sneaked in when rebasing off master. It's more consistent with the rest of the uncompressed variant methods, but I'm fine with changing it back.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Reverted it.

}

/// Create a pay to script address that embeds a witness pay to (uncompressed) public key
pub fn p2shwpkh_uncompressed (pk: &PublicKey, network: Network) -> Address {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use of uncompressed keys was discouraged before SegWit. We should not encourage doing this by adding a method to create scripts for them within SegWit.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Oh so we don't even want this to be possible?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It remains possible, no matter what we do. It is however not a type of script that we should encourage creating by adding a method for it. In contrast p2pk is discouraged but there are tens of thousands of UTXOs with it.

@dongcarl
Copy link
Copy Markdown
Member

Closing, see IRC and #183

@dongcarl dongcarl closed this Feb 11, 2019
yancyribbens pushed a commit to yancyribbens/rust-bitcoin that referenced this pull request Mar 23, 2024
…descriptor-pk

descriptor: fix `is_uncompressed()` for `DescriptorPublicKey`
PastaPastaPasta pushed a commit to PastaPastaPasta/rust-dashcore that referenced this pull request Feb 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants