[staging] refactor: use KeyPair in SignSchnorr

Use `KeyPair` instead of creating a `secp256k1_keypair` object. The
main change here is creating a `KeyPair` instead of a
`secp256k1_keypair` and then passing it to the libsec256k1 functions
using `reinterpret_cast<secp256k1_keypair*>(keypair)`.

The same variable name `keypair` is used here to simplify the diff in a
later commit when all of the logic in SignSchnorr is moved into the
KeyPair class, where `keypair` is replaced with the private member `m_keypair`.

Note: this is why `data()` is currently non-const. Since SignSchnorr
does not have access to the private `m_keypair` member directly, we are cheating
and accessing it through `data()` in this commit. This is to keep the
changes incremental and easier to review.

src/key.cpp

@@ -273,22 +273,13 @@ bool CKey::SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint2
 {
     assert(sig.size() == 64);
     bool ret = true;
-    secp256k1_keypair keypair;
-    if (!secp256k1_keypair_create(secp256k1_context_sign, &keypair, UCharCast(begin()))) return false;
-    if (merkle_root) {
-        secp256k1_xonly_pubkey pubkey;
-        ret &= secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, &keypair);
-        unsigned char pubkey_bytes[32];
-        ret &= secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey);
-        uint256 tweak = XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root);
-        ret &= secp256k1_keypair_xonly_tweak_add(secp256k1_context_static, &keypair, tweak.data());
-    }
-    if (!ret) return false;
-    ret &= secp256k1_schnorrsig_sign32(secp256k1_context_sign, sig.data(), hash.data(), &keypair, aux.data());
+    KeyPair keypair = ComputeKeyPair(merkle_root);
+    if (!keypair.IsValid()) return false;
+    ret &= secp256k1_schnorrsig_sign32(secp256k1_context_sign, sig.data(), hash.data(), reinterpret_cast<const secp256k1_keypair*>(keypair.data()), 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_keypair_xonly_pub(secp256k1_context_static, &pubkey_verify, nullptr, reinterpret_cast<const secp256k1_keypair*>(keypair.data()));
         ret &= secp256k1_schnorrsig_verify(secp256k1_context_static, sig.data(), hash.begin(), 32, &pubkey_verify);
     }
     if (!ret) memory_cleanse(sig.data(), sig.size());
@@ -365,9 +356,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
@@ -427,12 +418,20 @@ 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();
 
     bool ret = secp256k1_keypair_create(secp256k1_context_sign, reinterpret_cast<secp256k1_keypair*>(m_keypair->data()), UCharCast(key.data()));
+    if (merkle_root) {
+        secp256k1_xonly_pubkey pubkey;
+        ret &= secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, reinterpret_cast<const secp256k1_keypair*>(m_keypair->data()));
+        unsigned char pubkey_bytes[32];
+        ret &= secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey);
+        uint256 tweak = XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root);
+        ret &= secp256k1_keypair_xonly_tweak_add(secp256k1_context_static, reinterpret_cast<secp256k1_keypair*>(m_keypair->data()), tweak.data());
+    }
     if (!ret) ClearKeyPairData();
 }
 

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,20 +285,27 @@ 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;
 
     /**
-     * This method will be made read-only in a later commit.
-     * It is only read/write here to simplify the diff in the next two commits
+     * data() is provided as a read-only method for passing a KeyPair object to secp256k1 functions
+     * expecting a `secp256k1_keypair`. This avoids needing to create a temporary `secp256k1_keypair`
+     * object by allowing the KeyPair to be passed directly in the following manner:
+     *
+     *     reinterpret_cast<const secp256k1_keypair*>(keypair.data())
+     *
+     * Recall that `secp256k1_keypair` is an opaque data type, so this method should only be used
+     * for passing a KeyPair object as a secp256k1_keypair and should never be used to access the
+     * underlying keypair bytes directly.
      */
-    unsigned char* data() { return IsValid() ? m_keypair->data() : nullptr; }
+    const unsigned char* data() const { return IsValid() ? m_keypair->data() : nullptr; }
 
     //! 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;