From f14900b6e4eac26ae5f1c0badfa176d895851c97 Mon Sep 17 00:00:00 2001
From: josibake <josibake@protonmail.com>
Date: Sat, 3 Aug 2024 12:44:31 +0200
Subject: [PATCH 1/6] bench: add benchmark for signing with a taptweak

Add benchmarks for signing with null and non-null merkle_root arguments.
Null and non-null merkle_root arguments will apply the taptweaks
H_TapTweak(P) and H_TapTweak(P | merkle_root), respectively, to the
private key during signing.

This benchmark is added to verify there are no significant performance
changes after moving the taptweak signing logic in a later commit.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>
---
 src/bench/sign_transaction.cpp | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/src/bench/sign_transaction.cpp b/src/bench/sign_transaction.cpp
index 7a2e26e339..6f28f581af 100644
--- a/src/bench/sign_transaction.cpp
+++ b/src/bench/sign_transaction.cpp
@@ -12,6 +12,7 @@
 #include <script/script.h>
 #include <script/sign.h>
 #include <uint256.h>
+#include <test/util/random.h>
 #include <util/translation.h>
 
 enum class InputType {
@@ -66,5 +67,33 @@ static void SignTransactionSingleInput(benchmark::Bench& bench, InputType input_
 static void SignTransactionECDSA(benchmark::Bench& bench)   { SignTransactionSingleInput(bench, InputType::P2WPKH); }
 static void SignTransactionSchnorr(benchmark::Bench& bench) { SignTransactionSingleInput(bench, InputType::P2TR);   }
 
+static void SignSchnorrTapTweakBenchmark(benchmark::Bench& bench, bool use_null_merkle_root)
+{
+    ECC_Context ecc_context{};
+
+    auto key = GenerateRandomKey();
+    auto msg = InsecureRand256();
+    auto merkle_root = use_null_merkle_root ? uint256() : InsecureRand256();
+    auto aux = InsecureRand256();
+    std::vector<unsigned char> sig(64);
+
+    bench.minEpochIterations(100).run([&] {
+        bool success = key.SignSchnorr(msg, sig, &merkle_root, aux);
+        assert(success);
+    });
+}
+
+static void SignSchnorrWithMerkleRoot(benchmark::Bench& bench)
+{
+    SignSchnorrTapTweakBenchmark(bench, /*use_null_merkle_root=*/false);
+}
+
+static void SignSchnorrWithNullMerkleRoot(benchmark::Bench& bench)
+{
+    SignSchnorrTapTweakBenchmark(bench, /*use_null_merkle_root=*/true);
+}
+
 BENCHMARK(SignTransactionECDSA, benchmark::PriorityLevel::HIGH);
 BENCHMARK(SignTransactionSchnorr, benchmark::PriorityLevel::HIGH);
+BENCHMARK(SignSchnorrWithMerkleRoot, benchmark::PriorityLevel::HIGH);
+BENCHMARK(SignSchnorrWithNullMerkleRoot, benchmark::PriorityLevel::HIGH);

From 5d507a0091da1b6c013b00b6c76e19dd4d3b93a7 Mon Sep 17 00:00:00 2001
From: josibake <josibake@protonmail.com>
Date: Sun, 21 Jul 2024 18:43:00 +0200
Subject: [PATCH 2/6] tests: add key tweak smoke test

Sanity check that using CKey/CPubKey directly vs using secp256k1_keypair objects
returns the same results for BIP341 key tweaking.

Co-authored-by: l0rinc <pap.lorinc@gmail.com>
---
 src/test/key_tests.cpp | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/src/test/key_tests.cpp b/src/test/key_tests.cpp
index b897a0a153..6714d8445b 100644
--- a/src/test/key_tests.cpp
+++ b/src/test/key_tests.cpp
@@ -8,6 +8,7 @@
 #include <key_io.h>
 #include <span.h>
 #include <streams.h>
+#include <secp256k1_extrakeys.h>
 #include <test/util/random.h>
 #include <test/util/setup_common.h>
 #include <uint256.h>
@@ -345,4 +346,31 @@ BOOST_AUTO_TEST_CASE(bip341_test_h)
     BOOST_CHECK(XOnlyPubKey::NUMS_H == H);
 }
 
+BOOST_AUTO_TEST_CASE(key_schnorr_tweak_smoke_test)
+{
+    // Sanity check to ensure we get the same tweak using CPubKey vs secp256k1 functions
+    secp256k1_context* secp256k1_context_sign = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);
+
+    CKey key;
+    key.MakeNewKey(true);
+    uint256 merkle_root = InsecureRand256();
+
+    // secp256k1 functions
+    secp256k1_keypair keypair;
+    BOOST_CHECK(secp256k1_keypair_create(secp256k1_context_sign, &keypair, UCharCast(key.begin())));
+    secp256k1_xonly_pubkey xonly_pubkey;
+    BOOST_CHECK(secp256k1_keypair_xonly_pub(secp256k1_context_sign, &xonly_pubkey, nullptr, &keypair));
+    unsigned char xonly_bytes[32];
+    BOOST_CHECK(secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, xonly_bytes, &xonly_pubkey));
+    uint256 tweak_old = XOnlyPubKey(xonly_bytes).ComputeTapTweakHash(&merkle_root);
+
+    // CPubKey
+    CPubKey pubkey = key.GetPubKey();
+    uint256 tweak_new = XOnlyPubKey(pubkey).ComputeTapTweakHash(&merkle_root);
+
+    BOOST_CHECK_EQUAL(tweak_old, tweak_new);
+
+    secp256k1_context_destroy(secp256k1_context_sign);
+}
+
 BOOST_AUTO_TEST_SUITE_END()

From c39fd39ba868253b5118db2e1ac1461d29f0b4ce Mon Sep 17 00:00:00 2001
From: josibake <josibake@protonmail.com>
Date: Tue, 16 Jul 2024 18:34:10 +0200
Subject: [PATCH 3/6] crypto: add KeyPair wrapper class

Add a `KeyPair` class which wraps the `secp256k1_keypair`. This keeps
the secret data in secure memory and enables passing the
`KeyPair` object directly to libsecp256k1 functions expecting a
`secp256k1_keypair`.

Motivation: when passing `CKeys` for taproot outputs to libsecp256k1 functions,
the first step is to create a `secp256k1_keypair` data type and use that
instead. This is so the libsecp256k1 function can determine if the key
needs to be negated, e.g., when signing.

This is a bit clunky in that it creates an extra step when using a `CKey`
for a taproot output and also involves copying the secret data into a
temporary object, which the caller must then take care to cleanse. In
addition, the logic for applying the merkle_root tweak currently
only exists in the `SignSchnorr` function.

In a later commit, we will add the merkle_root tweaking logic to this
function, which will make the merkle_root logic reusable outside of
signing by using the `KeyPair` class directly.

Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
---
 src/key.cpp | 15 ++++++++++++++
 src/key.h   | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/src/key.cpp b/src/key.cpp
index 97d7821e74..3945170515 100644
--- a/src/key.cpp
+++ b/src/key.cpp
@@ -363,6 +363,11 @@ ECDHSecret CKey::ComputeBIP324ECDHSecret(const EllSwiftPubKey& their_ellswift, c
     return output;
 }
 
+KeyPair CKey::ComputeKeyPair() const
+{
+    return KeyPair(*this);
+}
+
 CKey GenerateRandomKey(bool compressed) noexcept
 {
     CKey key;
@@ -420,6 +425,16 @@ 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)
+{
+    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) ClearKeyPairData();
+}
+
 bool ECC_InitSanityCheck() {
     CKey key = GenerateRandomKey();
     CPubKey pubkey = key.GetPubKey();
diff --git a/src/key.h b/src/key.h
index bbca2c4dee..f84117f98f 100644
--- a/src/key.h
+++ b/src/key.h
@@ -28,6 +28,8 @@ constexpr static size_t ECDH_SECRET_SIZE = CSHA256::OUTPUT_SIZE;
 // Used to represent ECDH shared secret (ECDH_SECRET_SIZE bytes)
 using ECDHSecret = std::array<std::byte, ECDH_SECRET_SIZE>;
 
+class KeyPair;
+
 /** An encapsulated private key. */
 class CKey
 {
@@ -202,6 +204,11 @@ public:
     ECDHSecret ComputeBIP324ECDHSecret(const EllSwiftPubKey& their_ellswift,
                                        const EllSwiftPubKey& our_ellswift,
                                        bool initiating) const;
+    /** Compute a KeyPair
+     *
+     *  Wraps a `secp256k1_keypair` type.
+     */
+    KeyPair ComputeKeyPair() const;
 };
 
 CKey GenerateRandomKey(bool compressed = true) noexcept;
@@ -235,6 +242,58 @@ struct CExtKey {
     void SetSeed(Span<const std::byte> seed);
 };
 
+/** KeyPair
+ *
+ *  Wraps a `secp256k1_keypair` type, an opaque data structure for holding a secret and public key.
+ *  This is intended for BIP340 keys and allows us to easily determine if the secret key needs to
+ *  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.
+ */
+class KeyPair
+{
+public:
+    KeyPair() noexcept = default;
+    KeyPair(KeyPair&&) noexcept = default;
+    KeyPair& operator=(KeyPair&&) noexcept = default;
+    KeyPair& operator=(const KeyPair& other)
+    {
+        if (this != &other) {
+            if (other.m_keypair) {
+                MakeKeyPairData();
+                *m_keypair = *other.m_keypair;
+            } else {
+                ClearKeyPairData();
+            }
+        }
+        return *this;
+    }
+
+    KeyPair(const KeyPair& other) { *this = other; }
+
+    friend KeyPair CKey::ComputeKeyPair() 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);
+
+    using KeyType = std::array<unsigned char, 96>;
+    secure_unique_ptr<KeyType> m_keypair;
+
+    void MakeKeyPairData()
+    {
+        if (!m_keypair) m_keypair = make_secure_unique<KeyType>();
+    }
+
+    void ClearKeyPairData()
+    {
+        m_keypair.reset();
+    }
+};
+
 /** Check that required EC support is available at runtime. */
 bool ECC_InitSanityCheck();
 

From cebb08b121ce8c4c5e68bd043b8668c106e31169 Mon Sep 17 00:00:00 2001
From: josibake <josibake@protonmail.com>
Date: Tue, 16 Jul 2024 18:36:41 +0200
Subject: [PATCH 4/6] 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
---
 src/key.cpp | 54 ++++++++++++++++++++++++++++-------------------------
 src/key.h   | 20 +++++++++++++++++---
 2 files changed, 46 insertions(+), 28 deletions(-)

diff --git a/src/key.cpp b/src/key.cpp
index 3945170515..37b78d8e77 100644
--- a/src/key.cpp
+++ b/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();
diff --git a/src/key.h b/src/key.h
index f84117f98f..6ed6fd0fc3 100644
--- a/src/key.h
+++ b/src/key.h
@@ -207,8 +207,19 @@ public:
     /** 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 @@ public:
 
     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;

From 72a5822d43d47431b2838ebfcb1f2e21210f5ccb Mon Sep 17 00:00:00 2001
From: josibake <josibake@protonmail.com>
Date: Tue, 16 Jul 2024 17:42:32 +0200
Subject: [PATCH 5/6] tests: add tests for KeyPair

Reuse existing BIP340 tests, as there should be
no behavior change between the two
---
 src/test/key_tests.cpp | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/test/key_tests.cpp b/src/test/key_tests.cpp
index 6714d8445b..112d6db193 100644
--- a/src/test/key_tests.cpp
+++ b/src/test/key_tests.cpp
@@ -300,6 +300,13 @@ BOOST_AUTO_TEST_CASE(bip340_test_vectors)
         // Verify those signatures for good measure.
         BOOST_CHECK(pubkey.VerifySchnorr(msg256, sig64));
 
+        // Repeat the same check, but use the KeyPair directly without any merkle tweak
+        KeyPair keypair = key.ComputeKeyPair(/*merkle_root=*/nullptr);
+        bool kp_ok = keypair.SignSchnorr(msg256, sig64, aux256);
+        BOOST_CHECK(kp_ok);
+        BOOST_CHECK(pubkey.VerifySchnorr(msg256, sig64));
+        BOOST_CHECK(std::vector<unsigned char>(sig64, sig64 + 64) == sig);
+
         // Do 10 iterations where we sign with a random Merkle root to tweak,
         // and compare against the resulting tweaked keys, with random aux.
         // In iteration i=0 we tweak with empty Merkle tree.
@@ -313,6 +320,12 @@ BOOST_AUTO_TEST_CASE(bip340_test_vectors)
             bool ok = key.SignSchnorr(msg256, sig64, &merkle_root, aux256);
             BOOST_CHECK(ok);
             BOOST_CHECK(tweaked_key.VerifySchnorr(msg256, sig64));
+
+            // Repeat the same check, but use the KeyPair class directly
+            KeyPair keypair = key.ComputeKeyPair(&merkle_root);
+            bool kp_ok = keypair.SignSchnorr(msg256, sig64, aux256);
+            BOOST_CHECK(kp_ok);
+            BOOST_CHECK(tweaked_key.VerifySchnorr(msg256, sig64));
         }
     }
 }

From ec973dd19719541dbcd6f3a6facf6f5dd7cf439c Mon Sep 17 00:00:00 2001
From: josibake <josibake@protonmail.com>
Date: Sat, 3 Aug 2024 14:58:28 +0200
Subject: [PATCH 6/6] refactor: remove un-tested early returns

Replace early returns in KeyPair::KeyPair() with asserts.

The if statements imply there is an error we are handling, but keypair_xonly_pub
and xonly_pubkey_serialize can only fail if the keypair object is malformed, i.e.,
it was created with a bad secret key. Since we check that the keypair was created
successfully before attempting to extract the public key, using asserts more
accurately documents what we expect here and removes untested branches from the code.
---
 src/key.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/key.cpp b/src/key.cpp
index 37b78d8e77..360a1f46d3 100644
--- a/src/key.cpp
+++ b/src/key.cpp
@@ -414,9 +414,9 @@ KeyPair::KeyPair(const CKey& key, const uint256* merkle_root)
     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;
+        assert(secp256k1_keypair_xonly_pub(secp256k1_context_sign, &pubkey, nullptr, keypair));
+        assert(secp256k1_xonly_pubkey_serialize(secp256k1_context_sign, pubkey_bytes, &pubkey));
         uint256 tweak = XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root);
         success = secp256k1_keypair_xonly_tweak_add(secp256k1_context_static, keypair, tweak.data());
     }