Inconsistency: PrivateKey tracks network, but PublicKey doesn't

[Kixunil]

This is surprising and I wonder if it helps with anything. Supposedly the idea is to ensure that WIF-imported key would create correct address but this doesn't work since PublicKey doesn't contain Network. Is there any reason to do it like that?

It seems either both should contain Network or none.

[apoelstra]

It does seem that way, but there's nothing we can do. There are standard encodings for both and the encodings are inconsistent in the amount of data they contain.

[Kixunil]

We could drop Network from PrivateKey and return it in tuple when deserializing from WIF/accept as argumment when serializing into WIF. Or we could have low-level and high-level public key. Or have serialize_key method on PublicKey documented to only serialize the key, not network information - it's used for scripts only anyway, right?

[dpc]

"Key" != "Encoding of a Key"

[sanket1729]

I initially agreed with @Kixunil's suggestion. But now the more I think about it the compressed aspect is also only related to serialization. It is also awkward that we maintain one field for serialization and not the other field.

[dpc]

Should PrivateKey be just named WIFPrivateKey or something? Because to my uneducated eyes, that's the role it seems to serve? One could have WIFPrivateKey { network: ..., privateKey: PrivateKey } etc. or jest return (Netowrk, PrivateKey).

One way or another the documentation should explain the meaning and role of these types, and why are they different from the underlying types, in part for educational purposes, in part to settle this issue.

[apoelstra]

"Key" != "Encoding of a Key"

No, but you need to be able to obtain a key from the encoding of a key, and the encoding does not include the network.

Similarly if we drop the network from the private key, then we'll be unable to encode it.

[dpc]

Similarly if we drop the network from the private key, then we'll be unable to encode it.

When I think about it: most if not all Bitcoin applications that I ever used picks one network that it will handle very, very early. Mobile ones are typically whole separate app for testnet/mainnet. No app ever supports multiple networks at once, because Bitcoin node supports only one at the time, and it would just be confusing and error prone. Storing the network data on every single private key, even though it will be the same value 100% of the time, just because WIP has it encoded seems really backwards.

You can still encode network + key:

impl PrivateKey {
  pub fn to_wif(&self, network: Network) -> String {
     WIFKey { key: self, network }.to_string()
  }

  pub from_wif(wif_str: &str) -> (Network, Self)  { ... }
}

Additional type gives opportunity to document what WIF is, give https://docs.rs/bitcoin reader some links and so on.

[Kixunil]

Yeah, that's what I meant by "in tuple when deserializing from WIF/accept as argumment when serializing into WIF"

WIF key sounds kinda good but I was thinking maybe also deriving a key using BIP32 could carry the network because xprivs do.

[apoelstra]

Ok, I think I'm convinced. Can we keep around type WifKey = (PrivateKey,' Network); though (actually I guess it'll have to be a newtype since we want to impl ToString/FromStr on it, but it's fine if it's just a public-field POD type)? This would be decodable from a string and also extractable from bip32 keys.

[Kixunil]

I almost agree to having a public POD type, I just wonder if key.network = some_other_network should be trivially expressible. Maybe a method override_network(network: Network) (or a similar name hinting unusual usage) would be better.

[apoelstra]

I'd be fine adding convenience methods for both equality checking and for changing the network field, on an otherwise-POD type.

There's a temptation to try to go down the "express the network with the type system" but I'm not optimistic that that'd be fruitful.

[Kixunil]

I think network in the type system would just encourage people to hardcode mainnet which is pretty bad. There's already a quite popular project which I won't name that only supports mainnet and it worries me quite a bit. (How) do they test it?!

Also forgot to mention I'd like to see a method to derive address directly so people won't mess up networks.

[apoelstra]

FWIW I routinely test code on mainnet, at least when fees are cheap. It keeps me focused on writing bug-free code ;)

[Kixunil]

I assume it's not your only testing strategy and/or you don't expose less-tested code to unaware users.

[apoelstra]

I assume it's not your only testing strategy and/or you don't expose less-tested code to unaware users.

Correct :P. At Blockstream we have real regtest-based testing infrastructure that actually does use the Network enum; for my personal code I ward off casual users by having incomprehensible functionality and UX.

[dpc]

There's a temptation to try to go down the "express the network with the type system" but I'm not optimistic that that'd be fruitful.

Network is already an enum. Is there anything else that would be worth expressing?

[Kixunil]

I believe he meant PrivateKey<Network>, so the network is statically known.

[dpc]

Ah, I see. Imo, 99% not worth the trouble. This bound would infect all outter-types in a code that wants to handle any network.

[apoelstra]

I'm not so worried about it affecting outer types (the network is not encoded in any data on the p2p layer or blockchain), but I do think it'd have a high ergonomic cost for not a ton of benefit. Usually you want to support different networks and switch at runtime (even if "at runtime" just means "at startup").

[Kixunil]

Usually you want to support different networks and switch at runtime (even if "at runtime" just means "at startup").

Exactly.

[junderw]

I routinely test code on mainnet

♫ FLY INTO THE DANGER ZONE ♫

---

Just to recap current consensus (tell me if I'm wrong):

1. PrivateKey should remove Network.
2. A newtype tuple struct with pub fields WifKey(pub PrivateKey, pub Network) should be added.
3. WifKey should impl ToString/FromStr, derive Copy, PartialEq, Eq.

Question: What about compressed?

This is only really used in the context of WIF encoding and deciding whether to make a derived PublicKey also be compressed or not. Since we are removing the WIF use case, I think we might be able to remove compressed from PrivateKey by modifying the API a bit. (new + new_uncompressed to public_key + public_key_uncompressed etc.) This would also require WifKey to contain the bool.

[Kixunil]

@junderw I believe the idea was to use tuples, not newtype for WifKey however, now I realized that making it a proper type will allow Display and FromStr which would be impossible otherwise.

I think we might be able to remove compressed from PrivateKey

If we do that we can remove PrivateKey entirely because it only contains secp256k1::SecretKey with no additional invariants. However that would re-introduce inconsistency, just the other way around. I don't believe we should do that.

[apoelstra]

@junderw if you remove compressed from PrivateKey then you can't derive public keys or addresses. As @Kixunil is suggesting .. what then would be the point of the type?

[junderw]

I guess it does make sense to keep passing the compressed bool around through all the different conversions (ie. bip32 doesn't allow uncompressed pubkeys, so being able to only output PrivateKeys with compressed as true makes sense)

I was just trying to figure out the use case for it, and only saw WIF encoding and PublicKey creation.

[Kixunil]

@junderw note that there is #826 which however encodes "always compressed". Since uncompressed keys and WIF still exist I think it's best to support them even if they are discouraged.

[junderw]

I'm aware of CompressedPublicKey. I was talking about PrivateKey.

If I create a private key from a bip32 extended key and then try to use that to create an uncompressed PublicKey, something is wrong... so having the ability to output a PrivateKey with the compressed == true already inside of it has value for that use case, is what I'm saying.

[Kixunil]

Well, we probably also should have CompressedPrivateKey and output that one from BIP32 derivations. :)

[tcharding]

We have had a lot of discussion re Network and keys lately. Also PrivateKey now contains NetworkKind instead of Network as it did when this issue was created. I created #2420 so the conversation here would not get lost.

Can we close this issue?

[apoelstra]

Yeah, let's close this. We have rough agreement that we want some sort of WifKey but we need to sort out compressedness vs non-compressedness first, and I think the fate of PrivateKey will come along with that.