Merge bitcoin/bitcoin#30051: crypto, refactor: add new KeyPair class

ec973dd refactor: remove un-tested early returns (josibake)
72a5822 tests: add tests for KeyPair (josibake)
cebb08b refactor: move SignSchnorr to KeyPair (josibake)
c39fd39 crypto: add KeyPair wrapper class (josibake)
5d507a0 tests: add key tweak smoke test (josibake)
f14900b bench: add benchmark for signing with a taptweak (josibake)

Pull request description:

  Broken out from #28201

  ---

  The wallet returns an untweaked internal key for taproot outputs. If the output commits to a tree of scripts, this key needs to be tweaked with the merkle root. Even if the output does not commit to a tree of scripts, BIP341/342 recommend commiting to a hash of the public key.

  Previously, this logic for applying the taptweak was implemented in the `CKey::SignSchnorr` method.

  This PR moves introduces a KeyPair class which wraps a `secp256k1_keypair` type and refactors SignSchnorr to use this new KeyPair. The KeyPair class is created with an optional merkle_root argument and the logic from BIP341 is applied depending on the state of the merkle_root argument.

  The motivation for this refactor is to be able to use the tap tweak logic outside of signing, e.g. in silent payments when retrieving the private key (see #28201).

  Outside of silent payments, since we almost always convert a `CKey` to a `secp256k1_keypair` when doing anything with taproot keys, it seems generally useful to have a way to model this type in our code base.

ACKs for top commit:
  paplorinc:
    ACK ec973dd - will happily reack if you decide to apply @ismaelsadeeq's suggestions
  ismaelsadeeq:
    Code review ACK ec973dd
  itornaza:
    trACK ec973dd
  theStack:
    Code-review ACK ec973dd

Tree-SHA512: 34947e3eac39bd959807fa21b6045191fc80113bd650f6f08606e4bcd89aa17d6afd48dd034f6741ac4ff304b104fa8c1c1898e297467edcf262d5f97425da7b

Author: ryanofsky

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);

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,6 +344,11 @@ ECDHSecret CKey::ComputeBIP324ECDHSecret(const EllSwiftPubKey& their_ellswift, c
     return output;
 }
 
+KeyPair CKey::ComputeKeyPair(const uint256* merkle_root) const
+{
+    return KeyPair(*this, merkle_root);
+}
+
 CKey GenerateRandomKey(bool compressed) noexcept
 {
     CKey key;
@@ -420,6 +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, 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;
+        unsigned char pubkey_bytes[32];
+        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());
+    }
+    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

@@ -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,22 @@ class CKey
     ECDHSecret ComputeBIP324ECDHSecret(const EllSwiftPubKey& their_ellswift,
                                        const EllSwiftPubKey& our_ellswift,
                                        bool initiating) const;
+    /** 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 uint256* merkle_root) const;
 };
 
 CKey GenerateRandomKey(bool compressed = true) noexcept;
@@ -235,6 +253,61 @@ 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.
+ *
+ *  A KeyPair can be created from a CKey with an optional merkle_root tweak (per BIP342). See
+ *  CKey::ComputeKeyPair for more details.
+ */
+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 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, const uint256* merkle_root);
+
+    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();
 

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>
@@ -299,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.
@@ -312,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));
         }
     }
 }
@@ -345,4 +359,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()