lnwallet: return instead evaluated commitment instead of mutating

createCommitmentTx would earlier mutate the passed commitment struct
after evaluating the htlc view and calculating the final balances, which
was confusing since the balances are supposed to only be *after*
subtracting fees.

Instead we take the needed parameters as arguments, and return the final
balances, tx and fee to populate the commitment struct in a proper way.

lnwallet/channel.go

@@ -2167,33 +2167,41 @@ func (lc *LightningChannel) fetchCommitmentView(remoteChain bool,
 	keyRing *CommitmentKeyRing) (*commitment, error) {
 
 	commitChain := lc.localCommitChain
+	dustLimit := lc.channelState.LocalChanCfg.DustLimit
 	if remoteChain {
 		commitChain = lc.remoteCommitChain
+		dustLimit = lc.channelState.RemoteChanCfg.DustLimit
 	}
 
 	nextHeight := commitChain.tip().height + 1
 
 	// Run through all the HTLCs that will be covered by this transaction
 	// in order to update their commitment addition height, and to adjust
-	// the balances on the commitment transaction accordingly.
+	// the balances on the commitment transaction accordingly. Note that
+	// these balances will be *before* taking a commitment fee from the
+	// initiator.
 	htlcView := lc.fetchHTLCView(theirLogIndex, ourLogIndex)
 	ourBalance, theirBalance, _, filteredHTLCView := lc.computeView(
 		htlcView, remoteChain, true,
 	)
 	feePerKw := filteredHTLCView.feePerKw
 
-	// Determine how many current HTLCs are over the dust limit, and should
+	// Actually generate unsigned commitment transaction for this view.
-	// be counted for the purpose of fee calculation.
+	commitTx, err := lc.commitBuilder.createUnsignedCommitmentTx(
-	var dustLimit btcutil.Amount
+		ourBalance, theirBalance, !remoteChain, feePerKw, nextHeight,
-	if remoteChain {
+		filteredHTLCView, keyRing,
-		dustLimit = lc.channelState.RemoteChanCfg.DustLimit
+	)
-	} else {
+	if err != nil {
-		dustLimit = lc.channelState.LocalChanCfg.DustLimit
+		return nil, err
 	}
 
+	// With the commitment view created, store the resulting balances and
+	// transaction with the other parameters for this height.
 	c := &commitment{
-		ourBalance:        ourBalance,
+		ourBalance:        commitTx.ourBalance,
-		theirBalance:      theirBalance,
+		theirBalance:      commitTx.theirBalance,
+		txn:               commitTx.txn,
+		fee:               commitTx.fee,
 		ourMessageIndex:   ourLogIndex,
 		ourHtlcIndex:      ourHtlcIndex,
 		theirMessageIndex: theirLogIndex,
@@ -2204,14 +2212,6 @@ func (lc *LightningChannel) fetchCommitmentView(remoteChain bool,
 		isOurs:            !remoteChain,
 	}
 
-	// Actually generate unsigned commitment transaction for this view.
-	err := lc.commitBuilder.createCommitmentTx(
-		c, filteredHTLCView, keyRing,
-	)
-	if err != nil {
-		return nil, err
-	}
-
 	// In order to ensure _none_ of the HTLC's associated with this new
 	// commitment are mutated, we'll manually copy over each HTLC to its
 	// respective slice.

lnwallet/commitment.go

@@ -9,6 +9,7 @@ import (
 	"github.com/btcsuite/btcutil"
 	"github.com/lightningnetwork/lnd/channeldb"
 	"github.com/lightningnetwork/lnd/input"
+	"github.com/lightningnetwork/lnd/lnwallet/chainfee"
 	"github.com/lightningnetwork/lnd/lnwire"
 )
 
@@ -166,18 +167,41 @@ func createStateHintObfuscator(state *channeldb.OpenChannel) [StateHintSize]byte
 	)
 }
 
-// createCommitmentTx generates the unsigned commitment transaction for a
+// unsignedCommitmentTx is the final commitment created from evaluating an HTLC
-// commitment view and assigns to txn field.
+// view at a given height, along with some meta data.
-func (cb *CommitmentBuilder) createCommitmentTx(c *commitment,
+type unsignedCommitmentTx struct {
-	filteredHTLCView *htlcView, keyRing *CommitmentKeyRing) error {
+	// txn is the final, unsigned commitment transaction for this view.
+	txn *wire.MsgTx
 
-	ourBalance := c.ourBalance
+	// fee is the total fee of the commitment transaction.
-	theirBalance := c.theirBalance
+	fee btcutil.Amount
+
+	// ourBalance|theirBalance is the balances of this commitment. This can
+	// be different than the balances before creating the commitment
+	// transaction as one party must pay the commitment fee.
+	ourBalance   lnwire.MilliSatoshi
+	theirBalance lnwire.MilliSatoshi
+}
+
+// createUnsignedCommitmentTx generates the unsigned commitment transaction for
+// a commitment view and returns it as part of the unsignedCommitmentTx. The
+// passed in balances should be balances *before* subtracting any commitment
+// fees.
+func (cb *CommitmentBuilder) createUnsignedCommitmentTx(ourBalance,
+	theirBalance lnwire.MilliSatoshi, isOurs bool,
+	feePerKw chainfee.SatPerKWeight, height uint64,
+	filteredHTLCView *htlcView,
+	keyRing *CommitmentKeyRing) (*unsignedCommitmentTx, error) {
+
+	dustLimit := cb.chanState.LocalChanCfg.DustLimit
+	if !isOurs {
+		dustLimit = cb.chanState.RemoteChanCfg.DustLimit
+	}
 
 	numHTLCs := int64(0)
 	for _, htlc := range filteredHTLCView.ourUpdates {
-		if htlcIsDust(false, c.isOurs, c.feePerKw,
+		if htlcIsDust(false, isOurs, feePerKw,
-			htlc.Amount.ToSatoshis(), c.dustLimit) {
+			htlc.Amount.ToSatoshis(), dustLimit) {
 
 			continue
 		}
@@ -185,8 +209,8 @@ func (cb *CommitmentBuilder) createCommitmentTx(c *commitment,
 		numHTLCs++
 	}
 	for _, htlc := range filteredHTLCView.theirUpdates {
-		if htlcIsDust(true, c.isOurs, c.feePerKw,
+		if htlcIsDust(true, isOurs, feePerKw,
-			htlc.Amount.ToSatoshis(), c.dustLimit) {
+			htlc.Amount.ToSatoshis(), dustLimit) {
 
 			continue
 		}
@@ -202,7 +226,7 @@ func (cb *CommitmentBuilder) createCommitmentTx(c *commitment,
 
 	// With the weight known, we can now calculate the commitment fee,
 	// ensuring that we account for any dust outputs trimmed above.
-	commitFee := c.feePerKw.FeeForWeight(totalCommitWeight)
+	commitFee := feePerKw.FeeForWeight(totalCommitWeight)
 	commitFeeMSat := lnwire.NewMSatFromSatoshis(commitFee)
 
 	// Currently, within the protocol, the initiator always pays the fees.
@@ -232,7 +256,7 @@ func (cb *CommitmentBuilder) createCommitmentTx(c *commitment,
 	// CreateCommitTx with parameters matching the perspective, to generate
 	// a new commitment transaction with all the latest unsettled/un-timed
 	// out HTLCs.
-	if c.isOurs {
+	if isOurs {
 		commitTx, err = CreateCommitTx(
 			fundingTxIn(cb.chanState), keyRing, &cb.chanState.LocalChanCfg,
 			&cb.chanState.RemoteChanCfg, ourBalance.ToSatoshis(),
@@ -246,7 +270,7 @@ func (cb *CommitmentBuilder) createCommitmentTx(c *commitment,
 		)
 	}
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	// We'll now add all the HTLC outputs to the commitment transaction.
@@ -260,26 +284,26 @@ func (cb *CommitmentBuilder) createCommitmentTx(c *commitment,
 	// purposes of sorting.
 	cltvs := make([]uint32, len(commitTx.TxOut))
 	for _, htlc := range filteredHTLCView.ourUpdates {
-		if htlcIsDust(false, c.isOurs, c.feePerKw,
+		if htlcIsDust(false, isOurs, feePerKw,
-			htlc.Amount.ToSatoshis(), c.dustLimit) {
+			htlc.Amount.ToSatoshis(), dustLimit) {
 			continue
 		}
 
-		err := addHTLC(commitTx, c.isOurs, false, htlc, keyRing)
+		err := addHTLC(commitTx, isOurs, false, htlc, keyRing)
 		if err != nil {
-			return err
+			return nil, err
 		}
 		cltvs = append(cltvs, htlc.Timeout)
 	}
 	for _, htlc := range filteredHTLCView.theirUpdates {
-		if htlcIsDust(true, c.isOurs, c.feePerKw,
+		if htlcIsDust(true, isOurs, feePerKw,
-			htlc.Amount.ToSatoshis(), c.dustLimit) {
+			htlc.Amount.ToSatoshis(), dustLimit) {
 			continue
 		}
 
-		err := addHTLC(commitTx, c.isOurs, true, htlc, keyRing)
+		err := addHTLC(commitTx, isOurs, true, htlc, keyRing)
 		if err != nil {
-			return err
+			return nil, err
 		}
 		cltvs = append(cltvs, htlc.Timeout)
 	}
@@ -287,9 +311,9 @@ func (cb *CommitmentBuilder) createCommitmentTx(c *commitment,
 	// Set the state hint of the commitment transaction to facilitate
 	// quickly recovering the necessary penalty state in the case of an
 	// uncooperative broadcast.
-	err = SetStateNumHint(commitTx, c.height, cb.obfuscator)
+	err = SetStateNumHint(commitTx, height, cb.obfuscator)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	// Sort the transactions according to the agreed upon canonical
@@ -301,7 +325,7 @@ func (cb *CommitmentBuilder) createCommitmentTx(c *commitment,
 	// transaction which would be invalid by consensus.
 	uTx := btcutil.NewTx(commitTx)
 	if err := blockchain.CheckTransactionSanity(uTx); err != nil {
-		return err
+		return nil, err
 	}
 
 	// Finally, we'll assert that were not attempting to draw more out of
@@ -311,17 +335,18 @@ func (cb *CommitmentBuilder) createCommitmentTx(c *commitment,
 		totalOut += btcutil.Amount(txOut.Value)
 	}
 	if totalOut > cb.chanState.Capacity {
-		return fmt.Errorf("height=%v, for ChannelPoint(%v) attempts "+
+		return nil, fmt.Errorf("height=%v, for ChannelPoint(%v) "+
-			"to consume %v while channel capacity is %v",
+			"attempts to consume %v while channel capacity is %v",
-			c.height, cb.chanState.FundingOutpoint,
+			height, cb.chanState.FundingOutpoint,
 			totalOut, cb.chanState.Capacity)
 	}
 
-	c.txn = commitTx
+	return &unsignedCommitmentTx{
-	c.fee = commitFee
+		txn:          commitTx,
-	c.ourBalance = ourBalance
+		fee:          commitFee,
-	c.theirBalance = theirBalance
+		ourBalance:   ourBalance,
-	return nil
+		theirBalance: theirBalance,
+	}, nil
 }
 
 // CreateCommitTx creates a commitment transaction, spending from specified

lnwallet/transactions_test.go

@@ -794,23 +794,32 @@ func TestCommitmentAndHTLCTransactions(t *testing.T) {
 		}
 		theHTLCView := htlcViewFromHTLCs(htlcs)
 
+		feePerKw := chainfee.SatPerKWeight(test.commitment.FeePerKw)
+		isOurs := true
+		height := test.commitment.CommitHeight
+
 		// Create unsigned commitment transaction.
-		commitmentView := &commitment{
+		view, err := channel.commitBuilder.createUnsignedCommitmentTx(
-			height:       test.commitment.CommitHeight,
+			test.commitment.LocalBalance,
-			ourBalance:   test.commitment.LocalBalance,
+			test.commitment.RemoteBalance, isOurs, feePerKw,
-			theirBalance: test.commitment.RemoteBalance,
+			height, theHTLCView, keys,
-			feePerKw:     chainfee.SatPerKWeight(test.commitment.FeePerKw),
-			dustLimit:    tc.dustLimit,
-			isOurs:       true,
-		}
-		err = channel.commitBuilder.createCommitmentTx(
-			commitmentView, theHTLCView, keys,
 		)
 		if err != nil {
 			t.Errorf("Case %d: Failed to create new commitment tx: %v", i, err)
 			continue
 		}
 
+		commitmentView := &commitment{
+			ourBalance:   view.ourBalance,
+			theirBalance: view.theirBalance,
+			txn:          view.txn,
+			fee:          view.fee,
+			height:       height,
+			feePerKw:     feePerKw,
+			dustLimit:    tc.dustLimit,
+			isOurs:       isOurs,
+		}
+
 		// Initialize LocalCommit, which is used in getSignedCommitTx.
 		channelState.LocalCommitment = test.commitment
 		channelState.LocalCommitment.Htlcs = htlcs