Make PublicKey API consistent

[dr-orlovsky]

at least expose serialize method additionally to write_into.

Original discussion: rust-bitcoin/rust-miniscript#225 (comment)

Maybe we can go even further and rename confusing serialize method in rust-secp256k1 into to_bytes method (I assume it is computationally expensive since it calls to C FFI working with internal public key representation).

[apoelstra]

It is not expensive at all, it is basically a memcpy for the uncompressed serialization and a check of one bit for the compressed serialization.

What is your proposed function signature for a serialize method on bitcoin::PublicKey?

[dr-orlovsky]

Right, I see: the length of byte array will be different (depending on the key compression) and we can't have two functions since bitcoin key must force compressed/uncompressed state...

But this is really so frustrating and confusing all the time...

Now, when we have a dedicated xcoord-only public key, may be we need to transform bitcoin::PublicKey into an enum with three variants, which will allow to have as_bytes method on each internal type?

In general it seems really inconvenient that we have a lot of places where we can't accept uncompressed keys (everything segwit-related) and need to return Result with some custom error instead of requiring compression at type level. It may be that with the enum AND special internal compressed/uncompressed types (additionally to xcoord only) we may distinguish APIs for methods which (a) may take any key, (b) require compressed key only, (c) require xcoord key only.

[apoelstra]

Definitely agree, it would be nice if we could somehow enforce this at the type level.

[dr-orlovsky]

Had a private IRC chat with @sgeisler and at the end decided to publish it to GitHub:

20:56 sgeisler: Also: do we actually know if ECDSA and schnorr mixed usage is insecure? I was under the impression that we simply son't know …
20:56 sgeisler: so that's a good reason to avoid mixing
20:56 sgeisler: but is it a sufficient reason to make the API harder on ourselves?
20:58 dr_orlovsky: 05:17 sipa: you're right of course that using the same key in schnorr and ecdsa should be discouraged (i personally expect it is not less secure than just ecdsa with that key, but i also don't think anyone has formally analyzed this)... but in the context of bitcoin script signing, this advice is sort of preempted by the fact that you shouldn't be reusing keys at all for whatever purpose
20:58 sgeisler: yeah, that's what I expected
20:59 sgeisler: so given that, would you advocate different XPub types for ECDSA and schnorr keys?
20:59 dr_orlovsky: I think (1) we should not give a simple ability to convert bitcoin::PublicKey in/from XCoordOnlyKey
20:59 dr_orlovsky: otherwise users will abuse it
20:59 sgeisler: because at the top you can't say which type they have, only when you get to the subtree with the taproot usage you can tell
21:00 dr_orlovsky: also, because it will create problems on miniscript level
21:00 sgeisler: so you'll need some way to convert xpub(ECDSA) to xpub(schnorr) anyway
21:00 dr_orlovsky: we will need a new context with Tapscript
21:00 dr_orlovsky: and it will be the same nightmare like with uncompressed keys before
21:00 sgeisler: ah, right
21:00 dr_orlovsky: so it's not only about key reuse & security
21:01 dr_orlovsky: in fact not providing more complex API at the BIP32 level will render miniscript API much much more complex
21:01 dr_orlovsky: (2) BIP32 API should commit somehow to whether it is ECDSA or BIP340 context when derives keys - like bitcoin::PuiblicKey commits to the compression option
21:02 sgeisler: I guess so, but do we have a useful miniscript definition for tapscript already?
21:02 dr_orlovsky: and return Compressed PublicKey on one context and XCoordOnly on the other - but never return UncompressedPublicKey (since there were no BIP32 when uncompressed keys were used)
21:02 dr_orlovsky: no, we don't have tapscript there yet
21:03 dr_orlovsky: we actualy do not have it even in rust-bitcoin
21:03 dr_orlovsky: (some opcodes should change)
21:03 dr_orlovsky: but for that we need to refactor opcodes :D
21:03 sgeisler: because the main question would be: given that you can probably mix schnorr and ecdsa signing in tapscript (didn't check) you'd probably want two different node types for that in miniscript
21:03 dr_orlovsky: no, you can't mix with ECDSA
21:03 sgeisler: ah, good to know
21:04 dr_orlovsky: the meaning of CHECKSIG and CHECKMULTISIG had changed in Tapscript and the first does not allow ECDSA, only Schnorr, and the second is depricated - that why it's a different context for everything
21:05 sgeisler: Then I don't see why the xpub needs to know if it's a schnorr key or ECDSA, the context will make it clear.
21:05 sgeisler: Actually having it as a member would lead to wierd error cases when key and context mismatch, like segwit and uncompressed keys
21:06 sgeisler: only if we encode it in the type system it would be sane, but we'd still need conversions for ECDSA-XPub to Schnorr-XPub
21:06 sgeisler: (or from a neutral XPub)
21:07 sgeisler: since what type would the root key have otherwise? You don't want to have to decide if you are doing schnorr or ECDSA at the seed level
21:08 sgeisler: > but for that we need to refactor opcodes :D
21:08 sgeisler: I see, we are in it for the long haul :/ that sounds like it'll take some time
21:17 dr_orlovsky: If I did BIP32 from the scratch after Taproot, I probably did the following:
21:19 dr_orlovsky: Master extended public key should be of a special type not being able to do anything other than derive operation, leading to another master key - unless it's called in some derivation context (ECDSA or BIP340).
21:20 dr_orlovsky: Than, it will return a typed Xpub/Xpriv which will be able to produce keys only of certain type - without traits shared between them
21:20 dr_orlovsky: but yes, this implies rewriting of all rust-bitcoin aside of networking part
21:21 dr_orlovsky: but I am afraid with taproot we do not have other option
21:22 dr_orlovsky: Script changes. PublicKey changes. PrivateKey changes. BIP32 changes. Even for PSBT there is PSBT2 coming (unrelated to taproot though). Now this XKeyPair - what to do with it? Bech32m addresses.
21:22 dr_orlovsky: Descriptors must be changed (and I believe we need non-miniscript descriptors as well, at rust-bitcoin level)
21:24 dr_orlovsky: signatures and signing process changes. What does not change? Network messages. OutPoint. Transaction structure. I.e. <20% of the codebase
21:27 sgeisler: Re xkeys: in principle I like your idea, I just don't know if it's the right priority. Imo the "xkeys don't know what they are and can be converted to everything" approach gets us 80% of the way pretty cheaply to focus on the more complicated stuff like descriptors (assuming that ECDSA/schnorr reuse isn't horribly broken and just of unknown security)
21:28 sgeisler: They'll give us a way to experiment with what we actually need
21:29 sgeisler: otoh, we'd probably touch everything twice with the 80/20 strategy
21:30 sgeisler: I guess I'm just a bit worried about the colossal effort it will take, but we'll have to do it one way or another
21:30 dr_orlovsky: me too :(
21:30 dr_orlovsky: but it seems to me there is a certain feel of a deadlock with miniscript API with the way rust-bitcoin works today
21:31 dr_orlovsky: and its good we have taproot giving us ability to refactor something large-scale
21:33 sgeisler: So maybe the XPub<K: InnerDerivableKey> API would work? You can always convert to K and for the root K is some new key type that has no API except for serialization and when deriving this one key type you can specify which target type you want. For all others it has to stay the same.
21:33 sgeisler: This would work with specialization, but that's probably not available (I don't even know if it's stable xD)
21:34 dr_orlovsky: XPub<K: InnerDerivableKey> sounds good
21:34 sgeisler: So I guess that will be implemented as a different function: derive_to_key_type<K>, while all other Ks only expose the derive function
21:35 dr_orlovsky: probably no need in derive_to_key_type<K>
21:35 dr_orlovsky: there should be only downgrade_to_ecdsa|bip340() and that's all
21:36 dr_orlovsky: later if other key type appear we will add anothe function
21:36 dr_orlovsky: so the XPub type will be downgraded itself, not the derived key
21:37 sgeisler: I see, with one addition: let's use the chance to get the compressedness into the type system here too. So downgrade_to_legacy|ecdsa|bip340()
21:37 sgeisler: But that's definitely something we should discuss with the others, because my view on the ecosystem is limited, maybe there is some annoying flaw
21:38 sgeisler: There's still nothing that keeps you from downgrading the same xpub to different types, but I think there's only so much you can do …
21:39 dr_orlovsky: downgrade_to_legacy - no need, the downgrade_to_ecdsa will return compressed keys only. There is no Xpubs/BIP32 for uncompressed keys!
21:40 dr_orlovsky: of course should be discussed with everybody; perhaps we should had this discussion in rust-bitcoin
21:40 sgeisler: Are you sure? I didn't check all constructors, but at least we carry the compressedness flag around in our bip32 impl
21:40 dr_orlovsky: is there any way to transfer it to there? I assume no.
21:40 sgeisler: I think it's best to write a compressed version somewhere more permanent, like a GH issue
21:41 dr_orlovsky: well, of course you can use uncompressed key in any non-segwit script; but API should not allow users to do that since P2PKH
21:41 dr_orlovsky: if they have to - go and code your forked crate with BIP32 support for uncompressed keys
21:42 dr_orlovsky: or use pre-taproot rust-bitcoin version

[apoelstra]

It's not clear to me at all why we'd have to "rewrite 80% of rust-bitcoin". We just need to

• add accessor functions to the bip32 xkeys to get x-only keys
• add a new address variant for p2tr which takes an x-only key but is otherwise basically identical to the existing p2pkh variant
• add bech32m support, for the above address variant, and add yet another case to the address-parsing logic

Yes rust-miniscript will require more invasive changes. Probably ToPublicKey will need to have an output parameter, or it should be type-weakened to only require len/encode_bytes methods or something rather than actually converting to bitcoin::PublicKey.

This whole situation is way simpler than the uncompressed/compressed/hybrid situation because xonly keys can be used if and only if you are in a Taproot context; they have a fixed length; there are no kinds of x-only pubkeys which have no corresponding private key; etc etc.

[dr-orlovsky]

Replaced with #588