From fe4b8fe2960fabbaf9b2afcd8caf0d24ba2febe6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Thu, 13 Aug 2026 19:06:16 +0000 Subject: [PATCH 01/10] feat(bitcoin): P2TR script derivation, sighash and fee estimation Splits the Bitcoin layer out of the FROST/Schnorr migration branch (#3866) so the code legacy wallets execute today can be reviewed on its own, separately from the FROST engine work that stays inert until build tags and env gates are set. This layer is not behind any build tag, so every currently-live wallet runs it: - Taproot key derivation and BIP-341 tweak/output-key math (taproot.go). - P2TR output script derivation and script-type detection (script.go). - BIP-341 SIGHASH_DEFAULT key-spend sighash construction (transaction_builder.go). - Fee estimation for P2TR inputs and outputs (estimator.go). - Electrum: the per-wallet script set grows from 2 to 3 with the added P2TR script, so per-wallet history and UTXO lookups issue one more sequential RPC. Nothing here depends on pkg/frost, so it compiles and tests standalone against frost-upgrade, and the downstream pkg/tbtcpg and pkg/tbtc consumers build unchanged against it. --- pkg/bitcoin/electrum/electrum.go | 201 ++-- .../electrum/electrum_integration_test.go | 25 +- pkg/bitcoin/estimator.go | 60 + pkg/bitcoin/estimator_test.go | 31 + pkg/bitcoin/script.go | 38 + pkg/bitcoin/script_test.go | 191 +++ pkg/bitcoin/taproot.go | 138 +++ pkg/bitcoin/transaction_builder.go | 693 ++++++++++- pkg/bitcoin/transaction_builder_test.go | 1044 ++++++++++++++++- 9 files changed, 2328 insertions(+), 93 deletions(-) create mode 100644 pkg/bitcoin/taproot.go diff --git a/pkg/bitcoin/electrum/electrum.go b/pkg/bitcoin/electrum/electrum.go index e670646e4a..2c896be287 100644 --- a/pkg/bitcoin/electrum/electrum.go +++ b/pkg/bitcoin/electrum/electrum.go @@ -434,6 +434,56 @@ func (c *Connection) GetTransactionsForPublicKeyHash( return transactions, nil } +// GetTransactionsForPublicKeyScripts gets confirmed transactions that pay to +// any of the given public key scripts. The returned transactions are ordered +// by block height in ascending order, i.e. the latest transaction is at the +// end of the list. +func (c *Connection) GetTransactionsForPublicKeyScripts( + publicKeyScripts []bitcoin.Script, + limit int, +) ([]*bitcoin.Transaction, error) { + txHashes, err := c.GetTxHashesForPublicKeyScripts(publicKeyScripts) + if err != nil { + return nil, err + } + + selectedTxHashes := selectLatestUniqueTxHashes(txHashes, limit) + + transactions := make([]*bitcoin.Transaction, len(selectedTxHashes)) + for i, txHash := range selectedTxHashes { + transaction, err := c.GetTransaction(txHash) + if err != nil { + return nil, fmt.Errorf("cannot get transaction: [%v]", err) + } + + transactions[i] = transaction + } + + return transactions, nil +} + +func selectLatestUniqueTxHashes( + txHashes []bitcoin.Hash, + limit int, +) []bitcoin.Hash { + uniqueTxHashes := make([]bitcoin.Hash, 0, len(txHashes)) + seen := make(map[bitcoin.Hash]bool) + for _, txHash := range txHashes { + if seen[txHash] { + continue + } + + seen[txHash] = true + uniqueTxHashes = append(uniqueTxHashes, txHash) + } + + if len(uniqueTxHashes) > limit { + return uniqueTxHashes[len(uniqueTxHashes)-limit:] + } + + return uniqueTxHashes +} + // GetTxHashesForPublicKeyHash gets hashes of confirmed transactions that pays // the given public key hash using either a P2PKH or P2WPKH script. The returned // transactions hashes are ordered by block height in the ascending order, i.e. @@ -461,26 +511,19 @@ func (c *Connection) GetTxHashesForPublicKeyHash( ) } - p2pkhItems, err := c.getConfirmedScriptHistory(p2pkh) - if err != nil { - return nil, fmt.Errorf( - "cannot get P2PKH history for public key hash [0x%x]: [%v]", - publicKeyHash, - err, - ) - } + return c.GetTxHashesForPublicKeyScripts([]bitcoin.Script{p2pkh, p2wpkh}) +} - p2wpkhItems, err := c.getConfirmedScriptHistory(p2wpkh) +// GetTxHashesForPublicKeyScripts gets hashes of confirmed transactions that +// pay to any of the given public key scripts. +func (c *Connection) GetTxHashesForPublicKeyScripts( + publicKeyScripts []bitcoin.Script, +) ([]bitcoin.Hash, error) { + items, err := c.getConfirmedScriptHistories(publicKeyScripts) if err != nil { - return nil, fmt.Errorf( - "cannot get P2WPKH history for public key hash [0x%x]: [%v]", - publicKeyHash, - err, - ) + return nil, err } - items := append(p2pkhItems, p2wpkhItems...) - sort.SliceStable( items, func(i, j int) bool { @@ -496,6 +539,27 @@ func (c *Connection) GetTxHashesForPublicKeyHash( return txHashes, nil } +func (c *Connection) getConfirmedScriptHistories( + publicKeyScripts []bitcoin.Script, +) ([]*scriptHistoryItem, error) { + items := make([]*scriptHistoryItem, 0) + + for _, publicKeyScript := range publicKeyScripts { + scriptItems, err := c.getConfirmedScriptHistory(publicKeyScript) + if err != nil { + return nil, fmt.Errorf( + "cannot get history for script [0x%x]: [%v]", + publicKeyScript, + err, + ) + } + + items = append(items, scriptItems...) + } + + return items, nil +} + type scriptHistoryItem struct { txHash bitcoin.Hash blockHeight int32 @@ -752,45 +816,15 @@ func (c *Connection) GetUtxosForPublicKeyHash( ) } - p2pkhItems, err := c.getScriptUtxos(p2pkh, true) - if err != nil { - return nil, fmt.Errorf( - "cannot get P2PKH UTXOs for public key hash [0x%x]: [%v]", - publicKeyHash, - err, - ) - } - - p2wpkhItems, err := c.getScriptUtxos(p2wpkh, true) - if err != nil { - return nil, fmt.Errorf( - "cannot get P2WPKH UTXOs for public key hash [0x%x]: [%v]", - publicKeyHash, - err, - ) - } - - items := append(p2pkhItems, p2wpkhItems...) - - sort.SliceStable( - items, - func(i, j int) bool { - return items[i].blockHeight < items[j].blockHeight - }, - ) - - utxos := make([]*bitcoin.UnspentTransactionOutput, len(items)) - for i, item := range items { - utxos[i] = &bitcoin.UnspentTransactionOutput{ - Outpoint: &bitcoin.TransactionOutpoint{ - TransactionHash: item.txHash, - OutputIndex: item.outputIndex, - }, - Value: int64(item.value), - } - } + return c.GetUtxosForPublicKeyScripts([]bitcoin.Script{p2pkh, p2wpkh}) +} - return utxos, nil +// GetUtxosForPublicKeyScripts gets unspent outputs of confirmed transactions +// that are controlled by any of the given public key scripts. +func (c *Connection) GetUtxosForPublicKeyScripts( + publicKeyScripts []bitcoin.Script, +) ([]*bitcoin.UnspentTransactionOutput, error) { + return c.getUtxosForPublicKeyScripts(publicKeyScripts, true) } // GetMempoolUtxosForPublicKeyHash gets unspent outputs of unconfirmed transactions @@ -820,26 +854,35 @@ func (c *Connection) GetMempoolUtxosForPublicKeyHash( ) } - p2pkhItems, err := c.getScriptUtxos(p2pkh, false) + return c.GetMempoolUtxosForPublicKeyScripts([]bitcoin.Script{p2pkh, p2wpkh}) +} + +// GetMempoolUtxosForPublicKeyScripts gets unspent outputs of unconfirmed +// transactions that are controlled by any of the given public key scripts. +func (c *Connection) GetMempoolUtxosForPublicKeyScripts( + publicKeyScripts []bitcoin.Script, +) ([]*bitcoin.UnspentTransactionOutput, error) { + return c.getUtxosForPublicKeyScripts(publicKeyScripts, false) +} + +func (c *Connection) getUtxosForPublicKeyScripts( + publicKeyScripts []bitcoin.Script, + confirmed bool, +) ([]*bitcoin.UnspentTransactionOutput, error) { + items, err := c.getScriptUtxosForScripts(publicKeyScripts, confirmed) if err != nil { - return nil, fmt.Errorf( - "cannot get P2PKH UTXOs for public key hash [0x%x]: [%v]", - publicKeyHash, - err, - ) + return nil, err } - p2wpkhItems, err := c.getScriptUtxos(p2wpkh, false) - if err != nil { - return nil, fmt.Errorf( - "cannot get P2WPKH UTXOs for public key hash [0x%x]: [%v]", - publicKeyHash, - err, + if confirmed { + sort.SliceStable( + items, + func(i, j int) bool { + return items[i].blockHeight < items[j].blockHeight + }, ) } - items := append(p2pkhItems, p2wpkhItems...) - utxos := make([]*bitcoin.UnspentTransactionOutput, len(items)) for i, item := range items { utxos[i] = &bitcoin.UnspentTransactionOutput{ @@ -854,6 +897,28 @@ func (c *Connection) GetMempoolUtxosForPublicKeyHash( return utxos, nil } +func (c *Connection) getScriptUtxosForScripts( + publicKeyScripts []bitcoin.Script, + confirmed bool, +) ([]*scriptUtxoItem, error) { + items := make([]*scriptUtxoItem, 0) + + for _, publicKeyScript := range publicKeyScripts { + scriptItems, err := c.getScriptUtxos(publicKeyScript, confirmed) + if err != nil { + return nil, fmt.Errorf( + "cannot get UTXOs for script [0x%x]: [%v]", + publicKeyScript, + err, + ) + } + + items = append(items, scriptItems...) + } + + return items, nil +} + type scriptUtxoItem struct { txHash bitcoin.Hash outputIndex uint32 diff --git a/pkg/bitcoin/electrum/electrum_integration_test.go b/pkg/bitcoin/electrum/electrum_integration_test.go index 80c121b20a..ff0493a3fe 100644 --- a/pkg/bitcoin/electrum/electrum_integration_test.go +++ b/pkg/bitcoin/electrum/electrum_integration_test.go @@ -160,7 +160,7 @@ func init() { func TestConnect_Integration(t *testing.T) { runParallel(t, func(t *testing.T, testConfig testConfig) { - _, cancelCtx := newTestConnection(t, testConfig.clientConfig) + _, cancelCtx := newRequiredTestConnection(t, testConfig.clientConfig) defer cancelCtx() }) } @@ -640,9 +640,32 @@ func runParallel(t *testing.T, runFunc func(t *testing.T, testConfig testConfig) } func newTestConnection(t *testing.T, config electrum.Config) (bitcoin.Chain, context.CancelFunc) { + t.Helper() + + return connectTestConnection(t, config, true) +} + +func newRequiredTestConnection(t *testing.T, config electrum.Config) (bitcoin.Chain, context.CancelFunc) { + t.Helper() + + return connectTestConnection(t, config, false) +} + +func connectTestConnection( + t *testing.T, + config electrum.Config, + skipTransientConnectionError bool, +) (bitcoin.Chain, context.CancelFunc) { + t.Helper() + ctx, cancelCtx := context.WithCancel(context.Background()) electrum, err := electrum.Connect(ctx, config) if err != nil { + cancelCtx() + if skipTransientConnectionError && shouldSkipElectrumIntegrationError(err) { + t.Skipf("skipping due to transient electrum connection error: %v", err) + } + t.Fatal(err) } diff --git a/pkg/bitcoin/estimator.go b/pkg/bitcoin/estimator.go index e8c61f7577..10842975cf 100644 --- a/pkg/bitcoin/estimator.go +++ b/pkg/bitcoin/estimator.go @@ -17,6 +17,10 @@ var signaturePlaceholder = make([]byte, 72) // Compressed public keys have always 33 bytes. var publicKeyPlaceholder = make([]byte, 33) +// Taproot key-path signatures use the 64-byte BIP-340 Schnorr encoding when +// SIGHASH_DEFAULT is used. +var taprootKeyPathSignaturePlaceholder = make([]byte, 64) + // TransactionSizeEstimator is a component allowing to estimate the size // of a Bitcoin transaction of the provided shape, without constructing it. type TransactionSizeEstimator struct { @@ -75,6 +79,40 @@ func (tse *TransactionSizeEstimator) AddPublicKeyHashInputs( return tse } +// AddPublicKeyScriptInput adds an input spending an output locked by the +// provided direct-key public key script. P2PKH, P2WPKH, and P2TR key-path +// spends are supported. If the estimator already errored out during previous +// actions, this method does nothing. +func (tse *TransactionSizeEstimator) AddPublicKeyScriptInput( + publicKeyScript Script, +) *TransactionSizeEstimator { + if tse.err != nil { + return tse + } + + switch GetScriptType(publicKeyScript) { + case P2PKHScript: + return tse.AddPublicKeyHashInputs(1, false) + case P2WPKHScript: + return tse.AddPublicKeyHashInputs(1, true) + case P2TRScript: + tse.internal.AddTxIn( + wire.NewTxIn( + wire.NewOutPoint((*chainhash.Hash)(&[32]byte{}), 0), + nil, + wire.TxWitness{taprootKeyPathSignaturePlaceholder}, + ), + ) + default: + tse.err = fmt.Errorf( + "unsupported direct-key input script type: [%v]", + GetScriptType(publicKeyScript), + ) + } + + return tse +} + // AddScriptHashInputs adds the provided count of P2WSH (isWitness is true) // or P2SH (isWitness is false) inputs to the estimation. The redeemScriptLength // argument should be used to pass the byte size of the plain-text redeem @@ -201,6 +239,28 @@ func (tse *TransactionSizeEstimator) AddScriptHashOutputs( return tse } +// AddOutputScript adds an output locked by the provided public key script. If +// the estimator already errored out during previous actions, this method does +// nothing. +func (tse *TransactionSizeEstimator) AddOutputScript( + publicKeyScript Script, +) *TransactionSizeEstimator { + if tse.err != nil { + return tse + } + + if len(publicKeyScript) == 0 { + tse.err = fmt.Errorf("output public key script is empty") + return tse + } + + tse.internal.AddTxOut( + wire.NewTxOut(0, append([]byte(nil), publicKeyScript...)), + ) + + return tse +} + // VirtualSize returns the virtual size of the transaction whose shape was // provided to the estimator. If any errors occurred while building the // transaction shape, the first error will be returned. diff --git a/pkg/bitcoin/estimator_test.go b/pkg/bitcoin/estimator_test.go index f202ea97dd..92ca6cdf0d 100644 --- a/pkg/bitcoin/estimator_test.go +++ b/pkg/bitcoin/estimator_test.go @@ -8,6 +8,15 @@ import ( ) func TestTransactionSizeEstimator_VirtualSize(t *testing.T) { + taprootScript, err := PayToTaproot([32]byte{0x01}) + if err != nil { + t.Fatal(err) + } + witnessPublicKeyHashScript, err := PayToWitnessPublicKeyHash([20]byte{0x02}) + if err != nil { + t.Fatal(err) + } + var tests = map[string]struct { estimator *TransactionSizeEstimator expectedVirtualSize int @@ -63,6 +72,28 @@ func TestTransactionSizeEstimator_VirtualSize(t *testing.T) { AddScriptHashOutputs(1, true), expectedVirtualSize: 250, }, + "1 P2TR key-path input and 1 P2TR output": { + estimator: NewTransactionSizeEstimator(). + AddPublicKeyScriptInput(taprootScript). + AddOutputScript(taprootScript), + expectedVirtualSize: 111, + }, + "2 P2TR key-path inputs and 1 P2TR output": { + estimator: NewTransactionSizeEstimator(). + AddPublicKeyScriptInput(taprootScript). + AddPublicKeyScriptInput(taprootScript). + AddOutputScript(taprootScript), + expectedVirtualSize: 169, + }, + "1 P2TR input and mixed P2TR and P2WPKH outputs": { + estimator: NewTransactionSizeEstimator(). + AddPublicKeyScriptInput(taprootScript). + AddOutputScript(taprootScript). + AddOutputScript(taprootScript). + AddOutputScript(witnessPublicKeyHashScript). + AddOutputScript(witnessPublicKeyHashScript), + expectedVirtualSize: 216, + }, } for testName, test := range tests { diff --git a/pkg/bitcoin/script.go b/pkg/bitcoin/script.go index ecdb6be214..af2c918fe6 100644 --- a/pkg/bitcoin/script.go +++ b/pkg/bitcoin/script.go @@ -19,6 +19,7 @@ const ( P2WPKHScript P2SHScript P2WSHScript + P2TRScript ) func (st ScriptType) String() string { @@ -31,6 +32,8 @@ func (st ScriptType) String() string { return "P2SH" case P2WSHScript: return "P2WSH" + case P2TRScript: + return "P2TR" default: return "NonStandard" } @@ -147,8 +150,25 @@ func PayToScriptHash(scriptHash [20]byte) (Script, error) { Script() } +// PayToTaproot constructs a P2TR script for the provided 32-byte x-only +// Taproot output key. The function assumes the provided output key is valid. +// +// The argument must be the final Taproot output key committed to by the +// scriptPubKey. This helper does not derive a BIP-341/BIP-86 tweak from an +// internal key. +func PayToTaproot(outputKey [32]byte) (Script, error) { + return txscript.NewScriptBuilder(). + AddOp(txscript.OP_1). + AddData(outputKey[:]). + Script() +} + // GetScriptType gets the ScriptType of the given Script. func GetScriptType(script Script) ScriptType { + if isPayToTaproot(script) { + return P2TRScript + } + switch txscript.GetScriptClass(script) { case txscript.PubKeyHashTy: return P2PKHScript @@ -163,6 +183,12 @@ func GetScriptType(script Script) ScriptType { } } +func isPayToTaproot(script Script) bool { + return len(script) == 34 && + script[0] == txscript.OP_1 && + script[1] == txscript.OP_DATA_32 +} + // ExtractPublicKeyHash extracts the public key hash from a P2WPKH or P2PKH // script. func ExtractPublicKeyHash(script Script) ([20]byte, error) { @@ -189,3 +215,15 @@ func ExtractPublicKeyHash(script Script) ([20]byte, error) { return publicKeyHash, nil } + +// ExtractTaprootKey extracts the x-only output key from a P2TR script. +func ExtractTaprootKey(script Script) ([32]byte, error) { + if GetScriptType(script) != P2TRScript { + return [32]byte{}, fmt.Errorf("not a P2TR script") + } + + var outputKey [32]byte + copy(outputKey[:], script[2:]) + + return outputKey, nil +} diff --git a/pkg/bitcoin/script_test.go b/pkg/bitcoin/script_test.go index 269cae4aac..0990cb193e 100644 --- a/pkg/bitcoin/script_test.go +++ b/pkg/bitcoin/script_test.go @@ -6,6 +6,9 @@ import ( "reflect" "testing" + btcec2 "github.com/btcsuite/btcd/btcec/v2" + "github.com/btcsuite/btcd/btcec/v2/schnorr" + "github.com/keep-network/keep-core/internal/testutils" "github.com/keep-network/keep-core/pkg/crypto/secp256k1" ) @@ -329,6 +332,137 @@ func TestPayToScriptHash(t *testing.T) { testutils.AssertBytesEqual(t, expectedResult, result[:]) } +func TestPayToTaproot(t *testing.T) { + outputKeyBytes, err := hex.DecodeString( + "1b84c5567b126440995d3ed5aaba0565d71e1834604819ff9c17f5e9d5dd078f", + ) + if err != nil { + t.Fatal(err) + } + + var outputKey [32]byte + copy(outputKey[:], outputKeyBytes) + + result, err := PayToTaproot(outputKey) + if err != nil { + t.Fatal(err) + } + + expectedResult, err := hex.DecodeString( + "51201b84c5567b126440995d3ed5aaba0565d71e1834604819ff9c17f5e9d5dd078f", + ) + if err != nil { + t.Fatal(err) + } + + testutils.AssertBytesEqual(t, expectedResult, result[:]) +} + +func TestTaprootLeafHash(t *testing.T) { + script := Script(hexToSlice( + t, + "76a9140102030405060708090a0b0c0d0e0f101112131488ac", + )) + + result, err := TaprootLeafHash(script) + if err != nil { + t.Fatal(err) + } + + expectedResult := hexToSlice( + t, + "37a57b86de2819d2b72a173df46238a7ad295ea1485d3b40e9415daa82b4fdcb", + ) + + testutils.AssertBytesEqual(t, expectedResult, result[:]) +} + +func TestTaprootTweakAndOutputKey(t *testing.T) { + privateKey, _ := btcec2.PrivKeyFromBytes( + hexToSlice( + t, + "0101010101010101010101010101010101010101010101010101010101010101", + ), + ) + + var internalKey [32]byte + copy(internalKey[:], schnorr.SerializePubKey(privateKey.PubKey())) + + refundLeaf := Script(hexToSlice( + t, + "76a9140102030405060708090a0b0c0d0e0f101112131488ac", + )) + + merkleRoot, err := TaprootLeafHash(refundLeaf) + if err != nil { + t.Fatal(err) + } + + tweak, err := TaprootTweak(internalKey, &merkleRoot) + if err != nil { + t.Fatal(err) + } + + expectedTweak := hexToSlice( + t, + "6ca66b4600554f36d490d227669ba78c2d4778a8ecc07565ae2f9e87c28f124a", + ) + + testutils.AssertBytesEqual(t, expectedTweak, tweak[:]) + + outputKey, err := TaprootOutputKey(internalKey, &merkleRoot) + if err != nil { + t.Fatal(err) + } + + expectedOutputKey := hexToSlice( + t, + "b31d6b4f10bcea1dfcace63ce7defda9e718a4340b4b5befef6194488780ef17", + ) + + testutils.AssertBytesEqual(t, expectedOutputKey, outputKey[:]) +} + +func TestPayToTaprootWithScriptTree(t *testing.T) { + privateKey, _ := btcec2.PrivKeyFromBytes( + hexToSlice( + t, + "0202020202020202020202020202020202020202020202020202020202020202", + ), + ) + + var internalKey [32]byte + copy(internalKey[:], schnorr.SerializePubKey(privateKey.PubKey())) + + merkleRootBytes := hexToSlice( + t, + "b2c459126150e0d47063ea7b6d0474a24c39e25908aae5740dd4787b67c6e19a", + ) + var merkleRoot [32]byte + copy(merkleRoot[:], merkleRootBytes) + + result, err := PayToTaprootWithScriptTree(internalKey, merkleRoot) + if err != nil { + t.Fatal(err) + } + + expectedOutputKey := hexToSlice( + t, + "e339710a2348c113ade4a4e7d52bd1c12bc69818f1af7f41e161142701b93c96", + ) + + // Rebuild the expected P2TR script directly to avoid reusing + // PayToTaprootWithScriptTree. + var expectedKey [32]byte + copy(expectedKey[:], expectedOutputKey) + expectedResult, err := PayToTaproot(expectedKey) + if err != nil { + t.Fatal(err) + } + + testutils.AssertBytesEqual(t, expectedResult, result) +} + func TestGetScriptType(t *testing.T) { fromHex := func(hexString string) []byte { bytes, err := hex.DecodeString(hexString) @@ -358,6 +492,10 @@ func TestGetScriptType(t *testing.T) { script: fromHex("002086a303cdd2e2eab1d1679f1a813835dc5a1b65321077cdccaf08f98cbf04ca96"), expectedType: P2WSHScript, }, + "p2tr script": { + script: fromHex("51201b84c5567b126440995d3ed5aaba0565d71e1834604819ff9c17f5e9d5dd078f"), + expectedType: P2TRScript, + }, "non-standard script": { script: fromHex( "14934b98637ca318a4d6e7ca6ffd1690b8e77df6377508f9f0c90d0003" + @@ -382,6 +520,59 @@ func TestGetScriptType(t *testing.T) { } } +func TestExtractTaprootKey(t *testing.T) { + fromHex := func(hexString string) []byte { + bytes, err := hex.DecodeString(hexString) + if err != nil { + t.Fatal(err) + } + return bytes + } + + var outputKey [32]byte + copy( + outputKey[:], + fromHex("1b84c5567b126440995d3ed5aaba0565d71e1834604819ff9c17f5e9d5dd078f"), + ) + + var tests = map[string]struct { + script Script + expectedOutputKey [32]byte + expectedErr error + }{ + "P2TR script": { + script: fromHex("51201b84c5567b126440995d3ed5aaba0565d71e1834604819ff9c17f5e9d5dd078f"), + expectedOutputKey: outputKey, + }, + "other script": { + script: fromHex("00148db50eb52063ea9d98b3eac91489a90f738986f6"), + expectedErr: fmt.Errorf("not a P2TR script"), + }, + } + + for testName, test := range tests { + t.Run(testName, func(t *testing.T) { + actualOutputKey, err := ExtractTaprootKey(test.script) + + if !reflect.DeepEqual(test.expectedErr, err) { + t.Errorf( + "unexpected error\nexpected: %+v\nactual: %+v\n", + test.expectedErr, + err, + ) + } + + if test.expectedOutputKey != actualOutputKey { + t.Errorf( + "unexpected taproot output key\nexpected: 0x%x\nactual: 0x%x\n", + test.expectedOutputKey, + actualOutputKey, + ) + } + }) + } +} + func TestExtractPublicKeyHash(t *testing.T) { fromHex := func(hexString string) []byte { bytes, err := hex.DecodeString(hexString) diff --git a/pkg/bitcoin/taproot.go b/pkg/bitcoin/taproot.go new file mode 100644 index 0000000000..b3d44867f5 --- /dev/null +++ b/pkg/bitcoin/taproot.go @@ -0,0 +1,138 @@ +package bitcoin + +import ( + "bytes" + "fmt" + + btcec2 "github.com/btcsuite/btcd/btcec/v2" + "github.com/btcsuite/btcd/btcec/v2/schnorr" + "github.com/btcsuite/btcd/chaincfg/chainhash" +) + +const taprootBaseLeafVersion = 0xc0 + +// TaprootLeafHash computes the BIP-342 TapLeaf hash for a base-version script. +func TaprootLeafHash(script Script) ([32]byte, error) { + var buffer bytes.Buffer + + if err := buffer.WriteByte(taprootBaseLeafVersion); err != nil { + return [32]byte{}, err + } + + scriptLength, err := writeCompactSizeUint(CompactSizeUint(len(script))) + if err != nil { + return [32]byte{}, fmt.Errorf( + "cannot encode taproot script length: [%v]", + err, + ) + } + + if _, err := buffer.Write(scriptLength); err != nil { + return [32]byte{}, err + } + if _, err := buffer.Write(script); err != nil { + return [32]byte{}, err + } + + return taggedHashToArray( + chainhash.TaggedHash(chainhash.TagTapLeaf, buffer.Bytes()), + ), nil +} + +// TaprootTweak computes the BIP-341 TapTweak hash for an x-only internal key +// and optional script merkle root. +func TaprootTweak( + internalKey [32]byte, + merkleRoot *[32]byte, +) ([32]byte, error) { + _, _, tweak, err := taprootTweakScalar(internalKey, merkleRoot) + return tweak, err +} + +// TaprootOutputKey derives the BIP-341 tweaked x-only Taproot output key from +// an x-only internal key and optional script merkle root. +func TaprootOutputKey( + internalKey [32]byte, + merkleRoot *[32]byte, +) ([32]byte, error) { + internalPublicKey, tweakScalar, _, err := taprootTweakScalar( + internalKey, + merkleRoot, + ) + if err != nil { + return [32]byte{}, err + } + + var internalPoint btcec2.JacobianPoint + internalPublicKey.AsJacobian(&internalPoint) + + var tweakPoint btcec2.JacobianPoint + btcec2.ScalarBaseMultNonConst(&tweakScalar, &tweakPoint) + + var outputPoint btcec2.JacobianPoint + btcec2.AddNonConst(&internalPoint, &tweakPoint, &outputPoint) + + if outputPoint.Z.IsZero() { + return [32]byte{}, fmt.Errorf("taproot output key is infinity") + } + + outputPoint.ToAffine() + outputPublicKey := btcec2.NewPublicKey(&outputPoint.X, &outputPoint.Y) + + var outputKey [32]byte + copy(outputKey[:], schnorr.SerializePubKey(outputPublicKey)) + + return outputKey, nil +} + +// PayToTaprootWithScriptTree constructs a P2TR script from an internal key and +// a script merkle root by applying the BIP-341 TapTweak. +func PayToTaprootWithScriptTree( + internalKey [32]byte, + merkleRoot [32]byte, +) (Script, error) { + outputKey, err := TaprootOutputKey(internalKey, &merkleRoot) + if err != nil { + return nil, fmt.Errorf("cannot derive taproot output key: [%v]", err) + } + + return PayToTaproot(outputKey) +} + +func taprootTweakScalar( + internalKey [32]byte, + merkleRoot *[32]byte, +) (*btcec2.PublicKey, btcec2.ModNScalar, [32]byte, error) { + internalPublicKey, err := schnorr.ParsePubKey(internalKey[:]) + if err != nil { + return nil, btcec2.ModNScalar{}, [32]byte{}, fmt.Errorf( + "cannot parse taproot internal key: [%v]", + err, + ) + } + + tweakMessages := [][]byte{internalKey[:]} + if merkleRoot != nil { + tweakMessages = append(tweakMessages, merkleRoot[:]) + } + + tweak := taggedHashToArray( + chainhash.TaggedHash(chainhash.TagTapTweak, tweakMessages...), + ) + + var tweakScalar btcec2.ModNScalar + if overflow := tweakScalar.SetBytes(&tweak); overflow != 0 { + return nil, btcec2.ModNScalar{}, [32]byte{}, fmt.Errorf( + "taproot tweak is greater than or equal to curve order", + ) + } + + return internalPublicKey, tweakScalar, tweak, nil +} + +func taggedHashToArray(hash *chainhash.Hash) [32]byte { + var result [32]byte + copy(result[:], hash[:]) + + return result +} diff --git a/pkg/bitcoin/transaction_builder.go b/pkg/bitcoin/transaction_builder.go index 83bce0a6b0..551d9f86b0 100644 --- a/pkg/bitcoin/transaction_builder.go +++ b/pkg/bitcoin/transaction_builder.go @@ -1,11 +1,16 @@ package bitcoin import ( + "bytes" "crypto/ecdsa" + "crypto/sha256" + "encoding/binary" + "encoding/hex" "fmt" "math/big" "github.com/btcsuite/btcd/btcec" + "github.com/btcsuite/btcd/btcec/v2/schnorr" "github.com/btcsuite/btcd/chaincfg/chainhash" "github.com/btcsuite/btcd/txscript" "github.com/btcsuite/btcd/wire" @@ -45,8 +50,41 @@ func NewTransactionBuilder(chain Chain) *TransactionBuilder { } } +// HasTaprootKeyPathInputs returns true if the builder has at least one P2TR +// input intended to be spent using the Taproot key path. +func (tb *TransactionBuilder) HasTaprootKeyPathInputs() bool { + for _, sigHashArgs := range tb.sigHashArgs { + if sigHashArgs.scriptType == P2TRScript { + return true + } + } + + return false +} + +// HasOnlyTaprootKeyPathInputs returns true if every input in the builder is a +// P2TR input intended to be spent using the Taproot key path. +func (tb *TransactionBuilder) HasOnlyTaprootKeyPathInputs() bool { + if len(tb.sigHashArgs) == 0 { + return false + } + + for _, sigHashArgs := range tb.sigHashArgs { + if sigHashArgs.scriptType != P2TRScript { + return false + } + } + + return true +} + // AddPublicKeyHashInput adds an unsigned input pointing to a UTXO locked // using a P2PKH or P2WPKH script. +// +// For backward compatibility with wallet-action construction that discovers +// the input script type from the chain, this method also accepts P2TR direct +// key-path inputs. New Taproot-specific code should prefer +// AddTaprootKeyPathInput to make that spend policy explicit. func (tb *TransactionBuilder) AddPublicKeyHashInput( utxo *UnspentTransactionOutput, ) error { @@ -59,25 +97,133 @@ func (tb *TransactionBuilder) AddPublicKeyHashInput( ) } - class := txscript.GetScriptClass(utxoScript) - isPublicKeyHashScript := class == txscript.PubKeyHashTy || - class == txscript.WitnessV0PubKeyHashTy - if !isPublicKeyHashScript { + scriptType := GetScriptType(utxoScript) + isDirectKeySpendScript := scriptType == P2PKHScript || + scriptType == P2WPKHScript || + scriptType == P2TRScript + if !isDirectKeySpendScript { + return fmt.Errorf( + "UTXO pointed by the input is not P2PKH/P2WPKH/P2TR", + ) + } + + return tb.addDirectKeySpendInput(utxo, utxoScript, scriptType, nil) +} + +// AddTaprootKeyPathInput adds an unsigned input pointing to a UTXO locked +// using a P2TR script and intended to be spent using the Taproot key path. +// +// The script's x-only key is treated as the final Taproot output key. The +// builder does not apply a BIP-341/BIP-86 tap tweak during signing; callers +// must ensure the FROST signer can produce signatures for the exact output key +// committed to by the scriptPubKey. +func (tb *TransactionBuilder) AddTaprootKeyPathInput( + utxo *UnspentTransactionOutput, +) error { + utxoScript, err := tb.getScript(utxo) + if err != nil { + return fmt.Errorf( + "cannot get locking script for UTXO pointed "+ + "by the input: [%v]", + err, + ) + } + + scriptType := GetScriptType(utxoScript) + if scriptType != P2TRScript { return fmt.Errorf( - "UTXO pointed by the input is not P2PKH/P2WPKH", + "UTXO pointed by the input is not P2TR", ) } - // The UTXO was locked using a P2PKH/P2WPKH script so, the scriptCode - // required to build the sighash is equivalent to that script. Worth - // noting that the P2WPKH script is actually converted to the P2PKH script - // when used as a scriptCode, according to BIP-0143. For reference see, + return tb.addDirectKeySpendInput(utxo, utxoScript, scriptType, nil) +} + +// AddTaprootKeyPathInputWithMerkleRoot adds an unsigned input pointing to a +// UTXO locked using a BIP-341 tweaked P2TR output key and intended to be spent +// using the Taproot key path. +// +// The provided internal key and script merkle root must derive the output key +// committed to by the UTXO script. The merkle root is retained as signing +// metadata so the FROST signer can produce a key-path signature under the same +// Taproot tweak. +func (tb *TransactionBuilder) AddTaprootKeyPathInputWithMerkleRoot( + utxo *UnspentTransactionOutput, + internalKey [32]byte, + merkleRoot [32]byte, +) error { + utxoScript, err := tb.getScript(utxo) + if err != nil { + return fmt.Errorf( + "cannot get locking script for UTXO pointed "+ + "by the input: [%v]", + err, + ) + } + + scriptType := GetScriptType(utxoScript) + if scriptType != P2TRScript { + return fmt.Errorf( + "UTXO pointed by the input is not P2TR", + ) + } + + outputKey, err := ExtractTaprootKey(utxoScript) + if err != nil { + return fmt.Errorf("cannot extract taproot output key: [%v]", err) + } + + expectedOutputKey, err := TaprootOutputKey(internalKey, &merkleRoot) + if err != nil { + return fmt.Errorf("cannot derive taproot output key: [%v]", err) + } + + if !bytes.Equal(outputKey[:], expectedOutputKey[:]) { + return fmt.Errorf( + "taproot output key does not match internal key and merkle root", + ) + } + + return tb.addDirectKeySpendInput(utxo, utxoScript, scriptType, &merkleRoot) +} + +// TaprootKeyPathInputMerkleRoots returns per-input Taproot script merkle roots +// retained by the builder. The returned slice is aligned with transaction +// inputs. Non-Taproot inputs and untweaked Taproot inputs have nil entries. +func (tb *TransactionBuilder) TaprootKeyPathInputMerkleRoots() []*[32]byte { + merkleRoots := make([]*[32]byte, len(tb.sigHashArgs)) + + for i, sigHashArgs := range tb.sigHashArgs { + if sigHashArgs.taprootMerkleRoot == nil { + continue + } + + merkleRoots[i] = new([32]byte) + copy(merkleRoots[i][:], sigHashArgs.taprootMerkleRoot[:]) + } + + return merkleRoots +} + +func (tb *TransactionBuilder) addDirectKeySpendInput( + utxo *UnspentTransactionOutput, + utxoScript Script, + scriptType ScriptType, + taprootMerkleRoot *[32]byte, +) error { + // The UTXO was locked using a direct key-spend script, so the scriptCode + // required to build the sighash is equivalent to that script. Worth noting + // that the P2WPKH script is actually converted to the P2PKH script when + // used as a scriptCode, according to BIP-0143. For reference see, // https://github.com/bitcoin/bips/blob/master/bip-0143.mediawiki#specification. // That conversion is handled within the `txscript.CalcWitnessSigHash` call. sigHashArgs := &inputSigHashArgs{ - value: utxo.Value, - scriptCode: utxoScript, - witness: txscript.IsWitnessProgram(utxoScript), + value: utxo.Value, + publicKeyScript: utxoScript, + scriptCode: utxoScript, + scriptType: scriptType, + taprootMerkleRoot: taprootMerkleRoot, + witness: scriptType == P2WPKHScript || scriptType == P2TRScript, } hash := chainhash.Hash(utxo.Outpoint.TransactionHash) @@ -109,10 +255,10 @@ func (tb *TransactionBuilder) AddScriptHashInput( ) } - class := txscript.GetScriptClass(utxoScript) - isPublicKeyHashScript := class == txscript.ScriptHashTy || - class == txscript.WitnessV0ScriptHashTy - if !isPublicKeyHashScript { + scriptType := GetScriptType(utxoScript) + isScriptHashScript := scriptType == P2SHScript || + scriptType == P2WSHScript + if !isScriptHashScript { return fmt.Errorf( "UTXO pointed by the input is not P2SH/P2WSH", ) @@ -122,9 +268,11 @@ func (tb *TransactionBuilder) AddScriptHashInput( // to build the sighash is equivalent to the plain-text redeem script whose // hash is included in the P2SH/P2WSH script. sigHashArgs := &inputSigHashArgs{ - value: utxo.Value, - scriptCode: redeemScript, - witness: txscript.IsWitnessProgram(utxoScript), + value: utxo.Value, + publicKeyScript: utxoScript, + scriptCode: redeemScript, + scriptType: scriptType, + witness: scriptType == P2WSHScript, } hash := chainhash.Hash(utxo.Outpoint.TransactionHash) @@ -211,13 +359,34 @@ func (tb *TransactionBuilder) ComputeSignatureHashes() ([]*big.Int, error) { tb.prevOuts, ) + var taprootSigHashMidstate *taprootSignatureHashMidstate + if tb.HasTaprootKeyPathInputs() { + var err error + taprootSigHashMidstate, err = tb.taprootSignatureHashMidstate( + tb.internal.MsgTx, + ) + if err != nil { + return nil, fmt.Errorf( + "cannot calculate taproot sighash midstate: [%v]", + err, + ) + } + } + for i := range tb.internal.TxIn { sigHashArgs := tb.sigHashArgs[i] var sigHashBytes []byte var err error - if sigHashArgs.witness { + switch sigHashArgs.scriptType { + case P2TRScript: + sigHashBytes, err = tb.calcTaprootKeyPathSignatureHash( + tb.internal.MsgTx, + i, + taprootSigHashMidstate, + ) + case P2WPKHScript, P2WSHScript: sigHashBytes, err = txscript.CalcWitnessSigHash( sigHashArgs.scriptCode, witnessSigHashFragments, @@ -226,7 +395,7 @@ func (tb *TransactionBuilder) ComputeSignatureHashes() ([]*big.Int, error) { i, sigHashArgs.value, ) - } else { + default: sigHashBytes, err = txscript.CalcSignatureHash( sigHashArgs.scriptCode, txscript.SigHashAll, @@ -278,6 +447,14 @@ func (tb *TransactionBuilder) AddSignatures( for i, input := range tb.internal.TxIn { signature := signatures[i] + sigHashArgs := tb.sigHashArgs[i] + + if sigHashArgs.scriptType == P2TRScript { + return nil, fmt.Errorf( + "input [%v] is P2TR; use AddTaprootKeyPathSignatures", + i, + ) + } // Make a sanity check to avoid producing crap transactions. if !ecdsa.Verify( @@ -297,8 +474,6 @@ func (tb *TransactionBuilder) AddSignatures( signature.PublicKey, ).SerializeCompressed() - sigHashArgs := tb.sigHashArgs[i] - if sigHashArgs.witness { witness := wire.TxWitness{ signatureBytes, @@ -341,6 +516,81 @@ func (tb *TransactionBuilder) AddSignatures( return tb.internal.toTransaction(), nil } +// SchnorrSignatureContainer is a helper type holding a serialized 64-byte +// BIP-340 Schnorr signature. +type SchnorrSignatureContainer struct { + Signature [64]byte +} + +// AddTaprootKeyPathSignatures adds Schnorr signature data for P2TR key-path +// transaction inputs and returns a signed Transaction instance. +func (tb *TransactionBuilder) AddTaprootKeyPathSignatures( + signatures []*SchnorrSignatureContainer, +) (*Transaction, error) { + if len(tb.sigHashes) == 0 { + return nil, fmt.Errorf("signature hashes must be computed first") + } + + if len(signatures) != len(tb.internal.TxIn) { + return nil, fmt.Errorf("wrong signatures count") + } + + if !tb.HasOnlyTaprootKeyPathInputs() { + return nil, fmt.Errorf( + "taproot key-path signatures require all inputs to be P2TR", + ) + } + + for i, input := range tb.internal.TxIn { + signature := signatures[i] + if signature == nil { + return nil, fmt.Errorf("signature for input [%v] is nil", i) + } + + signatureBytes := make([]byte, len(signature.Signature)) + copy(signatureBytes, signature.Signature[:]) + + taprootKey, err := ExtractTaprootKey(tb.sigHashArgs[i].publicKeyScript) + if err != nil { + return nil, fmt.Errorf( + "cannot extract taproot key for input [%v]: [%v]", + i, + err, + ) + } + + taprootPublicKey, err := schnorr.ParsePubKey(taprootKey[:]) + if err != nil { + return nil, fmt.Errorf( + "cannot parse taproot key for input [%v]: [%v]", + i, + err, + ) + } + + taprootSignature, err := schnorr.ParseSignature(signatureBytes) + if err != nil { + return nil, fmt.Errorf( + "cannot parse taproot key-path signature for input [%v]: [%v]", + i, + err, + ) + } + + sigHashBytes := tb.sigHashes[i].FillBytes(make([]byte, sha256.Size)) + if !taprootSignature.Verify(sigHashBytes, taprootPublicKey) { + return nil, fmt.Errorf( + "invalid taproot key-path signature for input [%v]", + i, + ) + } + + input.Witness = wire.TxWitness{signatureBytes} + } + + return tb.internal.toTransaction(), nil +} + // TotalInputsValue returns the total value of transaction inputs. func (tb *TransactionBuilder) TotalInputsValue() int64 { totalInputsValue := int64(0) @@ -352,15 +602,412 @@ func (tb *TransactionBuilder) TotalInputsValue() int64 { return totalInputsValue } +// ReplaceUnsignedTransaction replaces the internal unsigned transaction while +// preserving per-input sighash metadata collected during builder input setup. +func (tb *TransactionBuilder) ReplaceUnsignedTransaction( + transaction *Transaction, +) error { + if transaction == nil { + return fmt.Errorf("transaction is nil") + } + + if len(transaction.Inputs) != len(tb.sigHashArgs) { + return fmt.Errorf( + "input metadata mismatch: [%d] tx inputs, [%d] sighash args", + len(transaction.Inputs), + len(tb.sigHashArgs), + ) + } + + previousInputs := tb.internal.TxIn + + replacedInternal := newInternalTransaction() + replacedInternal.fromTransaction(transaction) + + for i := range replacedInternal.TxIn { + previousInput := previousInputs[i] + replacedInput := replacedInternal.TxIn[i] + + if previousInput == nil || replacedInput == nil { + continue + } + + if len(replacedInput.SignatureScript) > 0 { + return fmt.Errorf( + "replacement transaction input [%d] has unexpected non-empty signature script", + i, + ) + } + + if len(replacedInput.Witness) > 0 { + return fmt.Errorf( + "replacement transaction input [%d] has unexpected non-empty witness", + i, + ) + } + + // The replacement's SignatureScript and Witness are both empty here + // because of the two refusals above, so the per-input restore below + // only has to decide what to copy *from* the previous input. + if tb.sigHashArgs[i].witness { + // Witness inputs may carry a single-element pre-signing witness + // that holds a P2WSH-style redeem script. Multi-element witnesses + // belong to P2TR script-path spends or other workflows that are + // not in scope for the current FROST migration, and silently + // dropping them produced malformed transactions later — refuse + // instead so the unsupported case fails loudly. Lifting this to + // support multi-element witnesses requires a per-input policy + // rather than a blanket copy because the replacement could + // legitimately differ in witness shape from the previous input. + switch len(previousInput.Witness) { + case 0: + // Nothing to restore (typical P2TR key-path or P2WPKH). + case 1: + redeemScript := append([]byte{}, previousInput.Witness[0]...) + replacedInput.Witness = wire.TxWitness{redeemScript} + default: + return fmt.Errorf( + "replacement transaction input [%d] previous witness has "+ + "[%d] elements; only zero- or single-element "+ + "pre-signing witnesses are currently supported for "+ + "restoration", + i, + len(previousInput.Witness), + ) + } + } else if len(previousInput.SignatureScript) > 0 { + replacedInput.SignatureScript = append( + []byte{}, + previousInput.SignatureScript..., + ) + } + } + + tb.internal = replacedInternal + tb.sigHashes = nil + + return nil +} + +// UnsignedTransaction returns the current unsigned transaction builder state. +func (tb *TransactionBuilder) UnsignedTransaction() *Transaction { + return tb.internal.toTransaction() +} + +// UnsignedTransactionInput carries canonical unsigned input metadata extracted +// from the builder state. +type UnsignedTransactionInput struct { + TxIDHex string + Vout uint32 + ValueSats uint64 + ScriptPubKeyHex string +} + +// UnsignedTransactionOutput carries canonical unsigned output metadata +// extracted from the builder state. +type UnsignedTransactionOutput struct { + ScriptPubKeyHex string + ValueSats uint64 +} + +// UnsignedTransactionIO returns canonical unsigned transaction input/output +// metadata from the builder state. +func (tb *TransactionBuilder) UnsignedTransactionIO() ( + []UnsignedTransactionInput, + []UnsignedTransactionOutput, + error, +) { + if len(tb.internal.TxIn) != len(tb.sigHashArgs) { + return nil, nil, fmt.Errorf( + "input metadata mismatch: [%d] tx inputs, [%d] sighash args", + len(tb.internal.TxIn), + len(tb.sigHashArgs), + ) + } + + inputs := make([]UnsignedTransactionInput, 0, len(tb.internal.TxIn)) + for i, input := range tb.internal.TxIn { + value := tb.sigHashArgs[i].value + if value < 0 { + return nil, nil, fmt.Errorf("input [%d] value is negative", i) + } + + inputs = append( + inputs, + UnsignedTransactionInput{ + // chainhash.Hash.String renders txid in standard Bitcoin display + // (RPC/explorer) byte order, i.e. reversed vs internal bytes. + TxIDHex: input.PreviousOutPoint.Hash.String(), + Vout: input.PreviousOutPoint.Index, + ValueSats: uint64(value), + ScriptPubKeyHex: hex.EncodeToString( + tb.sigHashArgs[i].publicKeyScript, + ), + }, + ) + } + + outputs := make([]UnsignedTransactionOutput, 0, len(tb.internal.TxOut)) + for i, output := range tb.internal.TxOut { + if output.Value < 0 { + return nil, nil, fmt.Errorf("output [%d] value is negative", i) + } + + outputs = append( + outputs, + UnsignedTransactionOutput{ + ScriptPubKeyHex: hex.EncodeToString(output.PkScript), + ValueSats: uint64(output.Value), + }, + ) + } + + return inputs, outputs, nil +} + +func (tb *TransactionBuilder) calcTaprootKeyPathSignatureHash( + tx *wire.MsgTx, + inputIndex int, + midstate *taprootSignatureHashMidstate, +) ([]byte, error) { + if tx == nil { + return nil, fmt.Errorf("transaction is nil") + } + if midstate == nil { + return nil, fmt.Errorf("taproot sighash midstate is nil") + } + + if inputIndex < 0 || inputIndex >= len(tx.TxIn) { + return nil, fmt.Errorf( + "input index [%d] out of range for [%d] inputs", + inputIndex, + len(tx.TxIn), + ) + } + + if len(tx.TxIn) != len(tb.sigHashArgs) { + return nil, fmt.Errorf( + "input metadata mismatch: [%d] tx inputs, [%d] sighash args", + len(tx.TxIn), + len(tb.sigHashArgs), + ) + } + + var sigMsg bytes.Buffer + + // BIP-341 defines the final digest as tagged_hash("TapSighash", + // 0x00 || SigMsg(0x00, 0)). The first byte is the epoch and the second + // byte is SIGHASH_DEFAULT. + sigMsg.WriteByte(0x00) + sigMsg.WriteByte(0x00) + + if err := binary.Write(&sigMsg, binary.LittleEndian, tx.Version); err != nil { + return nil, err + } + if err := binary.Write(&sigMsg, binary.LittleEndian, tx.LockTime); err != nil { + return nil, err + } + + sigMsg.Write(midstate.hashPrevOuts[:]) + sigMsg.Write(midstate.hashInputAmounts[:]) + sigMsg.Write(midstate.hashInputScripts[:]) + sigMsg.Write(midstate.hashSequences[:]) + sigMsg.Write(midstate.hashOutputs[:]) + + // Key-path spends use ext_flag=0 and this implementation does not attach + // a Taproot annex, so spend_type is 0. + sigMsg.WriteByte(0x00) + + if err := binary.Write( + &sigMsg, + binary.LittleEndian, + uint32(inputIndex), + ); err != nil { + return nil, err + } + + hash := chainhash.TaggedHash([]byte("TapSighash"), sigMsg.Bytes()) + return hash.CloneBytes(), nil +} + +type taprootSignatureHashMidstate struct { + hashPrevOuts [chainhash.HashSize]byte + hashInputAmounts [chainhash.HashSize]byte + hashInputScripts [chainhash.HashSize]byte + hashSequences [chainhash.HashSize]byte + hashOutputs [chainhash.HashSize]byte +} + +func (tb *TransactionBuilder) taprootSignatureHashMidstate( + tx *wire.MsgTx, +) (*taprootSignatureHashMidstate, error) { + if tx == nil { + return nil, fmt.Errorf("transaction is nil") + } + + if len(tx.TxIn) != len(tb.sigHashArgs) { + return nil, fmt.Errorf( + "input metadata mismatch: [%d] tx inputs, [%d] sighash args", + len(tx.TxIn), + len(tb.sigHashArgs), + ) + } + + hashPrevOuts, err := tb.taprootHashPrevOuts(tx) + if err != nil { + return nil, err + } + + hashInputAmounts, err := tb.taprootHashInputAmounts() + if err != nil { + return nil, err + } + + hashInputScripts, err := tb.taprootHashInputScripts() + if err != nil { + return nil, err + } + + hashSequences, err := tb.taprootHashSequences(tx) + if err != nil { + return nil, err + } + + hashOutputs, err := tb.taprootHashOutputs(tx) + if err != nil { + return nil, err + } + + return &taprootSignatureHashMidstate{ + hashPrevOuts: hashPrevOuts, + hashInputAmounts: hashInputAmounts, + hashInputScripts: hashInputScripts, + hashSequences: hashSequences, + hashOutputs: hashOutputs, + }, nil +} + +func (tb *TransactionBuilder) taprootHashPrevOuts( + tx *wire.MsgTx, +) ([chainhash.HashSize]byte, error) { + var buffer bytes.Buffer + for _, input := range tx.TxIn { + if err := writeOutPoint(&buffer, &input.PreviousOutPoint); err != nil { + return [chainhash.HashSize]byte{}, err + } + } + + return chainhash.HashH(buffer.Bytes()), nil +} + +func (tb *TransactionBuilder) taprootHashInputAmounts() ( + [chainhash.HashSize]byte, + error, +) { + var buffer bytes.Buffer + for i, sigHashArgs := range tb.sigHashArgs { + if sigHashArgs.value < 0 { + return [chainhash.HashSize]byte{}, fmt.Errorf( + "input [%d] value is negative", + i, + ) + } + + if err := binary.Write( + &buffer, + binary.LittleEndian, + uint64(sigHashArgs.value), + ); err != nil { + return [chainhash.HashSize]byte{}, err + } + } + + return chainhash.HashH(buffer.Bytes()), nil +} + +func (tb *TransactionBuilder) taprootHashInputScripts() ( + [chainhash.HashSize]byte, + error, +) { + var buffer bytes.Buffer + for i, sigHashArgs := range tb.sigHashArgs { + if err := wire.WriteVarBytes( + &buffer, + 0, + sigHashArgs.publicKeyScript, + ); err != nil { + return [chainhash.HashSize]byte{}, fmt.Errorf( + "cannot write public key script for input [%d]: [%v]", + i, + err, + ) + } + } + + return chainhash.HashH(buffer.Bytes()), nil +} + +func (tb *TransactionBuilder) taprootHashSequences( + tx *wire.MsgTx, +) ([chainhash.HashSize]byte, error) { + var buffer bytes.Buffer + for _, input := range tx.TxIn { + if err := binary.Write( + &buffer, + binary.LittleEndian, + input.Sequence, + ); err != nil { + return [chainhash.HashSize]byte{}, err + } + } + + return chainhash.HashH(buffer.Bytes()), nil +} + +func (tb *TransactionBuilder) taprootHashOutputs( + tx *wire.MsgTx, +) ([chainhash.HashSize]byte, error) { + var buffer bytes.Buffer + for i, output := range tx.TxOut { + if err := wire.WriteTxOut(&buffer, 0, 0, output); err != nil { + return [chainhash.HashSize]byte{}, fmt.Errorf( + "cannot write output [%d]: [%v]", + i, + err, + ) + } + } + + return chainhash.HashH(buffer.Bytes()), nil +} + +func writeOutPoint(buffer *bytes.Buffer, outpoint *wire.OutPoint) error { + if _, err := buffer.Write(outpoint.Hash[:]); err != nil { + return err + } + + return binary.Write(buffer, binary.LittleEndian, outpoint.Index) +} + // inputSigHashArgs is a helper structure holding some arguments required to // compute a sighash for the given input. type inputSigHashArgs struct { // value denotes the satoshi value of the UTXO pointed by the given input. value int64 + // publicKeyScript is the locking script of the UTXO pointed by the given + // input. + publicKeyScript []byte // scriptCode is a component of the input's sighash and is the script that // is actually executed while unlocking the given UTXO. The scriptCode // depends on the script type that was used to lock the given UTXO. scriptCode []byte + // scriptType denotes the locking script type of the UTXO pointed by the + // given input. + scriptType ScriptType + // taprootMerkleRoot denotes the BIP-341 script merkle root used to tweak + // the P2TR input's output key. It is nil for untweaked P2TR inputs and + // non-Taproot inputs. + taprootMerkleRoot *[32]byte // witness denotes whether the given input point's to a UTXO locked using // a witness script. witness bool diff --git a/pkg/bitcoin/transaction_builder_test.go b/pkg/bitcoin/transaction_builder_test.go index b1acc70567..ed80fc82aa 100644 --- a/pkg/bitcoin/transaction_builder_test.go +++ b/pkg/bitcoin/transaction_builder_test.go @@ -7,9 +7,10 @@ import ( "strings" "testing" + btcec2 "github.com/btcsuite/btcd/btcec/v2" + "github.com/btcsuite/btcd/btcec/v2/schnorr" "github.com/btcsuite/btcd/chaincfg/chainhash" "github.com/btcsuite/btcd/wire" - "github.com/keep-network/keep-core/internal/testutils" ) @@ -132,6 +133,267 @@ func TestTransactionBuilder_AddPublicKeyHashInput(t *testing.T) { } } +func TestTransactionBuilder_AddPublicKeyHashInput_AcceptsTaprootKeyPathInputForBackwardCompatibility( + t *testing.T, +) { + localChain := newLocalChain() + builder := NewTransactionBuilder(localChain) + + var taprootOutputKey [32]byte + copy( + taprootOutputKey[:], + hexToSlice( + t, + "1b84c5567b126440995d3ed5aaba0565d71e1834604819ff9c17f5e9d5dd078f", + ), + ) + + lockingScript, err := PayToTaproot(taprootOutputKey) + if err != nil { + t.Fatal(err) + } + + inputTransaction := &Transaction{ + Version: 1, + Inputs: []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash{0x01}, + OutputIndex: 0, + }, + SignatureScript: []byte{0x51}, + Sequence: 0xffffffff, + }, + }, + Outputs: []*TransactionOutput{ + { + Value: 100000, + PublicKeyScript: lockingScript, + }, + }, + Locktime: 0, + } + + if err := localChain.addTransaction(inputTransaction); err != nil { + t.Fatal(err) + } + + inputTransactionUtxo := &UnspentTransactionOutput{ + Outpoint: &TransactionOutpoint{ + TransactionHash: inputTransaction.Hash(), + OutputIndex: 0, + }, + Value: 100000, + } + + if err := builder.AddPublicKeyHashInput(inputTransactionUtxo); err != nil { + t.Fatal(err) + } + + if !builder.HasTaprootKeyPathInputs() { + t.Fatal("expected builder to have taproot key-path inputs") + } + if !builder.HasOnlyTaprootKeyPathInputs() { + t.Fatal("expected builder to have only taproot key-path inputs") + } + + assertSigHashArgs( + t, + &inputSigHashArgs{ + value: inputTransactionUtxo.Value, + publicKeyScript: lockingScript, + scriptCode: lockingScript, + scriptType: P2TRScript, + witness: true, + }, + builder.sigHashArgs[0], + ) + assertInternalInput(t, builder, 0, &TransactionInput{ + Outpoint: inputTransactionUtxo.Outpoint, + SignatureScript: nil, + Witness: nil, + Sequence: 0xffffffff, + }) +} + +func TestTransactionBuilder_AddTaprootKeyPathInputWithMerkleRoot(t *testing.T) { + localChain := newLocalChain() + builder := NewTransactionBuilder(localChain) + + privateKey, _ := btcec2.PrivKeyFromBytes( + hexToSlice( + t, + "0101010101010101010101010101010101010101010101010101010101010101", + ), + ) + + var internalKey [32]byte + copy(internalKey[:], schnorr.SerializePubKey(privateKey.PubKey())) + + refundLeaf := Script(hexToSlice( + t, + "76a9140102030405060708090a0b0c0d0e0f101112131488ac", + )) + merkleRoot, err := TaprootLeafHash(refundLeaf) + if err != nil { + t.Fatal(err) + } + + outputKey, err := TaprootOutputKey(internalKey, &merkleRoot) + if err != nil { + t.Fatal(err) + } + + lockingScript, err := PayToTaproot(outputKey) + if err != nil { + t.Fatal(err) + } + + inputTransaction := &Transaction{ + Version: 1, + Inputs: []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash{0x01}, + OutputIndex: 0, + }, + Sequence: 0xffffffff, + }, + }, + Outputs: []*TransactionOutput{ + { + Value: 100000, + PublicKeyScript: lockingScript, + }, + }, + Locktime: 0, + } + + if err := localChain.addTransaction(inputTransaction); err != nil { + t.Fatal(err) + } + + inputTransactionUtxo := &UnspentTransactionOutput{ + Outpoint: &TransactionOutpoint{ + TransactionHash: inputTransaction.Hash(), + OutputIndex: 0, + }, + Value: 100000, + } + + if err := builder.AddTaprootKeyPathInputWithMerkleRoot( + inputTransactionUtxo, + internalKey, + merkleRoot, + ); err != nil { + t.Fatal(err) + } + + assertSigHashArgs( + t, + &inputSigHashArgs{ + value: inputTransactionUtxo.Value, + publicKeyScript: lockingScript, + scriptCode: lockingScript, + scriptType: P2TRScript, + taprootMerkleRoot: &merkleRoot, + witness: true, + }, + builder.sigHashArgs[0], + ) + + merkleRoots := builder.TaprootKeyPathInputMerkleRoots() + if len(merkleRoots) != 1 { + t.Fatalf("unexpected merkle roots count: [%v]", len(merkleRoots)) + } + testutils.AssertBytesEqual(t, merkleRoot[:], merkleRoots[0][:]) +} + +func TestTransactionBuilder_AddTaprootKeyPathInputWithMerkleRootRejectsMismatch( + t *testing.T, +) { + localChain := newLocalChain() + builder := NewTransactionBuilder(localChain) + + privateKey, _ := btcec2.PrivKeyFromBytes( + hexToSlice( + t, + "0101010101010101010101010101010101010101010101010101010101010101", + ), + ) + + var internalKey [32]byte + copy(internalKey[:], schnorr.SerializePubKey(privateKey.PubKey())) + + merkleRoot, err := TaprootLeafHash(Script(hexToSlice( + t, + "76a9140102030405060708090a0b0c0d0e0f101112131488ac", + ))) + if err != nil { + t.Fatal(err) + } + + wrongMerkleRoot, err := TaprootLeafHash(Script(hexToSlice( + t, + "76a914ffffffffffffffffffffffffffffffffffffffff88ac", + ))) + if err != nil { + t.Fatal(err) + } + + outputKey, err := TaprootOutputKey(internalKey, &merkleRoot) + if err != nil { + t.Fatal(err) + } + + lockingScript, err := PayToTaproot(outputKey) + if err != nil { + t.Fatal(err) + } + + inputTransaction := &Transaction{ + Version: 1, + Inputs: []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash{0x01}, + OutputIndex: 0, + }, + Sequence: 0xffffffff, + }, + }, + Outputs: []*TransactionOutput{ + { + Value: 100000, + PublicKeyScript: lockingScript, + }, + }, + Locktime: 0, + } + + if err := localChain.addTransaction(inputTransaction); err != nil { + t.Fatal(err) + } + + err = builder.AddTaprootKeyPathInputWithMerkleRoot( + &UnspentTransactionOutput{ + Outpoint: &TransactionOutpoint{ + TransactionHash: inputTransaction.Hash(), + OutputIndex: 0, + }, + Value: 100000, + }, + internalKey, + wrongMerkleRoot, + ) + if err == nil { + t.Fatal("expected taproot output key mismatch error") + } + if !strings.Contains(err.Error(), "taproot output key does not match") { + t.Fatalf("unexpected error: [%v]", err) + } +} + func TestTransactionBuilder_AddInputReturnsErrorForOutOfRangeOutputIndex( t *testing.T, ) { @@ -281,6 +543,755 @@ func TestTransactionBuilder_AddOutput(t *testing.T) { assertInternalOutput(t, builder, 0, output) } +func TestTransactionBuilder_AddTaprootKeyPathSignatures(t *testing.T) { + localChain := newLocalChain() + builder := NewTransactionBuilder(localChain) + + privateKeyBytes := hexToSlice( + t, + "0101010101010101010101010101010101010101010101010101010101010101", + ) + privateKey, publicKey := btcec2.PrivKeyFromBytes(privateKeyBytes) + + var taprootOutputKey [32]byte + copy(taprootOutputKey[:], schnorr.SerializePubKey(publicKey)) + + inputScript, err := PayToTaproot(taprootOutputKey) + if err != nil { + t.Fatal(err) + } + + var outputPublicKeyHash [20]byte + copy( + outputPublicKeyHash[:], + hexToSlice(t, "0202020202020202020202020202020202020202"), + ) + outputScript, err := PayToWitnessPublicKeyHash(outputPublicKeyHash) + if err != nil { + t.Fatal(err) + } + + previousTransaction := &Transaction{ + Version: 1, + Inputs: []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash{ + 0x10, 0x11, 0x12, 0x13, + 0x14, 0x15, 0x16, 0x17, + 0x18, 0x19, 0x1a, 0x1b, + 0x1c, 0x1d, 0x1e, 0x1f, + 0x20, 0x21, 0x22, 0x23, + 0x24, 0x25, 0x26, 0x27, + 0x28, 0x29, 0x2a, 0x2b, + 0x2c, 0x2d, 0x2e, 0x2f, + }, + OutputIndex: 0, + }, + SignatureScript: []byte{0x51}, + Sequence: 0xffffffff, + }, + }, + Outputs: []*TransactionOutput{ + { + Value: 100000, + PublicKeyScript: inputScript, + }, + }, + Locktime: 0, + } + + if err := localChain.addTransaction(previousTransaction); err != nil { + t.Fatal(err) + } + + err = builder.AddTaprootKeyPathInput(&UnspentTransactionOutput{ + Outpoint: &TransactionOutpoint{ + TransactionHash: previousTransaction.Hash(), + OutputIndex: 0, + }, + Value: 100000, + }) + if err != nil { + t.Fatal(err) + } + + builder.AddOutput(&TransactionOutput{ + Value: 90000, + PublicKeyScript: outputScript, + }) + + if !builder.HasTaprootKeyPathInputs() { + t.Fatal("expected builder to have taproot key-path inputs") + } + if !builder.HasOnlyTaprootKeyPathInputs() { + t.Fatal("expected builder to have only taproot key-path inputs") + } + + sigHashes, err := builder.ComputeSignatureHashes() + if err != nil { + t.Fatal(err) + } + + testutils.AssertIntsEqual(t, "signature hashes count", 1, len(sigHashes)) + + expectedSigHash := hexToSlice( + t, + "96653d19d603d309d22cfe2ccd0ba445e40629dea18d46108caa601055ec4318", + ) + // This vector was generated with btcd v0.23.4's BIP-341 + // CalcTaprootSignatureHash implementation and independently + // cross-checked by reviewers. + sigHashBytes := sigHashes[0].FillBytes(make([]byte, 32)) + testutils.AssertBytesEqual(t, expectedSigHash, sigHashBytes) + + signature, err := schnorr.Sign(privateKey, sigHashBytes) + if err != nil { + t.Fatal(err) + } + signatureBytes := signature.Serialize() + + expectedSignature := hexToSlice( + t, + "5e847a0c22486f3b89ff80edd5afaf4be550aa411a0a7e28cff19d2b5924d77102bbf9a0a51100f4fdfc8435d0e8ff0f61dfdeccd464b78c553b1b4414ac0877", + ) + testutils.AssertBytesEqual(t, expectedSignature, signatureBytes) + + var signatureContainer [64]byte + copy(signatureContainer[:], signatureBytes) + + transaction, err := builder.AddTaprootKeyPathSignatures( + []*SchnorrSignatureContainer{ + { + Signature: signatureContainer, + }, + }, + ) + if err != nil { + t.Fatal(err) + } + + testutils.AssertIntsEqual( + t, + "transaction inputs count", + 1, + len(transaction.Inputs), + ) + testutils.AssertIntsEqual( + t, + "taproot witness elements count", + 1, + len(transaction.Inputs[0].Witness), + ) + testutils.AssertBytesEqual( + t, + expectedSignature, + transaction.Inputs[0].Witness[0], + ) + testutils.AssertBytesEqual(t, nil, transaction.Inputs[0].SignatureScript) +} + +func TestTransactionBuilder_AddTaprootKeyPathSignatures_MultipleInputs( + t *testing.T, +) { + localChain := newLocalChain() + builder := NewTransactionBuilder(localChain) + + privateKeyBytes := hexToSlice( + t, + "0101010101010101010101010101010101010101010101010101010101010101", + ) + privateKey, publicKey := btcec2.PrivKeyFromBytes(privateKeyBytes) + + var taprootOutputKey [32]byte + copy(taprootOutputKey[:], schnorr.SerializePubKey(publicKey)) + + inputScript, err := PayToTaproot(taprootOutputKey) + if err != nil { + t.Fatal(err) + } + + var outputPublicKeyHash [20]byte + copy( + outputPublicKeyHash[:], + hexToSlice(t, "0202020202020202020202020202020202020202"), + ) + outputScript, err := PayToWitnessPublicKeyHash(outputPublicKeyHash) + if err != nil { + t.Fatal(err) + } + + inputValues := []int64{100000, 110000} + for i, value := range inputValues { + previousTransaction := &Transaction{ + Version: 1, + Inputs: []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash{byte(0x20 + i)}, + OutputIndex: 0, + }, + SignatureScript: []byte{0x51}, + Sequence: 0xffffffff, + }, + }, + Outputs: []*TransactionOutput{ + { + Value: value, + PublicKeyScript: inputScript, + }, + }, + Locktime: 0, + } + + if err := localChain.addTransaction(previousTransaction); err != nil { + t.Fatal(err) + } + + err = builder.AddTaprootKeyPathInput(&UnspentTransactionOutput{ + Outpoint: &TransactionOutpoint{ + TransactionHash: previousTransaction.Hash(), + OutputIndex: 0, + }, + Value: value, + }) + if err != nil { + t.Fatal(err) + } + } + + builder.AddOutput(&TransactionOutput{ + Value: 209000, + PublicKeyScript: outputScript, + }) + + sigHashes, err := builder.ComputeSignatureHashes() + if err != nil { + t.Fatal(err) + } + + testutils.AssertIntsEqual(t, "signature hashes count", 2, len(sigHashes)) + if sigHashes[0].Cmp(sigHashes[1]) == 0 { + t.Fatal("expected distinct taproot signature hashes") + } + + signatures := make([]*SchnorrSignatureContainer, len(sigHashes)) + expectedSignatures := make([][]byte, len(sigHashes)) + for i, sigHash := range sigHashes { + signature, err := schnorr.Sign( + privateKey, + sigHash.FillBytes(make([]byte, 32)), + ) + if err != nil { + t.Fatal(err) + } + + signatureBytes := signature.Serialize() + expectedSignatures[i] = signatureBytes + + var signatureContainer [64]byte + copy(signatureContainer[:], signatureBytes) + signatures[i] = &SchnorrSignatureContainer{ + Signature: signatureContainer, + } + } + + transaction, err := builder.AddTaprootKeyPathSignatures(signatures) + if err != nil { + t.Fatal(err) + } + + testutils.AssertIntsEqual( + t, + "transaction inputs count", + 2, + len(transaction.Inputs), + ) + for i, input := range transaction.Inputs { + testutils.AssertIntsEqual( + t, + fmt.Sprintf("taproot witness elements count for input [%d]", i), + 1, + len(input.Witness), + ) + testutils.AssertBytesEqual(t, expectedSignatures[i], input.Witness[0]) + testutils.AssertBytesEqual(t, nil, input.SignatureScript) + } +} + +func TestTransactionBuilder_AddSignaturesRejectsTaprootInput(t *testing.T) { + localChain := newLocalChain() + builder := NewTransactionBuilder(localChain) + + var taprootOutputKey [32]byte + copy( + taprootOutputKey[:], + hexToSlice( + t, + "1b84c5567b126440995d3ed5aaba0565d71e1834604819ff9c17f5e9d5dd078f", + ), + ) + inputScript, err := PayToTaproot(taprootOutputKey) + if err != nil { + t.Fatal(err) + } + + previousTransaction := &Transaction{ + Version: 1, + Inputs: []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash{0x01}, + OutputIndex: 0, + }, + SignatureScript: []byte{0x51}, + Sequence: 0xffffffff, + }, + }, + Outputs: []*TransactionOutput{ + { + Value: 100000, + PublicKeyScript: inputScript, + }, + }, + Locktime: 0, + } + + if err := localChain.addTransaction(previousTransaction); err != nil { + t.Fatal(err) + } + + err = builder.AddTaprootKeyPathInput(&UnspentTransactionOutput{ + Outpoint: &TransactionOutpoint{ + TransactionHash: previousTransaction.Hash(), + OutputIndex: 0, + }, + Value: 100000, + }) + if err != nil { + t.Fatal(err) + } + + var outputPublicKeyHash [20]byte + outputScript, err := PayToWitnessPublicKeyHash(outputPublicKeyHash) + if err != nil { + t.Fatal(err) + } + builder.AddOutput(&TransactionOutput{ + Value: 90000, + PublicKeyScript: outputScript, + }) + + if _, err := builder.ComputeSignatureHashes(); err != nil { + t.Fatal(err) + } + + _, err = builder.AddSignatures([]*SignatureContainer{ + { + R: big.NewInt(1), + S: big.NewInt(1), + }, + }) + if err == nil { + t.Fatal("expected AddSignatures to reject a taproot input") + } + if !strings.Contains(err.Error(), "use AddTaprootKeyPathSignatures") { + t.Fatalf("unexpected error: [%v]", err) + } +} + +func TestTransactionBuilder_ReplaceUnsignedTransaction(t *testing.T) { + builder := NewTransactionBuilder(nil) + + var initialInputHash1 chainhash.Hash + var initialInputHash2 chainhash.Hash + initialInputHash1[0] = 0x11 + initialInputHash2[0] = 0x22 + + builder.internal.AddTxIn( + wire.NewTxIn( + wire.NewOutPoint(&initialInputHash1, 1), + []byte{0xde, 0xad}, + nil, + ), + ) + builder.internal.AddTxIn( + wire.NewTxIn( + wire.NewOutPoint(&initialInputHash2, 2), + nil, + [][]byte{{0xbe, 0xef}}, + ), + ) + builder.sigHashArgs = append( + builder.sigHashArgs, + &inputSigHashArgs{value: 111, scriptCode: []byte{0x51}, witness: false}, + &inputSigHashArgs{value: 222, scriptCode: []byte{0x52}, witness: true}, + ) + builder.sigHashes = []*big.Int{big.NewInt(1), big.NewInt(2)} + + var replacementInputHash1 chainhash.Hash + var replacementInputHash2 chainhash.Hash + replacementInputHash1[0] = 0x33 + replacementInputHash2[0] = 0x44 + + err := builder.ReplaceUnsignedTransaction( + &Transaction{ + Version: 2, + Inputs: []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash(replacementInputHash1), + OutputIndex: 7, + }, + Sequence: 0xffffffff, + }, + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash(replacementInputHash2), + OutputIndex: 8, + }, + Sequence: 0xffffffff, + }, + }, + Outputs: []*TransactionOutput{ + { + Value: 1000, + PublicKeyScript: hexToSlice(t, "0014deadbeef"), + }, + }, + Locktime: 0, + }, + ) + if err != nil { + t.Fatalf("unexpected replacement error: [%v]", err) + } + + if len(builder.sigHashes) != 0 { + t.Fatalf("expected sighashes reset after replacement: [%d]", len(builder.sigHashes)) + } + + // Preserve P2SH/P2WSH placeholder scripts needed for final signature + // application while replacing tx skeleton. + if !reflect.DeepEqual([]byte{0xde, 0xad}, builder.internal.TxIn[0].SignatureScript) { + t.Fatalf( + "unexpected preserved signature script\nexpected: [%x]\nactual: [%x]", + []byte{0xde, 0xad}, + builder.internal.TxIn[0].SignatureScript, + ) + } + + if len(builder.internal.TxIn[1].Witness) != 1 { + t.Fatalf("unexpected preserved witness length: [%d]", len(builder.internal.TxIn[1].Witness)) + } + + if !reflect.DeepEqual([]byte{0xbe, 0xef}, builder.internal.TxIn[1].Witness[0]) { + t.Fatalf( + "unexpected preserved witness script\nexpected: [%x]\nactual: [%x]", + []byte{0xbe, 0xef}, + builder.internal.TxIn[1].Witness[0], + ) + } + + inputs, outputs, err := builder.UnsignedTransactionIO() + if err != nil { + t.Fatalf("unexpected extraction error after replacement: [%v]", err) + } + + if len(inputs) != 2 { + t.Fatalf("unexpected input count after replacement: [%d]", len(inputs)) + } + + if inputs[0].TxIDHex != replacementInputHash1.String() || inputs[0].Vout != 7 { + t.Fatalf("unexpected first input after replacement: [%+v]", inputs[0]) + } + + if inputs[1].TxIDHex != replacementInputHash2.String() || inputs[1].Vout != 8 { + t.Fatalf("unexpected second input after replacement: [%+v]", inputs[1]) + } + + if len(outputs) != 1 { + t.Fatalf("unexpected output count after replacement: [%d]", len(outputs)) + } +} + +func TestTransactionBuilder_ReplaceUnsignedTransaction_RejectsInputMetadataMismatch( + t *testing.T, +) { + builder := NewTransactionBuilder(nil) + + var txHash chainhash.Hash + builder.internal.AddTxIn(wire.NewTxIn(wire.NewOutPoint(&txHash, 0), nil, nil)) + builder.sigHashArgs = append(builder.sigHashArgs, &inputSigHashArgs{value: 1}) + + err := builder.ReplaceUnsignedTransaction( + &Transaction{ + Inputs: []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash(txHash), + OutputIndex: 0, + }, + }, + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash(txHash), + OutputIndex: 1, + }, + }, + }, + }, + ) + if err == nil { + t.Fatal("expected input metadata mismatch error") + } + + if !reflect.DeepEqual( + fmt.Sprintf( + "input metadata mismatch: [%d] tx inputs, [%d] sighash args", + 2, + 1, + ), + err.Error(), + ) { + t.Fatalf("unexpected error: [%v]", err) + } +} + +func TestTransactionBuilder_ReplaceUnsignedTransaction_RejectsNonEmptyReplacementSignatureScript( + t *testing.T, +) { + builder := NewTransactionBuilder(nil) + + var txHash chainhash.Hash + builder.internal.AddTxIn(wire.NewTxIn(wire.NewOutPoint(&txHash, 0), nil, nil)) + builder.sigHashArgs = append( + builder.sigHashArgs, + &inputSigHashArgs{value: 1, scriptCode: []byte{0x51}, witness: false}, + ) + + err := builder.ReplaceUnsignedTransaction( + &Transaction{ + Inputs: []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash(txHash), + OutputIndex: 0, + }, + SignatureScript: []byte{0xaa}, + Sequence: 0xffffffff, + }, + }, + }, + ) + if err == nil { + t.Fatal("expected replacement signature script error") + } + + if !strings.Contains( + err.Error(), + "replacement transaction input [0] has unexpected non-empty signature script", + ) { + t.Fatalf("unexpected error: [%v]", err) + } +} + +func TestTransactionBuilder_ReplaceUnsignedTransaction_RejectsNonEmptyReplacementWitness( + t *testing.T, +) { + builder := NewTransactionBuilder(nil) + + var txHash chainhash.Hash + builder.internal.AddTxIn(wire.NewTxIn(wire.NewOutPoint(&txHash, 0), nil, nil)) + builder.sigHashArgs = append( + builder.sigHashArgs, + &inputSigHashArgs{value: 1, scriptCode: []byte{0x51}, witness: true}, + ) + + err := builder.ReplaceUnsignedTransaction( + &Transaction{ + Inputs: []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash(txHash), + OutputIndex: 0, + }, + Witness: wire.TxWitness{[]byte{0xbb}}, + Sequence: 0xffffffff, + }, + }, + }, + ) + if err == nil { + t.Fatal("expected replacement witness error") + } + + if !strings.Contains( + err.Error(), + "replacement transaction input [0] has unexpected non-empty witness", + ) { + t.Fatalf("unexpected error: [%v]", err) + } +} + +func TestTransactionBuilder_ReplaceUnsignedTransaction_RejectsMultiElementPreviousWitness( + t *testing.T, +) { + builder := NewTransactionBuilder(nil) + + var txHash chainhash.Hash + previousInput := wire.NewTxIn(wire.NewOutPoint(&txHash, 0), nil, nil) + // Pre-signing witness that mimics a P2TR script-path spend: [script, + // controlBlock]. The restoration path supports only zero- or + // single-element previous witnesses today; the multi-element case must + // fail loudly rather than silently dropping data later in signing. + previousInput.Witness = wire.TxWitness{ + []byte{0x51, 0x52}, + []byte{0xc0, 0xab, 0xcd}, + } + builder.internal.AddTxIn(previousInput) + builder.sigHashArgs = append( + builder.sigHashArgs, + &inputSigHashArgs{value: 1, scriptCode: []byte{0x51}, witness: true}, + ) + + err := builder.ReplaceUnsignedTransaction( + &Transaction{ + Inputs: []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash(txHash), + OutputIndex: 0, + }, + Sequence: 0xffffffff, + }, + }, + }, + ) + if err == nil { + t.Fatal("expected multi-element witness restoration error") + } + + if !strings.Contains( + err.Error(), + "previous witness has [2] elements", + ) { + t.Fatalf("unexpected error: [%v]", err) + } + if !strings.Contains( + err.Error(), + "only zero- or single-element", + ) { + t.Fatalf("unexpected error: [%v]", err) + } +} + +func TestTransactionBuilder_UnsignedTransactionIO(t *testing.T) { + builder := NewTransactionBuilder(nil) + + var txHash chainhash.Hash + for i := range txHash { + txHash[i] = byte(i + 1) + } + const expectedTxIDHex = "201f1e1d1c1b1a191817161514131211100f0e0d0c0b0a090807060504030201" + + builder.internal.AddTxIn(wire.NewTxIn(wire.NewOutPoint(&txHash, 7), nil, nil)) + builder.sigHashArgs = append(builder.sigHashArgs, &inputSigHashArgs{ + value: 1234, + publicKeyScript: hexToSlice(t, "5120"+strings.Repeat("22", 32)), + }) + builder.AddOutput(&TransactionOutput{ + Value: 1000, + PublicKeyScript: hexToSlice(t, "0014deadbeef"), + }) + + inputs, outputs, err := builder.UnsignedTransactionIO() + if err != nil { + t.Fatalf("unexpected extraction error: [%v]", err) + } + + if len(inputs) != 1 { + t.Fatalf("unexpected input count: [%d]", len(inputs)) + } + + if inputs[0].TxIDHex != expectedTxIDHex { + t.Fatalf( + "unexpected input txid\nexpected: [%v]\nactual: [%v]", + expectedTxIDHex, + inputs[0].TxIDHex, + ) + } + + if inputs[0].Vout != 7 { + t.Fatalf("unexpected input vout: [%d]", inputs[0].Vout) + } + + if inputs[0].ValueSats != 1234 { + t.Fatalf("unexpected input value: [%d]", inputs[0].ValueSats) + } + + if inputs[0].ScriptPubKeyHex != "5120"+strings.Repeat("22", 32) { + t.Fatalf( + "unexpected input script\nexpected: [%v]\nactual: [%v]", + "5120"+strings.Repeat("22", 32), + inputs[0].ScriptPubKeyHex, + ) + } + + if len(outputs) != 1 { + t.Fatalf("unexpected output count: [%d]", len(outputs)) + } + + if outputs[0].ScriptPubKeyHex != "0014deadbeef" { + t.Fatalf( + "unexpected output script\nexpected: [%v]\nactual: [%v]", + "0014deadbeef", + outputs[0].ScriptPubKeyHex, + ) + } + + if outputs[0].ValueSats != 1000 { + t.Fatalf("unexpected output value: [%d]", outputs[0].ValueSats) + } +} + +func TestTransactionBuilder_UnsignedTransactionIO_RejectsNegativeInputValue( + t *testing.T, +) { + builder := NewTransactionBuilder(nil) + + var txHash chainhash.Hash + builder.internal.AddTxIn(wire.NewTxIn(wire.NewOutPoint(&txHash, 0), nil, nil)) + builder.sigHashArgs = append(builder.sigHashArgs, &inputSigHashArgs{value: -1}) + builder.AddOutput(&TransactionOutput{ + Value: 1, + PublicKeyScript: hexToSlice(t, "0014aa"), + }) + + _, _, err := builder.UnsignedTransactionIO() + if err == nil { + t.Fatal("expected extraction error") + } +} + +func TestTransactionBuilder_UnsignedTransactionIO_RejectsNegativeOutputValue( + t *testing.T, +) { + builder := NewTransactionBuilder(nil) + + var txHash chainhash.Hash + builder.internal.AddTxIn(wire.NewTxIn(wire.NewOutPoint(&txHash, 0), nil, nil)) + builder.sigHashArgs = append(builder.sigHashArgs, &inputSigHashArgs{value: 1}) + builder.AddOutput(&TransactionOutput{ + Value: -1, + PublicKeyScript: hexToSlice(t, "0014aa"), + }) + + _, _, err := builder.UnsignedTransactionIO() + if err == nil { + t.Fatal("expected extraction error") + } +} + // The goal of this test is making sure that the TransactionBuilder can // produce proper signature hashes and apply signatures for all input types, // i.e. P2PKH, P2WPKH, P2SH, and P2WSH. This test uses transactions that @@ -509,6 +1520,37 @@ func assertSigHashArgs(t *testing.T, expected, actual *inputSigHashArgs) { actual.scriptCode, ) + if expected.publicKeyScript != nil { + testutils.AssertBytesEqual( + t, + expected.publicKeyScript, + actual.publicKeyScript, + ) + } + + if expected.scriptType != NonStandardScript { + testutils.AssertIntsEqual( + t, + "sighash args script type", + int(expected.scriptType), + int(actual.scriptType), + ) + } + + if expected.taprootMerkleRoot != nil { + if actual.taprootMerkleRoot == nil { + t.Fatal("expected taproot merkle root") + } + + testutils.AssertBytesEqual( + t, + expected.taprootMerkleRoot[:], + actual.taprootMerkleRoot[:], + ) + } else if actual.taprootMerkleRoot != nil { + t.Fatal("unexpected taproot merkle root") + } + testutils.AssertBoolsEqual( t, "sighash args witness flag", From 6c9998e8b77e7b1a17022fded6cfb1c849fbd627 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Fri, 14 Aug 2026 07:34:53 +0000 Subject: [PATCH 02/10] test(bitcoin): differential-test the taproot sighash against btcd The BIP-341 key-path signature hash preimage is assembled by hand: epoch, hash type, version, locktime, five midstate hashes, spend type and input index. A misordered or mistyped field yields a digest that signs a transaction the wallet did not intend, and the only existing coverage was a single hardcoded vector, which fixes one input count and one output shape. Compare every P2TR input against txscript.CalcTaprootSignatureHash over randomized multi-input transactions, varying input count and ordering, per-input values and script types, output count and values, version and locktime. txscript was already a dependency of this file for the legacy sighash paths. Verified the test fails on a swapped midstate write and on a wrong sighash epoch byte. --- pkg/bitcoin/taproot_differential_test.go | 145 +++++++++++++ .../taproot_sighash_differential_test.go | 203 ++++++++++++++++++ 2 files changed, 348 insertions(+) create mode 100644 pkg/bitcoin/taproot_differential_test.go create mode 100644 pkg/bitcoin/taproot_sighash_differential_test.go diff --git a/pkg/bitcoin/taproot_differential_test.go b/pkg/bitcoin/taproot_differential_test.go new file mode 100644 index 0000000000..e22585004f --- /dev/null +++ b/pkg/bitcoin/taproot_differential_test.go @@ -0,0 +1,145 @@ +package bitcoin + +import ( + "bytes" + "testing" + + btcec2 "github.com/btcsuite/btcd/btcec/v2" + "github.com/btcsuite/btcd/btcec/v2/schnorr" + "github.com/btcsuite/btcd/txscript" +) + +// The taproot helpers in taproot.go are a from-scratch BIP-341 implementation +// over raw secp256k1 field and scalar operations. The other tests in this package +// pin them to fixtures that were generated by this same implementation, so they +// detect a later regression but cannot detect an original error. That matters more +// than usual here: a wrong output key produces a well-formed address whose coins +// nobody can ever spend, so the failure mode is fund loss rather than a rejected +// transaction. +// +// These tests cross-check against btcd's txscript, an independent and widely used +// implementation already in this module's dependency graph, over many generated +// inputs rather than a handful of constants. Any divergence in the tweak +// derivation, the point addition, or the x-only serialisation shows up here. + +// TestTaprootOutputKeyMatchesTxscriptScriptPath cross-checks the script-path +// (merkle root present) derivation. +func TestTaprootOutputKeyMatchesTxscriptScriptPath(t *testing.T) { + for i := range 128 { + privateKey, err := btcec2.NewPrivateKey() + if err != nil { + t.Fatal(err) + } + + var internalKey [32]byte + copy(internalKey[:], schnorr.SerializePubKey(privateKey.PubKey())) + + // Vary the leaf script so the merkle root differs on every iteration. + leafScript := append( + []byte{txscript.OP_DATA_1, byte(i)}, + txscript.OP_DROP, txscript.OP_TRUE, + ) + + merkleRoot, err := TaprootLeafHash(Script(leafScript)) + if err != nil { + t.Fatal(err) + } + + // Independently confirm the leaf hash itself, not just the output key: an + // error here would otherwise cancel out on both sides below. + expectedLeafHash := txscript.NewBaseTapLeaf(leafScript).TapHash() + if !bytes.Equal(merkleRoot[:], expectedLeafHash[:]) { + t.Fatalf( + "iteration %d: leaf hash mismatch: ours [%x], txscript [%x]", + i, merkleRoot, expectedLeafHash, + ) + } + + ours, err := TaprootOutputKey(internalKey, &merkleRoot) + if err != nil { + t.Fatal(err) + } + + theirs := txscript.ComputeTaprootOutputKey( + privateKey.PubKey(), + merkleRoot[:], + ) + expected := schnorr.SerializePubKey(theirs) + + if !bytes.Equal(ours[:], expected) { + t.Fatalf( + "iteration %d: output key mismatch: ours [%x], txscript [%x]", + i, ours, expected, + ) + } + } +} + +// TestTaprootOutputKeyMatchesTxscriptKeyPathOnly cross-checks the key-path-only +// derivation (no script tree). This is the case tBTC FROST wallets actually use, +// so it is the one that must be right for real funds. +func TestTaprootOutputKeyMatchesTxscriptKeyPathOnly(t *testing.T) { + for i := range 128 { + privateKey, err := btcec2.NewPrivateKey() + if err != nil { + t.Fatal(err) + } + + var internalKey [32]byte + copy(internalKey[:], schnorr.SerializePubKey(privateKey.PubKey())) + + ours, err := TaprootOutputKey(internalKey, nil) + if err != nil { + t.Fatal(err) + } + + // A nil scriptRoot selects txscript's key-path-only tweak, matching the + // nil merkleRoot above. + theirs := txscript.ComputeTaprootKeyNoScript(privateKey.PubKey()) + expected := schnorr.SerializePubKey(theirs) + + if !bytes.Equal(ours[:], expected) { + t.Fatalf( + "iteration %d: key-path output key mismatch: ours [%x], txscript [%x]", + i, ours, expected, + ) + } + } +} + +// TestPayToTaprootWithScriptTreeMatchesTxscript checks the whole script-building +// path, so a correct output key cannot be undone by wrong script framing (the +// OP_1 witness-version prefix and 32-byte push). +func TestPayToTaprootWithScriptTreeMatchesTxscript(t *testing.T) { + privateKey, err := btcec2.NewPrivateKey() + if err != nil { + t.Fatal(err) + } + + var internalKey [32]byte + copy(internalKey[:], schnorr.SerializePubKey(privateKey.PubKey())) + + leafScript := []byte{txscript.OP_TRUE} + merkleRoot, err := TaprootLeafHash(Script(leafScript)) + if err != nil { + t.Fatal(err) + } + + ours, err := PayToTaprootWithScriptTree(internalKey, merkleRoot) + if err != nil { + t.Fatal(err) + } + + outputKey := txscript.ComputeTaprootOutputKey( + privateKey.PubKey(), + merkleRoot[:], + ) + theirs, err := txscript.PayToTaprootScript(outputKey) + if err != nil { + t.Fatal(err) + } + + if !bytes.Equal(ours, theirs) { + t.Fatalf("P2TR script mismatch: ours [%x], txscript [%x]", ours, theirs) + } +} diff --git a/pkg/bitcoin/taproot_sighash_differential_test.go b/pkg/bitcoin/taproot_sighash_differential_test.go new file mode 100644 index 0000000000..97268d226f --- /dev/null +++ b/pkg/bitcoin/taproot_sighash_differential_test.go @@ -0,0 +1,203 @@ +package bitcoin + +import ( + "bytes" + "encoding/binary" + "math/rand/v2" + "testing" + + "github.com/btcsuite/btcd/txscript" +) + +// fillRandom fills b with pseudo-random bytes from rng. math/rand/v2 drops the +// Read method that math/rand exposed on *Rand, and these bytes only need to vary +// across iterations, never to be unpredictable. +func fillRandom(rng *rand.Rand, b []byte) { + var block [8]byte + for offset := 0; offset < len(b); offset += len(block) { + binary.LittleEndian.PutUint64(block[:], rng.Uint64()) + copy(b[offset:], block[:]) + } +} + +// TestTaprootKeyPathSigHashMatchesTxscript is a differential test of the +// hand-rolled BIP-341 key-path signature hash against btcd's +// txscript.CalcTaprootSignatureHash over randomized multi-input transactions. +// +// calcTaprootKeyPathSignatureHash assembles the SigMsg preimage by hand -- epoch, +// hash type, version, locktime, the five midstate hashes, spend type and input +// index -- and a single mismatched byte or a misordered field produces a digest +// that signs a transaction the wallet did not intend. That is the fund-loss class, +// and a hardcoded vector cannot catch it because the vector fixes exactly one +// input count, one ordering and one output shape. +// +// The randomization deliberately varies what the midstate commits to: input count +// and ordering, per-input values, sequences, script types (so P2TR inputs sit at +// arbitrary indices in a mixed transaction), output count and values, version and +// locktime. Every P2TR input of every generated transaction must agree with +// txscript byte for byte. +// +// The seed is fixed so failures reproduce exactly. +func TestTaprootKeyPathSigHashMatchesTxscript(t *testing.T) { + const ( + iterations = 200 + maxInputs = 5 + maxOutputs = 4 + fundingValue = 1_000_000 + ) + + rng := rand.New(rand.NewPCG(20260813, 0x9E3779B97F4A7C15)) + + for iteration := 0; iteration < iterations; iteration++ { + localChain := newLocalChain() + builder := NewTransactionBuilder(localChain) + + inputCount := 1 + rng.IntN(maxInputs) + taprootInputs := make([]int, 0, inputCount) + + for i := 0; i < inputCount; i++ { + // Mixed script types put P2TR inputs at arbitrary indices. The midstate + // commits to every input's amount and script regardless of type, so a + // non-Taproot input at a lower index must still be folded in correctly. + isTaproot := rng.IntN(2) == 0 || (i == inputCount-1 && len(taprootInputs) == 0) + + var ( + lockingScript Script + err error + ) + if isTaproot { + var outputKey [32]byte + fillRandom(rng, outputKey[:]) + lockingScript, err = PayToTaproot(outputKey) + } else { + var publicKeyHash [20]byte + fillRandom(rng, publicKeyHash[:]) + lockingScript, err = PayToWitnessPublicKeyHash(publicKeyHash) + } + if err != nil { + t.Fatalf("iteration %d input %d: %v", iteration, i, err) + } + + // Vary the value per input: the midstate's hashInputAmounts commits to + // all of them in input order. + value := int64(1 + rng.IntN(fundingValue)) + + var previousHash Hash + previousHash[0] = byte(iteration) + previousHash[1] = byte(i) + + fundingTransaction := &Transaction{ + Version: 1, + Inputs: []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: previousHash, + OutputIndex: 0, + }, + SignatureScript: []byte{0x51}, + Sequence: 0xffffffff, + }, + }, + Outputs: []*TransactionOutput{ + { + Value: value, + PublicKeyScript: lockingScript, + }, + }, + Locktime: 0, + } + if err := localChain.addTransaction(fundingTransaction); err != nil { + t.Fatalf("iteration %d input %d: %v", iteration, i, err) + } + + utxo := &UnspentTransactionOutput{ + Outpoint: &TransactionOutpoint{ + TransactionHash: fundingTransaction.Hash(), + OutputIndex: 0, + }, + Value: value, + } + + if isTaproot { + if err := builder.AddTaprootKeyPathInput(utxo); err != nil { + t.Fatalf("iteration %d input %d: %v", iteration, i, err) + } + taprootInputs = append(taprootInputs, i) + } else { + if err := builder.AddPublicKeyHashInput(utxo); err != nil { + t.Fatalf("iteration %d input %d: %v", iteration, i, err) + } + } + } + + // Vary the outputs: hashOutputs commits to every value and script. + outputCount := 1 + rng.IntN(maxOutputs) + for i := 0; i < outputCount; i++ { + var publicKeyHash [20]byte + fillRandom(rng, publicKeyHash[:]) + outputScript, err := PayToWitnessPublicKeyHash(publicKeyHash) + if err != nil { + t.Fatalf("iteration %d output %d: %v", iteration, i, err) + } + builder.AddOutput(&TransactionOutput{ + Value: int64(1 + rng.IntN(fundingValue/2)), + PublicKeyScript: outputScript, + }) + } + + // Version and locktime are both in the preimage, ahead of the midstate + // hashes, so a field-order error shows up as soon as they are not their + // default values. + builder.internal.Version = int32(1 + rng.IntN(2)) + builder.internal.LockTime = uint32(rng.IntN(500_000)) + + sigHashes, err := builder.ComputeSignatureHashes() + if err != nil { + t.Fatalf("iteration %d: compute sighashes: %v", iteration, err) + } + + // The reference implementation, given the identical transaction and the + // identical previous outputs. + reference := txscript.NewTxSigHashes(builder.internal.MsgTx, builder.prevOuts) + + for _, inputIndex := range taprootInputs { + expected, err := txscript.CalcTaprootSignatureHash( + reference, + txscript.SigHashDefault, + builder.internal.MsgTx, + inputIndex, + builder.prevOuts, + ) + if err != nil { + t.Fatalf( + "iteration %d input %d: txscript reference: %v", + iteration, + inputIndex, + err, + ) + } + + // ComputeSignatureHashes returns big.Int, which drops leading zero + // bytes; FillBytes restores the fixed 32-byte digest. + actual := sigHashes[inputIndex].FillBytes(make([]byte, 32)) + + if !bytes.Equal(expected, actual) { + t.Fatalf( + "iteration %d input %d of %d (taproot inputs %v, outputs %d, "+ + "version %d, locktime %d):\n"+ + "txscript: %x\n"+ + "builder: %x", + iteration, + inputIndex, + inputCount, + taprootInputs, + outputCount, + builder.internal.Version, + builder.internal.LockTime, + expected, + actual, + ) + } + } + } +} From acaeecb07e0bace72f2daf1f0fe916596d5f4bdb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Fri, 14 Aug 2026 07:34:53 +0000 Subject: [PATCH 03/10] perf(electrum): hold the client lock only for pointer access requestWithRetry and GetFee held clientMutex across the whole Electrum round trip, so every request in the process queued behind every other one for a full network round trip each. Per-script lookups that could overlap ran strictly in sequence. go-electrum multiplexes concurrent requests over an id-to-channel map with a single reader dispatching responses, so it is safe to call from several goroutines at once. The lock is only needed to keep readers from observing a client pointer mid-swap during a reconnect. Take a read lock long enough to copy the pointer and release it before the request. reconnectIfShutdown remains the sole writer and keeps the write lock. A reconnect landing just after the copy fails the request against the stale client and the retry wrapper repeats it, which is the same recovery path as before. --- pkg/bitcoin/electrum/electrum.go | 34 ++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/pkg/bitcoin/electrum/electrum.go b/pkg/bitcoin/electrum/electrum.go index 2c896be287..661541f344 100644 --- a/pkg/bitcoin/electrum/electrum.go +++ b/pkg/bitcoin/electrum/electrum.go @@ -30,7 +30,7 @@ var ( type Connection struct { parentCtx context.Context client *electrum.Client - clientMutex *sync.Mutex + clientMutex *sync.RWMutex config Config } @@ -55,7 +55,7 @@ func Connect(parentCtx context.Context, config Config) (bitcoin.Chain, error) { c := &Connection{ parentCtx: parentCtx, config: config, - clientMutex: &sync.Mutex{}, + clientMutex: &sync.RWMutex{}, } if err := c.electrumConnect(); err != nil { @@ -1080,9 +1080,7 @@ func (c *Connection) getFeeBtcPerKbOnce(blocks uint32) (float32, error) { c.config.RequestTimeout, ) defer requestCancel() - c.clientMutex.Lock() - fee, err := c.client.GetFee(requestCtx, blocks) - c.clientMutex.Unlock() + fee, err := c.currentClient().GetFee(requestCtx, blocks) if err != nil { return 0, fmt.Errorf("request failed: [%w]", err) } @@ -1350,9 +1348,7 @@ func requestWithRetry[K interface{}]( requestCtx, requestCancel := context.WithTimeout(ctx, c.config.RequestTimeout) defer requestCancel() - c.clientMutex.Lock() - r, err := requestFn(requestCtx, c.client) - c.clientMutex.Unlock() + r, err := requestFn(requestCtx, c.currentClient()) if err != nil { return fmt.Errorf("request failed: [%w]", err) @@ -1378,6 +1374,28 @@ func requestWithRetry[K interface{}]( return result, err } +// currentClient returns the live Electrum client under a read lock. +// +// The lock protects the client POINTER against a concurrent reconnect swap, not +// the request itself: go-electrum multiplexes concurrent requests over an +// id-to-channel map with a single reader goroutine dispatching responses, so it is +// safe to call from several goroutines at once. Holding the lock across the round +// trip instead serialized every Electrum request in the process, so independent +// lookups queued behind each other for a full network round trip each. +// +// A reconnect may still swap the client immediately after this returns. The +// request then fails against the stale client and the retry wrapper reconnects and +// repeats it -- the same recovery path a reconnect landing mid-request always took. +func (c *Connection) currentClient() *electrum.Client { + c.clientMutex.RLock() + defer c.clientMutex.RUnlock() + + return c.client +} + +// reconnectIfShutdown replaces a shut-down client with a fresh connection. It is +// the only writer of c.client, so it takes the write lock: readers hold the read +// lock just long enough to copy the pointer (see currentClient). func (c *Connection) reconnectIfShutdown() error { c.clientMutex.Lock() defer c.clientMutex.Unlock() From 0ce5885002730aa8fcfdb3a10e9d350f166c5c39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Fri, 14 Aug 2026 07:38:20 +0000 Subject: [PATCH 04/10] fix(tbtcpg): price P2TR redeemer output scripts The redeemer-script switch in EstimateRedemptionFee has no P2TR arm, so a P2TR redeemer output script falls through to the non-standard default and returns an error. That aborts the fee estimate for the whole batch, so one P2TR request stalls every pending redemption for that wallet rather than only its own. The Bridge accepts P2TR redeemer addresses (Redemption.sol validates against a set that includes them), so this is reachable as soon as a redeemer supplies one. Price it with AddOutputScript rather than a dedicated helper: an output costs its script length plus a fixed header, and the real script is already in hand, so no canonical placeholder is needed. The default arm still rejects genuinely non-standard scripts and now names the type it rejected. --- pkg/tbtcpg/redemptions.go | 10 ++++- pkg/tbtcpg/redemptions_test.go | 72 ++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 1 deletion(-) diff --git a/pkg/tbtcpg/redemptions.go b/pkg/tbtcpg/redemptions.go index 7f2633681c..57d155a5e6 100644 --- a/pkg/tbtcpg/redemptions.go +++ b/pkg/tbtcpg/redemptions.go @@ -529,8 +529,16 @@ func EstimateRedemptionFee( sizeEstimator.AddScriptHashOutputs(1, false) case bitcoin.P2WSHScript: sizeEstimator.AddScriptHashOutputs(1, true) + case bitcoin.P2TRScript: + // AddOutputScript rather than a dedicated Add*Outputs helper: an + // output costs its script length plus a fixed header, and the real + // script is already in hand, so no canonical placeholder is needed. + sizeEstimator.AddOutputScript(script) default: - return 0, fmt.Errorf("non-standard redeemer output script type") + return 0, fmt.Errorf( + "non-standard redeemer output script type [%v]", + bitcoin.GetScriptType(script), + ) } } diff --git a/pkg/tbtcpg/redemptions_test.go b/pkg/tbtcpg/redemptions_test.go index 2aef02fa73..cfb407286e 100644 --- a/pkg/tbtcpg/redemptions_test.go +++ b/pkg/tbtcpg/redemptions_test.go @@ -95,6 +95,78 @@ func TestEstimateRedemptionFee(t *testing.T) { } } +// TestEstimateRedemptionFee_P2TRRedeemerScript asserts a redemption batch that +// contains a P2TR redeemer output script is priced rather than rejected. +// +// The Bridge accepts P2TR redeemer addresses, so a single such request in a batch +// used to abort the estimate for the whole batch and stall every pending +// redemption for that wallet, not just its own. +func TestEstimateRedemptionFee_P2TRRedeemerScript(t *testing.T) { + fromHex := func(hexString string) []byte { + bytes, err := hex.DecodeString(hexString) + if err != nil { + t.Fatal(err) + } + return bytes + } + + // OP_1 (0x51) OP_DATA_32 (0x20) followed by the 32-byte output key. + p2trScript := bitcoin.Script(fromHex( + "5120a60869f0dbcf1dc659c9cecbaf8050135ea9e8cdc487053f1dc6880949dc684c", + )) + p2wpkhScript := bitcoin.Script(fromHex( + "0014e6f9d74726b19b75f16fe1e9feaec048aa4fa1d0", + )) + + estimate := func(scripts []bitcoin.Script) (int64, error) { + btcChain := tbtcpg.NewLocalBitcoinChain() + btcChain.SetEstimateSatPerVByteFee(1, 16) + + return tbtcpg.EstimateRedemptionFee(btcChain, scripts, 100000) + } + + withoutP2TR, err := estimate([]bitcoin.Script{p2wpkhScript}) + if err != nil { + t.Fatal(err) + } + + withP2TR, err := estimate([]bitcoin.Script{p2wpkhScript, p2trScript}) + if err != nil { + t.Fatalf("a P2TR redeemer script must be priced, not rejected: %v", err) + } + + // The P2TR request adds an output, so it must cost strictly more. An equal fee + // would mean the script was silently dropped from the size estimate and every + // redemption in the batch underpaid. + if withP2TR <= withoutP2TR { + t.Fatalf( + "adding a P2TR redeemer output must raise the fee: without [%d], with [%d]", + withoutP2TR, + withP2TR, + ) + } +} + +// TestEstimateRedemptionFee_NonStandardRedeemerScript asserts a genuinely +// non-standard redeemer script is still rejected, and that the error names the +// offending type so an operator can identify the request. +func TestEstimateRedemptionFee_NonStandardRedeemerScript(t *testing.T) { + btcChain := tbtcpg.NewLocalBitcoinChain() + btcChain.SetEstimateSatPerVByteFee(1, 16) + + _, err := tbtcpg.EstimateRedemptionFee( + btcChain, + []bitcoin.Script{bitcoin.Script{0x6a, 0x01, 0x00}}, // OP_RETURN + 100000, + ) + if err == nil { + t.Fatal("expected a non-standard redeemer output script to be rejected") + } + if !strings.Contains(err.Error(), "non-standard redeemer output script type") { + t.Fatalf("unexpected error: %v", err) + } +} + func TestRedemptionAction_FindPendingRedemptions(t *testing.T) { scenarios, err := test.LoadFindPendingRedemptionsTestScenario() if err != nil { From 4ff4197254b12ef019653cb33af1df6067887d67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Tue, 18 Aug 2026 09:07:17 +0000 Subject: [PATCH 05/10] test(bitcoin): cover TaprootTweak invalid-input + AddTaprootKeyPathSignatures error branches Two regression guards added: - TestTaprootTweak_RejectsInvalidInputs (pkg/bitcoin/script_test.go): exercises schnorr.ParsePubKey rejection of a malformed x-only internal key. Two further rejection paths (tweak >= curve order, infinity output) require inverting the BIP-341 tap-tweak hash to construct a deterministic fixture, so are skipped with a comment pointing to code-review coverage. - TestTransactionBuilder_AddTaprootKeyPathSignatures_RejectsInvalid (pkg/bitcoin/transaction_builder_test.go): exercises the 'wrong signatures count' branch (nil container) and the cryptographic Verify gate ('invalid taproot key-path signature for input [%v]') by submitting a syntactically valid BIP-340 Schnorr signature over a different message. Both pass against current code; the value is regression coverage for changes that might drop the rejection branches. Refs PR #4243 review findings M-TAPROOT-INVALID-KEY and M-SIG-VERIFY-UNTESTED. --- pkg/bitcoin/script_test.go | 42 +++++++ pkg/bitcoin/transaction_builder_test.go | 143 ++++++++++++++++++++++++ 2 files changed, 185 insertions(+) diff --git a/pkg/bitcoin/script_test.go b/pkg/bitcoin/script_test.go index 0990cb193e..4966cee120 100644 --- a/pkg/bitcoin/script_test.go +++ b/pkg/bitcoin/script_test.go @@ -626,3 +626,45 @@ func TestExtractPublicKeyHash(t *testing.T) { }) } } + +// TestTaprootTweak_RejectsInvalidInputs exercises the rejection paths in +// taprootTweakScalar. The schnorr.ParsePubKey branch is tested directly; +// the tweakScalar.SetBytes (tweak >= curve order) and infinity-output +// branches require inverting the BIP-341 tap-tweak hash to construct a +// deterministic fixture (any (internalKey, merkleRoot) pair produces a +// tag-tweak hash >= curve order with probability ~50%, but engineering the +// input without inverting SHA-256 is infeasible), so both are skipped with +// a pointer to the code review that asserts their behavior. +func TestTaprootTweak_RejectsInvalidInputs(t *testing.T) { + // (1) Garbage x-only key: bytes that are not a valid x-coordinate on + // the secp256k1 curve. schnorr.ParsePubKey rejects the input and + // TaprootTweak wraps it in "cannot parse taproot internal key". + var garbageKey [32]byte + for i := range garbageKey { + garbageKey[i] = 0xff + } + _, err := TaprootTweak(garbageKey, nil) + if err == nil { + t.Fatal( + "TaprootTweak accepted an invalid x-only key; " + + "expected an error from schnorr.ParsePubKey", + ) + } + + // (2) Tweak >= curve order: deterministic test fixture requires + // inverting the BIP-341 tap-tweak hash to produce an + // (internalKey, merkleRoot) pair whose tagged hash exceeds the + // secp256k1 group order. Skipped. + t.Skip( + "tweak >= curve order rejection requires inverting " + + "BIP-341 tap-tweak hash; covered by code review", + ) + + // (3) Infinity output: deterministic test fixture requires inverting + // the BIP-341 tweak to produce an internal key + tweak that sum to + // the point at infinity. Skipped. + t.Skip( + "infinity-output rejection requires inverting " + + "BIP-341 tweak; covered by code review", + ) +} diff --git a/pkg/bitcoin/transaction_builder_test.go b/pkg/bitcoin/transaction_builder_test.go index ed80fc82aa..d5dbb13192 100644 --- a/pkg/bitcoin/transaction_builder_test.go +++ b/pkg/bitcoin/transaction_builder_test.go @@ -1643,3 +1643,146 @@ func TestTransactionBuilder_ComputeSignatureHashesMissingPrevOut(t *testing.T) { t.Fatalf("unexpected error: [%v]", err) } } + +// TestTransactionBuilder_AddTaprootKeyPathSignatures_RejectsInvalid exercises +// the Verify gate and the wrong-counts branch in +// AddTaprootKeyPathSignatures (pkg/bitcoin/transaction_builder.go:525-592). +// Both branches are currently untested; a regression weakening the Verify +// check would ship a transaction with an invalid Schnorr signature undetected. +func TestTransactionBuilder_AddTaprootKeyPathSignatures_RejectsInvalid( + t *testing.T, +) { + localChain := newLocalChain() + builder := NewTransactionBuilder(localChain) + + privateKeyBytes := hexToSlice( + t, + "0101010101010101010101010101010101010101010101010101010101010101", + ) + privateKey, publicKey := btcec2.PrivKeyFromBytes(privateKeyBytes) + + var taprootOutputKey [32]byte + copy(taprootOutputKey[:], schnorr.SerializePubKey(publicKey)) + + inputScript, err := PayToTaproot(taprootOutputKey) + if err != nil { + t.Fatal(err) + } + + var outputPublicKeyHash [20]byte + copy( + outputPublicKeyHash[:], + hexToSlice(t, "0202020202020202020202020202020202020202"), + ) + outputScript, err := PayToWitnessPublicKeyHash(outputPublicKeyHash) + if err != nil { + t.Fatal(err) + } + + previousTransaction := &Transaction{ + Version: 1, + Inputs: []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash{ + 0x10, 0x11, 0x12, 0x13, + 0x14, 0x15, 0x16, 0x17, + 0x18, 0x19, 0x1a, 0x1b, + 0x1c, 0x1d, 0x1e, 0x1f, + 0x20, 0x21, 0x22, 0x23, + 0x24, 0x25, 0x26, 0x27, + 0x28, 0x29, 0x2a, 0x2b, + 0x2c, 0x2d, 0x2e, 0x2f, + }, + OutputIndex: 0, + }, + SignatureScript: []byte{0x51}, + Sequence: 0xffffffff, + }, + }, + Outputs: []*TransactionOutput{ + { + Value: 100000, + PublicKeyScript: inputScript, + }, + }, + Locktime: 0, + } + if err := localChain.addTransaction(previousTransaction); err != nil { + t.Fatal(err) + } + err = builder.AddTaprootKeyPathInput(&UnspentTransactionOutput{ + Outpoint: &TransactionOutpoint{ + TransactionHash: previousTransaction.Hash(), + OutputIndex: 0, + }, + Value: 100000, + }) + if err != nil { + t.Fatal(err) + } + builder.AddOutput(&TransactionOutput{ + Value: 90000, + PublicKeyScript: outputScript, + }) + + if _, err := builder.ComputeSignatureHashes(); err != nil { + t.Fatal(err) + } + + // Case (a): nil signatures container triggers the "wrong signatures + // count" branch (len(nil) == 0, len(inputs) == 1). + _, err = builder.AddTaprootKeyPathSignatures(nil) + if err == nil { + t.Fatal( + "expected 'wrong signatures count' for nil signatures, got nil", + ) + } + if !strings.Contains(err.Error(), "wrong signatures count") { + t.Fatalf( + "expected 'wrong signatures count' error, got: [%v]", + err, + ) + } + + // Case (b): Verify gate - produce a valid Schnorr signature over a + // different 32-byte message. schnorr.ParseSignature succeeds because + // the bytes form a structurally valid BIP-340 signature, but + // signature.Verify(sigHashBytes, taprootPublicKey) returns false + // because the signature was produced for a different hash. + wrongMessage := []byte{ + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, + } + badSignature, err := schnorr.Sign(privateKey, wrongMessage) + if err != nil { + t.Fatal(err) + } + badSignatureBytes := badSignature.Serialize() + + var badSignatureContainer [64]byte + copy(badSignatureContainer[:], badSignatureBytes) + + _, err = builder.AddTaprootKeyPathSignatures( + []*SchnorrSignatureContainer{ + { + Signature: badSignatureContainer, + }, + }, + ) + if err == nil { + t.Fatal( + "expected 'invalid taproot key-path signature' for non-" + + "verifying signature, got nil", + ) + } + if !strings.Contains(err.Error(), "invalid taproot key-path signature") { + t.Fatalf( + "expected 'invalid taproot key-path signature' error, "+ + "got: [%v]", + err, + ) + } +} From 15702337ed6bdd78f92cf157170f880cb0428ebc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Tue, 18 Aug 2026 09:30:42 +0000 Subject: [PATCH 06/10] =?UTF-8?q?fix(bitcoin):=20apply=20PR=20#4243=20revi?= =?UTF-8?q?ew=20fixes=20=E2=80=94=20Phase=202=20(trivial=20+=202=20subagen?= =?UTF-8?q?t=20waves)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- pkg/bitcoin/electrum/electrum.go | 36 ++++++++++++++++--------- pkg/bitcoin/taproot.go | 2 +- pkg/bitcoin/transaction_builder.go | 5 ++-- pkg/bitcoin/transaction_builder_test.go | 16 ++++++----- 4 files changed, 38 insertions(+), 21 deletions(-) diff --git a/pkg/bitcoin/electrum/electrum.go b/pkg/bitcoin/electrum/electrum.go index 661541f344..786242af9e 100644 --- a/pkg/bitcoin/electrum/electrum.go +++ b/pkg/bitcoin/electrum/electrum.go @@ -414,12 +414,7 @@ func (c *Connection) GetTransactionsForPublicKeyHash( return nil, err } - var selectedTxHashes []bitcoin.Hash - if len(txHashes) > limit { - selectedTxHashes = txHashes[len(txHashes)-limit:] - } else { - selectedTxHashes = txHashes - } + selectedTxHashes := selectLatestUniqueTxHashes(txHashes, limit) transactions := make([]*bitcoin.Transaction, len(selectedTxHashes)) for i, txHash := range selectedTxHashes { @@ -466,6 +461,9 @@ func selectLatestUniqueTxHashes( txHashes []bitcoin.Hash, limit int, ) []bitcoin.Hash { + if limit <= 0 { + return []bitcoin.Hash{} + } uniqueTxHashes := make([]bitcoin.Hash, 0, len(txHashes)) seen := make(map[bitcoin.Hash]bool) for _, txHash := range txHashes { @@ -515,7 +513,9 @@ func (c *Connection) GetTxHashesForPublicKeyHash( } // GetTxHashesForPublicKeyScripts gets hashes of confirmed transactions that -// pay to any of the given public key scripts. +// pay to any of the given public key scripts. The returned transactions +// hashes are ordered by block height in the ascending order, i.e. the +// latest transaction hash is at the end of the list. func (c *Connection) GetTxHashesForPublicKeyScripts( publicKeyScripts []bitcoin.Script, ) ([]bitcoin.Hash, error) { @@ -902,6 +902,7 @@ func (c *Connection) getScriptUtxosForScripts( confirmed bool, ) ([]*scriptUtxoItem, error) { items := make([]*scriptUtxoItem, 0) + seen := make(map[bitcoin.TransactionOutpoint]bool) for _, publicKeyScript := range publicKeyScripts { scriptItems, err := c.getScriptUtxos(publicKeyScript, confirmed) @@ -913,7 +914,17 @@ func (c *Connection) getScriptUtxosForScripts( ) } - items = append(items, scriptItems...) + for _, item := range scriptItems { + outpoint := bitcoin.TransactionOutpoint{ + TransactionHash: item.txHash, + OutputIndex: item.outputIndex, + } + if seen[outpoint] { + continue + } + seen[outpoint] = true + items = append(items, item) + } } return items, nil @@ -1294,7 +1305,7 @@ func (c *Connection) keepAlive() { } case <-c.parentCtx.Done(): ticker.Stop() - c.client.Shutdown() + c.currentClient().Shutdown() return } } @@ -1383,9 +1394,10 @@ func requestWithRetry[K interface{}]( // trip instead serialized every Electrum request in the process, so independent // lookups queued behind each other for a full network round trip each. // -// A reconnect may still swap the client immediately after this returns. The -// request then fails against the stale client and the retry wrapper reconnects and -// repeats it -- the same recovery path a reconnect landing mid-request always took. +// A reconnect can now swap the client immediately after this returns, while +// the caller's request is still in flight against the old client. That request +// will fail and requestWithRetry's retry loop reconnects and repeats it -- the +// same fallback the retry loop already uses for any other request failure. func (c *Connection) currentClient() *electrum.Client { c.clientMutex.RLock() defer c.clientMutex.RUnlock() diff --git a/pkg/bitcoin/taproot.go b/pkg/bitcoin/taproot.go index b3d44867f5..16e2fde5ad 100644 --- a/pkg/bitcoin/taproot.go +++ b/pkg/bitcoin/taproot.go @@ -11,7 +11,7 @@ import ( const taprootBaseLeafVersion = 0xc0 -// TaprootLeafHash computes the BIP-342 TapLeaf hash for a base-version script. +// TaprootLeafHash computes the BIP-341 TapLeaf hash for a base-version script. func TaprootLeafHash(script Script) ([32]byte, error) { var buffer bytes.Buffer diff --git a/pkg/bitcoin/transaction_builder.go b/pkg/bitcoin/transaction_builder.go index 551d9f86b0..5912db6978 100644 --- a/pkg/bitcoin/transaction_builder.go +++ b/pkg/bitcoin/transaction_builder.go @@ -328,6 +328,7 @@ func (tb *TransactionBuilder) getScript( // AddOutput adds a new transaction's output. func (tb *TransactionBuilder) AddOutput(output *TransactionOutput) { tb.internal.AddTxOut(wire.NewTxOut(output.Value, output.PublicKeyScript)) + tb.sigHashes = nil } // ComputeSignatureHashes computes the signature hashes for all transaction @@ -459,7 +460,7 @@ func (tb *TransactionBuilder) AddSignatures( // Make a sanity check to avoid producing crap transactions. if !ecdsa.Verify( signature.PublicKey, - tb.sigHashes[i].Bytes(), + tb.sigHashes[i].FillBytes(make([]byte, sha256.Size)), signature.R, signature.S, ) { @@ -826,7 +827,7 @@ func (tb *TransactionBuilder) calcTaprootKeyPathSignatureHash( return nil, err } - hash := chainhash.TaggedHash([]byte("TapSighash"), sigMsg.Bytes()) + hash := chainhash.TaggedHash(chainhash.TagTapSighash, sigMsg.Bytes()) return hash.CloneBytes(), nil } diff --git a/pkg/bitcoin/transaction_builder_test.go b/pkg/bitcoin/transaction_builder_test.go index d5dbb13192..21f39c8c1f 100644 --- a/pkg/bitcoin/transaction_builder_test.go +++ b/pkg/bitcoin/transaction_builder_test.go @@ -931,8 +931,12 @@ func TestTransactionBuilder_ReplaceUnsignedTransaction(t *testing.T) { var replacementInputHash1 chainhash.Hash var replacementInputHash2 chainhash.Hash - replacementInputHash1[0] = 0x33 - replacementInputHash2[0] = 0x44 + // Preserve the outpoints that the builder was initialized with so the + // replacement's per-index PreviousOutPoint matches the prior builder state + // (transaction_builder.go now binds the replacement's outpoints and TxOut + // set to the builder's prior state before rebinding tb.internal). + replacementInputHash1 = initialInputHash1 + replacementInputHash2 = initialInputHash2 err := builder.ReplaceUnsignedTransaction( &Transaction{ @@ -941,14 +945,14 @@ func TestTransactionBuilder_ReplaceUnsignedTransaction(t *testing.T) { { Outpoint: &TransactionOutpoint{ TransactionHash: Hash(replacementInputHash1), - OutputIndex: 7, + OutputIndex: 1, }, Sequence: 0xffffffff, }, { Outpoint: &TransactionOutpoint{ TransactionHash: Hash(replacementInputHash2), - OutputIndex: 8, + OutputIndex: 2, }, Sequence: 0xffffffff, }, @@ -1001,11 +1005,11 @@ func TestTransactionBuilder_ReplaceUnsignedTransaction(t *testing.T) { t.Fatalf("unexpected input count after replacement: [%d]", len(inputs)) } - if inputs[0].TxIDHex != replacementInputHash1.String() || inputs[0].Vout != 7 { + if inputs[0].TxIDHex != initialInputHash1.String() || inputs[0].Vout != 1 { t.Fatalf("unexpected first input after replacement: [%+v]", inputs[0]) } - if inputs[1].TxIDHex != replacementInputHash2.String() || inputs[1].Vout != 8 { + if inputs[1].TxIDHex != initialInputHash2.String() || inputs[1].Vout != 2 { t.Fatalf("unexpected second input after replacement: [%+v]", inputs[1]) } From 9d0d9ec0a0b56fbdbc485fd5e8df860814f8ff09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Tue, 18 Aug 2026 10:03:10 +0000 Subject: [PATCH 07/10] =?UTF-8?q?fix(bitcoin):=20apply=20PR=20#4243=20revi?= =?UTF-8?q?ew=20fixes=20=E2=80=94=20Phase=202=20substantive=20edits?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Direct edits to transaction_builder.go: - P3 TAPSIIGHASH-TAG: use chainhash.TagTapSighash constant instead of the []byte("TapSighash") literal (line 884). - P2 STALE-SIGHASH-CACHE: AddOutput now invalidates cached sighashes so post-computation mutations force recomputation (line 331). - P2 DIGEST-WIRE-FORMAT: ECDSA verification reads digests via FillBytes(make([]byte, sha256.Size)) so both ECDSA and Taproot paths agree on a fixed-width 32-byte digest wire format (line 469). - P1 REPLACE-TX: ReplaceUnsignedTransaction now binds the replacement to the builder's prior state — per-index PreviousOutPoint equality + TxOut set equality (when outputs were committed) before restoring pre-signing witness/signature-script. Prevents a caller-controlled replacement from redirecting funds or mis-aligning sigHashArgs[i] with a reordered tx.TxIn[i], producing a self-consistent-but-wrong digest that survives the local Verify gate and broadcasts an unintended transaction. - P2 P2TR-BACKCOMPAT: hoist the HasTaprootKeyPathInputs guard above the per-input loop in AddSignatures so a mixed transaction's preceding inputs are not mutated before the rejection returns. - Doc item (5): FROST-migration scope note tightened to a state invariant — multi-element pre-signing witnesses are not supported by the restore path; refuse rather than silently drop witness data. - Doc item (6): ReplaceUnsignedTransaction doc now reflects the binding checks + witness restore. - Doc item (9): AddTaprootKeyPathSignatures doc notes the verify-before-accept behavior. Refs PR #4243 review findings M-TAPSIIGHASH-TAG, M-STALE-SIGHASH-CACHE, M-DIGEST-WIRE-FORMAT, M-P2TR-BACKCOMPAT, M-REPLACE-TX. --- pkg/bitcoin/transaction_builder.go | 75 ++++++++++++++++++++++++++---- 1 file changed, 66 insertions(+), 9 deletions(-) diff --git a/pkg/bitcoin/transaction_builder.go b/pkg/bitcoin/transaction_builder.go index 5912db6978..10cc471f5f 100644 --- a/pkg/bitcoin/transaction_builder.go +++ b/pkg/bitcoin/transaction_builder.go @@ -446,6 +446,15 @@ func (tb *TransactionBuilder) AddSignatures( return nil, fmt.Errorf("wrong signatures count") } + // Hoist the P2TR guard above the per-input loop so that, for a mixed + // transaction, the loop does not mutate Witness/SignatureScript on + // preceding inputs before erroring out. The per-input guard below is + // kept as a defense-in-depth check. + if tb.HasTaprootKeyPathInputs() { + return nil, fmt.Errorf( + "transaction has P2TR inputs; use AddTaprootKeyPathSignatures", + ) + } for i, input := range tb.internal.TxIn { signature := signatures[i] sigHashArgs := tb.sigHashArgs[i] @@ -524,7 +533,9 @@ type SchnorrSignatureContainer struct { } // AddTaprootKeyPathSignatures adds Schnorr signature data for P2TR key-path -// transaction inputs and returns a signed Transaction instance. +// transaction inputs and returns a signed Transaction instance. Each signature +// is verified against the corresponding input's sighash and an error is +// produced if any signature is invalid. func (tb *TransactionBuilder) AddTaprootKeyPathSignatures( signatures []*SchnorrSignatureContainer, ) (*Transaction, error) { @@ -605,6 +616,12 @@ func (tb *TransactionBuilder) TotalInputsValue() int64 { // ReplaceUnsignedTransaction replaces the internal unsigned transaction while // preserving per-input sighash metadata collected during builder input setup. +// It also validates that the replacement's inputs carry no pre-existing signature +// data, that each replacement input's PreviousOutPoint matches the prior input +// at the same index, that the TxOut set matches the prior builder state (when +// outputs had been committed), and restores each input's pre-signing witness +// or signature-script from the previous builder state; replacement inputs whose +// previous witness had more than one element are rejected. func (tb *TransactionBuilder) ReplaceUnsignedTransaction( transaction *Transaction, ) error { @@ -621,10 +638,53 @@ func (tb *TransactionBuilder) ReplaceUnsignedTransaction( } previousInputs := tb.internal.TxIn + previousOutputs := append([]*wire.TxOut{}, tb.internal.TxOut...) replacedInternal := newInternalTransaction() replacedInternal.fromTransaction(transaction) + // Bind the replacement to the builder's prior state: per-index PreviousOutPoint + // must match, and TxOut set must match when the builder had committed to + // outputs. Without these checks, a caller-controlled replacement could redirect + // funds (different TxOut set) or misalign sigHashArgs[i] with the new-order + // tx.TxIn[i], producing a self-consistent-but-wrong digest that survives the + // local Verify gate and broadcasts an unintended transaction. + for i, prevIn := range previousInputs { + replIn := replacedInternal.TxIn[i] + if replIn.PreviousOutPoint != prevIn.PreviousOutPoint { + return fmt.Errorf( + "replacement input [%d] PreviousOutPoint differs from builder state", + i, + ) + } + } + if len(previousOutputs) > 0 { + if len(replacedInternal.TxOut) != len(previousOutputs) { + return fmt.Errorf( + "replacement TxOut set has [%d] entries; builder state has [%d]", + len(replacedInternal.TxOut), + len(previousOutputs), + ) + } + for i, prevOut := range previousOutputs { + replOut := replacedInternal.TxOut[i] + if replOut.Value != prevOut.Value { + return fmt.Errorf( + "replacement TxOut [%d] value [%d] differs from builder state [%d]", + i, + replOut.Value, + prevOut.Value, + ) + } + if !bytes.Equal(replOut.PkScript, prevOut.PkScript) { + return fmt.Errorf( + "replacement TxOut [%d] PkScript differs from builder state", + i, + ) + } + } + } + for i := range replacedInternal.TxIn { previousInput := previousInputs[i] replacedInput := replacedInternal.TxIn[i] @@ -653,13 +713,11 @@ func (tb *TransactionBuilder) ReplaceUnsignedTransaction( if tb.sigHashArgs[i].witness { // Witness inputs may carry a single-element pre-signing witness // that holds a P2WSH-style redeem script. Multi-element witnesses - // belong to P2TR script-path spends or other workflows that are - // not in scope for the current FROST migration, and silently - // dropping them produced malformed transactions later — refuse - // instead so the unsupported case fails loudly. Lifting this to - // support multi-element witnesses requires a per-input policy - // rather than a blanket copy because the replacement could - // legitimately differ in witness shape from the previous input. + // belong to P2TR script-path spends or other workflows. Multi- + // element witnesses are not supported by this restore path: refuse + // rather than silently drop pre-signing witness data, which would + // produce a malformed transaction. + switch len(previousInput.Witness) { case 0: // Nothing to restore (typical P2TR key-path or P2WPKH). @@ -826,7 +884,6 @@ func (tb *TransactionBuilder) calcTaprootKeyPathSignatureHash( ); err != nil { return nil, err } - hash := chainhash.TaggedHash(chainhash.TagTapSighash, sigMsg.Bytes()) return hash.CloneBytes(), nil } From 948ed27337444a6abb2abbd9c7310c20fdbf4932 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Tue, 18 Aug 2026 16:18:37 +0000 Subject: [PATCH 08/10] fix(bitcoin): replace hand-rolled BIP-341 sighash with txscript.CalcTaprootSignatureHash Replaces the hand-rolled midstate-and-preimage BIP-341 sighash implementation in ComputeSignatureHashes with btcd's well-tested txscript.CalcTaprootSignatureHash, eliminating the consensus-critical duplication of the spec. The taproot_sighash_differential_test that previously proved agreement between the two implementations is removed, since the implementation under test no longer exists. Coverage moves to a new subtest under TestTransactionBuilder_AddTaprootKeyPathSignatures that exercises non-default sequences and outpoint indices against a reference transaction assembled independently of the builder. --- .../taproot_sighash_differential_test.go | 203 --------------- pkg/bitcoin/transaction_builder.go | 244 +----------------- pkg/bitcoin/transaction_builder_test.go | 145 +++++++++++ 3 files changed, 149 insertions(+), 443 deletions(-) delete mode 100644 pkg/bitcoin/taproot_sighash_differential_test.go diff --git a/pkg/bitcoin/taproot_sighash_differential_test.go b/pkg/bitcoin/taproot_sighash_differential_test.go deleted file mode 100644 index 97268d226f..0000000000 --- a/pkg/bitcoin/taproot_sighash_differential_test.go +++ /dev/null @@ -1,203 +0,0 @@ -package bitcoin - -import ( - "bytes" - "encoding/binary" - "math/rand/v2" - "testing" - - "github.com/btcsuite/btcd/txscript" -) - -// fillRandom fills b with pseudo-random bytes from rng. math/rand/v2 drops the -// Read method that math/rand exposed on *Rand, and these bytes only need to vary -// across iterations, never to be unpredictable. -func fillRandom(rng *rand.Rand, b []byte) { - var block [8]byte - for offset := 0; offset < len(b); offset += len(block) { - binary.LittleEndian.PutUint64(block[:], rng.Uint64()) - copy(b[offset:], block[:]) - } -} - -// TestTaprootKeyPathSigHashMatchesTxscript is a differential test of the -// hand-rolled BIP-341 key-path signature hash against btcd's -// txscript.CalcTaprootSignatureHash over randomized multi-input transactions. -// -// calcTaprootKeyPathSignatureHash assembles the SigMsg preimage by hand -- epoch, -// hash type, version, locktime, the five midstate hashes, spend type and input -// index -- and a single mismatched byte or a misordered field produces a digest -// that signs a transaction the wallet did not intend. That is the fund-loss class, -// and a hardcoded vector cannot catch it because the vector fixes exactly one -// input count, one ordering and one output shape. -// -// The randomization deliberately varies what the midstate commits to: input count -// and ordering, per-input values, sequences, script types (so P2TR inputs sit at -// arbitrary indices in a mixed transaction), output count and values, version and -// locktime. Every P2TR input of every generated transaction must agree with -// txscript byte for byte. -// -// The seed is fixed so failures reproduce exactly. -func TestTaprootKeyPathSigHashMatchesTxscript(t *testing.T) { - const ( - iterations = 200 - maxInputs = 5 - maxOutputs = 4 - fundingValue = 1_000_000 - ) - - rng := rand.New(rand.NewPCG(20260813, 0x9E3779B97F4A7C15)) - - for iteration := 0; iteration < iterations; iteration++ { - localChain := newLocalChain() - builder := NewTransactionBuilder(localChain) - - inputCount := 1 + rng.IntN(maxInputs) - taprootInputs := make([]int, 0, inputCount) - - for i := 0; i < inputCount; i++ { - // Mixed script types put P2TR inputs at arbitrary indices. The midstate - // commits to every input's amount and script regardless of type, so a - // non-Taproot input at a lower index must still be folded in correctly. - isTaproot := rng.IntN(2) == 0 || (i == inputCount-1 && len(taprootInputs) == 0) - - var ( - lockingScript Script - err error - ) - if isTaproot { - var outputKey [32]byte - fillRandom(rng, outputKey[:]) - lockingScript, err = PayToTaproot(outputKey) - } else { - var publicKeyHash [20]byte - fillRandom(rng, publicKeyHash[:]) - lockingScript, err = PayToWitnessPublicKeyHash(publicKeyHash) - } - if err != nil { - t.Fatalf("iteration %d input %d: %v", iteration, i, err) - } - - // Vary the value per input: the midstate's hashInputAmounts commits to - // all of them in input order. - value := int64(1 + rng.IntN(fundingValue)) - - var previousHash Hash - previousHash[0] = byte(iteration) - previousHash[1] = byte(i) - - fundingTransaction := &Transaction{ - Version: 1, - Inputs: []*TransactionInput{ - { - Outpoint: &TransactionOutpoint{ - TransactionHash: previousHash, - OutputIndex: 0, - }, - SignatureScript: []byte{0x51}, - Sequence: 0xffffffff, - }, - }, - Outputs: []*TransactionOutput{ - { - Value: value, - PublicKeyScript: lockingScript, - }, - }, - Locktime: 0, - } - if err := localChain.addTransaction(fundingTransaction); err != nil { - t.Fatalf("iteration %d input %d: %v", iteration, i, err) - } - - utxo := &UnspentTransactionOutput{ - Outpoint: &TransactionOutpoint{ - TransactionHash: fundingTransaction.Hash(), - OutputIndex: 0, - }, - Value: value, - } - - if isTaproot { - if err := builder.AddTaprootKeyPathInput(utxo); err != nil { - t.Fatalf("iteration %d input %d: %v", iteration, i, err) - } - taprootInputs = append(taprootInputs, i) - } else { - if err := builder.AddPublicKeyHashInput(utxo); err != nil { - t.Fatalf("iteration %d input %d: %v", iteration, i, err) - } - } - } - - // Vary the outputs: hashOutputs commits to every value and script. - outputCount := 1 + rng.IntN(maxOutputs) - for i := 0; i < outputCount; i++ { - var publicKeyHash [20]byte - fillRandom(rng, publicKeyHash[:]) - outputScript, err := PayToWitnessPublicKeyHash(publicKeyHash) - if err != nil { - t.Fatalf("iteration %d output %d: %v", iteration, i, err) - } - builder.AddOutput(&TransactionOutput{ - Value: int64(1 + rng.IntN(fundingValue/2)), - PublicKeyScript: outputScript, - }) - } - - // Version and locktime are both in the preimage, ahead of the midstate - // hashes, so a field-order error shows up as soon as they are not their - // default values. - builder.internal.Version = int32(1 + rng.IntN(2)) - builder.internal.LockTime = uint32(rng.IntN(500_000)) - - sigHashes, err := builder.ComputeSignatureHashes() - if err != nil { - t.Fatalf("iteration %d: compute sighashes: %v", iteration, err) - } - - // The reference implementation, given the identical transaction and the - // identical previous outputs. - reference := txscript.NewTxSigHashes(builder.internal.MsgTx, builder.prevOuts) - - for _, inputIndex := range taprootInputs { - expected, err := txscript.CalcTaprootSignatureHash( - reference, - txscript.SigHashDefault, - builder.internal.MsgTx, - inputIndex, - builder.prevOuts, - ) - if err != nil { - t.Fatalf( - "iteration %d input %d: txscript reference: %v", - iteration, - inputIndex, - err, - ) - } - - // ComputeSignatureHashes returns big.Int, which drops leading zero - // bytes; FillBytes restores the fixed 32-byte digest. - actual := sigHashes[inputIndex].FillBytes(make([]byte, 32)) - - if !bytes.Equal(expected, actual) { - t.Fatalf( - "iteration %d input %d of %d (taproot inputs %v, outputs %d, "+ - "version %d, locktime %d):\n"+ - "txscript: %x\n"+ - "builder: %x", - iteration, - inputIndex, - inputCount, - taprootInputs, - outputCount, - builder.internal.Version, - builder.internal.LockTime, - expected, - actual, - ) - } - } - } -} diff --git a/pkg/bitcoin/transaction_builder.go b/pkg/bitcoin/transaction_builder.go index 10cc471f5f..618b157b80 100644 --- a/pkg/bitcoin/transaction_builder.go +++ b/pkg/bitcoin/transaction_builder.go @@ -4,7 +4,6 @@ import ( "bytes" "crypto/ecdsa" "crypto/sha256" - "encoding/binary" "encoding/hex" "fmt" "math/big" @@ -360,20 +359,6 @@ func (tb *TransactionBuilder) ComputeSignatureHashes() ([]*big.Int, error) { tb.prevOuts, ) - var taprootSigHashMidstate *taprootSignatureHashMidstate - if tb.HasTaprootKeyPathInputs() { - var err error - taprootSigHashMidstate, err = tb.taprootSignatureHashMidstate( - tb.internal.MsgTx, - ) - if err != nil { - return nil, fmt.Errorf( - "cannot calculate taproot sighash midstate: [%v]", - err, - ) - } - } - for i := range tb.internal.TxIn { sigHashArgs := tb.sigHashArgs[i] @@ -382,10 +367,12 @@ func (tb *TransactionBuilder) ComputeSignatureHashes() ([]*big.Int, error) { switch sigHashArgs.scriptType { case P2TRScript: - sigHashBytes, err = tb.calcTaprootKeyPathSignatureHash( + sigHashBytes, err = txscript.CalcTaprootSignatureHash( + witnessSigHashFragments, + txscript.SigHashDefault, tb.internal.MsgTx, i, - taprootSigHashMidstate, + tb.prevOuts, ) case P2WPKHScript, P2WSHScript: sigHashBytes, err = txscript.CalcWitnessSigHash( @@ -824,229 +811,6 @@ func (tb *TransactionBuilder) UnsignedTransactionIO() ( return inputs, outputs, nil } -func (tb *TransactionBuilder) calcTaprootKeyPathSignatureHash( - tx *wire.MsgTx, - inputIndex int, - midstate *taprootSignatureHashMidstate, -) ([]byte, error) { - if tx == nil { - return nil, fmt.Errorf("transaction is nil") - } - if midstate == nil { - return nil, fmt.Errorf("taproot sighash midstate is nil") - } - - if inputIndex < 0 || inputIndex >= len(tx.TxIn) { - return nil, fmt.Errorf( - "input index [%d] out of range for [%d] inputs", - inputIndex, - len(tx.TxIn), - ) - } - - if len(tx.TxIn) != len(tb.sigHashArgs) { - return nil, fmt.Errorf( - "input metadata mismatch: [%d] tx inputs, [%d] sighash args", - len(tx.TxIn), - len(tb.sigHashArgs), - ) - } - - var sigMsg bytes.Buffer - - // BIP-341 defines the final digest as tagged_hash("TapSighash", - // 0x00 || SigMsg(0x00, 0)). The first byte is the epoch and the second - // byte is SIGHASH_DEFAULT. - sigMsg.WriteByte(0x00) - sigMsg.WriteByte(0x00) - - if err := binary.Write(&sigMsg, binary.LittleEndian, tx.Version); err != nil { - return nil, err - } - if err := binary.Write(&sigMsg, binary.LittleEndian, tx.LockTime); err != nil { - return nil, err - } - - sigMsg.Write(midstate.hashPrevOuts[:]) - sigMsg.Write(midstate.hashInputAmounts[:]) - sigMsg.Write(midstate.hashInputScripts[:]) - sigMsg.Write(midstate.hashSequences[:]) - sigMsg.Write(midstate.hashOutputs[:]) - - // Key-path spends use ext_flag=0 and this implementation does not attach - // a Taproot annex, so spend_type is 0. - sigMsg.WriteByte(0x00) - - if err := binary.Write( - &sigMsg, - binary.LittleEndian, - uint32(inputIndex), - ); err != nil { - return nil, err - } - hash := chainhash.TaggedHash(chainhash.TagTapSighash, sigMsg.Bytes()) - return hash.CloneBytes(), nil -} - -type taprootSignatureHashMidstate struct { - hashPrevOuts [chainhash.HashSize]byte - hashInputAmounts [chainhash.HashSize]byte - hashInputScripts [chainhash.HashSize]byte - hashSequences [chainhash.HashSize]byte - hashOutputs [chainhash.HashSize]byte -} - -func (tb *TransactionBuilder) taprootSignatureHashMidstate( - tx *wire.MsgTx, -) (*taprootSignatureHashMidstate, error) { - if tx == nil { - return nil, fmt.Errorf("transaction is nil") - } - - if len(tx.TxIn) != len(tb.sigHashArgs) { - return nil, fmt.Errorf( - "input metadata mismatch: [%d] tx inputs, [%d] sighash args", - len(tx.TxIn), - len(tb.sigHashArgs), - ) - } - - hashPrevOuts, err := tb.taprootHashPrevOuts(tx) - if err != nil { - return nil, err - } - - hashInputAmounts, err := tb.taprootHashInputAmounts() - if err != nil { - return nil, err - } - - hashInputScripts, err := tb.taprootHashInputScripts() - if err != nil { - return nil, err - } - - hashSequences, err := tb.taprootHashSequences(tx) - if err != nil { - return nil, err - } - - hashOutputs, err := tb.taprootHashOutputs(tx) - if err != nil { - return nil, err - } - - return &taprootSignatureHashMidstate{ - hashPrevOuts: hashPrevOuts, - hashInputAmounts: hashInputAmounts, - hashInputScripts: hashInputScripts, - hashSequences: hashSequences, - hashOutputs: hashOutputs, - }, nil -} - -func (tb *TransactionBuilder) taprootHashPrevOuts( - tx *wire.MsgTx, -) ([chainhash.HashSize]byte, error) { - var buffer bytes.Buffer - for _, input := range tx.TxIn { - if err := writeOutPoint(&buffer, &input.PreviousOutPoint); err != nil { - return [chainhash.HashSize]byte{}, err - } - } - - return chainhash.HashH(buffer.Bytes()), nil -} - -func (tb *TransactionBuilder) taprootHashInputAmounts() ( - [chainhash.HashSize]byte, - error, -) { - var buffer bytes.Buffer - for i, sigHashArgs := range tb.sigHashArgs { - if sigHashArgs.value < 0 { - return [chainhash.HashSize]byte{}, fmt.Errorf( - "input [%d] value is negative", - i, - ) - } - - if err := binary.Write( - &buffer, - binary.LittleEndian, - uint64(sigHashArgs.value), - ); err != nil { - return [chainhash.HashSize]byte{}, err - } - } - - return chainhash.HashH(buffer.Bytes()), nil -} - -func (tb *TransactionBuilder) taprootHashInputScripts() ( - [chainhash.HashSize]byte, - error, -) { - var buffer bytes.Buffer - for i, sigHashArgs := range tb.sigHashArgs { - if err := wire.WriteVarBytes( - &buffer, - 0, - sigHashArgs.publicKeyScript, - ); err != nil { - return [chainhash.HashSize]byte{}, fmt.Errorf( - "cannot write public key script for input [%d]: [%v]", - i, - err, - ) - } - } - - return chainhash.HashH(buffer.Bytes()), nil -} - -func (tb *TransactionBuilder) taprootHashSequences( - tx *wire.MsgTx, -) ([chainhash.HashSize]byte, error) { - var buffer bytes.Buffer - for _, input := range tx.TxIn { - if err := binary.Write( - &buffer, - binary.LittleEndian, - input.Sequence, - ); err != nil { - return [chainhash.HashSize]byte{}, err - } - } - - return chainhash.HashH(buffer.Bytes()), nil -} - -func (tb *TransactionBuilder) taprootHashOutputs( - tx *wire.MsgTx, -) ([chainhash.HashSize]byte, error) { - var buffer bytes.Buffer - for i, output := range tx.TxOut { - if err := wire.WriteTxOut(&buffer, 0, 0, output); err != nil { - return [chainhash.HashSize]byte{}, fmt.Errorf( - "cannot write output [%d]: [%v]", - i, - err, - ) - } - } - - return chainhash.HashH(buffer.Bytes()), nil -} - -func writeOutPoint(buffer *bytes.Buffer, outpoint *wire.OutPoint) error { - if _, err := buffer.Write(outpoint.Hash[:]); err != nil { - return err - } - - return binary.Write(buffer, binary.LittleEndian, outpoint.Index) -} - // inputSigHashArgs is a helper structure holding some arguments required to // compute a sighash for the given input. type inputSigHashArgs struct { diff --git a/pkg/bitcoin/transaction_builder_test.go b/pkg/bitcoin/transaction_builder_test.go index 21f39c8c1f..864dd6f932 100644 --- a/pkg/bitcoin/transaction_builder_test.go +++ b/pkg/bitcoin/transaction_builder_test.go @@ -10,6 +10,7 @@ import ( btcec2 "github.com/btcsuite/btcd/btcec/v2" "github.com/btcsuite/btcd/btcec/v2/schnorr" "github.com/btcsuite/btcd/chaincfg/chainhash" + "github.com/btcsuite/btcd/txscript" "github.com/btcsuite/btcd/wire" "github.com/keep-network/keep-core/internal/testutils" ) @@ -689,6 +690,150 @@ func TestTransactionBuilder_AddTaprootKeyPathSignatures(t *testing.T) { transaction.Inputs[0].Witness[0], ) testutils.AssertBytesEqual(t, nil, transaction.Inputs[0].SignatureScript) + + // The hardcoded vector above pins a transaction shape where both the + // input sequence (0xffffffff) and the spent outpoint index (0) are + // byte-order-symmetric constants, so it cannot detect an endianness or + // width regression in those two committed sighash fields. This subtest + // covers them with non-default values and compares against a reference + // transaction assembled independently of the builder, so the assertion + // also covers what the builder committed to (per-input sequences, + // outpoint indices, values and scripts), not only the digest arithmetic. + t.Run("non-default sequences and outpoint indices", func(t *testing.T) { + localChain := newLocalChain() + builder := NewTransactionBuilder(localChain) + + makeFundingTransaction := func(marker byte) *Transaction { + outputs := make([]*TransactionOutput, 3) + for i := range outputs { + outputs[i] = &TransactionOutput{ + Value: int64(50000 + 1000*i), + PublicKeyScript: inputScript, + } + } + + return &Transaction{ + Version: 1, + Inputs: []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash{marker}, + OutputIndex: 0, + }, + SignatureScript: []byte{0x51}, + Sequence: 0xffffffff, + }, + }, + Outputs: outputs, + Locktime: 0, + } + } + + firstFunding := makeFundingTransaction(0x31) + secondFunding := makeFundingTransaction(0x32) + + for _, fundingTransaction := range []*Transaction{ + firstFunding, + secondFunding, + } { + if err := localChain.addTransaction(fundingTransaction); err != nil { + t.Fatal(err) + } + } + + type spentOutput struct { + transactionHash Hash + outputIndex uint32 + value int64 + sequence uint32 + } + + spentOutputs := []spentOutput{ + {firstFunding.Hash(), 2, 52000, 0xfffffffd}, + {secondFunding.Hash(), 1, 51000, 0x12345678}, + } + + for _, spent := range spentOutputs { + if err := builder.AddTaprootKeyPathInput( + &UnspentTransactionOutput{ + Outpoint: &TransactionOutpoint{ + TransactionHash: spent.transactionHash, + OutputIndex: spent.outputIndex, + }, + Value: spent.value, + }, + ); err != nil { + t.Fatal(err) + } + } + + // The builder always creates inputs with the default sequence so the + // non-default values must be applied to the assembled transaction. + for i, spent := range spentOutputs { + builder.internal.TxIn[i].Sequence = spent.sequence + } + + builder.AddOutput(&TransactionOutput{ + Value: 40000, + PublicKeyScript: outputScript, + }) + builder.AddOutput(&TransactionOutput{ + Value: 60000, + PublicKeyScript: inputScript, + }) + + sigHashes, err := builder.ComputeSignatureHashes() + if err != nil { + t.Fatal(err) + } + + testutils.AssertIntsEqual( + t, + "signature hashes count", + len(spentOutputs), + len(sigHashes), + ) + + reference := wire.NewMsgTx(1) + referencePrevOuts := make(map[wire.OutPoint]*wire.TxOut) + + for _, spent := range spentOutputs { + hash := chainhash.Hash(spent.transactionHash) + outpoint := wire.NewOutPoint(&hash, spent.outputIndex) + input := wire.NewTxIn(outpoint, nil, nil) + input.Sequence = spent.sequence + reference.AddTxIn(input) + referencePrevOuts[*outpoint] = wire.NewTxOut( + spent.value, + inputScript, + ) + } + + reference.AddTxOut(wire.NewTxOut(40000, outputScript)) + reference.AddTxOut(wire.NewTxOut(60000, inputScript)) + + fetcher := txscript.NewMultiPrevOutFetcher(referencePrevOuts) + fragments := txscript.NewTxSigHashes(reference, fetcher) + + for i := range spentOutputs { + expected, err := txscript.CalcTaprootSignatureHash( + fragments, + txscript.SigHashDefault, + reference, + i, + fetcher, + ) + if err != nil { + t.Fatal(err) + } + + testutils.AssertBytesEqual( + t, + expected, + sigHashes[i].FillBytes(make([]byte, 32)), + ) + } + }) } func TestTransactionBuilder_AddTaprootKeyPathSignatures_MultipleInputs( From afb9eb1519b709a5fb4a80b12261b221c2797112 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Tue, 18 Aug 2026 16:21:57 +0000 Subject: [PATCH 09/10] fix(bitcoin): enforce input/output consistency on mutators and ReplaceUnsignedTransaction Three latent consistency gaps that the prior review surfaced but Phase 1 did not close: - addDirectKeySpendInput and AddScriptHashInput now refuse to append a P2TR input to a builder holding only non-P2TR inputs (or vice versa), via the new assertUniformTaprootShape helper. The signing paths refuse such mixed shapes but only after the entire input vector is built, so a typo in one Add call previously surfaced as a confusing signing-time error rather than an immediate construction-time refusal. - Both input mutators now reset the cached signature hashes, mirroring the behavior AddOutput already had. Storing a stale digest after a later Add call would silently produce a self-consistent-but-wrong signature. - ReplaceUnsignedTransaction now binds the replacement's TxOut set unconditionally rather than only when the builder had previously committed outputs. A replacement that drops or changes outputs after the builder had committed to them is now rejected with an explicit error instead of producing a transaction whose sigHashArgs[i] no longer matches its rebuilt TxOut set. Coverage: TestTransactionBuilder_RejectsMixedP2TRNonP2TR covers the construction-time rejection in both directions; TestTransactionBuilder_AddOutput now also seeds and asserts the sigHashes reset; TestTransactionBuilder_ReplaceUnsignedTransaction_Rejects{Outpoint, TxOutValue,TxOutScript,OutputCount}Mismatch pin each rejection branch. --- pkg/bitcoin/transaction_builder.go | 92 ++++--- pkg/bitcoin/transaction_builder_test.go | 315 +++++++++++++++++++++++- 2 files changed, 376 insertions(+), 31 deletions(-) diff --git a/pkg/bitcoin/transaction_builder.go b/pkg/bitcoin/transaction_builder.go index 618b157b80..5ae15a0b54 100644 --- a/pkg/bitcoin/transaction_builder.go +++ b/pkg/bitcoin/transaction_builder.go @@ -204,12 +204,42 @@ func (tb *TransactionBuilder) TaprootKeyPathInputMerkleRoots() []*[32]byte { return merkleRoots } +// assertUniformTaprootShape rejects transaction shapes that mix P2TR inputs +// with non-P2TR inputs. Such a mix is not supported by the signing paths: +// AddSignatures refuses builders holding any P2TR input and +// AddTaprootKeyPathSignatures requires all inputs to be P2TR. Catching the mix +// at construction time avoids discovering it only after a completed +// distributed signing round. +func (tb *TransactionBuilder) assertUniformTaprootShape( + scriptType ScriptType, +) error { + isTaproot := scriptType == P2TRScript + + for i, existing := range tb.sigHashArgs { + if (existing.scriptType == P2TRScript) != isTaproot { + return fmt.Errorf( + "cannot add [%v] input: mixed P2TR and non-P2TR inputs are "+ + "not supported; existing input [%d] is [%v]", + scriptType, + i, + existing.scriptType, + ) + } + } + + return nil +} + func (tb *TransactionBuilder) addDirectKeySpendInput( utxo *UnspentTransactionOutput, utxoScript Script, scriptType ScriptType, taprootMerkleRoot *[32]byte, ) error { + if err := tb.assertUniformTaprootShape(scriptType); err != nil { + return err + } + // The UTXO was locked using a direct key-spend script, so the scriptCode // required to build the sighash is equivalent to that script. Worth noting // that the P2WPKH script is actually converted to the P2PKH script when @@ -234,6 +264,8 @@ func (tb *TransactionBuilder) addDirectKeySpendInput( tb.internal.AddTxIn(wire.NewTxIn(outpoint, nil, nil)) tb.sigHashArgs = append(tb.sigHashArgs, sigHashArgs) + // Adding an input invalidates any previously computed signature hashes. + tb.sigHashes = nil return nil } @@ -263,6 +295,10 @@ func (tb *TransactionBuilder) AddScriptHashInput( ) } + if err := tb.assertUniformTaprootShape(scriptType); err != nil { + return err + } + // The UTXO was locked using a P2SH/P2WSH script so, the scriptCode required // to build the sighash is equivalent to the plain-text redeem script whose // hash is included in the P2SH/P2WSH script. @@ -291,6 +327,8 @@ func (tb *TransactionBuilder) AddScriptHashInput( } tb.sigHashArgs = append(tb.sigHashArgs, sigHashArgs) + // Adding an input invalidates any previously computed signature hashes. + tb.sigHashes = nil return nil } @@ -605,10 +643,10 @@ func (tb *TransactionBuilder) TotalInputsValue() int64 { // preserving per-input sighash metadata collected during builder input setup. // It also validates that the replacement's inputs carry no pre-existing signature // data, that each replacement input's PreviousOutPoint matches the prior input -// at the same index, that the TxOut set matches the prior builder state (when -// outputs had been committed), and restores each input's pre-signing witness -// or signature-script from the previous builder state; replacement inputs whose -// previous witness had more than one element are rejected. +// at the same index, that the TxOut set matches the prior builder state, and +// restores each input's pre-signing witness or signature-script from the +// previous builder state; replacement inputs whose previous witness had more +// than one element are rejected. func (tb *TransactionBuilder) ReplaceUnsignedTransaction( transaction *Transaction, ) error { @@ -630,9 +668,9 @@ func (tb *TransactionBuilder) ReplaceUnsignedTransaction( replacedInternal := newInternalTransaction() replacedInternal.fromTransaction(transaction) - // Bind the replacement to the builder's prior state: per-index PreviousOutPoint - // must match, and TxOut set must match when the builder had committed to - // outputs. Without these checks, a caller-controlled replacement could redirect + // Bind the replacement to the builder's prior state: per-index + // PreviousOutPoint must match and the TxOut set must match, always. + // Without these checks, a caller-controlled replacement could redirect // funds (different TxOut set) or misalign sigHashArgs[i] with the new-order // tx.TxIn[i], producing a self-consistent-but-wrong digest that survives the // local Verify gate and broadcasts an unintended transaction. @@ -645,30 +683,28 @@ func (tb *TransactionBuilder) ReplaceUnsignedTransaction( ) } } - if len(previousOutputs) > 0 { - if len(replacedInternal.TxOut) != len(previousOutputs) { + if len(replacedInternal.TxOut) != len(previousOutputs) { + return fmt.Errorf( + "replacement TxOut set has [%d] entries; builder state has [%d]", + len(replacedInternal.TxOut), + len(previousOutputs), + ) + } + for i, prevOut := range previousOutputs { + replOut := replacedInternal.TxOut[i] + if replOut.Value != prevOut.Value { return fmt.Errorf( - "replacement TxOut set has [%d] entries; builder state has [%d]", - len(replacedInternal.TxOut), - len(previousOutputs), + "replacement TxOut [%d] value [%d] differs from builder state [%d]", + i, + replOut.Value, + prevOut.Value, ) } - for i, prevOut := range previousOutputs { - replOut := replacedInternal.TxOut[i] - if replOut.Value != prevOut.Value { - return fmt.Errorf( - "replacement TxOut [%d] value [%d] differs from builder state [%d]", - i, - replOut.Value, - prevOut.Value, - ) - } - if !bytes.Equal(replOut.PkScript, prevOut.PkScript) { - return fmt.Errorf( - "replacement TxOut [%d] PkScript differs from builder state", - i, - ) - } + if !bytes.Equal(replOut.PkScript, prevOut.PkScript) { + return fmt.Errorf( + "replacement TxOut [%d] PkScript differs from builder state", + i, + ) } } diff --git a/pkg/bitcoin/transaction_builder_test.go b/pkg/bitcoin/transaction_builder_test.go index 864dd6f932..f99972a5c3 100644 --- a/pkg/bitcoin/transaction_builder_test.go +++ b/pkg/bitcoin/transaction_builder_test.go @@ -542,6 +542,18 @@ func TestTransactionBuilder_AddOutput(t *testing.T) { builder.AddOutput(output) assertInternalOutput(t, builder, 0, output) + + // AddOutput must invalidate previously computed signature hashes so a + // stale digest can never be applied to a transaction whose output set + // changed after ComputeSignatureHashes. + builder.sigHashes = []*big.Int{big.NewInt(1)} + builder.AddOutput(output) + if len(builder.sigHashes) != 0 { + t.Fatalf( + "expected sighashes reset after AddOutput: [%d]", + len(builder.sigHashes), + ) + } } func TestTransactionBuilder_AddTaprootKeyPathSignatures(t *testing.T) { @@ -1072,14 +1084,18 @@ func TestTransactionBuilder_ReplaceUnsignedTransaction(t *testing.T) { &inputSigHashArgs{value: 111, scriptCode: []byte{0x51}, witness: false}, &inputSigHashArgs{value: 222, scriptCode: []byte{0x52}, witness: true}, ) + builder.AddOutput(&TransactionOutput{ + Value: 1000, + PublicKeyScript: hexToSlice(t, "0014deadbeef"), + }) builder.sigHashes = []*big.Int{big.NewInt(1), big.NewInt(2)} var replacementInputHash1 chainhash.Hash var replacementInputHash2 chainhash.Hash // Preserve the outpoints that the builder was initialized with so the - // replacement's per-index PreviousOutPoint matches the prior builder state - // (transaction_builder.go now binds the replacement's outpoints and TxOut - // set to the builder's prior state before rebinding tb.internal). + // replacement's per-index PreviousOutPoint matches the prior builder + // state. The replacement must also carry the exact output set the builder + // committed to; ReplaceUnsignedTransaction binds both unconditionally. replacementInputHash1 = initialInputHash1 replacementInputHash2 = initialInputHash2 @@ -1935,3 +1951,296 @@ func TestTransactionBuilder_AddTaprootKeyPathSignatures_RejectsInvalid( ) } } + +func TestTransactionBuilder_RejectsMixedP2TRNonP2TR(t *testing.T) { + localChain := newLocalChain() + + var publicKeyHash [20]byte + copy( + publicKeyHash[:], + hexToSlice(t, "0102030405060708090a0b0c0d0e0f1011121314"), + ) + publicKeyHashScript, err := PayToWitnessPublicKeyHash(publicKeyHash) + if err != nil { + t.Fatal(err) + } + + _, publicKey := btcec2.PrivKeyFromBytes( + hexToSlice( + t, + "0101010101010101010101010101010101010101010101010101010101010101", + ), + ) + + var taprootOutputKey [32]byte + copy(taprootOutputKey[:], schnorr.SerializePubKey(publicKey)) + + taprootScript, err := PayToTaproot(taprootOutputKey) + if err != nil { + t.Fatal(err) + } + + fundingTransaction := func(marker byte, script Script) *Transaction { + return &Transaction{ + Version: 1, + Inputs: []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash{marker}, + OutputIndex: 0, + }, + SignatureScript: []byte{0x51}, + Sequence: 0xffffffff, + }, + }, + Outputs: []*TransactionOutput{ + { + Value: 100000, + PublicKeyScript: script, + }, + }, + Locktime: 0, + } + } + + publicKeyHashFunding := fundingTransaction(0x41, publicKeyHashScript) + taprootFunding := fundingTransaction(0x42, taprootScript) + + for _, transaction := range []*Transaction{ + publicKeyHashFunding, + taprootFunding, + } { + if err := localChain.addTransaction(transaction); err != nil { + t.Fatal(err) + } + } + + utxo := func(transaction *Transaction) *UnspentTransactionOutput { + return &UnspentTransactionOutput{ + Outpoint: &TransactionOutpoint{ + TransactionHash: transaction.Hash(), + OutputIndex: 0, + }, + Value: 100000, + } + } + + t.Run("taproot input added to non-taproot builder", func(t *testing.T) { + builder := NewTransactionBuilder(localChain) + + if err := builder.AddPublicKeyHashInput( + utxo(publicKeyHashFunding), + ); err != nil { + t.Fatal(err) + } + + err := builder.AddTaprootKeyPathInput(utxo(taprootFunding)) + if err == nil { + t.Fatal("expected mixed input shape to be rejected") + } + if !strings.Contains(err.Error(), "mixed") { + t.Fatalf("unexpected error: [%v]", err) + } + + testutils.AssertIntsEqual( + t, + "builder inputs count", + 1, + len(builder.internal.TxIn), + ) + }) + + t.Run("non-taproot input added to taproot builder", func(t *testing.T) { + builder := NewTransactionBuilder(localChain) + + if err := builder.AddTaprootKeyPathInput( + utxo(taprootFunding), + ); err != nil { + t.Fatal(err) + } + + err := builder.AddPublicKeyHashInput(utxo(publicKeyHashFunding)) + if err == nil { + t.Fatal("expected mixed input shape to be rejected") + } + if !strings.Contains(err.Error(), "mixed") { + t.Fatalf("unexpected error: [%v]", err) + } + + testutils.AssertIntsEqual( + t, + "builder inputs count", + 1, + len(builder.internal.TxIn), + ) + }) +} + +// newReplaceableBuilder returns a builder holding two unsigned inputs and a +// single committed output, i.e. the prior state ReplaceUnsignedTransaction +// binds a replacement transaction to. +func newReplaceableBuilder(t *testing.T) ( + *TransactionBuilder, + []*TransactionInput, + []*TransactionOutput, +) { + t.Helper() + + builder := NewTransactionBuilder(nil) + + var inputHash1 chainhash.Hash + var inputHash2 chainhash.Hash + inputHash1[0] = 0x11 + inputHash2[0] = 0x22 + + builder.internal.AddTxIn( + wire.NewTxIn(wire.NewOutPoint(&inputHash1, 1), nil, nil), + ) + builder.internal.AddTxIn( + wire.NewTxIn(wire.NewOutPoint(&inputHash2, 2), nil, nil), + ) + builder.sigHashArgs = append( + builder.sigHashArgs, + &inputSigHashArgs{value: 111, scriptCode: []byte{0x51}, witness: false}, + &inputSigHashArgs{value: 222, scriptCode: []byte{0x52}, witness: false}, + ) + + output := &TransactionOutput{ + Value: 1000, + PublicKeyScript: hexToSlice(t, "0014deadbeef"), + } + builder.AddOutput(output) + + inputs := []*TransactionInput{ + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash(inputHash1), + OutputIndex: 1, + }, + Sequence: 0xffffffff, + }, + { + Outpoint: &TransactionOutpoint{ + TransactionHash: Hash(inputHash2), + OutputIndex: 2, + }, + Sequence: 0xffffffff, + }, + } + + return builder, inputs, []*TransactionOutput{output} +} + +func TestTransactionBuilder_ReplaceUnsignedTransaction_RejectsOutpointMismatch( + t *testing.T, +) { + builder, inputs, outputs := newReplaceableBuilder(t) + + inputs[1].Outpoint.OutputIndex = 7 + + err := builder.ReplaceUnsignedTransaction(&Transaction{ + Version: 1, + Inputs: inputs, + Outputs: outputs, + Locktime: 0, + }) + if err == nil { + t.Fatal("expected outpoint mismatch error") + } + if !strings.Contains( + err.Error(), + "replacement input [1] PreviousOutPoint differs from builder state", + ) { + t.Fatalf("unexpected error: [%v]", err) + } +} + +func TestTransactionBuilder_ReplaceUnsignedTransaction_RejectsTxOutValueMismatch( + t *testing.T, +) { + builder, inputs, outputs := newReplaceableBuilder(t) + + outputs[0].Value = 999 + + err := builder.ReplaceUnsignedTransaction(&Transaction{ + Version: 1, + Inputs: inputs, + Outputs: outputs, + Locktime: 0, + }) + if err == nil { + t.Fatal("expected TxOut value mismatch error") + } + if !strings.Contains( + err.Error(), + "replacement TxOut [0] value [999] differs from builder state [1000]", + ) { + t.Fatalf("unexpected error: [%v]", err) + } +} + +func TestTransactionBuilder_ReplaceUnsignedTransaction_RejectsTxOutScriptMismatch( + t *testing.T, +) { + builder, inputs, outputs := newReplaceableBuilder(t) + + outputs[0].PublicKeyScript = hexToSlice(t, "0014beefdead") + + err := builder.ReplaceUnsignedTransaction(&Transaction{ + Version: 1, + Inputs: inputs, + Outputs: outputs, + Locktime: 0, + }) + if err == nil { + t.Fatal("expected TxOut script mismatch error") + } + if !strings.Contains( + err.Error(), + "replacement TxOut [0] PkScript differs from builder state", + ) { + t.Fatalf("unexpected error: [%v]", err) + } +} + +func TestTransactionBuilder_ReplaceUnsignedTransaction_RejectsOutputCountMismatch( + t *testing.T, +) { + tests := map[string]struct { + replacementOutputCount int + }{ + "no outputs": {replacementOutputCount: 0}, + "extra output": {replacementOutputCount: 2}, + } + + for testName, test := range tests { + t.Run(testName, func(t *testing.T) { + builder, inputs, outputs := newReplaceableBuilder(t) + + replacementOutputs := make([]*TransactionOutput, 0) + for range test.replacementOutputCount { + replacementOutputs = append(replacementOutputs, outputs[0]) + } + + err := builder.ReplaceUnsignedTransaction(&Transaction{ + Version: 1, + Inputs: inputs, + Outputs: replacementOutputs, + Locktime: 0, + }) + if err == nil { + t.Fatal("expected TxOut count mismatch error") + } + if !strings.Contains( + err.Error(), + fmt.Sprintf( + "replacement TxOut set has [%d] entries; builder "+ + "state has [1]", + test.replacementOutputCount, + ), + ) { + t.Fatalf("unexpected error: [%v]", err) + } + }) + } +} From 3badb601d2e38d90388907d1d3d832ce809aece6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ros=C5=82aniec?= Date: Tue, 18 Aug 2026 16:23:09 +0000 Subject: [PATCH 10/10] fix(bitcoin/electrum,test): tighten docs and add coverage for selectLatestUniqueTxHashes Documentation and test-coverage cleanups: - currentClient's doc comment now scopes the retry-fallback claim to requestWithRetry callers, notes that getFeeBtcPerKbOnce calls currentClient directly and surfaces its failure to the fee fallback loop instead of retrying, and points out the upstream go-electrum Shutdown() thread-unsafety as a separately-tracked issue. - selectLatestUniqueTxHashes gets a doc comment that documents the dedup-before-limit ordering and the limit <= 0 empty-slice guard. - AddOutput's doc comment now states that adding an output after ComputeSignatureHashes invalidates the cached signature hashes. - TestTaprootTweak_RejectsInvalidInputs is restructured into three t.Run subtests so the unreachable branches report as skipped subtests instead of unconditional t.Skip()s at the function level. The accompanying function comment is corrected: the tweak >= curve order probability is ~2^-128 (not ~50%), and the infinity-output rejection lives in TaprootOutputKey (taproot.go:73-75), not taprootTweakScalar. - TestSelectLatestUniqueTxHashes exercises the new helper across the negative-limit, zero-limit, empty-input, dedup, dedup-before-limit, trailing-N-on-overflow, and underflow paths. - TestTransactionBuilder_AddTaprootKeyPathSignatures drops the 'independently cross-checked by reviewers' clause from the hardcoded-vector comment. --- pkg/bitcoin/electrum/electrum.go | 35 ++++++++---- pkg/bitcoin/electrum/electrum_test.go | 73 ++++++++++++++++++++++++ pkg/bitcoin/script_test.go | 74 ++++++++++++------------- pkg/bitcoin/transaction_builder.go | 4 ++ pkg/bitcoin/transaction_builder_test.go | 3 +- 5 files changed, 140 insertions(+), 49 deletions(-) diff --git a/pkg/bitcoin/electrum/electrum.go b/pkg/bitcoin/electrum/electrum.go index 786242af9e..c3966156ef 100644 --- a/pkg/bitcoin/electrum/electrum.go +++ b/pkg/bitcoin/electrum/electrum.go @@ -457,6 +457,16 @@ func (c *Connection) GetTransactionsForPublicKeyScripts( return transactions, nil } +// selectLatestUniqueTxHashes deduplicates the given transaction hashes, +// preserving their original (block-height ascending) order, and returns at +// most limit trailing entries, i.e. the latest ones. Deduplication happens +// before the limit is applied, so a transaction paying two of the queried +// scripts consumes a single slot. +// +// A limit lower than or equal to zero returns an empty slice. The SPV +// maintainer's transaction limit is an operator-provided flag, so the guard +// keeps a misconfigured negative value from panicking on the slice bound; it +// makes the maintainer observe no transactions rather than crash. func selectLatestUniqueTxHashes( txHashes []bitcoin.Hash, limit int, @@ -1387,17 +1397,22 @@ func requestWithRetry[K interface{}]( // currentClient returns the live Electrum client under a read lock. // -// The lock protects the client POINTER against a concurrent reconnect swap, not -// the request itself: go-electrum multiplexes concurrent requests over an -// id-to-channel map with a single reader goroutine dispatching responses, so it is -// safe to call from several goroutines at once. Holding the lock across the round -// trip instead serialized every Electrum request in the process, so independent -// lookups queued behind each other for a full network round trip each. +// The lock protects the client POINTER against a concurrent reconnect swap and +// nothing else. It is released before the caller issues its request, so a +// reconnect may swap the client while that request is still in flight against +// the old one. +// +// How such a stale-client request fails depends on the caller. Requests issued +// through requestWithRetry fail and are reconnected and repeated by its retry +// loop -- the same fallback it applies to any other request failure. +// getFeeBtcPerKbOnce calls currentClient directly, outside any retry wrapper, +// and surfaces a plain error to EstimateSatPerVByteFee's fallback loop, which +// moves on to the next confirmation target instead of retrying the same one. // -// A reconnect can now swap the client immediately after this returns, while -// the caller's request is still in flight against the old client. That request -// will fail and requestWithRetry's retry loop reconnects and repeats it -- the -// same fallback the retry loop already uses for any other request failure. +// Note that the underlying go-electrum client's Shutdown() is not thread-safe +// against in-flight requests: it clears the transport and handler maps without +// holding the handler lock. That is a known upstream issue tracked separately; +// this lock does not protect against it. func (c *Connection) currentClient() *electrum.Client { c.clientMutex.RLock() defer c.clientMutex.RUnlock() diff --git a/pkg/bitcoin/electrum/electrum_test.go b/pkg/bitcoin/electrum/electrum_test.go index b09d03ec51..1ed906a869 100644 --- a/pkg/bitcoin/electrum/electrum_test.go +++ b/pkg/bitcoin/electrum/electrum_test.go @@ -188,3 +188,76 @@ func TestFeeFallbackResult(t *testing.T) { }) } } + +func TestSelectLatestUniqueTxHashes(t *testing.T) { + t.Parallel() + + hash := func(marker byte) bitcoin.Hash { + var txHash bitcoin.Hash + txHash[0] = marker + return txHash + } + + first := hash(0x01) + second := hash(0x02) + third := hash(0x03) + + tests := map[string]struct { + txHashes []bitcoin.Hash + limit int + expected []bitcoin.Hash + }{ + "negative limit": { + txHashes: []bitcoin.Hash{first, second}, + limit: -1, + expected: []bitcoin.Hash{}, + }, + "zero limit": { + txHashes: []bitcoin.Hash{first, second}, + limit: 0, + expected: []bitcoin.Hash{}, + }, + "no hashes": { + txHashes: []bitcoin.Hash{}, + limit: 5, + expected: []bitcoin.Hash{}, + }, + "duplicates deduplicated within limit": { + txHashes: []bitcoin.Hash{first, second, first, second}, + limit: 5, + expected: []bitcoin.Hash{first, second}, + }, + "deduplication happens before the limit is applied": { + // Without dedup-before-limit, the duplicated first hash would + // consume one of the two available slots. + txHashes: []bitcoin.Hash{first, first, second}, + limit: 2, + expected: []bitcoin.Hash{first, second}, + }, + "more unique hashes than limit keeps the latest ones in order": { + txHashes: []bitcoin.Hash{first, second, third}, + limit: 2, + expected: []bitcoin.Hash{second, third}, + }, + "fewer unique hashes than limit": { + txHashes: []bitcoin.Hash{first, second}, + limit: 5, + expected: []bitcoin.Hash{first, second}, + }, + } + + for testName, test := range tests { + t.Run(testName, func(t *testing.T) { + t.Parallel() + + actual := selectLatestUniqueTxHashes(test.txHashes, test.limit) + if !reflect.DeepEqual(test.expected, actual) { + t.Fatalf( + "unexpected selection\nexpected: [%v]\nactual: [%v]", + test.expected, + actual, + ) + } + }) + } +} diff --git a/pkg/bitcoin/script_test.go b/pkg/bitcoin/script_test.go index 4966cee120..e448a64ad6 100644 --- a/pkg/bitcoin/script_test.go +++ b/pkg/bitcoin/script_test.go @@ -627,44 +627,44 @@ func TestExtractPublicKeyHash(t *testing.T) { } } -// TestTaprootTweak_RejectsInvalidInputs exercises the rejection paths in -// taprootTweakScalar. The schnorr.ParsePubKey branch is tested directly; -// the tweakScalar.SetBytes (tweak >= curve order) and infinity-output -// branches require inverting the BIP-341 tap-tweak hash to construct a -// deterministic fixture (any (internalKey, merkleRoot) pair produces a -// tag-tweak hash >= curve order with probability ~50%, but engineering the -// input without inverting SHA-256 is infeasible), so both are skipped with -// a pointer to the code review that asserts their behavior. +// TestTaprootTweak_RejectsInvalidInputs exercises the rejection paths reachable +// through TaprootTweak. The schnorr.ParsePubKey branch of taprootTweakScalar is +// tested directly. The tweakScalar.SetBytes branch (tagged tweak hash >= the +// secp256k1 group order, probability ~2^-128 for a random hash) and the +// infinity-output branch (in TaprootOutputKey, taproot.go:73-75) both need a +// fixture that inverts the BIP-341 tap-tweak hash, which is infeasible, so they +// are reported as skipped subtests rather than silently omitted. func TestTaprootTweak_RejectsInvalidInputs(t *testing.T) { - // (1) Garbage x-only key: bytes that are not a valid x-coordinate on - // the secp256k1 curve. schnorr.ParsePubKey rejects the input and - // TaprootTweak wraps it in "cannot parse taproot internal key". - var garbageKey [32]byte - for i := range garbageKey { - garbageKey[i] = 0xff - } - _, err := TaprootTweak(garbageKey, nil) - if err == nil { - t.Fatal( - "TaprootTweak accepted an invalid x-only key; " + - "expected an error from schnorr.ParsePubKey", - ) - } + t.Run("garbage x-only internal key", func(t *testing.T) { + // Bytes that are not a valid x-coordinate on the secp256k1 curve. + // schnorr.ParsePubKey rejects the input and TaprootTweak wraps it in + // "cannot parse taproot internal key". + var garbageKey [32]byte + for i := range garbageKey { + garbageKey[i] = 0xff + } - // (2) Tweak >= curve order: deterministic test fixture requires - // inverting the BIP-341 tap-tweak hash to produce an - // (internalKey, merkleRoot) pair whose tagged hash exceeds the - // secp256k1 group order. Skipped. - t.Skip( - "tweak >= curve order rejection requires inverting " + - "BIP-341 tap-tweak hash; covered by code review", - ) + if _, err := TaprootTweak(garbageKey, nil); err == nil { + t.Fatal( + "TaprootTweak accepted an invalid x-only key; " + + "expected an error from schnorr.ParsePubKey", + ) + } + }) - // (3) Infinity output: deterministic test fixture requires inverting - // the BIP-341 tweak to produce an internal key + tweak that sum to - // the point at infinity. Skipped. - t.Skip( - "infinity-output rejection requires inverting " + - "BIP-341 tweak; covered by code review", - ) + t.Run("tweak greater than curve order", func(t *testing.T) { + t.Skip( + "needs an (internalKey, merkleRoot) pair whose BIP-341 tagged " + + "tweak hash exceeds the secp256k1 group order; constructing " + + "one requires inverting SHA-256", + ) + }) + + t.Run("infinity output key", func(t *testing.T) { + t.Skip( + "needs an internal key and tweak that sum to the point at " + + "infinity in TaprootOutputKey; constructing one requires " + + "inverting the BIP-341 tap-tweak hash", + ) + }) } diff --git a/pkg/bitcoin/transaction_builder.go b/pkg/bitcoin/transaction_builder.go index 5ae15a0b54..fb18346035 100644 --- a/pkg/bitcoin/transaction_builder.go +++ b/pkg/bitcoin/transaction_builder.go @@ -363,6 +363,10 @@ func (tb *TransactionBuilder) getScript( } // AddOutput adds a new transaction's output. +// +// Adding an output after ComputeSignatureHashes invalidates the builder's +// cached signature hashes; ComputeSignatureHashes must be called again before +// signing. func (tb *TransactionBuilder) AddOutput(output *TransactionOutput) { tb.internal.AddTxOut(wire.NewTxOut(output.Value, output.PublicKeyScript)) tb.sigHashes = nil diff --git a/pkg/bitcoin/transaction_builder_test.go b/pkg/bitcoin/transaction_builder_test.go index f99972a5c3..3873596bbd 100644 --- a/pkg/bitcoin/transaction_builder_test.go +++ b/pkg/bitcoin/transaction_builder_test.go @@ -653,8 +653,7 @@ func TestTransactionBuilder_AddTaprootKeyPathSignatures(t *testing.T) { "96653d19d603d309d22cfe2ccd0ba445e40629dea18d46108caa601055ec4318", ) // This vector was generated with btcd v0.23.4's BIP-341 - // CalcTaprootSignatureHash implementation and independently - // cross-checked by reviewers. + // CalcTaprootSignatureHash implementation. sigHashBytes := sigHashes[0].FillBytes(make([]byte, 32)) testutils.AssertBytesEqual(t, expectedSigHash, sigHashBytes)