diff --git a/lnwallet/channel.go b/lnwallet/channel.go index fbf6116f..c9ca227d 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -67,6 +67,10 @@ var ( ErrBelowMinHTLC = fmt.Errorf("proposed HTLC value is below minimum " + "allowed HTLC value") + // ErrInvalidHTLCAmt signals that a proposed HTLC has a value that is + // not positive. + ErrInvalidHTLCAmt = fmt.Errorf("proposed HTLC value must be positive") + // ErrCannotSyncCommitChains is returned if, upon receiving a ChanSync // message, the state machine deems that is unable to properly // synchronize states with the remote peer. In this case we should fail @@ -549,7 +553,7 @@ type commitment struct { // transition. This ensures that we don't assign multiple HTLC's to the same // index within the commitment transaction. func locateOutputIndex(p *PaymentDescriptor, tx *wire.MsgTx, ourCommit bool, - dups map[PaymentHash][]int32) (int32, error) { + dups map[PaymentHash][]int32, cltvs []uint32) (int32, error) { // Checks to see if element (e) exists in slice (s). contains := func(s []int32, e int32) bool { @@ -571,8 +575,11 @@ func locateOutputIndex(p *PaymentDescriptor, tx *wire.MsgTx, ourCommit bool, } for i, txOut := range tx.TxOut { + cltv := cltvs[i] + if bytes.Equal(txOut.PkScript, pkScript) && - txOut.Value == int64(p.Amount.ToSatoshis()) { + txOut.Value == int64(p.Amount.ToSatoshis()) && + cltv == p.Timeout { // If this payment hash and index has already been // found, then we'll continue in order to avoid any @@ -587,8 +594,8 @@ func locateOutputIndex(p *PaymentDescriptor, tx *wire.MsgTx, ourCommit bool, } } - return 0, fmt.Errorf("unable to find htlc: script=%x, value=%v", - pkScript, p.Amount) + return 0, fmt.Errorf("unable to find htlc: script=%x, value=%v, "+ + "cltv=%v", pkScript, p.Amount, p.Timeout) } // populateHtlcIndexes modifies the set of HTLC's locked-into the target view @@ -596,7 +603,9 @@ func locateOutputIndex(p *PaymentDescriptor, tx *wire.MsgTx, ourCommit bool, // we need to keep track of the indexes of each HTLC in order to properly write // the current state to disk, and also to locate the PaymentDescriptor // corresponding to HTLC outputs in the commitment transaction. -func (c *commitment) populateHtlcIndexes(chanType channeldb.ChannelType) error { +func (c *commitment) populateHtlcIndexes(chanType channeldb.ChannelType, + cltvs []uint32) error { + // First, we'll set up some state to allow us to locate the output // index of the all the HTLC's within the commitment transaction. We // must keep this index so we can validate the HTLC signatures sent to @@ -632,7 +641,7 @@ func (c *commitment) populateHtlcIndexes(chanType channeldb.ChannelType) error { // signatures. case c.isOurs: htlc.localOutputIndex, err = locateOutputIndex( - htlc, c.txn, c.isOurs, dups, + htlc, c.txn, c.isOurs, dups, cltvs, ) if err != nil { return err @@ -653,7 +662,7 @@ func (c *commitment) populateHtlcIndexes(chanType channeldb.ChannelType) error { // index within the HTLC index. case !c.isOurs: htlc.remoteOutputIndex, err = locateOutputIndex( - htlc, c.txn, c.isOurs, dups, + htlc, c.txn, c.isOurs, dups, cltvs, ) if err != nil { return err @@ -766,7 +775,8 @@ func (c *commitment) toDiskCommit(ourCommit bool) *channeldb.ChannelCommitment { // restart a channel session. func (lc *LightningChannel) diskHtlcToPayDesc(feeRate chainfee.SatPerKWeight, commitHeight uint64, htlc *channeldb.HTLC, localCommitKeys, - remoteCommitKeys *CommitmentKeyRing) (PaymentDescriptor, error) { + remoteCommitKeys *CommitmentKeyRing, isLocal bool) (PaymentDescriptor, + error) { // The proper pkScripts for this PaymentDescriptor must be // generated so we can easily locate them within the commitment @@ -811,6 +821,19 @@ func (lc *LightningChannel) diskHtlcToPayDesc(feeRate chainfee.SatPerKWeight, } } + // Reconstruct the proper local/remote output indexes from the HTLC's + // persisted output index depending on whose commitment we are + // generating. + var ( + localOutputIndex int32 + remoteOutputIndex int32 + ) + if isLocal { + localOutputIndex = htlc.OutputIndex + } else { + remoteOutputIndex = htlc.OutputIndex + } + // With the scripts reconstructed (depending on if this is our commit // vs theirs or a pending commit for the remote party), we can now // re-create the original payment descriptor. @@ -822,6 +845,8 @@ func (lc *LightningChannel) diskHtlcToPayDesc(feeRate chainfee.SatPerKWeight, HtlcIndex: htlc.HtlcIndex, LogIndex: htlc.LogIndex, OnionBlob: htlc.OnionBlob, + localOutputIndex: localOutputIndex, + remoteOutputIndex: remoteOutputIndex, ourPkScript: ourP2WSH, ourWitnessScript: ourWitnessScript, theirPkScript: theirP2WSH, @@ -837,7 +862,8 @@ func (lc *LightningChannel) diskHtlcToPayDesc(feeRate chainfee.SatPerKWeight, // for each side. func (lc *LightningChannel) extractPayDescs(commitHeight uint64, feeRate chainfee.SatPerKWeight, htlcs []channeldb.HTLC, localCommitKeys, - remoteCommitKeys *CommitmentKeyRing) ([]PaymentDescriptor, []PaymentDescriptor, error) { + remoteCommitKeys *CommitmentKeyRing, isLocal bool) ([]PaymentDescriptor, + []PaymentDescriptor, error) { var ( incomingHtlcs []PaymentDescriptor @@ -855,6 +881,7 @@ func (lc *LightningChannel) extractPayDescs(commitHeight uint64, payDesc, err := lc.diskHtlcToPayDesc( feeRate, commitHeight, &htlc, localCommitKeys, remoteCommitKeys, + isLocal, ) if err != nil { return incomingHtlcs, outgoingHtlcs, err @@ -905,6 +932,7 @@ func (lc *LightningChannel) diskCommitToMemCommit(isLocal bool, diskCommit.CommitHeight, chainfee.SatPerKWeight(diskCommit.FeePerKw), diskCommit.Htlcs, localCommitKeys, remoteCommitKeys, + isLocal, ) if err != nil { return nil, err @@ -934,13 +962,6 @@ func (lc *LightningChannel) diskCommitToMemCommit(isLocal bool, commit.dustLimit = lc.channelState.RemoteChanCfg.DustLimit } - // Finally, we'll re-populate the HTLC index for this state so we can - // properly locate each HTLC within the commitment transaction. - err = commit.populateHtlcIndexes(lc.channelState.ChanType) - if err != nil { - return nil, err - } - return commit, nil } @@ -2428,8 +2449,11 @@ func (lc *LightningChannel) fetchCommitmentView(remoteChain bool, } // Finally, we'll populate all the HTLC indexes so we can track the - // locations of each HTLC in the commitment state. - if err := c.populateHtlcIndexes(lc.channelState.ChanType); err != nil { + // locations of each HTLC in the commitment state. We pass in the sorted + // slice of CLTV deltas in order to properly locate HTLCs that otherwise + // have the same payment hash and amount. + err = c.populateHtlcIndexes(lc.channelState.ChanType, commitTx.cltvs) + if err != nil { return nil, err } @@ -3214,6 +3238,11 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter, amtInFlight += entry.Amount numInFlight++ + // Check that the HTLC amount is positive. + if entry.Amount == 0 { + return ErrInvalidHTLCAmt + } + // Check that the value of the HTLC they added // is above our minimum. if entry.Amount < constraints.MinHTLC { diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 47355f57..c266db1c 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -450,6 +450,139 @@ func TestCheckCommitTxSize(t *testing.T) { } } +// TestCommitHTLCSigTieBreak asserts that HTLC signatures are sent the proper +// BIP69+CLTV sorting expected by BOLT 3 when multiple HTLCs have identical +// payment hashes and amounts, but differing CLTVs. This is exercised by adding +// the HTLCs in the descending order of their CLTVs, and asserting that their +// order is reversed when signing. +func TestCommitHTLCSigTieBreak(t *testing.T) { + t.Run("no restart", func(t *testing.T) { + testCommitHTLCSigTieBreak(t, false) + }) + t.Run("restart", func(t *testing.T) { + testCommitHTLCSigTieBreak(t, true) + }) +} + +func testCommitHTLCSigTieBreak(t *testing.T, restart bool) { + aliceChannel, bobChannel, cleanUp, err := CreateTestChannels( + channeldb.SingleFunderTweaklessBit, + ) + if err != nil { + t.Fatalf("unable to create test channels; %v", err) + } + defer cleanUp() + + const ( + htlcAmt = lnwire.MilliSatoshi(20000000) + numHtlcs = 2 + ) + + // Add HTLCs with identical payment hashes and amounts, but descending + // CLTV values. We will expect the signatures to appear in the reverse + // order that the HTLCs are added due to the commitment sorting. + for i := 0; i < numHtlcs; i++ { + var ( + preimage lntypes.Preimage + hash = preimage.Hash() + ) + + htlc := &lnwire.UpdateAddHTLC{ + ID: uint64(i), + PaymentHash: hash, + Amount: htlcAmt, + Expiry: uint32(numHtlcs - i), + } + + if _, err := aliceChannel.AddHTLC(htlc, nil); err != nil { + t.Fatalf("alice unable to add htlc: %v", err) + } + if _, err := bobChannel.ReceiveHTLC(htlc); err != nil { + t.Fatalf("bob unable to receive htlc: %v", err) + } + } + + // Have Alice initiate the first half of the commitment dance. The + // tie-breaking for commitment sorting won't affect the commitment + // signed by Alice because received HTLC scripts commit to the CLTV + // directly, so the outputs will have different scriptPubkeys. + aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment() + if err != nil { + t.Fatalf("unable to sign alice's commitment: %v", err) + } + + err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs) + if err != nil { + t.Fatalf("unable to receive alice's commitment: %v", err) + } + + bobRevocation, _, err := bobChannel.RevokeCurrentCommitment() + if err != nil { + t.Fatalf("unable to revoke bob's commitment: %v", err) + } + _, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation) + if err != nil { + t.Fatalf("unable to receive bob's revocation: %v", err) + } + + // Now have Bob initiate the second half of the commitment dance. Here + // the offered HTLC scripts he adds for Alice will need to have the + // tie-breaking applied because the CLTV is not committed, but instead + // implicit via the construction of the second-level transactions. + bobSig, bobHtlcSigs, bobHtlcs, err := bobChannel.SignNextCommitment() + if err != nil { + t.Fatalf("unable to sign bob's commitment: %v", err) + } + + if len(bobHtlcs) != numHtlcs { + t.Fatalf("expected %d htlcs, got: %v", numHtlcs, len(bobHtlcs)) + } + + // Ensure that our HTLCs appear in the reverse order from which they + // were added by inspecting each's outpoint index. We expect the output + // indexes to be in descending order, i.e. the first HTLC added had the + // highest CLTV and should end up last. + lastIndex := bobHtlcs[0].OutputIndex + for i, htlc := range bobHtlcs[1:] { + if htlc.OutputIndex >= lastIndex { + t.Fatalf("htlc %d output index %d is not descending", + i, htlc.OutputIndex) + } + + lastIndex = htlc.OutputIndex + } + + // If requsted, restart Alice so that we can test that the necessary + // indexes can be reconstructed before needing to validate the + // signatures from Bob. + if restart { + aliceState := aliceChannel.channelState + aliceChannels, err := aliceState.Db.FetchOpenChannels( + aliceState.IdentityPub, + ) + if err != nil { + t.Fatalf("unable to fetch channel: %v", err) + } + + aliceChannelNew, err := NewLightningChannel( + aliceChannel.Signer, aliceChannels[0], + aliceChannel.sigPool, + ) + if err != nil { + t.Fatalf("unable to create new channel: %v", err) + } + + aliceChannel = aliceChannelNew + } + + // Finally, have Alice validate the signatures to ensure that she is + // expecting the signatures in the proper order. + err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs) + if err != nil { + t.Fatalf("unable to receive bob's commitment: %v", err) + } +} + func TestCooperativeChannelClosure(t *testing.T) { t.Parallel() @@ -6503,6 +6636,41 @@ func TestMinHTLC(t *testing.T) { } } +// TestInvalidHTLCAmt tests that ErrInvalidHTLCAmt is returned when trying to +// add HTLCs that don't carry a positive value. +func TestInvalidHTLCAmt(t *testing.T) { + t.Parallel() + + // We'll kick off the test by creating our channels which both are + // loaded with 5 BTC each. + aliceChannel, bobChannel, cleanUp, err := CreateTestChannels( + channeldb.SingleFunderTweaklessBit, + ) + if err != nil { + t.Fatalf("unable to create test channels: %v", err) + } + defer cleanUp() + + // We'll set the min HTLC values for each party to zero, which + // technically would permit zero-value HTLCs. + aliceChannel.channelState.LocalChanCfg.MinHTLC = 0 + bobChannel.channelState.RemoteChanCfg.MinHTLC = 0 + + // Create a zero-value HTLC. + htlcAmt := lnwire.MilliSatoshi(0) + htlc, _ := createHTLC(0, htlcAmt) + + // Sending or receiving the HTLC should fail with ErrInvalidHTLCAmt. + _, err = aliceChannel.AddHTLC(htlc, nil) + if err != ErrInvalidHTLCAmt { + t.Fatalf("expected ErrInvalidHTLCAmt, got: %v", err) + } + _, err = bobChannel.ReceiveHTLC(htlc) + if err != ErrInvalidHTLCAmt { + t.Fatalf("expected ErrInvalidHTLCAmt, got: %v", err) + } +} + // TestNewBreachRetributionSkipsDustHtlcs ensures that in the case of a // contract breach, all dust HTLCs are ignored and not reflected in the // produced BreachRetribution struct. We ignore these HTLCs as they aren't diff --git a/lnwallet/commitment.go b/lnwallet/commitment.go index 251f7b20..d70f3ca0 100644 --- a/lnwallet/commitment.go +++ b/lnwallet/commitment.go @@ -371,12 +371,21 @@ type unsignedCommitmentTx struct { // fee is the total fee of the commitment transaction. fee btcutil.Amount - // ourBalance|theirBalance are the balances of this commitment *after* - // subtracting commitment fees and anchor outputs. This can be - // different than the balances before creating the commitment - // transaction as one party must pay the commitment fee. - ourBalance lnwire.MilliSatoshi + // ourBalance is our balance on this commitment *after* subtracting + // commitment fees and anchor outputs. This can be different than the + // balances before creating the commitment transaction as one party must + // pay the commitment fee. + ourBalance lnwire.MilliSatoshi + + // theirBalance is their balance of this commitment *after* subtracting + // commitment fees and anchor outputs. This can be different than the + // balances before creating the commitment transaction as one party must + // pay the commitment fee. theirBalance lnwire.MilliSatoshi + + // cltvs is a sorted list of CLTV deltas for each HTLC on the commitment + // transaction. Any non-htlc outputs will have a CLTV delay of zero. + cltvs []uint32 } // createUnsignedCommitmentTx generates the unsigned commitment transaction for @@ -557,6 +566,7 @@ func (cb *CommitmentBuilder) createUnsignedCommitmentTx(ourBalance, fee: commitFee, ourBalance: ourBalance, theirBalance: theirBalance, + cltvs: cltvs, }, nil }