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.

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

Don't allow uncompressed pks in witness addresses #428

Merged
merged 2 commits into from
Sep 9, 2020
Merged

Don't allow uncompressed pks in witness addresses #428

merged 2 commits into from
Sep 9, 2020

Conversation

stevenroose
Copy link
Collaborator

Solves #427.

@stevenroose stevenroose mentioned this pull request May 23, 2020
Closed
@codecov-commenter
Copy link

codecov-commenter commented May 23, 2020 •
edited
Loading

Codecov Report

Merging #428 into master will increase coverage by 0.54%.
The diff coverage is 63.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #428      +/-   ##
==========================================
+ Coverage   85.64%   86.19%   +0.54%     
==========================================
  Files          39       39              
  Lines        7446     7402      -44     
==========================================
+ Hits         6377     6380       +3     
+ Misses       1069     1022      -47
Impacted Files Coverage Δ
src/util/address.rs 85.37% <63.15%> (-0.15%) ⬇️
src/util/mod.rs 0.00% <0.00%> (ø)
src/network/mod.rs 0.00% <0.00%> (ø)
src/util/psbt/error.rs 0.00% <0.00%> (ø)
src/consensus/encode.rs 89.21% <0.00%> (ø)
src/blockdata/script.rs 78.88% <0.00%> (+0.14%) ⬆️
src/util/amount.rs 90.76% <0.00%> (+0.35%) ⬆️
src/blockdata/transaction.rs 94.74% <0.00%> (+0.75%) ⬆️
src/util/bip158.rs 93.78% <0.00%> (+0.78%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31a5760...ed9bf41. Read the comment docs.

sgeisler
sgeisler previously approved these changes May 23, 2020
View reviewed changes
Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

Looks good. Kinda annoying to have to return a Result, but it's probably not worth the hassle to re-engineer the whole key API again 😅.

src/util/address.rs Outdated Show resolved Hide resolved
@stevenroose
Copy link
Collaborator Author

Updated, fixed the typo and put the format and typo change in a separate commit.

junderw
junderw approved these changes May 23, 2020
View reviewed changes
Copy link
Contributor

@junderw junderw left a comment

Choose a reason for hiding this comment

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

LGTM

What about p2wsh?

We decided to parse the witnessScript and run a quick check to see if any chunks are 65 bytes starting with a 0x04, and if there are any we check if it is a valid secp256k1 point. Then error if it is a valid point.

There is probably near 0% chance of some useful data being 65 bytes and starting with 0x04, so maybe even checking for a valid key is overkill.

Thoughts?

@stevenroose
Copy link
Collaborator Author

What about p2wsh?

Hmm, I'm a bit reluctant to parse scripts, to be honest. Perhaps this can be considered in a separate PR? I think this PRs change is quite straightforward, so I'd prefer to just merge it. If we want to do something for p2wsh, I think it might benefit from a discussion on IRC or so.

@junderw
Copy link
Contributor

junderw commented Jun 3, 2020

sounds fair enough.

Related: blockdata::Script.to_v0_p2wsh method should also check before hashing.

@junderw junderw mentioned this pull request Jun 4, 2020
Open
@yancyribbens
Copy link
Contributor

I agree with @sgeisler that it's unfortunate to need to return a Result. It would be better if the type system didn't allow a uncompressed key to be passed to p2wpkh() imo. That would also avoid needing to check explicitly for !pk.compressed.

@sgeisler
Copy link
Contributor

@yancyribbens it's a tricky trade-off. We could have a PrivateKey trait with the implementations CompressedPK and MaybeCompressedPK and implement everything generically around the trait. But the added complexity might not be worth it. And it would be a separate PR anyway imo. So I'd like to see this one merged first and then maybe explore ways to fix the underlying problem.

@yancyribbens
Copy link
Contributor

@sgeisler I agree that the current PR solves the issue. It would be nice to see either using trait bounds or possibly wrapping in a enum? Either way, I agree it's probably a fair amount of work that might not be worth doing for this PR but possibly could be a followup PR.

apoelstra
apoelstra approved these changes Sep 9, 2020
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.

Yeah, I think this approach is fine.

We'll need to revisit compressedness in a bit anyway when we have Taproot keys, and we can consider API changes then.

@apoelstra apoelstra merged commit 45da3ad into rust-bitcoin:master Sep 9, 2020
Davidson-Souza pushed a commit to Davidson-Souza/rust-bitcoin that referenced this pull request Jul 12, 2023
@apoelstra
Merge rust-bitcoin/rust-secp256k1#430: Add convenience methods for keys
a30e9bb 
f08276a Add convenience methods for keys (Tobin Harding)
b4c7fa0 Let the compiler work out int size (Tobin Harding)
c612130 Borrow secret key (Tobin Harding)

Pull request description:

  We have a bunch of `from_<key>` methods for converting between key types. To make the API more ergonomic to use we can add methods that do the same but called on a variable e.g., once applied the following are equivalent:

  - `let pk = PublicKey::from_keypair(kp)`
  - `let pk = kp.public_key()`

  Do this for `SecretKey`, `PublicKey`, `KeyPair`, and `XOnlyKeyPair`.

  Fixes: rust-bitcoin#428

  ### Note to reviewers

  - `XOnlyPublicKey` -> `PublicKey` logic is made up by me, I could not work out how to get `libsecp256k1` to do this.
  - Please review the tests carefully, they include assumptions based on my current understanding of the cryptography :)

ACKs for top commit:
  sanket1729:
    ACK f08276a. Thanks for going through all the iterations.
  apoelstra:
    ACK f08276a

Tree-SHA512: 1503a6e570a3958110c6f24cd6d075fe5694b3b32b91a7a9d332c63aa0806198ff10bdd95e7f9de0cf73cbf4e3655c6826bd04e5044d1b019f551471b187c8ea
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoelstra apoelstra apoelstra approved these changes

@sgeisler sgeisler sgeisler approved these changes

@junderw junderw junderw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@stevenroose @codecov-commenter @junderw @yancyribbens @sgeisler @apoelstra