Ready for Review: Introduce util::key and deprecate util::privkey by dongcarl · Pull Request #183 · rust-bitcoin/rust-bitcoin

dongcarl · 2018-10-30T09:02:36Z

Move util::privkey::PrivKey to util::key::PrivateKey.
Clean up util::key::PrivateKey.
Add new util::key::PublicKey struct.
Modify util::address::Address to appropriately wrap new
util::key::PublicKey.

@apoelstra @stevenroose I've introduced a few .clone()'s that I would like to remove, and am not entirely happy about the copying that happens in util::key::PublicKey::serialize. Would love your opinions on how to design this better.

src/util/address.rs

src/util/key.rs

stevenroose · 2018-10-30T12:31:42Z

src/util/key.rs

Given the amount of ways you can go from a key to an address, I think it might be better to just have the explicit Address constructors and no getters here at all.

What do you mean by this? Do you mean that we should eliminate PrivateKey::to_{legacy_,}address?

Yeah. I mean there is p2pk, p2pkh, p2wpkh. Which one is "address" and which one is "legacy address"? Every time there is a new way to make an address from a key, this will get more confusing. The Address constructors are explicit and very clear.

If you really care about the getters on the key, I'd make them more explicit. Like to_p2pkh_address, ... But I think the constructors are sufficient.

@stevenroose Agreed.

@stevenroose Actually... I've thought about it a little... It's a little un-ergonomic that we would have the use the constructor like so: Address::p2pk(sk.public_key(&secp), sk.network).

Perhaps we should do PrivateKey::to_p2pkh_address(&secp) etc., just for ergonomics sake.

@stevenroose correct

I feel like that's kind of a pity. I mean WIF is just an "import format", right? I feel like it would be perfectly acceptable to have wif just as a util/wif.rs thing for compatibility sake if it's actually being annoying wrt other things.

A bit like how an Address is different from a PublicKey, a Wif could be different from a PrivateKey.

We could do that. It would mean that you couldn't serialize a PrivateKey without converting to a WIf first, and you couldn't do that without knowing your network (which you'd need to obtain from somewhere)...and you'd then be unable to obtain an address from a PrivateKey without doing the same thing ... so the PrivateKey type becomes very unergonomic.

Yeah it becomes basically useless because of secp256k1::key::SecretKey. But that's not necessarily a problem. Address::p2pkh(secp_priv.public_key(), network) doesn't seem wrong though (considering context-less priv-pub conversion).
And you really only need WIF strings when you actually want WIF strings, no? bitcoin::utils::wif::to_wif(my_pk, network) also not.

Question: will the secp256k1 SecretKey and PublicKey look different for Schnorr?

apoelstra · 2018-10-30T12:38:07Z

Can you please break this up into multiple commits? At least,

One that renames Privkey to PrivateKey
One that adds the public key field to Address
One that moves stuff into the key module
One that does everything else, if there's anything left

stevenroose · 2018-10-30T12:40:38Z

Should we future-proof this wrt Schnorr signatures coming in the (hopefully) near future?

apoelstra · 2018-10-30T14:00:56Z

We cannot future proof against a future output version with semantics we don't know yet.

stevenroose · 2018-12-03T17:00:30Z

So I think when we concluded discussions about this, my personal conclusion was that I'd take network out of the private key type. And just take a network argument for the WIF method.

dongcarl · 2018-12-03T18:50:39Z

@stevenroose you mean take a network argument for encoding to WIF? What about decoding from WIF?

dongcarl · 2018-12-03T18:50:52Z

@apoelstra This is ready for review BTW

stevenroose · 2018-12-03T19:09:52Z

Decoding could just return a privkey and a network. I don't think the network has any relevance when decoding a WIF. Perhaps as a protection measure, but that's up to the user using the WIF stuff IMO.

instagibbs · 2018-12-12T14:20:28Z

src/util/key.rs

how to grammar

not even sure what this is saying

src/util/key.rs

stevenroose · 2019-01-08T14:34:51Z

What's the latest on the holdup for this one?

dongcarl · 2019-01-21T21:13:54Z

Addressed nits from @instagibbs and rebased.

sgeisler

Looks good except some confusing variable names.

src/util/address.rs

stevenroose · 2019-01-22T14:51:15Z

Like I mentioned, I personally feel that an approach like #221 makes more sense. No hard feelings if this gets merged, though :)

sgeisler

It's probably not the best solution we could come up with but I feel it's good enough to move forward and eventually improve incrementally on it.

dongcarl · 2019-01-25T19:58:18Z

Rebased and ready for review!

ariard

Good to me, imo documentation could be clearer, but can go forward.

ariard · 2019-02-02T01:58:39Z

src/util/key.rs

+
+    /// Deserialize a public key from a slice
+    pub fn from_slice(data: &[u8]) -> Result<PublicKey, encode::Error> {
+        let key: secp256k1::PublicKey = secp256k1::PublicKey::from_slice(data)


Seems you don't need type ascription there (1.22, 1.29.2, stable)
Same for the io::Result and bool below (or are you doing it for code readability ?)

I think you were looking at an outdated hunk, it's been changed :-)

At top on 5fdf9e6, you can still prune bool and io::Result<()> type ascriptions

ariard · 2019-02-02T02:15:43Z

src/util/key.rs

 //!
-//! A private key represents the secret data associated with its proposed use
+//! Structures and methods representing the public/secret data associated with
+//! its proposed use


By "proposed use" what is mean there ? Being able to export it as WIF compressed pubkey ? Could be clarified :)

dongcarl · 2019-02-04T15:29:46Z

Fixed doc nit from @ariard

dongcarl · 2019-02-05T17:45:45Z

@tamasblummer @ariard Please re-ACK, perhaps https://git-scm.com/docs/git-range-diff would make your life easier too

ariard

Good to me, just type ascription nit

ariard · 2019-02-07T00:16:31Z

src/util/key.rs

+
+    /// Deserialize a public key from a slice
+    pub fn from_slice(data: &[u8]) -> Result<PublicKey, encode::Error> {
+        let key: secp256k1::PublicKey = secp256k1::PublicKey::from_slice(data)


At top on 5fdf9e6, you can still prune bool and io::Result<()> type ascriptions

dongcarl · 2019-02-07T15:21:34Z

@ariard Let's fix the ascription stuff after merge!

sgeisler

Renewing previous ACK after rebase.

- Rename privkey::PrivKey to privkey::PrivateKey - Remove unnecessary methods for privkey::PrivateKey - Modify tests to work with above changes

- Move util::privkey to util::key - Add PublicKey struct to util::key - Implement de/serialization methods for util::key::PublicKey

- Switch util::address::Payload::Pubkey variant to wrap util::key::PublicKey - Switch util::address::Address::p*k* constructors to use util::key::PublicKey - Fix tests for aforementioned switch - Add convenience methods for util::key::PublicKey to util::key::PrivateKey conversion - Switch BIP143 tests to use util::key::PublicKey

This change also moves the secp256k1::Error wrapper from util::Error to consensus::encode::Error, since we do not use it anywhere else. We can add it back to util::Error once we have instances of secp256k1::Error that are not related to consensus::encode.

apoelstra · 2019-02-11T21:00:40Z

ACK pending travis

sgeisler

Renewing previous ACK after rebase.

…itcoin#185) PR rust-bitcoin#180 used `MultiPeerNetworkManager`, this was renamed in rust-bitcoin#183 which was merged before rust-bitcoin#180 but after its CI run.

…-bitcoin#197) * feat(spv): flush headers on shutdown * move fn lower in the impl * refactor: `MultiPeerNetworkManager` -> `PeerNetworkManager` (rust-bitcoin#184) * refactor: `MultiPeerNetworkManager` -> `PeerNetworkManager` * Fix formatting and apply review * feat: Update ffi headers (rust-bitcoin#183) * feat(spv): broadcast transaction (rust-bitcoin#180) * fix: Fix `PeerNetworkManager` cast in `broadcast_transaction` (rust-bitcoin#185) PR rust-bitcoin#180 used `MultiPeerNetworkManager`, this was renamed in rust-bitcoin#183 which was merged before rust-bitcoin#180 but after its CI run. * fix: Use non-blocking `TcpStream` in `dash-spv::network::TcpConnection` (rust-bitcoin#188) * refactor: Improve SPV shutdown handling with `CancellationToken` (rust-bitcoin#187) * refactor: `TcpConnection` -> `Peer` and `ConnectionPool` -> `PeerPool` (rust-bitcoin#190) * fix: Locking issue after rust-bitcoin#190 (rust-bitcoin#191) rust-bitcoin#190 removed the read timeouts of the `Peer::receive_message` which currently leads to a lockup of the peer because the write lock is held while waiting for the message. Needs some more refactoring but this works for now. * fix: More follow-up to rust-bitcoin#190 (rust-bitcoin#193) The sleep timeout branch introduced in rust-bitcoin#191 returns an `Err(NetworkError::Timeout)` which leads to a misbehavior update below in the `msg_result` match and eventually in a peer ban. This shouldn't happen because the `sleep` timing out only means that there is no data available right now. Instead, it now returns `Ok(None)` which will just keep things going. * flush after mn list sync too * move flush to after header sync instead of mnlist * fix --------- Co-authored-by: Kevin Rombach <35775977+xdustinface@users.noreply.github.com>

stevenroose reviewed Oct 30, 2018

View reviewed changes

dongcarl mentioned this pull request Nov 4, 2018

PublicKey de/serialization should support io::{Read,Write} traits. rust-bitcoin/rust-secp256k1#76

Closed

dongcarl force-pushed the 2018-9-pubkey-wrapper branch 2 times, most recently from 0879209 to 3a7b7e6 Compare November 16, 2018 04:33

stevenroose mentioned this pull request Nov 17, 2018

[WIP] Expand on #183: Extract network and compressedness from key types #194

Closed

dongcarl changed the title ~~WIP: Introduce util::key and deprecate util::privkey~~ Ready for Review: Introduce util::key and deprecate util::privkey Nov 18, 2018

dongcarl force-pushed the 2018-9-pubkey-wrapper branch from 3a7b7e6 to 84b16a0 Compare December 3, 2018 04:07

instagibbs reviewed Dec 12, 2018

View reviewed changes

stevenroose mentioned this pull request Jan 21, 2019

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

Closed

dongcarl force-pushed the 2018-9-pubkey-wrapper branch 2 times, most recently from 58aff07 to d6ecba0 Compare January 21, 2019 21:00

sgeisler suggested changes Jan 21, 2019

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

dongcarl force-pushed the 2018-9-pubkey-wrapper branch from d6ecba0 to 4dfab0e Compare January 22, 2019 03:51

sgeisler previously approved these changes Jan 22, 2019

View reviewed changes

dongcarl dismissed sgeisler’s stale review via 966597c January 25, 2019 19:52

dongcarl force-pushed the 2018-9-pubkey-wrapper branch from 4dfab0e to 966597c Compare January 25, 2019 19:52

ariard reviewed Feb 2, 2019

View reviewed changes

tamasblummer previously approved these changes Feb 2, 2019

View reviewed changes

dongcarl dismissed tamasblummer’s stale review via 5fdf9e6 February 4, 2019 15:28

dongcarl force-pushed the 2018-9-pubkey-wrapper branch from 966597c to 5fdf9e6 Compare February 4, 2019 15:28

ariard previously approved these changes Feb 7, 2019

View reviewed changes

sgeisler previously approved these changes Feb 7, 2019

View reviewed changes

tamasblummer previously approved these changes Feb 9, 2019

View reviewed changes

dongcarl added 5 commits February 11, 2019 14:56

Cleanup util::privkey in preparation for PublicKey

60c93c3

- Rename privkey::PrivKey to privkey::PrivateKey - Remove unnecessary methods for privkey::PrivateKey - Modify tests to work with above changes

Add PublicKey struct encapsulating compressedness

53a6efe

- Move util::privkey to util::key - Add PublicKey struct to util::key - Implement de/serialization methods for util::key::PublicKey

key: Reword and clarify comments

fc448ba

dongcarl dismissed stale reviews from tamasblummer, sgeisler, and ariard via a944c7f February 11, 2019 20:47

dongcarl force-pushed the 2018-9-pubkey-wrapper branch from 5fdf9e6 to a944c7f Compare February 11, 2019 20:47

sgeisler approved these changes Feb 11, 2019

View reviewed changes

apoelstra approved these changes Feb 11, 2019

View reviewed changes

apoelstra merged commit 8ae4aee into rust-bitcoin:master Feb 11, 2019

dongcarl mentioned this pull request Feb 11, 2019

Add util::wif module #220

Closed

PastaPastaPasta pushed a commit to PastaPastaPasta/rust-dashcore that referenced this pull request Feb 2, 2026

feat: Update ffi headers (rust-bitcoin#183)

03fe3c2

Conversation

dongcarl commented Oct 30, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Oct 30, 2018

Uh oh!

stevenroose commented Oct 30, 2018

Uh oh!

apoelstra commented Oct 30, 2018

Uh oh!

stevenroose commented Dec 3, 2018

Uh oh!

dongcarl commented Dec 3, 2018

Uh oh!

dongcarl commented Dec 3, 2018

Uh oh!

stevenroose commented Dec 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevenroose commented Jan 8, 2019

Uh oh!

dongcarl commented Jan 21, 2019

Uh oh!

sgeisler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevenroose commented Jan 22, 2019

Uh oh!

sgeisler left a comment

Choose a reason for hiding this comment

Uh oh!

dongcarl commented Jan 25, 2019

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard Feb 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ariard Feb 7, 2019 •

edited

Loading

ariard Feb 2, 2019 •

edited

Loading

ariard Feb 7, 2019 •

edited

Loading