From fc47c31b458b52a059a178c076b1499e200d4b34 Mon Sep 17 00:00:00 2001
From: Olaoluwa Osuntokun <laolu32@gmail.com>
Date: Thu, 27 Oct 2022 16:19:32 -0700
Subject: [PATCH] txscript: modify TweakTaprootPrivKey to operate on private
 key copy

In this commit, we fix an inadvertent mutation bug that would at times
cause the private key passed into the tweak function to actually be
*modified* in place.

We fix this by accepting the value instead of a pointer. The actual
private key struct itself contains no pointer fields, so this is
effectively a deep copy via dereference.

We also add a new unit test that fails w/o this change, to show that the
private key was indeed being modified.
---
 txscript/sign.go         |  2 +-
 txscript/taproot.go      |  4 ++--
 txscript/taproot_test.go | 42 +++++++++++++++++++++++++++++++++++++---
 3 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/txscript/sign.go b/txscript/sign.go
index fc89312f..f3001d89 100644
--- a/txscript/sign.go
+++ b/txscript/sign.go
@@ -82,7 +82,7 @@ func RawTxInTaprootSignature(tx *wire.MsgTx, sigHashes *TxSigHashes, idx int,
 
 	// Before we sign the sighash, we'll need to apply the taptweak to the
 	// private key based on the tapScriptRootHash.
-	privKeyTweak := TweakTaprootPrivKey(key, tapScriptRootHash)
+	privKeyTweak := TweakTaprootPrivKey(*key, tapScriptRootHash)
 
 	// With the sighash constructed, we can sign it with the specified
 	// private key.
diff --git a/txscript/taproot.go b/txscript/taproot.go
index 2e452f92..cd73aff4 100644
--- a/txscript/taproot.go
+++ b/txscript/taproot.go
@@ -296,12 +296,12 @@ func ComputeTaprootKeyNoScript(internalKey *btcec.PublicKey) *btcec.PublicKey {
 // but on the private key instead. The final key is derived as: privKey +
 // h_tapTweak(internalKey || merkleRoot) % N, where N is the order of the
 // secp256k1 curve, and merkleRoot is the root hash of the tapscript tree.
-func TweakTaprootPrivKey(privKey *btcec.PrivateKey,
+func TweakTaprootPrivKey(privKey btcec.PrivateKey,
 	scriptRoot []byte) *btcec.PrivateKey {
 
 	// If the corresponding public key has an odd y coordinate, then we'll
 	// negate the private key as specified in BIP 341.
-	privKeyScalar := &privKey.Key
+	privKeyScalar := privKey.Key
 	pubKeyBytes := privKey.PubKey().SerializeCompressed()
 	if pubKeyBytes[0] == secp.PubKeyFormatCompressedOdd {
 		privKeyScalar.Negate()
diff --git a/txscript/taproot_test.go b/txscript/taproot_test.go
index 178405b5..01b3780e 100644
--- a/txscript/taproot_test.go
+++ b/txscript/taproot_test.go
@@ -166,8 +166,8 @@ func TestControlBlockParsing(t *testing.T) {
 // key, then generating a public key from that. This test a quickcheck test to
 // assert the following invariant:
 //
-// * taproot_tweak_pubkey(pubkey_gen(seckey), h)[1] ==
-//   pubkey_gen(taproot_tweak_seckey(seckey, h))
+//   - taproot_tweak_pubkey(pubkey_gen(seckey), h)[1] ==
+//     pubkey_gen(taproot_tweak_seckey(seckey, h))
 func TestTaprootScriptSpendTweak(t *testing.T) {
 	t.Parallel()
 
@@ -186,7 +186,7 @@ func TestTaprootScriptSpendTweak(t *testing.T) {
 		tweakedPub := ComputeTaprootOutputKey(privKey.PubKey(), x[:])
 
 		// Now we'll generate the corresponding tweaked private key.
-		tweakedPriv := TweakTaprootPrivKey(privKey, x[:])
+		tweakedPriv := TweakTaprootPrivKey(*privKey, x[:])
 
 		// The public key for this private key should be the same as
 		// the tweaked public key we generate above.
@@ -204,6 +204,42 @@ func TestTaprootScriptSpendTweak(t *testing.T) {
 
 }
 
+// TestTaprootTweakNoMutation tests that the underlying private key passed into
+// TweakTaprootPrivKey is never mutated.
+func TestTaprootTweakNoMutation(t *testing.T) {
+	t.Parallel()
+
+	// Assert that given a random tweak, and a random private key, that if
+	// we tweak the private key it remains unaffected.
+	f := func(privBytes, tweak [32]byte) bool {
+		privKey, _ := btcec.PrivKeyFromBytes(privBytes[:])
+
+		// Now we'll generate the corresponding tweaked private key.
+		tweakedPriv := TweakTaprootPrivKey(*privKey, tweak[:])
+
+		// The tweaked private key and the original private key should
+		// NOT be the same.
+		if *privKey == *tweakedPriv {
+			t.Logf("private key was mutated")
+			return false
+		}
+
+		// We shuold be able to re-derive the private key from raw
+		// bytes and have that match up again.
+		privKeyCopy, _ := btcec.PrivKeyFromBytes(privBytes[:])
+		if *privKey != *privKeyCopy {
+			t.Logf("private doesn't match")
+			return false
+		}
+
+		return true
+	}
+
+	if err := quick.Check(f, nil); err != nil {
+		t.Fatalf("private key modified: %v", err)
+	}
+}
+
 // TestTaprootConstructKeyPath tests the key spend only taproot construction.
 func TestTaprootConstructKeyPath(t *testing.T) {
 	checkPath := func(branch uint32, expectedAddresses []string) {