Taproot PublicKey refactoring

[dr-orlovsky]

First attempt to fit in Taproot keys into current public key type system. Consists of:

1. Renaming PublicKey into EcdsaPublicKey, otherwise leaving the type intact. This will simplify migration (ppl can just do use bitcoin::EcdsaPublicKey as PublicKey and that's all), however will not allow enforcing of compressed keys as a part of API as was discussed in Make PublicKey API consistent #554
2. Making new PublicKey type: an enum with two options for older ECDSA public keys and Schorr public keys (however considering of naming them Taproot and Secp, since the encoding format is a part of either Taproot-related BIP-340 and Secp paper and not Schnorr signature or ECDSA standard)

This also includes a number of improvements to existing API, including making simple PublicKey methods inline, extracting Key trait for public (and in future private) key functionality shared across types, and additional key::Error variant which was previously incorrectly mapped to base58 error (since it has nothing to do with Base58 and is about binary-encoded data).

The largest question however is the universal deserialization for the new PublicKey type, since there is no unambiguous way to distinguish Taproot and Secp keys when read from stream: https://github.com/rust-bitcoin/rust-bitcoin/pull/561/files#diff-6e396f6881bb94d1206a4d0d1b3e582f16bbd098c091b07afd8a6a95b450c199R215-R241. Not sure how to solve that w/o abandoning shared API. Had created discussion on the matter here: #554

[dr-orlovsky]

Found a better way of avoiding key ambiguity during deserialization: db50adb

[sanket1729]

Sorry, did not review the PR yet. But I think we should separate the keys from the signatures. Schnorr and ESDSA are signatures, compressed keys can have schnorr signatures and Xonlykeys can have ECDSA signatures.

I think we should ideally have, XOnlyKey, CompressedKey, and UncompressedKey as types. I think this will be a lot of work downstream for the rust-ecosystem, but if we were to start rust-bitcoin again :), I would probably use this.

I don't think it is useful to have PublicKey enum containing both types. Users of the application are expected to know what sort of keys they are dealing with. In Witness 1 version programs they will be xonly keys. So, I think it is best if we keep PublicKey as is and add another new type for XonlyKey basically avoiding the entire deserialization question.

Rust-bitcoin does not have enough context to figure what keys the user is dealing with, so it's best to leave it to the user when they additional metadata to deserialize the keytype.

[elichai]

Sorry, did not review the PR yet. But I think we should separate the keys from the signatures. Schnorr and ESDSA are signatures, compressed keys can have schnorr signatures and Xonlykeys can have ECDSA signatures.

@sanket1729 I actually tend to disagree.
Even though BIP-340 is defined in a way that minimizes the dangers of mixing ECDSA and Schnorr with the same keys, I still believe this is bad practice and should not be encouraged.
I'd rather push people to use separate keys for ECDSA and Schnorr than to encourage them to use both.

and if we really need to support this for the sake of compatibility I'd rather it be one way (ie ECDSA keys can be used for schnorr but not the opposite)

[sanket1729]

@elichai, I understand your point and I don't think we contradict :) .

My initial understanding was that we should name schnorr keys as XonlyKey instead of schnorrkey::Publickey and keep bitcoin::PublicKey as it is.

But I see, what I say may make more sense from libsecp point of view, but from rust-bitcoin(which only really cares about signatures from keys and not anything else), we can name them to SchnorrKey or ECDSAKey.

[dr-orlovsky]

We have two contradictory goals: (a) make rust-bitcoin strictly follow Bitcoin Core names and type organization and (b) do an efficient rust-idiomatic API :) Thus, if we prefer (a), we do not have a lot of choice. I understand the reasoning for (a) option really well; however deeply inside I want to believe © that rust-bitcoin will become one day a new foundation for the next "rustenized" generation of Bitcoin Core (may be with the exception of libbitcoinconsensus). Thus it might be nice to fix some Bitcoin Core inconsistencies/legacy-determined stuff even when we stick to the (a) option in all other cases.

I have a plan to do an alternative PR with separate types for each key kind - just for demo purposes. I had the same idea like @sanket1729, but with four types instead of three:

1. LegacyPubkey - for uncompressed keys. It is not named "uncompressed" to denote that they are discouraged from being used everywhere and kept just for historical compatibility purposes.
2. CompressedPubkey
3. TaprootPubkey, named Taproot since it's more about where in API it can be used, while a serialization method is really secondary.
4. [bitcoin::]PublicKey - the same type as today for compatibility purposes, but deprecated.

This will allow slow migration for depended software. Also at Miniscript level the key types may be more strictly enforced and bitcoin::PublicKey should not be used nor implement MiniscriptKey trait; miniscript functions must always require CompressedKey or TaprootPubkey (in Taproot context) explicitly. I do not see where and why CompressedKey can be interchanged with TaprootPubkey and tend to think that enum pubkey type is not required - at least at the rust-bitcoin level. It might be present at the level above, for situations like public key derivation in certain contexts – but even there I think users should not have an option to choose which key to derive with a single method call and must explicitly use different APIs/method names for different key types (so I agree with @elichai in this context).

P.S. As for miniscript it's really odd to see the ability to use uncompressed keys - for which reason? Miniscript can't even be used with composing Lightning-specific scripts (which are not that legacy), so why we need to keep compatibility in one part when not being compatible with more recent stuff? You can't do roundtrips on Bitcoin scripts from transactions anyway, and removing uncompressed key support (by converting all keys to compressed on parsing) will not make that anyhow worse.

[sanket1729]

I have a plan to do an alternative PR with separate types for each key kind - just for demo purposes. I had the same idea like @sanket1729, but with four types instead of three:

I guess this is something that will impact rust-bitcoin ecosystem as a whole so it's better to get as many opinions as possible, but I have a strong +1 for clean separation of PublicKey types. If we ever do this, it might be worth first testing out by updating the dependent libraries onto the candidate see the impact and iterate again.

P.S. As for miniscript it's really odd to see the ability to use uncompressed keys - for which reason? Miniscript can't even be used with composing Lightning-specific scripts (which are not that legacy), so why we need to keep compatibility in one part when not being compatible with more recent stuff? You can't do roundtrips on Bitcoin scripts from transactions anyway, and removing uncompressed key support (by converting all keys to compressed on parsing) will not make that anyhow worse.

Unfortunately, lightning scripts are not Miniscripts type system because they rely on checking on the size of some item, and then casting it either as a publickey or a preimage. (I may be wrong on the details here, but there is some casting involved). But that being said, there is nothing us from stopping from creating a legacy lightning descriptor for all the various lightning scripts and we would gladly merge that into rust-miniscript as an additional descriptor.

I am not involved in the lighting space, but are the output descriptors for accepted htlc(receiver/sender side), funding tx, claim tx etc?

[dr-orlovsky]

@sanket1729 the problem with miniscript is not the lightning scripts only, and the problem can't be solved with a dedicated descriptor. The problem is that there is no deterministic way for general bitcoin script -> miniscript -> bitcoin script roundtrip in which bitcoin script on the left side will be always byte-equal to the bitcoin script on the right. So, while any bitcoin script can be converted into miniscript, you can't use miniscript to describe all bitcoin scripts with miniscript that will generate the same bitcoin script as it was present in the original transaction. This unfortunately renders miniscript information-losing system that can't replace bitcoin script; it can be used for simple and efficient generation of new scripts (or determinisctic script analysis) - but not for generic work with blockchain, existing transactions or lightning network.

[Kixunil]

FTR I also prefer being Rust-idiomatic over being similar to Core.

[sgeisler]

Another consideration: how do we want to work with extended keys going forward? The current impl in this PR would limit them to ECDSA keys. But from the comments I gather that if we do this we want to do a complete key-overhaul with the 3 relevant (and one deprecated) key types, which I personally like so far. But when we do that we need to find a way to make our BIP32 API compatible, two ideas:

• Either Extended(Pub|Priv)Key becomes generic over the inner key type. For that these key types would need a common trait for derivation.
• Or we see x-keys as not having a certain type and just implement conversion functions into all key types.

Looking at BIP340 it seems to propose to essentially go with the second variant by converting ECDSA keys. The flexibility of v2 is probably also nice when working with one master seed with different sub trees for taproot accounts and other older accounts.

[dr-orlovsky]

Re-read BIP340, had a small discussion with Pieter Wuille and came to conclusion that

1. XCoordOnly keys must not be used for ECDSA sigs and vise verse - otherwise if the user does not careful about key reuse (and we can't assume he is) there might be a leak of the private key information through Schnorr's sigs
2. XCoordOnly keys must be derived from the same BIP32 xpubs/xprivs BUT with a different purpose field. So to support their derivation we will need to provide a support for BIP43 as well - or we risk in ending up with a problem related to pt1
3. I have made a proposal for purpose field for BIP340-signature compatible key derivation: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-February/018381.html

[dr-orlovsky]

I think I found a relatively non-disrupting way of adding Taproot keys to rust-bitcoin. The plan is:

1. Leave bitcoin::PublicKey as is
2. Change ExtendedP(ub/priv)Key to hold secp256k1::Key rather than bitcoin equivalents. It is a necessary change anyway, since BIP32 does not support uncompressed keys and using type with compression flag is a mistake
3. Rename current Extended*key::derive* functions into derive_compressed_*
4. Add a set of derive_taproot_* functions to Extended*key::derive*
   and that's all for the first release.

Next, more invasive change will be to remove bitcoin::PublicKey type, create UncompressedKey types and introduce enum with Copressed and Taproot options.

Upd: after @sgeisler suggestion we do not need derive functions to return keys and it will remain as is: returning xkeys. Instead there will be two to_compressed and to_taproot functions to get keys.

[sipa]

Descriptors should just keep using the same type of keys as they always do (either hex 33-byte/65-byte keys, or xpubs + derivation paths). Taproot descriptors would require compressed keys, but ignore the first byte.

[stevenroose]

Why to_compressed instead of just to_ecdsa? And to_taproot instead of to_schnorr? Keys are not addresses, right? @dr-orlovsky Just for some contexts, there has been some back and forth about the merit of the bitcoin::*Key types. I did some efforts twice to try phase them out in favor of pure secp256k1::Key keys and default everything to compressed and just add special casing for the uncompressed case because it's rarely used.

Here is the latest (since abandoned) effort of that: #221

Before we start discussions again here (excuse my absence on rust-bitcoin the past 2 months), I'm curious how (rust-)secp256k1 will incorporate the Schnorr/Taproot changes.

[apoelstra]

rust-secp follows libsecp. It has new types for xonly public keys and keypairs.

[apoelstra]

What is the state of this PR? @dr-orlovsky's latest message says that he is going to leave PublicKey alone but the contents of the PR do a bunch of dramatic things.

[apoelstra]

NACK

This code, as written, will always throw away the byte immediately following an x-only pubkey, and 50% of the time it will also incorrectly return an error.

[sgeisler]

Important to note, the SecretKey equivalent type is called KeyPair, which is kinda confusing imo since it's just an internal optimization that the type contains the pub key too (it can always be calculated from the secret key).

[dr-orlovsky]

@apoelstra this was the initial PR, following which we had a different discussion (here and in IRC), where we decided to try a different much less invasive approach from this specific comment: #561 (comment). As I posted to IRC, looking a concept ACK on it to start working on it.

[stevenroose]

How about util::ecdsa::PublicKey and util::schnorr::PublicKey instead? I'd be happy to implement that, I have a history of making large MRs that change all the key types here 😅 Very happy that bitcoin::util::schnorr::PublicKey can just be a pub export secp256k1::schnorrsig::PublicKey this time without the need for a compression flag.

[apoelstra]

What would the type of bitcoin::PublicKey be? How would users use the schnorr publickey?

[dr-orlovsky]

I think this PR is about an outdated approach, with the new one described in #561 (comment). Probably we should close this PR or how to prevent further review of the outdated code? (though discussion here may be kept for further decision making)

[dr-orlovsky]

All results of the discussion are put as a separate issue #588, thus this PR is not needed anymore