refactor: move SignSchnorr to KeyPair

Move `SignSchnorr` to `KeyPair`. This makes `CKey::SignSchnorr` now
compute a `KeyPair` object and then call `KeyPair::SignSchorr`. The
notable changes are:

    * Move the merkle_root tweaking out of the sign function and into
      the KeyPair constructor
    * Remove the temporary secp256k1_keypair object and have the
      functions access m_keypair->data() directly

Author: josibake

src/key.cpp

@@ -271,27 +271,8 @@ bool CKey::SignCompact(const uint256 &hash, std::vector<unsigned char>& vchSig)
 
 bool CKey::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint256* merkle_root, const uint256& aux) const
 {
-    assert(sig.size() == 64);
-    secp256k1_keypair keypair;
-    if (!secp256k1_keypair_create(secp256k1_context_sign, &keypair, UCharCast(begin()))) return false;
-    if (merkle_root) {
-        secp256k1_xonly_pubkey pubkey;
-        if (!secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, &keypair)) return false;
-        unsigned char pubkey_bytes[32];
-        if (!secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey)) return false;
-        uint256 tweak = XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root);
-        if (!secp256k1_keypair_xonly_tweak_add(secp256k1_context_static, &keypair, tweak.data())) return false;
-    }
-    bool ret = secp256k1_schnorrsig_sign32(secp256k1_context_sign, sig.data(), hash.data(), &keypair, aux.data());
-    if (ret) {
-        // Additional verification step to prevent using a potentially corrupted signature
-        secp256k1_xonly_pubkey pubkey_verify;
-        ret = secp256k1_keypair_xonly_pub(secp256k1_context_static, &pubkey_verify, nullptr, &keypair);
-        ret &= secp256k1_schnorrsig_verify(secp256k1_context_static, sig.data(), hash.begin(), 32, &pubkey_verify);
-    }
-    if (!ret) memory_cleanse(sig.data(), sig.size());
-    memory_cleanse(&keypair, sizeof(keypair));
-    return ret;
+    KeyPair kp = ComputeKeyPair(merkle_root);
+    return kp.SignSchnorr(hash, sig, aux);
 }
 
 bool CKey::Load(const CPrivKey &seckey, const CPubKey &vchPubKey, bool fSkipCheck=false) {
@@ -363,9 +344,9 @@ ECDHSecret CKey::ComputeBIP324ECDHSecret(const EllSwiftPubKey& their_ellswift, c
     return output;
 }
 
-KeyPair CKey::ComputeKeyPair() const
+KeyPair CKey::ComputeKeyPair(const uint256* merkle_root) const
 {
-    return KeyPair(*this);
+    return KeyPair(*this, merkle_root);
 }
 
 CKey GenerateRandomKey(bool compressed) noexcept
@@ -425,16 +406,39 @@ void CExtKey::Decode(const unsigned char code[BIP32_EXTKEY_SIZE]) {
     if ((nDepth == 0 && (nChild != 0 || ReadLE32(vchFingerprint) != 0)) || code[41] != 0) key = CKey();
 }
 
-KeyPair::KeyPair(const CKey& key)
+KeyPair::KeyPair(const CKey& key, const uint256* merkle_root)
 {
     static_assert(std::tuple_size<KeyType>() == sizeof(secp256k1_keypair));
     MakeKeyPairData();
     auto keypair = reinterpret_cast<secp256k1_keypair*>(m_keypair->data());
-
     bool success = secp256k1_keypair_create(secp256k1_context_sign, keypair, UCharCast(key.data()));
+    if (success && merkle_root) {
+        secp256k1_xonly_pubkey pubkey;
+        if (!secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, keypair)) return;
+        unsigned char pubkey_bytes[32];
+        if (!secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey)) return;
+        uint256 tweak = XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root);
+        success = secp256k1_keypair_xonly_tweak_add(secp256k1_context_static, keypair, tweak.data());
+    }
     if (!success) ClearKeyPairData();
 }
 
+bool KeyPair::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint256& aux) const
+{
+    assert(sig.size() == 64);
+    if (!IsValid()) return false;
+    auto keypair = reinterpret_cast<const secp256k1_keypair*>(m_keypair->data());
+    bool ret = secp256k1_schnorrsig_sign32(secp256k1_context_sign, sig.data(), hash.data(), keypair, aux.data());
+    if (ret) {
+        // Additional verification step to prevent using a potentially corrupted signature
+        secp256k1_xonly_pubkey pubkey_verify;
+        ret = secp256k1_keypair_xonly_pub(secp256k1_context_static, &pubkey_verify, nullptr, keypair);
+        ret &= secp256k1_schnorrsig_verify(secp256k1_context_static, sig.data(), hash.begin(), 32, &pubkey_verify);
+    }
+    if (!ret) memory_cleanse(sig.data(), sig.size());
+    return ret;
+}
+
 bool ECC_InitSanityCheck() {
     CKey key = GenerateRandomKey();
     CPubKey pubkey = key.GetPubKey();

src/key.h

@@ -207,8 +207,19 @@ class CKey
     /** Compute a KeyPair
      *
      *  Wraps a `secp256k1_keypair` type.
+     *
+     *  `merkle_root` is used to optionally perform tweaking of
+     *  the internal key, as specified in BIP341:
+     *
+     *  - If merkle_root == nullptr: no tweaking is done, use the internal key directly (this is
+     *                               used for signatures in BIP342 script).
+     *  - If merkle_root->IsNull():  tweak the internal key with H_TapTweak(pubkey) (this is used for
+     *                               key path spending when no scripts are present).
+     *  - Otherwise:                 tweak the internal key with H_TapTweak(pubkey || *merkle_root)
+     *                               (this is used for key path spending with the
+     *                               Merkle root of the script tree).
      */
-    KeyPair ComputeKeyPair() const;
+    KeyPair ComputeKeyPair(const uint256* merkle_root) const;
 };
 
 CKey GenerateRandomKey(bool compressed = true) noexcept;
@@ -249,6 +260,9 @@ struct CExtKey {
  *  be negated by checking the parity of the public key. This class primarily intended for passing
  *  secret keys to libsecp256k1 functions expecting a `secp256k1_keypair`. For all other cases,
  *  CKey should be preferred.
+ *
+ *  A KeyPair can be created from a CKey with an optional merkle_root tweak (per BIP342). See
+ *  CKey::ComputeKeyPair for more details.
  */
 class KeyPair
 {
@@ -271,14 +285,14 @@ class KeyPair
 
     KeyPair(const KeyPair& other) { *this = other; }
 
-    friend KeyPair CKey::ComputeKeyPair() const;
+    friend KeyPair CKey::ComputeKeyPair(const uint256* merkle_root) const;
     [[nodiscard]] bool SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint256& aux) const;
 
     //! Check whether this keypair is valid.
     bool IsValid() const { return !!m_keypair; }
 
 private:
-    KeyPair(const CKey& key);
+    KeyPair(const CKey& key, const uint256* merkle_root);
 
     using KeyType = std::array<unsigned char, 96>;
     secure_unique_ptr<KeyType> m_keypair;