From 97761833d215bb5387bf337dc258a5fa749be823 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Mon, 30 Mar 2020 15:49:16 -0700 Subject: [PATCH 1/5] lnwallet/channel: populate htlc indexes from disk commit We currently write each HTLCs OutputIndex to disk, but we don't use it when restoring. The restoration is modified to use these directly, since we will have lost access to the sorting of CLTVs after the initial signing process. --- lnwallet/channel.go | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 6f64d34d..cc00c7ca 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -766,7 +766,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 +812,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 +836,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 +853,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 +872,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 +923,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 +953,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 } From 294e13eefcf2eee9d075908a23234738e8f3ff4d Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Mon, 30 Mar 2020 15:49:43 -0700 Subject: [PATCH 2/5] lnwallet/commitment: split our/theirBalance comment --- lnwallet/commitment.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/lnwallet/commitment.go b/lnwallet/commitment.go index 251f7b20..2c794b4c 100644 --- a/lnwallet/commitment.go +++ b/lnwallet/commitment.go @@ -371,11 +371,16 @@ 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 } From 3ea3b1d0b53b1c6dc2bafc4ebcb57de1a62834b6 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Mon, 30 Mar 2020 15:50:10 -0700 Subject: [PATCH 3/5] lnwallet/channel: tie-break HTLCs by cltv when locating This commit fixes #4118 by properly sorting the HTLC signatures sent over the wire to match the BOLT3 BIP69+CLTV sorting of the commitment outputs. To do so, we expose the slice of cltv deltas for HTLCs on the unsigned commitment after applying the commitment sorting. This will be used to locate the proper output index, as the CLTV serves as a tie breaker between HTLCs that otherwise have the same payment hash and amount. Note that #3412 fixed the issue partially by ensuring the commitment was constructed properly (and the second-level prev outpoint's txid was correct), but failed to address that the HTLC signatures were still sent out in the incorrect order. With this, we pass the test case introduce in the next commit. --- lnwallet/channel.go | 26 +++++++++++++++++--------- lnwallet/commitment.go | 5 +++++ 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index cc00c7ca..c08a90b1 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -549,7 +549,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 +571,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 +590,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 +599,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 +637,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 +658,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 @@ -2440,8 +2445,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 } diff --git a/lnwallet/commitment.go b/lnwallet/commitment.go index 2c794b4c..d70f3ca0 100644 --- a/lnwallet/commitment.go +++ b/lnwallet/commitment.go @@ -382,6 +382,10 @@ type unsignedCommitmentTx struct { // 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 @@ -562,6 +566,7 @@ func (cb *CommitmentBuilder) createUnsignedCommitmentTx(ourBalance, fee: commitFee, ourBalance: ourBalance, theirBalance: theirBalance, + cltvs: cltvs, }, nil } From b0c3072ff798a31965fd4b9976804188055e0e75 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Mon, 30 Mar 2020 15:50:25 -0700 Subject: [PATCH 4/5] lnwallet/channel_test: assert commit sorting of commit diff htlcs This commit adds a test to exercise that HTLC signatures are sent in the correct order, i.e. they match the sorting of the HTLC outputs on the commitment after applying BOLT 3's BIP69+CLTV sort. --- lnwallet/channel_test.go | 133 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 47355f57..f5c6fb35 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() From 3f4dc0decd192ecdf5878fb554fdeebe3f9660b3 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Thu, 2 Apr 2020 09:31:51 -0700 Subject: [PATCH 5/5] lnwallet/channel: increase htlc validation strictness This commit adds an additional santity check that rejects zero-value HTLCs, preventing them from being added to the channel state even if the channel config's minhtlc value is zero. --- lnwallet/channel.go | 9 +++++++++ lnwallet/channel_test.go | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index c08a90b1..96f53e3e 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 @@ -3234,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 f5c6fb35..c266db1c 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -6636,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