Merge pull request #2397 from halseth/reject-commitment-expected-fee-reset-bug

[bugfix] Process fee updates as any other update message

htlcswitch/link.go

@@ -681,9 +681,14 @@ func (l *channelLink) resolveFwdPkg(fwdPkg *channeldb.FwdPkg) (bool, error) {
 	// If the package is fully acked but not completed, it must still have
 	// settles and fails to propagate.
 	if !fwdPkg.SettleFailFilter.IsFull() {
-		settleFails := lnwallet.PayDescsFromRemoteLogUpdates(
+		settleFails, err := lnwallet.PayDescsFromRemoteLogUpdates(
 			fwdPkg.Source, fwdPkg.Height, fwdPkg.SettleFails,
 		)
+		if err != nil {
+			l.errorf("Unable to process remote log updates: %v",
+				err)
+			return false, err
+		}
 		l.processRemoteSettleFails(fwdPkg, settleFails)
 	}
 
@@ -693,9 +698,14 @@ func (l *channelLink) resolveFwdPkg(fwdPkg *channeldb.FwdPkg) (bool, error) {
 	// batch of adds presented to the sphinx router does not ever change.
 	var needUpdate bool
 	if !fwdPkg.AckFilter.IsFull() {
-		adds := lnwallet.PayDescsFromRemoteLogUpdates(
+		adds, err := lnwallet.PayDescsFromRemoteLogUpdates(
 			fwdPkg.Source, fwdPkg.Height, fwdPkg.Adds,
 		)
+		if err != nil {
+			l.errorf("Unable to process remote log updates: %v",
+				err)
+			return false, err
+		}
 		needUpdate = l.processRemoteAdds(fwdPkg, adds)
 
 		// If the link failed during processing the adds, we must

htlcswitch/switch.go

@@ -1781,9 +1781,14 @@ func (s *Switch) loadChannelFwdPkgs(source lnwire.ShortChannelID) ([]*channeldb.
 // NOTE: This should mimic the behavior processRemoteSettleFails.
 func (s *Switch) reforwardSettleFails(fwdPkgs []*channeldb.FwdPkg) {
 	for _, fwdPkg := range fwdPkgs {
-		settleFails := lnwallet.PayDescsFromRemoteLogUpdates(
+		settleFails, err := lnwallet.PayDescsFromRemoteLogUpdates(
 			fwdPkg.Source, fwdPkg.Height, fwdPkg.SettleFails,
 		)
+		if err != nil {
+			log.Errorf("Unable to process remote log updates: %v",
+				err)
+			continue
+		}
 
 		switchPackets := make([]*htlcPacket, 0, len(settleFails))
 		for i, pd := range settleFails {

lnwallet/channel.go

@@ -165,6 +165,10 @@ const (
 	// original add entry from the remote party's log after the next state
 	// transition.
 	Settle
+
+	// FeeUpdate is an update type sent by the channel initiator that
+	// updates the fee rate used when signing the commitment transaction.
+	FeeUpdate
 )
 
 // String returns a human readable string that uniquely identifies the target
@@ -179,6 +183,8 @@ func (u updateType) String() string {
 		return "MalformedFail"
 	case Settle:
 		return "Settle"
+	case FeeUpdate:
+		return "FeeUpdate"
 	default:
 		return "<unknown type>"
 	}
@@ -190,7 +196,7 @@ func (u updateType) String() string {
 // the original added HTLC.
 //
 // TODO(roasbeef): LogEntry interface??
-//  * need to separate attrs for cancel/add/settle
+//  * need to separate attrs for cancel/add/settle/feeupdate
 type PaymentDescriptor struct {
 	// RHash is the payment hash for this HTLC. The HTLC can be settled iff
 	// the preimage to this hash is presented.
@@ -345,7 +351,7 @@ type PaymentDescriptor struct {
 // NOTE: The provided `logUpdates` MUST corresponding exactly to either the Adds
 // or SettleFails in this channel's forwarding package at `height`.
 func PayDescsFromRemoteLogUpdates(chanID lnwire.ShortChannelID, height uint64,
-	logUpdates []channeldb.LogUpdate) []*PaymentDescriptor {
+	logUpdates []channeldb.LogUpdate) ([]*PaymentDescriptor, error) {
 
 	// Allocate enough space to hold all of the payment descriptors we will
 	// reconstruct, and also the list of pointers that will be returned to
@@ -417,13 +423,18 @@ func PayDescsFromRemoteLogUpdates(chanID lnwire.ShortChannelID, height uint64,
 					Index:  uint16(i),
 				},
 			}
+
+		// NOTE: UpdateFee is not expected since they are not forwarded.
+		case *lnwire.UpdateFee:
+			return nil, fmt.Errorf("unexpected update fee")
+
 		}
 
 		payDescs = append(payDescs, pd)
 		payDescPtrs = append(payDescPtrs, &payDescs[i])
 	}
 
-	return payDescPtrs
+	return payDescPtrs, nil
 }
 
 // commitment represents a commitment to a new state within an active channel.
@@ -1200,6 +1211,9 @@ func compactLogs(ourLog, theirLog *updateLog,
 			nextA = e.Next()
 
 			htlc := e.Value.(*PaymentDescriptor)
+
+			// We skip Adds, as they will be removed along with the
+			// fail/settles below.
 			if htlc.EntryType == Add {
 				continue
 			}
@@ -1218,6 +1232,16 @@ func compactLogs(ourLog, theirLog *updateLog,
 			if remoteChainTail >= htlc.removeCommitHeightRemote &&
 				localChainTail >= htlc.removeCommitHeightLocal {
 
+				// Fee updates have no parent htlcs, so we only
+				// remove the update itself.
+				if htlc.EntryType == FeeUpdate {
+					logA.removeUpdate(htlc.LogIndex)
+					continue
+				}
+
+				// The other types (fail/settle) do have a
+				// parent HTLC, so we'll remove that HTLC from
+				// the other log.
 				logA.removeUpdate(htlc.LogIndex)
 				logB.removeHtlc(htlc.ParentIndex)
 			}
@@ -1322,17 +1346,6 @@ type LightningChannel struct {
 	localUpdateLog  *updateLog
 	remoteUpdateLog *updateLog
 
-	// pendingFeeUpdate is set to the fee-per-kw we last sent (if we are
-	// channel initiator) or received (if non-initiator) in an update fee
-	// message, which haven't yet been included in a commitment.  It will
-	// be nil if no fee update is un-committed.
-	pendingFeeUpdate *SatPerKWeight
-
-	// pendingAckFeeUpdate is set to the last committed fee update which is
-	// not yet ACKed. This value will be nil if a fee update hasn't been
-	// initiated.
-	pendingAckFeeUpdate *SatPerKWeight
-
 	// LocalFundingKey is the public key under control by the wallet that
 	// was used for the 2-of-2 funding output which created this channel.
 	LocalFundingKey *btcec.PublicKey
@@ -1562,6 +1575,23 @@ func (lc *LightningChannel) logUpdateToPayDesc(logUpdate *channeldb.LogUpdate,
 			ShaOnionBlob:             wireMsg.ShaOnionBlob,
 			removeCommitHeightRemote: commitHeight,
 		}
+
+	// For fee updates we'll create a FeeUpdate type to add to the log. We
+	// reuse the amount field to hold the fee rate. Since the amount field
+	// is denominated in msat we won't lose precision when storing the
+	// sat/kw denominated feerate. Note that we set both the add and remove
+	// height to the same value, as we consider the fee update locked in by
+	// adding and removing it at the same height.
+	case *lnwire.UpdateFee:
+		pd = &PaymentDescriptor{
+			LogIndex: logUpdate.LogIndex,
+			Amount: lnwire.NewMSatFromSatoshis(
+				btcutil.Amount(wireMsg.FeePerKw),
+			),
+			EntryType:                FeeUpdate,
+			addCommitHeightRemote:    commitHeight,
+			removeCommitHeightRemote: commitHeight,
+		}
 	}
 
 	return pd, nil
@@ -1765,15 +1795,6 @@ func (lc *LightningChannel) restoreStateLogs(
 	// If we did have a dangling commit, then we'll examine which updates
 	// we included in that state and re-insert them into our update log.
 	for _, logUpdate := range pendingRemoteCommitDiff.LogUpdates {
-		// If the log update is a fee update, then it doesn't need an
-		// entry within the updateLog, so we'll just apply it and move
-		// on.
-		if feeUpdate, ok := logUpdate.UpdateMsg.(*lnwire.UpdateFee); ok {
-			newFeeRate := SatPerKWeight(feeUpdate.FeePerKw)
-			lc.pendingAckFeeUpdate = &newFeeRate
-			continue
-		}
-
 		payDesc, err := lc.logUpdateToPayDesc(
 			&logUpdate, lc.remoteUpdateLog, pendingHeight,
 			SatPerKWeight(pendingCommit.FeePerKw), pendingRemoteKeys,
@@ -1783,7 +1804,8 @@ func (lc *LightningChannel) restoreStateLogs(
 			return err
 		}
 
-		if payDesc.EntryType == Add {
+		switch payDesc.EntryType {
+		case Add:
 			// The HtlcIndex of the added HTLC _must_ be equal to
 			// the log's htlcCounter at this point. If it is not we
 			// panic to catch this.
@@ -1796,7 +1818,11 @@ func (lc *LightningChannel) restoreStateLogs(
 			}
 
 			lc.localUpdateLog.appendHtlc(payDesc)
-		} else {
+
+		case FeeUpdate:
+			lc.localUpdateLog.appendUpdate(payDesc)
+
+		default:
 			lc.localUpdateLog.appendUpdate(payDesc)
 
 			lc.remoteUpdateLog.markHtlcModified(payDesc.ParentIndex)
@@ -2171,6 +2197,7 @@ func htlcIsDust(incoming, ourCommit bool, feePerKw SatPerKWeight,
 type htlcView struct {
 	ourUpdates   []*PaymentDescriptor
 	theirUpdates []*PaymentDescriptor
+	feePerKw     SatPerKWeight
 }
 
 // fetchHTLCView returns all the candidate HTLC updates which should be
@@ -2228,8 +2255,10 @@ func (lc *LightningChannel) fetchCommitmentView(remoteChain bool,
 	// in order to update their commitment addition height, and to adjust
 	// the balances on the commitment transaction accordingly.
 	htlcView := lc.fetchHTLCView(theirLogIndex, ourLogIndex)
-	ourBalance, theirBalance, _, filteredHTLCView, feePerKw :=
+	ourBalance, theirBalance, _, filteredHTLCView := lc.computeView(
-		lc.computeView(htlcView, remoteChain, true)
+		htlcView, remoteChain, true,
+	)
+	feePerKw := filteredHTLCView.feePerKw
 
 	// Determine how many current HTLCs are over the dust limit, and should
 	// be counted for the purpose of fee calculation.
@@ -2431,9 +2460,9 @@ func (lc *LightningChannel) createCommitmentTx(c *commitment,
 
 // evaluateHTLCView processes all update entries in both HTLC update logs,
 // producing a final view which is the result of properly applying all adds,
-// settles, and timeouts found in both logs. The resulting view returned
+// settles, timeouts and fee updates found in both logs. The resulting view
-// reflects the current state of HTLCs within the remote or local commitment
+// returned reflects the current state of HTLCs within the remote or local
-// chain.
+// commitment chain, and the current commitment fee rate.
 //
 // If mutateState is set to true, then the add height of all added HTLCs
 // will be set to nextHeight, and the remove height of all removed HTLCs
@@ -2445,7 +2474,12 @@ func (lc *LightningChannel) evaluateHTLCView(view *htlcView, ourBalance,
 	theirBalance *lnwire.MilliSatoshi, nextHeight uint64,
 	remoteChain, mutateState bool) *htlcView {
 
-	newView := &htlcView{}
+	// We initialize the view's fee rate to the fee rate of the unfiltered
+	// view. If any fee updates are found when evaluating the view, it will
+	// be updated.
+	newView := &htlcView{
+		feePerKw: view.feePerKw,
+	}
 
 	// We use two maps, one for the local log and one for the remote log to
 	// keep track of which entries we need to skip when creating the final
@@ -2458,7 +2492,17 @@ func (lc *LightningChannel) evaluateHTLCView(view *htlcView, ourBalance,
 	// skip sets and mutating the current chain state (crediting balances,
 	// etc) to reflect the settle/timeout entry encountered.
 	for _, entry := range view.ourUpdates {
-		if entry.EntryType == Add {
+		switch entry.EntryType {
+		// Skip adds for now. They will be processed below.
+		case Add:
+			continue
+
+		// Process fee updates, updating the current feePerKw.
+		case FeeUpdate:
+			processFeeUpdate(
+				entry, nextHeight, remoteChain, mutateState,
+				newView,
+			)
 			continue
 		}
 
@@ -2492,7 +2536,17 @@ func (lc *LightningChannel) evaluateHTLCView(view *htlcView, ourBalance,
 			nextHeight, remoteChain, true, mutateState)
 	}
 	for _, entry := range view.theirUpdates {
-		if entry.EntryType == Add {
+		switch entry.EntryType {
+		// Skip adds for now. They will be processed below.
+		case Add:
+			continue
+
+		// Process fee updates, updating the current feePerKw.
+		case FeeUpdate:
+			processFeeUpdate(
+				entry, nextHeight, remoteChain, mutateState,
+				newView,
+			)
 			continue
 		}
 
@@ -2642,6 +2696,38 @@ func processRemoveEntry(htlc *PaymentDescriptor, ourBalance,
 	}
 }
 
+// processFeeUpdate processes a log update that updates the current commitment
+// fee.
+func processFeeUpdate(feeUpdate *PaymentDescriptor, nextHeight uint64,
+	remoteChain bool, mutateState bool, view *htlcView) {
+
+	// Fee updates are applied for all commitments after they are
+	// sent/received, so we consider them being added and removed at the
+	// same height.
+	var addHeight *uint64
+	var removeHeight *uint64
+	if remoteChain {
+		addHeight = &feeUpdate.addCommitHeightRemote
+		removeHeight = &feeUpdate.removeCommitHeightRemote
+	} else {
+		addHeight = &feeUpdate.addCommitHeightLocal
+		removeHeight = &feeUpdate.removeCommitHeightLocal
+	}
+
+	if *addHeight != 0 {
+		return
+	}
+
+	// If the update wasn't already locked in, update the current fee rate
+	// to reflect this update.
+	view.feePerKw = SatPerKWeight(feeUpdate.Amount.ToSatoshis())
+
+	if mutateState {
+		*addHeight = nextHeight
+		*removeHeight = nextHeight
+	}
+}
+
 // generateRemoteHtlcSigJobs generates a series of HTLC signature jobs for the
 // sig pool, along with a channel that if closed, will cancel any jobs after
 // they have been submitted to the sigPool. This method is to be used when
@@ -2792,21 +2878,8 @@ func (lc *LightningChannel) createCommitDiff(
 	// out.
 	chanID := lnwire.NewChanIDFromOutPoint(&lc.channelState.FundingOutpoint)
 
-	// If we have a fee update that we're waiting on an ACK of, then we'll
-	// create an entry so this is properly retransmitted. Note that we can
-	// only send an UpdateFee message if we're the initiator of the
-	// channel.
-	var logUpdates []channeldb.LogUpdate
-	if lc.channelState.IsInitiator && lc.pendingFeeUpdate != nil {
-		logUpdates = append(logUpdates, channeldb.LogUpdate{
-			UpdateMsg: &lnwire.UpdateFee{
-				ChanID:   chanID,
-				FeePerKw: uint32(*lc.pendingFeeUpdate),
-			},
-		})
-	}
-
 	var (
+		logUpdates        []channeldb.LogUpdate
 		ackAddRefs        []channeldb.AddRef
 		settleFailRefs    []channeldb.SettleFailRef
 		openCircuitKeys   []channeldb.CircuitKey
@@ -2887,6 +2960,15 @@ func (lc *LightningChannel) createCommitDiff(
 				ShaOnionBlob: pd.ShaOnionBlob,
 				FailureCode:  pd.FailCode,
 			}
+
+		case FeeUpdate:
+			// The Amount field holds the feerate denominated in
+			// msat. Since feerates are only denominated in sat/kw,
+			// we can convert it without loss of precision.
+			logUpdate.UpdateMsg = &lnwire.UpdateFee{
+				ChanID:   chanID,
+				FeePerKw: uint32(pd.Amount.ToSatoshis()),
+			}
 		}
 
 		// Gather the fwd pkg references from any settle or fail
@@ -3074,16 +3156,6 @@ func (lc *LightningChannel) SignNextCommitment() (lnwire.Sig, []lnwire.Sig, erro
 	// latest commitment update.
 	lc.remoteCommitChain.addCommitment(newCommitView)
 
-	// If we are the channel initiator then we would have signed any sent
-	// fee update at this point, so mark this update as pending ACK, and
-	// set pendingFeeUpdate to nil. We can do this since we know we won't
-	// sign any new commitment before receiving a RevokeAndAck, because of
-	// the revocation window of 1.
-	if lc.channelState.IsInitiator {
-		lc.pendingAckFeeUpdate = lc.pendingFeeUpdate
-		lc.pendingFeeUpdate = nil
-	}
-
 	return sig, htlcSigs, nil
 }
 
@@ -3483,7 +3555,7 @@ func ChanSyncMsg(c *channeldb.OpenChannel) (*lnwire.ChannelReestablish, error) {
 // HTLCs will be set to the next commitment height.
 func (lc *LightningChannel) computeView(view *htlcView, remoteChain bool,
 	updateState bool) (lnwire.MilliSatoshi, lnwire.MilliSatoshi, int64,
-	*htlcView, SatPerKWeight) {
+	*htlcView) {
 
 	commitChain := lc.localCommitChain
 	dustLimit := lc.localChanCfg.DustLimit
@@ -3511,47 +3583,20 @@ func (lc *LightningChannel) computeView(view *htlcView, remoteChain bool,
 	}
 	nextHeight := commitChain.tip().height + 1
 
-	// We evaluate the view at this stage, meaning settled and failed HTLCs
-	// will remove their corresponding added HTLCs.  The resulting filtered
-	// view will only have Add entries left, making it easy to compare the
-	// channel constraints to the final commitment state.
-	filteredHTLCView := lc.evaluateHTLCView(view, &ourBalance,
-		&theirBalance, nextHeight, remoteChain, updateState)
-
 	// Initiate feePerKw to the last committed fee for this chain as we'll
 	// need this to determine which HTLCs are dust, and also the final fee
 	// rate.
-	feePerKw := commitChain.tip().feePerKw
+	view.feePerKw = commitChain.tip().feePerKw
 
-	// Check if any fee updates have taken place since that last
+	// We evaluate the view at this stage, meaning settled and failed HTLCs
-	// commitment.
+	// will remove their corresponding added HTLCs.  The resulting filtered
-	if lc.channelState.IsInitiator {
+	// view will only have Add entries left, making it easy to compare the
-		switch {
+	// channel constraints to the final commitment state. If any fee
-		// We've sent an update_fee message since our last commitment,
+	// updates are found in the logs, the comitment fee rate should be
-		// and now are now creating a commitment that reflects the new
+	// changed, so we'll also set the feePerKw to this new value.
-		// fee update.
+	filteredHTLCView := lc.evaluateHTLCView(view, &ourBalance,
-		case remoteChain && lc.pendingFeeUpdate != nil:
+		&theirBalance, nextHeight, remoteChain, updateState)
-			feePerKw = *lc.pendingFeeUpdate
+	feePerKw := filteredHTLCView.feePerKw
-
-		// We've created a new commitment for the remote chain that
-		// includes a fee update, and have not received a commitment
-		// after the fee update has been ACKed.
-		case !remoteChain && lc.pendingAckFeeUpdate != nil:
-			feePerKw = *lc.pendingAckFeeUpdate
-		}
-	} else {
-		switch {
-		// We've received a fee update since the last local commitment,
-		// so we'll include the fee update in the current view.
-		case !remoteChain && lc.pendingFeeUpdate != nil:
-			feePerKw = *lc.pendingFeeUpdate
-
-		// Earlier we received a commitment that signed an earlier fee
-		// update, and now we must ACK that update.
-		case remoteChain && lc.pendingAckFeeUpdate != nil:
-			feePerKw = *lc.pendingAckFeeUpdate
-		}
-	}
 
 	// Now go through all HTLCs at this stage, to calculate the total
 	// weight, needed to calculate the transaction fee.
@@ -3574,7 +3619,7 @@ func (lc *LightningChannel) computeView(view *htlcView, remoteChain bool,
 	}
 
 	totalCommitWeight := CommitWeight + totalHtlcWeight
-	return ourBalance, theirBalance, totalCommitWeight, filteredHTLCView, feePerKw
+	return ourBalance, theirBalance, totalCommitWeight, filteredHTLCView
 }
 
 // validateCommitmentSanity is used to validate the current state of the
@@ -3605,9 +3650,10 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter,
 	ourInitialBalance := commitChain.tip().ourBalance
 	theirInitialBalance := commitChain.tip().theirBalance
 
-	ourBalance, theirBalance, commitWeight, filteredView, feePerKw := lc.computeView(
+	ourBalance, theirBalance, commitWeight, filteredView := lc.computeView(
 		view, remoteChain, false,
 	)
+	feePerKw := filteredView.feePerKw
 
 	// Calculate the commitment fee, and subtract it from the initiator's
 	// balance.
@@ -4089,16 +4135,6 @@ func (lc *LightningChannel) ReceiveNewCommitment(commitSig lnwire.Sig,
 	localCommitmentView.sig = commitSig.ToSignatureBytes()
 	lc.localCommitChain.addCommitment(localCommitmentView)
 
-	// If we are not channel initiator, then the commitment just received
-	// would've signed any received fee update since last commitment. Mark
-	// any such fee update as pending ACK (so we remember to ACK it on our
-	// next commitment), and set pendingFeeUpdate to nil. We can do this
-	// since we won't receive any new commitment before ACKing.
-	if !lc.channelState.IsInitiator {
-		lc.pendingAckFeeUpdate = lc.pendingFeeUpdate
-		lc.pendingFeeUpdate = nil
-	}
-
 	return nil
 }
 
@@ -4119,21 +4155,7 @@ func (lc *LightningChannel) FullySynced() bool {
 	remoteUpdatesSynced := (lastLocalCommit.theirMessageIndex ==
 		lastRemoteCommit.theirMessageIndex)
 
-	pendingFeeAck := false
+	return localUpdatesSynced && remoteUpdatesSynced
-
-	// If we have received a fee update which we haven't yet ACKed, then
-	// we owe a commitment.
-	if !lc.channelState.IsInitiator {
-		pendingFeeAck = lc.pendingAckFeeUpdate != nil
-	}
-
-	// If we have sent a fee update which we haven't yet signed, then
-	// we owe a commitment.
-	if lc.channelState.IsInitiator {
-		pendingFeeAck = lc.pendingFeeUpdate != nil
-	}
-
-	return localUpdatesSynced && remoteUpdatesSynced && !pendingFeeAck
 }
 
 // RevokeCurrentCommitment revokes the next lowest unrevoked commitment
@@ -4258,6 +4280,12 @@ func (lc *LightningChannel) ReceiveRevocation(revMsg *lnwire.RevokeAndAck) (
 	for e := lc.remoteUpdateLog.Front(); e != nil; e = e.Next() {
 		pd := e.Value.(*PaymentDescriptor)
 
+		// Fee updates are local to this particular channel, and should
+		// never be forwarded.
+		if pd.EntryType == FeeUpdate {
+			continue
+		}
+
 		if pd.isForwarded {
 			continue
 		}
@@ -5883,12 +5911,12 @@ func (lc *LightningChannel) availableBalance() (lnwire.MilliSatoshi, int64) {
 		lc.localUpdateLog.logIndex)
 
 	// Then compute our current balance for that view.
-	ourBalance, _, commitWeight, _, feePerKw :=
+	ourBalance, _, commitWeight, filteredView :=
 		lc.computeView(htlcView, false, false)
 
 	// If we are the channel initiator, we must remember to subtract the
 	// commitment fee from our available balance.
-	commitFee := feePerKw.FeeForWeight(commitWeight)
+	commitFee := filteredView.feePerKw.FeeForWeight(commitWeight)
 	if lc.channelState.IsInitiator {
 		ourBalance -= lnwire.NewMSatFromSatoshis(commitFee)
 	}
@@ -5971,7 +5999,13 @@ func (lc *LightningChannel) UpdateFee(feePerKw SatPerKWeight) error {
 		return err
 	}
 
-	lc.pendingFeeUpdate = &feePerKw
+	pd := &PaymentDescriptor{
+		LogIndex:  lc.localUpdateLog.logIndex,
+		Amount:    lnwire.NewMSatFromSatoshis(btcutil.Amount(feePerKw)),
+		EntryType: FeeUpdate,
+	}
+
+	lc.localUpdateLog.appendUpdate(pd)
 
 	return nil
 }
@@ -5989,8 +6023,13 @@ func (lc *LightningChannel) ReceiveUpdateFee(feePerKw SatPerKWeight) error {
 	}
 
 	// TODO(roasbeef): or just modify to use the other balance?
+	pd := &PaymentDescriptor{
+		LogIndex:  lc.remoteUpdateLog.logIndex,
+		Amount:    lnwire.NewMSatFromSatoshis(btcutil.Amount(feePerKw)),
+		EntryType: FeeUpdate,
+	}
 
-	lc.pendingFeeUpdate = &feePerKw
+	lc.remoteUpdateLog.appendUpdate(pd)
 
 	return nil
 }

lnwallet/channel_test.go

@@ -1395,6 +1395,45 @@ func TestStateUpdatePersistence(t *testing.T) {
 		t.Fatalf("unable to recv bob's htlc: %v", err)
 	}
 
+	// Also add a fee update to the update logs.
+	fee := SatPerKWeight(111)
+	if err := aliceChannel.UpdateFee(fee); err != nil {
+		t.Fatalf("unable to send fee update")
+	}
+	if err := bobChannel.ReceiveUpdateFee(fee); err != nil {
+		t.Fatalf("unable to receive fee update")
+	}
+
+	// Helper method that asserts the expected number of updates are found
+	// in the update logs.
+	assertNumLogUpdates := func(numAliceUpdates, numBobUpdates int) {
+		if aliceChannel.localUpdateLog.Len() != numAliceUpdates {
+			t.Fatalf("expected %d local updates, found %d",
+				numAliceUpdates,
+				aliceChannel.localUpdateLog.Len())
+		}
+		if aliceChannel.remoteUpdateLog.Len() != numBobUpdates {
+			t.Fatalf("expected %d remote updates, found %d",
+				numBobUpdates,
+				aliceChannel.remoteUpdateLog.Len())
+		}
+
+		if bobChannel.localUpdateLog.Len() != numBobUpdates {
+			t.Fatalf("expected %d local updates, found %d",
+				numBobUpdates,
+				bobChannel.localUpdateLog.Len())
+		}
+		if bobChannel.remoteUpdateLog.Len() != numAliceUpdates {
+			t.Fatalf("expected %d remote updates, found %d",
+				numAliceUpdates,
+				bobChannel.remoteUpdateLog.Len())
+		}
+	}
+
+	// Both nodes should now have Alice's 3 Adds and 1 FeeUpdate in the
+	// log, and Bob's 1 Add.
+	assertNumLogUpdates(4, 1)
+
 	// Next, Alice initiates a state transition to include the HTLC's she
 	// added above in a new commitment state.
 	if err := forceStateTransition(aliceChannel, bobChannel); err != nil {
@@ -1409,6 +1448,16 @@ func TestStateUpdatePersistence(t *testing.T) {
 		t.Fatalf("unable to complete bob's state transition: %v", err)
 	}
 
+	// After the state transition the fee update is fully locked in, and
+	// should've been removed from both channels' update logs.
+	if aliceChannel.localCommitChain.tail().feePerKw != fee {
+		t.Fatalf("fee not locked in")
+	}
+	if bobChannel.localCommitChain.tail().feePerKw != fee {
+		t.Fatalf("fee not locked in")
+	}
+	assertNumLogUpdates(3, 1)
+
 	// The latest commitment from both sides should have all the HTLCs.
 	numAliceOutgoing := aliceChannel.localCommitChain.tail().outgoingHTLCs
 	numAliceIncoming := aliceChannel.localCommitChain.tail().incomingHTLCs
@@ -1999,6 +2048,88 @@ func TestUpdateFeeFail(t *testing.T) {
 
 }
 
+// TestUpdateFeeConcurrentSig tests that the channel can properly handle a fee
+// update that it receives concurrently with signing its next commitment.
+func TestUpdateFeeConcurrentSig(t *testing.T) {
+	t.Parallel()
+
+	aliceChannel, bobChannel, cleanUp, err := CreateTestChannels()
+	if err != nil {
+		t.Fatalf("unable to create test channels: %v", err)
+	}
+	defer cleanUp()
+
+	paymentPreimage := bytes.Repeat([]byte{1}, 32)
+	paymentHash := sha256.Sum256(paymentPreimage)
+	htlc := &lnwire.UpdateAddHTLC{
+		PaymentHash: paymentHash,
+		Amount:      btcutil.SatoshiPerBitcoin,
+		Expiry:      uint32(5),
+	}
+
+	// First Alice adds the outgoing HTLC to her local channel's state
+	// update log. Then Alice sends this wire message over to Bob who
+	// adds this htlc to his remote state update log.
+	if _, err := aliceChannel.AddHTLC(htlc, nil); err != nil {
+		t.Fatalf("unable to add htlc: %v", err)
+	}
+	if _, err := bobChannel.ReceiveHTLC(htlc); err != nil {
+		t.Fatalf("unable to recv htlc: %v", err)
+	}
+
+	// Simulate Alice sending update fee message to bob.
+	fee := SatPerKWeight(111)
+	if err := aliceChannel.UpdateFee(fee); err != nil {
+		t.Fatalf("unable to send fee update")
+	}
+
+	// Alice signs a commitment, and sends this to bob.
+	aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment()
+	if err != nil {
+		t.Fatalf("alice unable to sign commitment: %v", err)
+	}
+
+	// At the same time, Bob signs a commitment.
+	bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment()
+	if err != nil {
+		t.Fatalf("bob unable to sign alice's commitment: %v", err)
+	}
+
+	// ...that Alice receives.
+	err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs)
+	if err != nil {
+		t.Fatalf("alice unable to process bob's new commitment: %v", err)
+	}
+
+	// Now let Bob receive the fee update + commitment that Alice sent.
+	if err := bobChannel.ReceiveUpdateFee(fee); err != nil {
+		t.Fatalf("unable to receive fee update")
+	}
+
+	// Bob receives this signature message, and verifies that it is
+	// consistent with the state he had for Alice, including the received
+	// HTLC and fee update.
+	err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs)
+	if err != nil {
+		t.Fatalf("bob unable to process alice's new commitment: %v", err)
+	}
+
+	if SatPerKWeight(bobChannel.channelState.LocalCommitment.FeePerKw) == fee {
+		t.Fatalf("bob's feePerKw was unexpectedly locked in")
+	}
+
+	// Bob can revoke the prior commitment he had. This should lock in the
+	// fee update for him.
+	_, _, err = bobChannel.RevokeCurrentCommitment()
+	if err != nil {
+		t.Fatalf("unable to generate bob revocation: %v", err)
+	}
+
+	if SatPerKWeight(bobChannel.channelState.LocalCommitment.FeePerKw) != fee {
+		t.Fatalf("bob's feePerKw was not locked in")
+	}
+}
+
 // TestUpdateFeeSenderCommits verifies that the state machine progresses as
 // expected if we send a fee update, and then the sender of the fee update
 // sends a commitment signature.