From e6ccdc58477b199a792e634355a79e2d62c77f17 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 27 Nov 2024 16:12:59 +0100 Subject: [PATCH 1/7] tapchannel: tweak HTLC script key internal key to enforce uniqueness This commit tweaks the internal key of the asset-level script key with the HTLC index to enforce uniqueness of script keys across multiple HTLCs with the same payment hash and timeout (MPP shards of the same payment). The internal key we tweak is the revocation key. So we must also apply that same tweak when doing a revocation/breach sweep transaction. But since that functionality is not yet implemented, we'll just leave a TODO in a follow-up commit for that. Therefore, no tweak will currently need to be applied at _sign_ time. --- tapchannel/aux_leaf_signer.go | 108 +++++++++++ tapchannel/aux_leaf_signer_test.go | 286 +++++++++++++++++++++++++++++ tapchannel/commitment.go | 16 +- 3 files changed, 407 insertions(+), 3 deletions(-) diff --git a/tapchannel/aux_leaf_signer.go b/tapchannel/aux_leaf_signer.go index f728b1ec4..e8f4fe9a6 100644 --- a/tapchannel/aux_leaf_signer.go +++ b/tapchannel/aux_leaf_signer.go @@ -3,12 +3,15 @@ package tapchannel import ( "bytes" "fmt" + "math" + "math/big" "sync" "github.com/btcsuite/btcd/btcec/v2" "github.com/btcsuite/btcd/btcutil/psbt" "github.com/btcsuite/btcd/txscript" "github.com/btcsuite/btcd/wire" + "github.com/decred/dcrd/dcrec/secp256k1/v4" "github.com/lightninglabs/taproot-assets/address" "github.com/lightninglabs/taproot-assets/asset" "github.com/lightninglabs/taproot-assets/commitment" @@ -751,3 +754,108 @@ func (v *schnorrSigValidator) validateSchnorrSig(virtualTx *wire.MsgTx, return nil } + +// ScriptKeyTweakFromHtlcIndex converts the given HTLC index into a modulo N +// scalar that can be used to tweak the internal key of the HTLC script key on +// the asset level. The value of 1 is always added to the index to make sure +// this value is always non-zero. +func ScriptKeyTweakFromHtlcIndex(index input.HtlcIndex) *secp256k1.ModNScalar { + // If we're at math.MaxUint64, we'd wrap around to 0 if we incremented + // by 1, but we need to make sure the tweak is 1 to not cause a + // multiplication by zero. This should never happen, as it would mean we + // have more than math.MaxUint64 updates in a channel, which exceeds the + // protocol's maximum. + if index == math.MaxUint64 { + return new(secp256k1.ModNScalar).SetInt(1) + } + + // We need to avoid the tweak being zero, so we always add 1 to the + // index. Otherwise, we'd multiply G by zero. + index++ + + indexAsBytes := new(big.Int).SetUint64(index).Bytes() + indexAsScalar := new(secp256k1.ModNScalar) + _ = indexAsScalar.SetByteSlice(indexAsBytes) + + return indexAsScalar +} + +// TweakPubKeyWithIndex tweaks the given internal public key with the given +// HTLC index. The tweak is derived from the index in a way that never results +// in a zero tweak. The value of 1 is always added to the index to make sure +// this value is always non-zero. The public key is tweaked like this: +// +// tweakedKey = key + (index+1) * G +func TweakPubKeyWithIndex(pubKey *btcec.PublicKey, + index input.HtlcIndex) *btcec.PublicKey { + + // Avoid panic if input is nil. + if pubKey == nil { + return nil + } + + // We need to operate on Jacobian points, which is just a different + // representation of the public key that allows us to do scalar + // multiplication. + var ( + pubKeyJacobian, tweakTimesG, tweakedKey btcec.JacobianPoint + ) + pubKey.AsJacobian(&pubKeyJacobian) + + // Derive the tweak from the HTLC index in a way that never results in + // a zero tweak. Then we multiply G by the tweak. + tweak := ScriptKeyTweakFromHtlcIndex(index) + secp256k1.ScalarBaseMultNonConst(tweak, &tweakTimesG) + + // And finally we add the result to the key to get the tweaked key. + secp256k1.AddNonConst(&pubKeyJacobian, &tweakTimesG, &tweakedKey) + + // Convert the tweaked key back to an affine point and create a new + // taproot key from it. + tweakedKey.ToAffine() + return btcec.NewPublicKey(&tweakedKey.X, &tweakedKey.Y) +} + +// TweakHtlcTree tweaks the internal key of the given HTLC script tree with the +// given index, then returns the tweaked tree with the updated taproot key. +// The tapscript tree and tapscript root are not modified. +// The internal key is tweaked like this: +// +// tweakedInternalKey = internalKey + (index+1) * G +func TweakHtlcTree(tree input.ScriptTree, + index input.HtlcIndex) input.ScriptTree { + + // The tapscript tree and root are not modified, only the internal key + // is tweaked, which inherently modifies the taproot key. + tweakedInternalPubKey := TweakPubKeyWithIndex(tree.InternalKey, index) + newTaprootKey := txscript.ComputeTaprootOutputKey( + tweakedInternalPubKey, tree.TapscriptRoot, + ) + + return input.ScriptTree{ + InternalKey: tweakedInternalPubKey, + TaprootKey: newTaprootKey, + TapscriptTree: tree.TapscriptTree, + TapscriptRoot: tree.TapscriptRoot, + } +} + +// AddTweakWithIndex adds the given index to the given tweak. If the tweak is +// empty, the index is used as the tweak directly. The value of 1 is always +// added to the index to make sure this value is always non-zero. +func AddTweakWithIndex(maybeTweak []byte, index input.HtlcIndex) []byte { + indexTweak := ScriptKeyTweakFromHtlcIndex(index) + + // If we don't already have a tweak, we just use the index as the tweak. + if len(maybeTweak) == 0 { + return fn.ByteSlice(indexTweak.Bytes()) + } + + // If we have a tweak, we need to parse/decode it as a scalar, then add + // the index as a scalar, and encode it back to a byte slice. + tweak := new(secp256k1.ModNScalar) + _ = tweak.SetByteSlice(maybeTweak) + newTweak := tweak.Add(indexTweak) + + return fn.ByteSlice(newTweak.Bytes()) +} diff --git a/tapchannel/aux_leaf_signer_test.go b/tapchannel/aux_leaf_signer_test.go index a2fe08656..75167c60e 100644 --- a/tapchannel/aux_leaf_signer_test.go +++ b/tapchannel/aux_leaf_signer_test.go @@ -3,13 +3,19 @@ package tapchannel import ( "bytes" "crypto/rand" + "encoding/binary" "encoding/hex" "fmt" + "math" + "math/big" "testing" "github.com/btcsuite/btcd/btcec/v2" "github.com/btcsuite/btcd/txscript" + "github.com/decred/dcrd/dcrec/secp256k1/v4" "github.com/lightninglabs/taproot-assets/asset" + "github.com/lightninglabs/taproot-assets/fn" + "github.com/lightninglabs/taproot-assets/internal/test" cmsg "github.com/lightninglabs/taproot-assets/tapchannelmsg" "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/lnwire" @@ -63,6 +69,16 @@ func somePartialSigWithNonce(t *testing.T) lnwire.OptPartialSigWithNonceTLV { ](*sig)) } +func pubKeyFromUint64(num uint64) *btcec.PublicKey { + var ( + buf = make([]byte, 8) + scalar = new(secp256k1.ModNScalar) + ) + binary.BigEndian.PutUint64(buf, num) + _ = scalar.SetByteSlice(buf) + return secp256k1.NewPrivateKey(scalar).PubKey() +} + // TestMaxCommitSigMsgSize attempts to find values for the max number of asset // IDs we want to allow per channel and the resulting maximum number of HTLCs // that channel can allow. The maximum number of different asset IDs that can be @@ -163,3 +179,273 @@ func makeCommitSig(t *testing.T, numAssetIDs, numHTLCs int) *lnwire.CommitSig { return msg } + +// TestHtlcIndexAsScriptKeyTweak tests the ScriptKeyTweakFromHtlcIndex function. +func TestHtlcIndexAsScriptKeyTweak(t *testing.T) { + var ( + buf = make([]byte, 8) + maxUint64MinusOne = new(secp256k1.ModNScalar) + maxUint64 = new(secp256k1.ModNScalar) + ) + binary.BigEndian.PutUint64(buf, math.MaxUint64-1) + _ = maxUint64MinusOne.SetByteSlice(buf) + + binary.BigEndian.PutUint64(buf, math.MaxUint64) + _ = maxUint64.SetByteSlice(buf) + + testCases := []struct { + name string + index uint64 + result *secp256k1.ModNScalar + }{ + { + name: "index 0", + index: 0, + result: new(secp256k1.ModNScalar).SetInt(1), + }, + { + name: "index math.MaxUint32-1", + index: math.MaxUint32 - 1, + result: new(secp256k1.ModNScalar).SetInt( + math.MaxUint32, + ), + }, + { + name: "index math.MaxUint64-2", + index: math.MaxUint64 - 2, + result: maxUint64MinusOne, + }, + { + name: "index math.MaxUint64-1", + index: math.MaxUint64 - 1, + result: maxUint64, + }, + { + name: "index math.MaxUint64, wraps around to 1", + index: math.MaxUint64, + result: new(secp256k1.ModNScalar).SetInt(1), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tweak := ScriptKeyTweakFromHtlcIndex(tc.index) + require.Equal(t, tc.result, tweak) + }) + } +} + +// TestTweakPubKeyWithIndex tests the TweakPubKeyWithIndex function. +func TestTweakPubKeyWithIndex(t *testing.T) { + // We want a random number in the range of uint32 but will need it as + // an uint64 for the test cases. + randNum := uint64(test.RandInt[uint32]()) + startKey := pubKeyFromUint64(randNum) + + testCases := []struct { + name string + pubKey *btcec.PublicKey + index uint64 + result *btcec.PublicKey + }{ + { + name: "index 0", + pubKey: startKey, + index: 0, + result: pubKeyFromUint64(randNum + 1), + }, + { + name: "index 1", + pubKey: startKey, + index: 1, + result: pubKeyFromUint64(randNum + 2), + }, + { + name: "index 99", + pubKey: startKey, + index: 99, + result: pubKeyFromUint64(randNum + 100), + }, + { + name: "index math.MaxUint32-1", + pubKey: startKey, + index: math.MaxUint32 - 1, + result: pubKeyFromUint64(randNum + math.MaxUint32), + }, + { + // Because we always increment by 1, there is a + // "collision" at 0 and math.MaxUint64. For the purpose + // of tweaking channel related keys with the HTLC index, + // that is okay, as there isn't expected to ever be that + // many HTLCs during the lifetime of a channel. + name: "index math.MaxUint64, wrap around", + pubKey: startKey, + index: math.MaxUint64, + result: pubKeyFromUint64(randNum + 1), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tweakedKey := TweakPubKeyWithIndex(tc.pubKey, tc.index) + require.Equal( + t, tc.result.SerializeCompressed(), + tweakedKey.SerializeCompressed(), + ) + }) + } +} + +// TestTweakHtlcTree tests the TweakHtlcTree function. +func TestTweakHtlcTree(t *testing.T) { + randTree := txscript.AssembleTaprootScriptTree( + test.RandTapLeaf(nil), test.RandTapLeaf(nil), + test.RandTapLeaf(nil), + ) + randRoot := randTree.RootNode.TapHash() + + // We want a random number in the range of uint32 but will need it as + // an uint64 for the test cases. + randNum := uint64(test.RandInt[uint32]()) + + makeTaprootKey := func(num uint64) *btcec.PublicKey { + return txscript.ComputeTaprootOutputKey( + pubKeyFromUint64(num), randRoot[:], + ) + } + startKey := pubKeyFromUint64(randNum) + startTaprootKey := makeTaprootKey(randNum) + startTree := input.ScriptTree{ + InternalKey: startKey, + TaprootKey: startTaprootKey, + TapscriptTree: randTree, + TapscriptRoot: randRoot[:], + } + + testCases := []struct { + name string + tree input.ScriptTree + index uint64 + result input.ScriptTree + }{ + { + name: "index 0", + tree: startTree, + index: 0, + result: input.ScriptTree{ + InternalKey: pubKeyFromUint64(randNum + 1), + TaprootKey: makeTaprootKey(randNum + 1), + TapscriptTree: randTree, + TapscriptRoot: randRoot[:], + }, + }, + { + name: "index 1", + tree: startTree, + index: 1, + result: input.ScriptTree{ + InternalKey: pubKeyFromUint64(randNum + 2), + TaprootKey: makeTaprootKey(randNum + 2), + TapscriptTree: randTree, + TapscriptRoot: randRoot[:], + }, + }, + { + name: "index 99", + tree: startTree, + index: 99, + result: input.ScriptTree{ + InternalKey: pubKeyFromUint64(randNum + 100), + TaprootKey: makeTaprootKey(randNum + 100), + TapscriptTree: randTree, + TapscriptRoot: randRoot[:], + }, + }, + { + name: "index math.MaxUint32-1", + tree: startTree, + index: math.MaxUint32 - 1, + result: input.ScriptTree{ + InternalKey: pubKeyFromUint64( + randNum + math.MaxUint32, + ), + TaprootKey: makeTaprootKey( + randNum + math.MaxUint32, + ), + TapscriptTree: randTree, + TapscriptRoot: randRoot[:], + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tweakedTree := TweakHtlcTree(tc.tree, tc.index) + require.Equal( + t, tc.result.InternalKey.SerializeCompressed(), + tweakedTree.InternalKey.SerializeCompressed(), + ) + require.Equal( + t, tc.result.TaprootKey.SerializeCompressed(), + tweakedTree.TaprootKey.SerializeCompressed(), + ) + require.Equal(t, tc.result, tweakedTree) + }) + } +} + +// TestAddTweakWithIndex tests the AddTweakWithIndex function. +func TestAddTweakWithIndex(t *testing.T) { + var ( + bufMaxUint64 = make([]byte, 8) + maxUint64 = new(secp256k1.ModNScalar) + ) + binary.BigEndian.PutUint64(bufMaxUint64, math.MaxUint64) + _ = maxUint64.SetByteSlice(bufMaxUint64) + maxUint64Double := new(secp256k1.ModNScalar). + Set(maxUint64).Add(maxUint64) + + testCases := []struct { + name string + tweak []byte + index uint64 + result *secp256k1.ModNScalar + }{ + { + name: "empty tweak, index 0", + index: 0, + result: new(secp256k1.ModNScalar).SetInt(1), + }, + { + name: "five as tweak, index 123", + tweak: []byte{0x05}, + index: 123, + result: new(secp256k1.ModNScalar).SetInt(129), + }, + { + name: "all zero tweak, index 123", + tweak: bytes.Repeat([]byte{0}, 32), + index: 123, + result: new(secp256k1.ModNScalar).SetInt(124), + }, + { + name: "tweak math.MaxUint64, index math.MaxUint64-1", + tweak: fn.ByteSlice(maxUint64.Bytes()), + index: math.MaxUint64 - 1, + result: maxUint64Double, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tweak := AddTweakWithIndex(tc.tweak, tc.index) + resultBytes := tc.result.Bytes() + resultBigInt := new(big.Int).SetBytes(resultBytes[:]) + tweakBigInt := new(big.Int).SetBytes(tweak) + + require.Equalf(t, resultBytes[:], tweak, "expected: "+ + "%s, got: %s", resultBigInt.String(), + tweakBigInt.String()) + }) + } +} diff --git a/tapchannel/commitment.go b/tapchannel/commitment.go index a7c3b117a..808ae2e57 100644 --- a/tapchannel/commitment.go +++ b/tapchannel/commitment.go @@ -756,6 +756,13 @@ func CreateAllocations(chanState lnwallet.AuxChanState, ourBalance, "allocation, HTLC is dust") } + // To ensure uniqueness of the script key across HTLCs with the + // same payment hash and timeout (which would be equal + // otherwise), we tweak the asset level internal key of the + // script key with the HTLC index. We'll ONLY use this for the + // asset level, NOT for the BTC level. + tweakedTree := TweakHtlcTree(htlcTree, htlc.HtlcIndex) + allocations = append(allocations, &Allocation{ Type: allocType, Amount: rfqmsg.Sum(htlc.AssetBalances), @@ -766,16 +773,19 @@ func CreateAllocations(chanState lnwallet.AuxChanState, ourBalance, NonAssetLeaves: sibling, ScriptKey: asset.ScriptKey{ PubKey: asset.NewScriptKey( - htlcTree.TaprootKey, + tweakedTree.TaprootKey, ).PubKey, TweakedScriptKey: &asset.TweakedScriptKey{ RawKey: keychain.KeyDescriptor{ - PubKey: htlcTree.InternalKey, + PubKey: tweakedTree.InternalKey, }, - Tweak: htlcTree.TapscriptRoot, + Tweak: tweakedTree.TapscriptRoot, }, }, SortTaprootKeyBytes: schnorr.SerializePubKey( + // This _must_ remain the non-tweaked key, since + // this is used for sorting _before_ applying + // any TAP tweaks. htlcTree.TaprootKey, ), CLTV: htlc.Timeout, From 52ee51803aa45e0dd40b66dfe655b36b348ffa94 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 27 Nov 2024 16:13:00 +0100 Subject: [PATCH 2/7] tapchannel: apply tweaks when sweeping HTLCs --- tapchannel/aux_sweeper.go | 122 +++++++++++++++++++++++++------------- 1 file changed, 82 insertions(+), 40 deletions(-) diff --git a/tapchannel/aux_sweeper.go b/tapchannel/aux_sweeper.go index 89be2c08f..56f7861a5 100644 --- a/tapchannel/aux_sweeper.go +++ b/tapchannel/aux_sweeper.go @@ -192,8 +192,7 @@ func (a *AuxSweeper) Stop() error { // set of asset inputs into the backing wallet. func (a *AuxSweeper) createSweepVpackets(sweepInputs []*cmsg.AssetOutput, tapscriptDesc lfn.Result[tapscriptSweepDesc], - resReq lnwallet.ResolutionReq, -) lfn.Result[[]*tappsbt.VPacket] { + resReq lnwallet.ResolutionReq) lfn.Result[[]*tappsbt.VPacket] { type returnType = []*tappsbt.VPacket @@ -727,23 +726,26 @@ func commitRevokeSweepDesc(keyRing *lnwallet.CommitmentKeyRing, // remoteHtlcTimeoutSweepDesc creates a sweep desc for an HTLC output that is // close to timing out on the remote party's commitment transaction. -func remoteHtlcTimeoutSweepDesc(keyRing *lnwallet.CommitmentKeyRing, +func remoteHtlcTimeoutSweepDesc(originalKeyRing *lnwallet.CommitmentKeyRing, payHash []byte, csvDelay uint32, htlcExpiry uint32, -) lfn.Result[tapscriptSweepDescs] { + index input.HtlcIndex) lfn.Result[tapscriptSweepDescs] { + + // We're sweeping an HTLC output, which has a tweaked script key. To be + // able to create the correct control block, we need to tweak the key + // ring with the index of the HTLC. + tweakedKeyRing := TweakedRevocationKeyRing(originalKeyRing, index) // We're sweeping a timed out HTLC, which means that we'll need to // create the receiver's HTLC script tree (from the remote party's PoV). htlcScriptTree, err := input.ReceiverHTLCScriptTaproot( - htlcExpiry, keyRing.LocalHtlcKey, keyRing.RemoteHtlcKey, - keyRing.RevocationKey, payHash, lntypes.Remote, - input.NoneTapLeaf(), + htlcExpiry, tweakedKeyRing.LocalHtlcKey, + tweakedKeyRing.RemoteHtlcKey, tweakedKeyRing.RevocationKey, + payHash, lntypes.Remote, input.NoneTapLeaf(), ) if err != nil { return lfn.Err[tapscriptSweepDescs](err) } - // TODO(roasbeef): use GenTaprootHtlcScript instead? - // Now that we have the script tree, we'll make the control block needed // to spend it, but taking the revoked path. ctrlBlock, err := htlcScriptTree.CtrlBlockForPath( @@ -770,15 +772,21 @@ func remoteHtlcTimeoutSweepDesc(keyRing *lnwallet.CommitmentKeyRing, // remoteHtlcSuccessSweepDesc creates a sweep desc for an HTLC output present on // the remote party's commitment transaction that we can sweep with the // preimage. -func remoteHtlcSuccessSweepDesc(keyRing *lnwallet.CommitmentKeyRing, - payHash []byte, csvDelay uint32) lfn.Result[tapscriptSweepDescs] { +func remoteHtlcSuccessSweepDesc(originalKeyRing *lnwallet.CommitmentKeyRing, + payHash []byte, csvDelay uint32, + index input.HtlcIndex) lfn.Result[tapscriptSweepDescs] { + + // We're sweeping an HTLC output, which has a tweaked script key. To be + // able to create the correct control block, we need to tweak the key + // ring with the index of the HTLC. + tweakedKeyRing := TweakedRevocationKeyRing(originalKeyRing, index) // We're planning on sweeping an HTLC that we know the preimage to, // which the remote party sent, so we'll construct the sender version of // the HTLC script tree (from their PoV, they're the sender). htlcScriptTree, err := input.SenderHTLCScriptTaproot( - keyRing.RemoteHtlcKey, keyRing.LocalHtlcKey, - keyRing.RevocationKey, payHash, lntypes.Remote, + tweakedKeyRing.RemoteHtlcKey, tweakedKeyRing.LocalHtlcKey, + tweakedKeyRing.RevocationKey, payHash, lntypes.Remote, input.NoneTapLeaf(), ) if err != nil { @@ -811,9 +819,9 @@ func remoteHtlcSuccessSweepDesc(keyRing *lnwallet.CommitmentKeyRing, // present on our local commitment transaction. These are second level HTLCs, so // we'll need to perform two stages of sweeps. func localHtlcTimeoutSweepDesc(req lnwallet.ResolutionReq, -) lfn.Result[tapscriptSweepDescs] { + index input.HtlcIndex) lfn.Result[tapscriptSweepDescs] { - isIncoming := false + const isIncoming = false payHash, err := req.PayHash.UnwrapOrErr( fmt.Errorf("no pay hash"), @@ -828,11 +836,16 @@ func localHtlcTimeoutSweepDesc(req lnwallet.ResolutionReq, return lfn.Err[tapscriptSweepDescs](err) } + // We're sweeping an HTLC output, which has a tweaked script key. To be + // able to create the correct control block, we need to tweak the key + // ring with the index of the HTLC. + tweakedKeyRing := TweakedRevocationKeyRing(req.KeyRing, index) + // We'll need to complete the control block to spend the second-level // HTLC, so first we'll make the script tree for the HTLC. htlcScriptTree, err := lnwallet.GenTaprootHtlcScript( - isIncoming, lntypes.Local, htlcExpiry, - payHash, req.KeyRing, lfn.None[txscript.TapLeaf](), + isIncoming, lntypes.Local, htlcExpiry, payHash, tweakedKeyRing, + lfn.None[txscript.TapLeaf](), ) if err != nil { return lfn.Errf[tapscriptSweepDescs]("error creating "+ @@ -900,13 +913,13 @@ func localHtlcTimeoutSweepDesc(req lnwallet.ResolutionReq, }) } -// localHtlcSucessSweepDesc creates a sweep desc for an HTLC output that is +// localHtlcSuccessSweepDesc creates a sweep desc for an HTLC output that is // present on our local commitment transaction that we can sweep with a // preimage. These sweeps take two stages, so we'll add that extra information. -func localHtlcSucessSweepDesc(req lnwallet.ResolutionReq, -) lfn.Result[tapscriptSweepDescs] { +func localHtlcSuccessSweepDesc(req lnwallet.ResolutionReq, + index input.HtlcIndex) lfn.Result[tapscriptSweepDescs] { - isIncoming := true + const isIncoming = true payHash, err := req.PayHash.UnwrapOrErr( fmt.Errorf("no pay hash"), @@ -921,11 +934,16 @@ func localHtlcSucessSweepDesc(req lnwallet.ResolutionReq, return lfn.Err[tapscriptSweepDescs](err) } + // We're sweeping an HTLC output, which has a tweaked script key. To be + // able to create the correct control block, we need to tweak the key + // ring with the index of the HTLC. + tweakedKeyRing := TweakedRevocationKeyRing(req.KeyRing, index) + // We'll need to complete the control block to spend the second-level // HTLC, so first we'll make the script tree for the HTLC. htlcScriptTree, err := lnwallet.GenTaprootHtlcScript( - isIncoming, lntypes.Local, htlcExpiry, - payHash, req.KeyRing, lfn.None[txscript.TapLeaf](), + isIncoming, lntypes.Local, htlcExpiry, payHash, tweakedKeyRing, + lfn.None[txscript.TapLeaf](), ) if err != nil { return lfn.Errf[tapscriptSweepDescs]("error creating "+ @@ -1707,10 +1725,9 @@ func (a *AuxSweeper) resolveContract( // assets for the remote party, which are actually the HTLCs we // sent outgoing. We only care about this particular HTLC, so // we'll filter out the rest. + htlcID := req.HtlcID.UnwrapOr(math.MaxUint64) htlcOutputs := commitState.OutgoingHtlcAssets.Val - assetOutputs = htlcOutputs.FilterByHtlcIndex( - req.HtlcID.UnwrapOr(math.MaxUint64), - ) + assetOutputs = htlcOutputs.FilterByHtlcIndex(htlcID) payHash, err := req.PayHash.UnwrapOrErr(errNoPayHash) if err != nil { @@ -1721,7 +1738,7 @@ func (a *AuxSweeper) resolveContract( // sweep desc for the timeout txn. sweepDesc = remoteHtlcTimeoutSweepDesc( req.KeyRing, payHash[:], req.CsvDelay, - req.CltvDelay.UnwrapOr(0), + req.CltvDelay.UnwrapOr(0), htlcID, ) // The remote party broadcasted a commitment transaction which held an @@ -1730,10 +1747,9 @@ func (a *AuxSweeper) resolveContract( // In this case, it's an outgoing HTLC from the PoV of the // remote party, which is incoming for us. We'll only sweep this // HTLC, so we'll filter out the rest. + htlcID := req.HtlcID.UnwrapOr(math.MaxUint64) htlcOutputs := commitState.IncomingHtlcAssets.Val - assetOutputs = htlcOutputs.FilterByHtlcIndex( - req.HtlcID.UnwrapOr(math.MaxUint64), - ) + assetOutputs = htlcOutputs.FilterByHtlcIndex(htlcID) payHash, err := req.PayHash.UnwrapOrErr(errNoPayHash) if err != nil { @@ -1743,7 +1759,7 @@ func (a *AuxSweeper) resolveContract( // Now that we know which output we'll be sweeping, we'll make a // sweep desc for the timeout txn. sweepDesc = remoteHtlcSuccessSweepDesc( - req.KeyRing, payHash[:], req.CsvDelay, + req.KeyRing, payHash[:], req.CsvDelay, htlcID, ) // In this case, we broadcast a commitment transaction which held an @@ -1753,14 +1769,13 @@ func (a *AuxSweeper) resolveContract( case input.TaprootHtlcLocalOfferedTimeout: // Like the other HTLC cases, there's only a single output we // care about here. + htlcID := req.HtlcID.UnwrapOr(math.MaxUint64) htlcOutputs := commitState.OutgoingHtlcAssets.Val - assetOutputs = htlcOutputs.FilterByHtlcIndex( - req.HtlcID.UnwrapOr(math.MaxUint64), - ) + assetOutputs = htlcOutputs.FilterByHtlcIndex(htlcID) // With the output and pay desc located, we'll now create the // sweep desc. - sweepDesc = localHtlcTimeoutSweepDesc(req) + sweepDesc = localHtlcTimeoutSweepDesc(req, htlcID) needsSecondLevel = true @@ -1769,21 +1784,26 @@ func (a *AuxSweeper) resolveContract( // needed to sweep both this output, as well as the second level // output it creates. case input.TaprootHtlcAcceptedLocalSuccess: + htlcID := req.HtlcID.UnwrapOr(math.MaxUint64) htlcOutputs := commitState.IncomingHtlcAssets.Val - assetOutputs = htlcOutputs.FilterByHtlcIndex( - req.HtlcID.UnwrapOr(math.MaxUint64), - ) + assetOutputs = htlcOutputs.FilterByHtlcIndex(htlcID) // With the output and pay desc located, we'll now create the // sweep desc. - sweepDesc = localHtlcSucessSweepDesc(req) + sweepDesc = localHtlcSuccessSweepDesc(req, htlcID) needsSecondLevel = true default: + // TODO(guggero): Need to do HTLC revocation cases here. + // IMPORTANT: Remember that we applied the HTLC index as a tweak + // to the revocation key on the asset level! That means the + // tweak to the first-level HTLC script key's internal key + // (which is the revocation key) MUST be applied when creating + // a breach sweep transaction! + return lfn.Errf[returnType]("unknown resolution type: %v", req.Type) - // TODO(roasbeef): need to do HTLC revocation casesj:w } tapSweepDesc, err := sweepDesc.Unpack() @@ -2574,3 +2594,25 @@ func (a *AuxSweeper) NotifyBroadcast(req *sweep.BumpRequest, return resp } + +// TweakedRevocationKeyRing returns a new commitment key ring with the +// revocation key tweaked by the given HTLC index. The revocation key is tweaked +// in order to achieve uniqueness for each HTLC output on the asset level. This +// same tweak will need to be applied to the revocation private key in case of +// a breach. +func TweakedRevocationKeyRing(keyRing *lnwallet.CommitmentKeyRing, + index input.HtlcIndex) *lnwallet.CommitmentKeyRing { + + return &lnwallet.CommitmentKeyRing{ + CommitPoint: keyRing.CommitPoint, + LocalCommitKeyTweak: keyRing.LocalCommitKeyTweak, + LocalHtlcKeyTweak: keyRing.LocalHtlcKeyTweak, + LocalHtlcKey: keyRing.LocalHtlcKey, + RemoteHtlcKey: keyRing.RemoteHtlcKey, + ToLocalKey: keyRing.ToLocalKey, + ToRemoteKey: keyRing.ToRemoteKey, + RevocationKey: TweakPubKeyWithIndex( + keyRing.RevocationKey, index, + ), + } +} From cfc459c6ad20fbc59b30fdd675c2a6f8aa5a15ae Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 27 Nov 2024 16:13:02 +0100 Subject: [PATCH 3/7] tapchannel: add HtlcIndex to allocation sort This fixes a bug where the order of the HTLCs would not be deterministic between two peers if they were identical due to them being shards of the same MPP payment. By adding the HTLC index as a further sort argument, the order is again stable and deterministic. --- tapchannel/allocation_sort.go | 63 ++++++++---------------------- tapchannel/allocation_sort_test.go | 36 +++++++++++++++++ 2 files changed, 53 insertions(+), 46 deletions(-) diff --git a/tapchannel/allocation_sort.go b/tapchannel/allocation_sort.go index 469e42737..12be25943 100644 --- a/tapchannel/allocation_sort.go +++ b/tapchannel/allocation_sort.go @@ -2,7 +2,8 @@ package tapchannel import ( "bytes" - "sort" + "cmp" + "slices" ) // InPlaceAllocationSort performs an in-place sort of output allocations. @@ -14,51 +15,21 @@ import ( // transactions, the script does not directly commit to them. Instead, the CLTVs // must be supplied separately to act as a tie-breaker, otherwise we may produce // invalid HTLC signatures if the receiver produces an alternative ordering -// during verification. +// during verification. Because multiple shards of the same MPP payment can be +// identical in all other fields, we also use the HtlcIndex as a final +// tie-breaker. // -// NOTE: Commitment and commitment anchor outputs should have a 0 CLTV value. +// NOTE: Commitment and commitment anchor outputs should have a 0 CLTV and +// HtlcIndex value. func InPlaceAllocationSort(allocations []*Allocation) { - sort.Sort(sortableAllocationSlice{allocations}) -} - -// sortableAllocationSlice is a slice of allocations and the corresponding CLTV -// values of any HTLCs. Commitment and commitment anchor outputs should have a -// CLTV of 0. -type sortableAllocationSlice struct { - allocations []*Allocation -} - -// Len returns the length of the sortableAllocationSlice. -// -// NOTE: Part of the sort.Interface interface. -func (s sortableAllocationSlice) Len() int { - return len(s.allocations) -} - -// Swap exchanges the position of outputs i and j. -// -// NOTE: Part of the sort.Interface interface. -func (s sortableAllocationSlice) Swap(i, j int) { - s.allocations[i], s.allocations[j] = s.allocations[j], s.allocations[i] -} - -// Less is a modified BIP69 output comparison, that sorts based on value, then -// pkScript, then CLTV value. -// -// NOTE: Part of the sort.Interface interface. -func (s sortableAllocationSlice) Less(i, j int) bool { - allocI, allocJ := s.allocations[i], s.allocations[j] - - if allocI.BtcAmount != allocJ.BtcAmount { - return allocI.BtcAmount < allocJ.BtcAmount - } - - pkScriptCmp := bytes.Compare( - allocI.SortTaprootKeyBytes, allocJ.SortTaprootKeyBytes, - ) - if pkScriptCmp != 0 { - return pkScriptCmp < 0 - } - - return allocI.CLTV < allocJ.CLTV + slices.SortFunc(allocations, func(i, j *Allocation) int { + return cmp.Or( + cmp.Compare(i.BtcAmount, j.BtcAmount), + bytes.Compare( + i.SortTaprootKeyBytes, j.SortTaprootKeyBytes, + ), + cmp.Compare(i.CLTV, j.CLTV), + cmp.Compare(i.HtlcIndex, j.HtlcIndex), + ) + }) } diff --git a/tapchannel/allocation_sort_test.go b/tapchannel/allocation_sort_test.go index 5e9b6b7dd..a2f385992 100644 --- a/tapchannel/allocation_sort_test.go +++ b/tapchannel/allocation_sort_test.go @@ -38,6 +38,24 @@ func TestInPlaceAllocationSort(t *testing.T) { SortTaprootKeyBytes: []byte("b"), CLTV: 100, }, + { + BtcAmount: 1000, + SortTaprootKeyBytes: []byte("b"), + CLTV: 100, + HtlcIndex: 1, + }, + { + BtcAmount: 1000, + SortTaprootKeyBytes: []byte("b"), + CLTV: 100, + HtlcIndex: 9, + }, + { + BtcAmount: 1000, + SortTaprootKeyBytes: []byte("b"), + CLTV: 100, + HtlcIndex: 3, + }, { BtcAmount: 1000, SortTaprootKeyBytes: []byte("a"), @@ -60,6 +78,24 @@ func TestInPlaceAllocationSort(t *testing.T) { SortTaprootKeyBytes: []byte("b"), CLTV: 100, }, + { + BtcAmount: 1000, + SortTaprootKeyBytes: []byte("b"), + CLTV: 100, + HtlcIndex: 1, + }, + { + BtcAmount: 1000, + SortTaprootKeyBytes: []byte("b"), + CLTV: 100, + HtlcIndex: 3, + }, + { + BtcAmount: 1000, + SortTaprootKeyBytes: []byte("b"), + CLTV: 100, + HtlcIndex: 9, + }, { BtcAmount: 2000, SortTaprootKeyBytes: []byte("b"), From 9b9e4aab48953f38e818daddafec494d07d6d7ad Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 27 Nov 2024 16:13:03 +0100 Subject: [PATCH 4/7] tapchannel: tweak HTLC second level transaction internal key --- tapchannel/aux_leaf_creator.go | 2 ++ tapchannel/aux_leaf_signer.go | 10 +++++----- tapchannel/aux_sweeper.go | 13 ++++++------- tapchannel/commitment.go | 25 +++++++++++++++++-------- 4 files changed, 30 insertions(+), 20 deletions(-) diff --git a/tapchannel/aux_leaf_creator.go b/tapchannel/aux_leaf_creator.go index 44cc43848..792c2b6e4 100644 --- a/tapchannel/aux_leaf_creator.go +++ b/tapchannel/aux_leaf_creator.go @@ -129,6 +129,7 @@ func FetchLeavesFromCommit(chainParams *address.ChainParams, leaf, err := CreateSecondLevelHtlcTx( chanState, com.CommitTx, htlc.Amt.ToSatoshis(), keys, chainParams, htlcOutputs, cltvTimeout, + htlc.HtlcIndex, ) if err != nil { return lfn.Err[returnType](fmt.Errorf("unable "+ @@ -169,6 +170,7 @@ func FetchLeavesFromCommit(chainParams *address.ChainParams, leaf, err := CreateSecondLevelHtlcTx( chanState, com.CommitTx, htlc.Amt.ToSatoshis(), keys, chainParams, htlcOutputs, cltvTimeout, + htlc.HtlcIndex, ) if err != nil { return lfn.Err[returnType](fmt.Errorf("unable "+ diff --git a/tapchannel/aux_leaf_signer.go b/tapchannel/aux_leaf_signer.go index e8f4fe9a6..c76350451 100644 --- a/tapchannel/aux_leaf_signer.go +++ b/tapchannel/aux_leaf_signer.go @@ -370,7 +370,7 @@ func verifyHtlcSignature(chainParams *address.ChainParams, vPackets, err := htlcSecondLevelPacketsFromCommit( chainParams, chanState, commitTx, baseJob.KeyRing, htlcOutputs, - baseJob, htlcTimeout, + baseJob, htlcTimeout, baseJob.HTLC.HtlcIndex, ) if err != nil { return fmt.Errorf("error generating second level packets: %w", @@ -514,7 +514,7 @@ func (s *AuxLeafSigner) generateHtlcSignature(chanState lnwallet.AuxChanState, vPackets, err := htlcSecondLevelPacketsFromCommit( s.cfg.ChainParams, chanState, commitTx, baseJob.KeyRing, - htlcOutputs, baseJob, htlcTimeout, + htlcOutputs, baseJob, htlcTimeout, baseJob.HTLC.HtlcIndex, ) if err != nil { return lnwallet.AuxSigJobResp{}, fmt.Errorf("error generating "+ @@ -602,12 +602,12 @@ func (s *AuxLeafSigner) generateHtlcSignature(chanState lnwallet.AuxChanState, func htlcSecondLevelPacketsFromCommit(chainParams *address.ChainParams, chanState lnwallet.AuxChanState, commitTx *wire.MsgTx, keyRing lnwallet.CommitmentKeyRing, htlcOutputs []*cmsg.AssetOutput, - baseJob lnwallet.BaseAuxJob, - htlcTimeout fn.Option[uint32]) ([]*tappsbt.VPacket, error) { + baseJob lnwallet.BaseAuxJob, htlcTimeout fn.Option[uint32], + htlcIndex uint64) ([]*tappsbt.VPacket, error) { packets, _, err := CreateSecondLevelHtlcPackets( chanState, commitTx, baseJob.HTLC.Amount.ToSatoshis(), - keyRing, chainParams, htlcOutputs, htlcTimeout, + keyRing, chainParams, htlcOutputs, htlcTimeout, htlcIndex, ) if err != nil { return nil, fmt.Errorf("error creating second level HTLC "+ diff --git a/tapchannel/aux_sweeper.go b/tapchannel/aux_sweeper.go index 56f7861a5..c23bcabe9 100644 --- a/tapchannel/aux_sweeper.go +++ b/tapchannel/aux_sweeper.go @@ -217,10 +217,12 @@ func (a *AuxSweeper) createSweepVpackets(sweepInputs []*cmsg.AssetOutput, cltvTimeout = fn.Some(uint32(delay)) }) + htlcIndex := resReq.HtlcID.UnwrapOr(math.MaxUint64) alloc, err := createSecondLevelHtlcAllocations( resReq.ChanType, resReq.Initiator, sweepInputs, resReq.HtlcAmt, resReq.CommitCsvDelay, *resReq.KeyRing, fn.Some(resReq.ContractPoint.Index), cltvTimeout, + htlcIndex, ) if err != nil { return lfn.Err[returnType](err) @@ -364,8 +366,7 @@ func (a *AuxSweeper) signSweepVpackets(vPackets []*tappsbt.VPacket, // tweaks to generate the key we'll use to verify the // signature. signingKey, leafToSign := applySignDescToVIn( - signDesc, vIn, &a.cfg.ChainParams, - tapTweak, + signDesc, vIn, &a.cfg.ChainParams, tapTweak, ) // In this case, the witness isn't special, so we'll set the @@ -876,7 +877,7 @@ func localHtlcTimeoutSweepDesc(req lnwallet.ResolutionReq, // As this is an HTLC on our local commitment transaction, we'll also // need to generate a sweep desc for second level HTLC. secondLevelScriptTree, err := input.TaprootSecondLevelScriptTree( - req.KeyRing.RevocationKey, req.KeyRing.ToLocalKey, + tweakedKeyRing.RevocationKey, req.KeyRing.ToLocalKey, req.CommitCsvDelay, lfn.None[txscript.TapLeaf](), ) if err != nil { @@ -978,7 +979,7 @@ func localHtlcSuccessSweepDesc(req lnwallet.ResolutionReq, // As this is an HTLC on our local commitment transaction, we'll also // need to generate a sweep desc for second level HTLC. secondLevelScriptTree, err := input.TaprootSecondLevelScriptTree( - req.KeyRing.RevocationKey, req.KeyRing.ToLocalKey, + tweakedKeyRing.RevocationKey, req.KeyRing.ToLocalKey, req.CommitCsvDelay, lfn.None[txscript.TapLeaf](), ) if err != nil { @@ -2408,9 +2409,7 @@ func (a *AuxSweeper) registerAndBroadcastSweep(req *sweep.BumpRequest, } // Now that we have our vPkts, we'll re-create the output commitments. - outCommitments, err := tapsend.CreateOutputCommitments( - vPkts.allPkts(), - ) + outCommitments, err := tapsend.CreateOutputCommitments(vPkts.allPkts()) if err != nil { return fmt.Errorf("unable to create output "+ "commitments: %w", err) diff --git a/tapchannel/commitment.go b/tapchannel/commitment.go index 808ae2e57..a68eb025b 100644 --- a/tapchannel/commitment.go +++ b/tapchannel/commitment.go @@ -1263,7 +1263,7 @@ func createSecondLevelHtlcAllocations(chanType channeldb.ChannelType, initiator bool, htlcOutputs []*cmsg.AssetOutput, htlcAmt btcutil.Amount, commitCsvDelay uint32, keys lnwallet.CommitmentKeyRing, outputIndex fn.Option[uint32], htlcTimeout fn.Option[uint32], -) ([]*Allocation, error) { + htlcIndex uint64) ([]*Allocation, error) { // TODO(roasbeef): thaw height not implemented for taproot chans rn // (lease expiry) @@ -1284,6 +1284,13 @@ func createSecondLevelHtlcAllocations(chanType channeldb.ChannelType, "script sibling: %w", err) } + // To ensure uniqueness of the script key across HTLCs with the same + // payment hash and timeout (which would be equal otherwise), we tweak + // the asset level internal key of the second-level script key as well + // with the HTLC index. We'll ONLY use this for the asset level, NOT for + // the BTC level. + tweakedTree := TweakHtlcTree(htlcTree, htlcIndex) + allocations := []*Allocation{{ Type: SecondLevelHtlcAllocation, // If we're making the second-level transaction just to sign, @@ -1299,12 +1306,14 @@ func createSecondLevelHtlcAllocations(chanType channeldb.ChannelType, ), InternalKey: htlcTree.InternalKey, NonAssetLeaves: sibling, - ScriptKey: asset.NewScriptKey(htlcTree.TaprootKey), + ScriptKey: asset.NewScriptKey(tweakedTree.TaprootKey), SortTaprootKeyBytes: schnorr.SerializePubKey( + // This _must_ remain the non-tweaked key, since this is + // used for sorting _before_ applying any TAP tweaks. htlcTree.TaprootKey, ), - // TODO(roasbeef): don't need it here? - CLTV: htlcTimeout.UnwrapOr(0), + CLTV: htlcTimeout.UnwrapOr(0), + HtlcIndex: htlcIndex, }} return allocations, nil @@ -1316,12 +1325,12 @@ func CreateSecondLevelHtlcPackets(chanState lnwallet.AuxChanState, commitTx *wire.MsgTx, htlcAmt btcutil.Amount, keys lnwallet.CommitmentKeyRing, chainParams *address.ChainParams, htlcOutputs []*cmsg.AssetOutput, htlcTimeout fn.Option[uint32], -) ([]*tappsbt.VPacket, []*Allocation, error) { + htlcIndex uint64) ([]*tappsbt.VPacket, []*Allocation, error) { allocations, err := createSecondLevelHtlcAllocations( chanState.ChanType, chanState.IsInitiator, htlcOutputs, htlcAmt, uint32(chanState.LocalChanCfg.CsvDelay), - keys, fn.None[uint32](), htlcTimeout, + keys, fn.None[uint32](), htlcTimeout, htlcIndex, ) if err != nil { return nil, nil, err @@ -1367,13 +1376,13 @@ func CreateSecondLevelHtlcTx(chanState lnwallet.AuxChanState, commitTx *wire.MsgTx, htlcAmt btcutil.Amount, keys lnwallet.CommitmentKeyRing, chainParams *address.ChainParams, htlcOutputs []*cmsg.AssetOutput, htlcTimeout fn.Option[uint32], -) (input.AuxTapLeaf, error) { + htlcIndex uint64) (input.AuxTapLeaf, error) { none := input.NoneTapLeaf() vPackets, allocations, err := CreateSecondLevelHtlcPackets( chanState, commitTx, htlcAmt, keys, chainParams, htlcOutputs, - htlcTimeout, + htlcTimeout, htlcIndex, ) if err != nil { return none, fmt.Errorf("error creating second level HTLC "+ From 2ee822ad7a39324fb6c9422cd47ac81e127bc4ac Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 27 Nov 2024 16:13:04 +0100 Subject: [PATCH 5/7] multi: add trace log outputs This commit adds a couple of trace outputs that were helpful in finding two bugs fixed in this series of commits. We also increase the number of levels the spew logger is allowed to print, so we can see a bit more. We need this limit to avoid endless loops in self-referencing structs such as proofs. --- tapchannel/commitment.go | 14 ++++++++++++++ tapchannel/log.go | 2 +- tapsend/send.go | 4 ++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/tapchannel/commitment.go b/tapchannel/commitment.go index a68eb025b..e8bf200ff 100644 --- a/tapchannel/commitment.go +++ b/tapchannel/commitment.go @@ -763,6 +763,13 @@ func CreateAllocations(chanState lnwallet.AuxChanState, ourBalance, // asset level, NOT for the BTC level. tweakedTree := TweakHtlcTree(htlcTree, htlc.HtlcIndex) + log.Tracef("Tweaking HTLC script key with index %d: internal "+ + "key %x -> %x, script key %x -> %x", htlc.HtlcIndex, + htlcTree.InternalKey.SerializeCompressed(), + tweakedTree.InternalKey.SerializeCompressed(), + schnorr.SerializePubKey(htlcTree.TaprootKey), + schnorr.SerializePubKey(tweakedTree.TaprootKey)) + allocations = append(allocations, &Allocation{ Type: allocType, Amount: rfqmsg.Sum(htlc.AssetBalances), @@ -1291,6 +1298,13 @@ func createSecondLevelHtlcAllocations(chanType channeldb.ChannelType, // the BTC level. tweakedTree := TweakHtlcTree(htlcTree, htlcIndex) + log.Tracef("Tweaking second level HTLC script key with index %d: "+ + "internal key %x -> %x, script key %x -> %x", htlcIndex, + htlcTree.InternalKey.SerializeCompressed(), + tweakedTree.InternalKey.SerializeCompressed(), + schnorr.SerializePubKey(htlcTree.TaprootKey), + schnorr.SerializePubKey(tweakedTree.TaprootKey)) + allocations := []*Allocation{{ Type: SecondLevelHtlcAllocation, // If we're making the second-level transaction just to sign, diff --git a/tapchannel/log.go b/tapchannel/log.go index c87da7dfd..8836cb689 100644 --- a/tapchannel/log.go +++ b/tapchannel/log.go @@ -17,7 +17,7 @@ var log = btclog.Disabled // to 4 levels, so it can safely be used for things that contain an MS-SMT tree. var limitSpewer = &spew.ConfigState{ Indent: " ", - MaxDepth: 5, + MaxDepth: 7, } // DisableLog disables all library log output. Logging output is disabled diff --git a/tapsend/send.go b/tapsend/send.go index 8fd1363a4..773fdb823 100644 --- a/tapsend/send.go +++ b/tapsend/send.go @@ -19,6 +19,7 @@ import ( "github.com/btcsuite/btcd/txscript" "github.com/btcsuite/btcd/wire" "github.com/btcsuite/btclog" + "github.com/davecgh/go-spew/spew" "github.com/lightninglabs/lndclient" "github.com/lightninglabs/taproot-assets/address" "github.com/lightninglabs/taproot-assets/asset" @@ -974,6 +975,9 @@ func CreateTaprootSignature(vIn *tappsbt.VInput, virtualTx *wire.MsgTx, "from virtual transaction packet") } + log.Tracef("Signing virtual TX with descriptor %v", + spew.Sdump(spendDesc)) + sig, err := txSigner.SignVirtualTx(&spendDesc, virtualTx, prevOut) if err != nil { return nil, err From 3451af2c53a07c08afc0793a397342d0c8f72239 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 27 Nov 2024 16:13:05 +0100 Subject: [PATCH 6/7] tapchannel: fix change output index calculation This fixes a bug where we previously assumed there would only ever be one asset input in a sweep. But when we sweep multiple HTLCs at the same time, the change output index isn't static and needs to be calculated based on the asset commitment output indexes. --- tapchannel/aux_sweeper.go | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/tapchannel/aux_sweeper.go b/tapchannel/aux_sweeper.go index c23bcabe9..1efe09bb5 100644 --- a/tapchannel/aux_sweeper.go +++ b/tapchannel/aux_sweeper.go @@ -2256,26 +2256,20 @@ func (a *AuxSweeper) sweepContracts(inputs []input.Input, // sweepExclusionProofGen is a helper function that generates an exclusion // proof for the internal key of the change output. func sweepExclusionProofGen(sweepInternalKey keychain.KeyDescriptor, -) tapsend.ExclusionProofGenerator { + changeOutputIndex uint32) tapsend.ExclusionProofGenerator { return func(target *proof.BaseProofParams, isAnchor tapsend.IsAnchor) error { - tsProof, err := proof.CreateTapscriptProof(nil) - if err != nil { - return fmt.Errorf("error creating tapscript proof: %w", - err) - } - // We only need to generate an exclusion proof for the second // output in the commitment transaction. - // - // TODO(roasbeef): case of no change? target.ExclusionProofs = append( target.ExclusionProofs, proof.TaprootProof{ - OutputIndex: 1, - InternalKey: sweepInternalKey.PubKey, - TapscriptProof: tsProof, + OutputIndex: changeOutputIndex, + InternalKey: sweepInternalKey.PubKey, + TapscriptProof: &proof.TapscriptProof{ + Bip86: true, + }, }, ) @@ -2415,6 +2409,15 @@ func (a *AuxSweeper) registerAndBroadcastSweep(req *sweep.BumpRequest, "commitments: %w", err) } + // We need to find out what the highest output index of any asset output + // commitments is, so we know the change output will be one higher. + highestOutputIndex := uint32(0) + for outIdx := range outCommitments { + if outIdx > highestOutputIndex { + highestOutputIndex = outIdx + } + } + changeInternalKey, err := req.DeliveryAddress.InternalKey.UnwrapOrErr( fmt.Errorf("change internal key not populated"), ) @@ -2435,8 +2438,11 @@ func (a *AuxSweeper) registerAndBroadcastSweep(req *sweep.BumpRequest, for idx := range allVpkts { vPkt := allVpkts[idx] for outIdx := range vPkt.Outputs { + // The change output is always the last output in the + // commitment transaction, one index higher than the + // highest asset commitment output index. exclusionCreator := sweepExclusionProofGen( - changeInternalKey, + changeInternalKey, highestOutputIndex+1, ) proofSuffix, err := tapsend.CreateProofSuffixCustom( From 38b28f242e497968f181b9342d778ec60d1ae124 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 27 Nov 2024 16:13:06 +0100 Subject: [PATCH 7/7] tapdb: allow tweak to be upserted This fixes a bug where if we ever inserted a script key before knowing its tweak, we could never update the tweak later on when we learn it. This meant that such keys would be seen as BIP-086 keys, even though we later learn they aren't. --- tapdb/addrs_test.go | 50 +++++++++++++++++++++++++++++++++++ tapdb/sqlc/assets.sql.go | 12 ++++++--- tapdb/sqlc/queries/assets.sql | 12 ++++++--- 3 files changed, 68 insertions(+), 6 deletions(-) diff --git a/tapdb/addrs_test.go b/tapdb/addrs_test.go index 443197fe4..c3e306ace 100644 --- a/tapdb/addrs_test.go +++ b/tapdb/addrs_test.go @@ -695,6 +695,14 @@ func assertKeyKnowledge(t *testing.T, ctx context.Context, require.Equal(t, known, dbScriptKey.DeclaredKnown) } +func assertTweak(t *testing.T, ctx context.Context, addrBook *TapAddressBook, + scriptKey asset.ScriptKey, tweak []byte) { + + dbScriptKey, err := addrBook.FetchScriptKey(ctx, scriptKey.PubKey) + require.NoError(t, err) + require.Equal(t, tweak, dbScriptKey.Tweak) +} + // TestScriptKeyKnownUpsert tests that we can insert a script key, then insert // it again declared as known. func TestScriptKeyKnownUpsert(t *testing.T) { @@ -757,3 +765,45 @@ func TestScriptKeyKnownUpsert(t *testing.T) { assertKeyKnowledge(t, ctx, addrBook, scriptKey, known) }) } + +// TestScriptKeyTweakUpsert tests that we can insert a script key, then insert +// it again when we know the tweak for it. +func TestScriptKeyTweakUpsert(t *testing.T) { + t.Parallel() + + // First, make a new addr book instance we'll use in the test below. + testClock := clock.NewTestClock(time.Now()) + addrBook, _ := newAddrBook(t, testClock) + + ctx := context.Background() + + // In this test, we insert the tweak as NULL, and make sure we overwrite + // it with an actual value again later. + t.Run("null_to_value", func(t *testing.T) { + known := false + scriptKey := randScriptKey(t) + scriptKey.Tweak = nil + + // We'll insert a random script key into the database. We won't + // declare it as known though, and it doesn't have the tweak. + err := addrBook.InsertScriptKey(ctx, scriptKey, known) + require.NoError(t, err) + + // We'll fetch the script key and confirm that it's not known. + assertKeyKnowledge(t, ctx, addrBook, scriptKey, known) + assertTweak(t, ctx, addrBook, scriptKey, nil) + + known = true + randTweak := test.RandBytes(32) + scriptKey.Tweak = randTweak + + // We'll now insert it again, but this time declare it as known + // and also know the tweak. + err = addrBook.InsertScriptKey(ctx, scriptKey, known) + require.NoError(t, err) + + // We'll fetch the script key and confirm that it's known. + assertKeyKnowledge(t, ctx, addrBook, scriptKey, known) + assertTweak(t, ctx, addrBook, scriptKey, randTweak) + }) +} diff --git a/tapdb/sqlc/assets.sql.go b/tapdb/sqlc/assets.sql.go index fa1ca8f87..a518ebbc3 100644 --- a/tapdb/sqlc/assets.sql.go +++ b/tapdb/sqlc/assets.sql.go @@ -2878,8 +2878,8 @@ INSERT INTO script_keys ( ) VALUES ( $1, $2, $3, $4 ) ON CONFLICT (tweaked_script_key) - -- As a NOP, we just set the script key to the one that triggered the - -- conflict. + -- Overwrite the declared_known and tweak fields if they were previously + -- unknown. DO UPDATE SET tweaked_script_key = EXCLUDED.tweaked_script_key, -- If the script key was previously unknown, we'll update to the new @@ -2888,7 +2888,13 @@ INSERT INTO script_keys ( WHEN script_keys.declared_known IS NULL OR script_keys.declared_known = FALSE THEN COALESCE(EXCLUDED.declared_known, script_keys.declared_known) ELSE script_keys.declared_known - END + END, + -- If the tweak was previously unknown, we'll update to the new value. + tweak = CASE + WHEN script_keys.tweak IS NULL + THEN COALESCE(EXCLUDED.tweak, script_keys.tweak) + ELSE script_keys.tweak + END RETURNING script_key_id ` diff --git a/tapdb/sqlc/queries/assets.sql b/tapdb/sqlc/queries/assets.sql index a16302d27..78bfa313b 100644 --- a/tapdb/sqlc/queries/assets.sql +++ b/tapdb/sqlc/queries/assets.sql @@ -852,8 +852,8 @@ INSERT INTO script_keys ( ) VALUES ( $1, $2, $3, $4 ) ON CONFLICT (tweaked_script_key) - -- As a NOP, we just set the script key to the one that triggered the - -- conflict. + -- Overwrite the declared_known and tweak fields if they were previously + -- unknown. DO UPDATE SET tweaked_script_key = EXCLUDED.tweaked_script_key, -- If the script key was previously unknown, we'll update to the new @@ -862,7 +862,13 @@ INSERT INTO script_keys ( WHEN script_keys.declared_known IS NULL OR script_keys.declared_known = FALSE THEN COALESCE(EXCLUDED.declared_known, script_keys.declared_known) ELSE script_keys.declared_known - END + END, + -- If the tweak was previously unknown, we'll update to the new value. + tweak = CASE + WHEN script_keys.tweak IS NULL + THEN COALESCE(EXCLUDED.tweak, script_keys.tweak) + ELSE script_keys.tweak + END RETURNING script_key_id; -- name: FetchScriptKeyIDByTweakedKey :one