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

Taproot keys implementation strategy #588

Closed
4 tasks done
dr-orlovsky opened this issue Apr 12, 2021 · 9 comments
Closed
4 tasks done

Taproot keys implementation strategy #588

dr-orlovsky opened this issue Apr 12, 2021 · 9 comments
Assignees
@dr-orlovsky @stevenroose
Labels
API break This PR requires a version bump for the next release minor API Change This PR should get a minor version bump
Projects
Taproot
Milestone
0.28.0

Comments

@dr-orlovsky
Copy link
Collaborator

dr-orlovsky commented Apr 12, 2021 •
edited

After #561 (comment) and #561 (comment)

  1. Leave bitcoin::PublicKey as is, probably adding bitcoin::util::ecdsa::PublicKey alias to it (the same for private keys).
  2. Re-export Taproot keys under bitcoin::util::schnorr
  3. 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
  4. Introduce to Extended*key* functions to_ecdsa (returning always compressed ECDSA key) and to_shchnorr.
    ... and that's all for the first non-API breaking release.

Next, more invasive change will be:

  1. Remove bitcoin::PublicKey type
  2. Create UncompressedKey types (for legacy purposes) and
  3. Introduce PublicKey enum with just Compressed and Taproot variants (such that it can be used in SegWit miniscript/descriptor context without the need for errors/panics).

The first phase is implemented with the following PRs:
@dr-orlovsky dr-orlovsky added API break This PR requires a version bump for the next release minor API Change This PR should get a minor version bump labels Apr 12, 2021
@dr-orlovsky dr-orlovsky added this to Todo in Taproot Apr 12, 2021
@dr-orlovsky dr-orlovsky added this to the 0.27.0 milestone Apr 12, 2021
This was referenced Apr 12, 2021
Closed
Closed
Merged
Merged
Merged
@apoelstra
Copy link
Member

What type would Script::push_key take?

@apoelstra
Copy link
Member

I also don't think ecdsa::PublicKey is the right name for 33-byte pubkeys, because 33-byte pubkeys are not only used for ECDSA. You also need full keys when doing Taproot derivtaions or when working wtih bip32, even if you are ultimately deriving x-only keys.

@dr-orlovsky
Copy link
Collaborator Author

What type would Script::push_key take?

It looks like the script must become witness-version aware (move context from miniscript?)...

@apoelstra
Copy link
Member

Then what would the type of TxOut::script_pubkey be?

@JeremyRubin
Copy link
Contributor

personally, i don't think it makes sense to have a separate type of key for taproot since the keys are technically compatible, right? if there is such a need, the keys could be e.g. backed by some underlying type?

this might become an issue when it comes to BIP-118 though...

@sanket1729
Copy link
Member

I am a bit late for the party. But I don't like mixing Public Keys with signature algorithms. Keys are always full, and should not be associated with any signature algorithm.

My vote is for having separate modules for keys and signature checking. For most users, keys are just for signature checking, so it does make some sense. But the cryptographer inside me just wants to highlight this difference.
I would propose changing

  • ecdsa to ec for the keys.
  • changing schnorr::Keypair to ec::XonlyKeyPair
  • moving all the signature-related stuff to ecdsa and schnorr modules.

This also matches how it is implemented in secp256k1 and IMO makes more conceptual sense if you consider applications for keys beyond signature checking.

@Kixunil
Copy link
Collaborator

Kixunil commented Aug 2, 2021

While I agree with @sanket1729 maybe it'd be sensible to define newtypes around PublicKey similar to newtypes around hashes?

Also nit: I think XOnlyKeyPair is more appropriate casing.

@dr-orlovsky
Copy link
Collaborator Author

dr-orlovsky commented Aug 9, 2021 •
edited

My vote is for having separate modules for keys and signature checking.
moving all the signature-related stuff to ecdsa and schnorr modules.

Agree. Just for consistency, the same should be applied to secp256k1 lib

Regarding the rest, I will try to re-cap what key type restrictions are we having in post-SegWitV1 world?

  1. ScriptPubkey can take any key type, however this is context-dependent. Currently, script context can't be addressed in rust-bitcoin (and is addressed in rust-miniscript), which creates a lot of exceptions and error handling there (and pain). Ideally, we can have a refactored script::Builder in rust-bitcoin, which will be context-aware and will fail on improper key type usage, and remove Script::push_key completely (in favor of the builder). Then, Script type should continue to be backed just by a byte sequence, thus no propagation of the problem to the level of TxOut/Transaction. I think this addresses @apoelstra concern above.

    If removing Script::push_key and re-doing Builder considered too invasive, (while I think this is a good API practice preventing from pushing random data into random script), we may have more key-specific functions on Script, one for each of pubkey types we will agree upon.

  2. Witness is anyway just a vec of bytestrings, so keys story does not affect it - they are all get there via length-independent slice representation anyway.

  3. BIP32/PSBTs should rely on secp256k1-provided types, like currently proposed in Taproot: BIP32 extended keys using Scep256k1 keys instead of bitcoin ECDSA #590 and PSBT BIP32 keys using to Secp256k1 keys instead of bitcoin ECDSA #591

Thus, all use-cases inside rust-bitcoin itself are independent from the selected key type system. For external use (miniscript, wallets, block explorers), I propose to have dedicated key type per each format, since they will like to differentiate between their usage in different contexts. Thus, we will end up with having:

  • introduce a newtype UncompressedPubkey over secp256k1::PublicKey
  • do a newtype ec::CompressedPubkey over secp256k1::PublicKey for SegWit v0 contexts (like in miniscript)
  • PublicKey remains as is (may be we can rename it into FullPubkey), representing either compressed or uncompressed key (for pre-SegWit world)
  • re-export secp256k1::schnorrsig::PublicKey as ec::XOnlyPubkey to disambiguate the naming

@dr-orlovsky dr-orlovsky mentioned this issue Aug 9, 2021
Closed
@dr-orlovsky dr-orlovsky moved this from Todo to In process in Taproot Sep 13, 2021
@dr-orlovsky dr-orlovsky mentioned this issue Sep 14, 2021
Closed
@dr-orlovsky dr-orlovsky mentioned this issue Jan 8, 2022
Merged
sanket1729 added a commit that referenced this issue Jan 9, 2022
@sanket1729
Merge #590: Taproot: BIP32 extended keys using Scep256k1 keys instead…
476eed7 
… of bitcoin ECDSA

cf0c48c Improve Debug for PrivateKey (Dr Maxim Orlovsky)
b65a6ae Test for extended private key keypair generation  f5875a (Dr Maxim Orlovsky)
e6a3d60 BIP32 extended key `to_ecdsa()` and `to_schnorr()` methods (Dr Maxim Orlovsky)
b72f56c BIP32 extended keys are using Scep256k1 keys instead of bitcoin ECDSA (Dr Maxim Orlovsky)

Pull request description:

  This is third step required to introduce Schnorr key support according to #588. This PR starts API-breaking changes and is follow-up to non-API breaking #589, which is already merged.

  PR rationale: BIP32 does not support uncompressed keys and using type with compression flag was a mistake

ACKs for top commit:
  apoelstra:
    ACK cf0c48c
  sanket1729:
    ACK cf0c48c. #757 might need rework after this

Tree-SHA512: 6356a65004e7517256bacbf9aaeb69a22fd8536b341e567c5c4e819288e1105d083fe12ac0641404c407c97acf039bdc525f8e02b1b594a6cdda90106f3b1bdc
@dr-orlovsky dr-orlovsky moved this from In process to Ready for Review in Taproot Jan 10, 2022
@dr-orlovsky dr-orlovsky moved this from Ready for Review to In review in Taproot Jan 10, 2022
sanket1729 added a commit that referenced this issue Jan 11, 2022
@sanket1729
Merge #591: PSBT BIP32 keys using to Secp256k1 keys instead of bitcoi…
e4d5039 
…n ECDSA

a6e8f58 PSBT BIP32 keys moved to Secp256k1 from bitcoin ECDSA (Dr Maxim Orlovsky)

Pull request description:

  Fourth step in implementation of Schnorr key support after #588. This PR is a follow-up to non-API breaking #589 and API-breaking #590, which must be reviewed and merged first. ~~(The current PR includes all commits from #589 and #590, which should be reviewed there. The only commit specific to this PR is b8105e9)~~

  UPDATE: All related PRs are merged now and this PR is ready for the review

  PR description:
  While PSBT BIP174 does not specify whether uncompressed keys are supported in BIP32-related fields, from BIP32 it follows that it is impossible to use uncompressed keys within the extended keys.  This PR fixes this situation and is a companion to BIP174 PR clarifying key serialization: bitcoin/bips#1100

ACKs for top commit:
  apoelstra:
    ACK a6e8f58
  sanket1729:
    ACK a6e8f58. Not sure which order to merge since there are many ready PRs which that would break each other.

Tree-SHA512: 198ba646bbce1949b255a54a97957d952acdad8b7f9580be123116c0f44d773e6d90e0cac0d5993ec9a6b3328aa43aced0908522817861585877c50008fec835
sanket1729 added a commit that referenced this issue Jan 15, 2022
@sanket1729
Merge #757: Minimally-invasive separation of bitcoin keys from ECDSA …
d1f051c 
…signature types

8a993e8 Properly deprecate util::ecdsa key re-exports (Dr Maxim Orlovsky)
bcb8932 Re-org keys and ecdsa mods - pt.3 (Dr Maxim Orlovsky)
d1c2213 Re-org keys and ecdsa mods - pt.2 (Dr Maxim Orlovsky)
b917016 Re-org keys and ecdsa mods - pt.1 (Dr Maxim Orlovsky)
2d9de78 Re-export all key types under `util::key`. Deprecate other exports. (Dr Maxim Orlovsky)

Pull request description:

  This PR tries to do a minimally-invazive separation of signature- and key-related types, previously mixed in a single `util::ecdsa` module.

  Rationale: bitcoin key types are not specific for signature algorithm. See discussion at #588.

  This PR became possible after we moved on new `secp256k1` version exposing `XonlyPublicKey` type, since now all key types may co-exist in a single module under different names

  The PR goal is achieved through
  - Renaming ecdsa mod into private ec module such that the code is not copied and diff size is small;
  - Introducing dummy ecdsa mod back in the next commit and re-exporiting only signature types from internal `ec` mod in it;
  - Re-exporting all key types under `key` module, removing previous depreciation message for bitcoin keys.

ACKs for top commit:
  apoelstra:
    ACK 8a993e8
  sanket1729:
    utACK 8a993e8

Tree-SHA512: 9f71edaa2cf4cdab4b239cb1d57576e2ba0fc3c2ec0ea19ae232005967b9400da6ded992b33d10b190ca617a66dca9b99be430bc5058a064f0be1489723c4a3a
@dr-orlovsky
Copy link
Collaborator Author

Closed with #757 merged

Taproot automation moved this from In review to Done Jan 15, 2022
@dr-orlovsky dr-orlovsky mentioned this issue Feb 17, 2022
Closed
Davidson-Souza pushed a commit to Davidson-Souza/rust-bitcoin that referenced this issue Jul 12, 2023
@apoelstra
Merge rust-bitcoin/rust-secp256k1#590: Bump secp256k1-sys to v0.9.0
d738998 
5ae136d Bump secp256k1-sys version to 0.8.1 (Tobin C. Harding)

Pull request description:

  We just bumped the version of `secp256k1`, since we recently added a new public function to `secp256k1-sys` we need to bump the minor version number there too.

  Should have been done as part of rust-bitcoin#588, its hard to get good help :)

ACKs for top commit:
  apoelstra:
    ACK 5ae136d

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

@dr-orlovsky dr-orlovsky

@stevenroose stevenroose

Labels
API break This PR requires a version bump for the next release minor API Change This PR should get a minor version bump
Projects
Taproot
Done
Milestone
0.28.0
Development

No branches or pull requests
6 participants
@dr-orlovsky @stevenroose @JeremyRubin @Kixunil @apoelstra @sanket1729