From 36857a1042cd6339f7ca3ef43be9656d13f63ce9 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 10 Jan 2019 12:23:56 +0100 Subject: [PATCH 1/8] lnwallet/channel: add new PaymentDescriptor type `FeeUpdate` This commit adds a new updateType that can be used for PaymentDescriptors: FeeUpdate. We repurpose the fields of the existing PaymentDescriptor struct such that we can easily re-use the commit/ack logic used for other update types also for fee updates. --- lnwallet/channel.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 6175b36e..bf2fe526 100644 --- a/lnwallet/channel.go +++ b/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 "" } @@ -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. From 480f43f1dc1188f43f8bb4ef022056c94b06a9e5 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 10 Jan 2019 12:23:56 +0100 Subject: [PATCH 2/8] lnwallet/channel: add lnwire<->PaymentDescriptor FeeUpdate conversion This commit adds conversion between the lnwire.UpdateFee message and the new FeeUpdate PaymentDescriptor. We re-purpose the existing Amount field in the PaymentDescriptor stuct to hold the feerate. --- htlcswitch/link.go | 14 ++++++++++++-- htlcswitch/switch.go | 7 ++++++- lnwallet/channel.go | 41 +++++++++++++++++++++++++++++++++++++++-- 3 files changed, 57 insertions(+), 5 deletions(-) diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 3f59b89b..f0b60c18 100644 --- a/htlcswitch/link.go +++ b/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 diff --git a/htlcswitch/switch.go b/htlcswitch/switch.go index ce2bfc58..5e5578b5 100644 --- a/htlcswitch/switch.go +++ b/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 { diff --git a/lnwallet/channel.go b/lnwallet/channel.go index bf2fe526..7911d4ac 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -351,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 @@ -423,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. @@ -1568,6 +1573,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 @@ -2883,6 +2905,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 @@ -4254,6 +4285,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 } From 277949cb0e08095091a6a56c44d34d553a4d02d1 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 10 Jan 2019 12:23:57 +0100 Subject: [PATCH 3/8] lnwallet/channel: remove FeeUpdates when compacting logs When compacting the update logs we remove any fee updates when they remove height is passed. We do this since we'll assume fee updates are added and removed at the same commit height, as they will apply for all commitments following the fee update. --- lnwallet/channel.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 7911d4ac..8d3d25ff 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -1211,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 } @@ -1229,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) } From 810c56cdb94d1fdf469c8292639e5f6bcda4d055 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 10 Jan 2019 12:23:57 +0100 Subject: [PATCH 4/8] lnwallet/channel: process FeeUpdate found in update log This commit makes the evaluateHTLCView method process any found FeeUpdates in the logs, by returning the last set feerate. --- lnwallet/channel.go | 89 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 75 insertions(+), 14 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 8d3d25ff..200bfb89 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -2202,6 +2202,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 @@ -2462,9 +2463,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 -// reflects the current state of HTLCs within the remote or local commitment -// chain. +// settles, timeouts and fee updates found in both logs. The resulting view +// returned reflects the current state of HTLCs within the remote or local +// 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 @@ -2476,7 +2477,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 @@ -2489,7 +2495,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 } @@ -2523,7 +2539,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 } @@ -2673,6 +2699,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 @@ -3551,17 +3609,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 + + // 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. If any fee + // updates are found in the logs, the comitment fee rate should be + // changed, so we'll also set the feePerKw to this new value. + filteredHTLCView := lc.evaluateHTLCView(view, &ourBalance, + &theirBalance, nextHeight, remoteChain, updateState) + feePerKw := filteredHTLCView.feePerKw // Check if any fee updates have taken place since that last // commitment. From 6ffd354d63e287bcd4e08f8e3ed9b78fc23c65a4 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 10 Jan 2019 12:23:57 +0100 Subject: [PATCH 5/8] lnwallet/channel: add fee updates to update logs Instead of special casing the UpdateFee messages, we instead add them to the update logs like any other HTLC update message. This lets us avoid having to keep an extra set of variables to keep track of the fee updates, and instead reuse the commit/ack logic used for other updates. This fixes a bug where we would reset the pendingFeeUpdate variable after signing our next commitment, which would make us calculate the new fee incorrectly if the remote sent a commitment concurrently. When restoring state logs, we also make sure to re-add any fee updates. --- lnwallet/channel.go | 125 ++++++++------------------------------------ 1 file changed, 22 insertions(+), 103 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 200bfb89..5d9f6dc9 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -1346,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 @@ -1806,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, @@ -1824,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. @@ -1837,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) @@ -2881,21 +2866,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 @@ -3172,16 +3144,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 } @@ -3624,36 +3586,6 @@ func (lc *LightningChannel) computeView(view *htlcView, remoteChain bool, &theirBalance, nextHeight, remoteChain, updateState) feePerKw := filteredHTLCView.feePerKw - // Check if any fee updates have taken place since that last - // commitment. - if lc.channelState.IsInitiator { - switch { - // We've sent an update_fee message since our last commitment, - // and now are now creating a commitment that reflects the new - // fee update. - case remoteChain && lc.pendingFeeUpdate != nil: - feePerKw = *lc.pendingFeeUpdate - - // 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. var totalHtlcWeight int64 @@ -4190,16 +4122,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 } @@ -4220,21 +4142,7 @@ func (lc *LightningChannel) FullySynced() bool { remoteUpdatesSynced := (lastLocalCommit.theirMessageIndex == lastRemoteCommit.theirMessageIndex) - pendingFeeAck := false - - // 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 + return localUpdatesSynced && remoteUpdatesSynced } // RevokeCurrentCommitment revokes the next lowest unrevoked commitment @@ -6078,7 +5986,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 } @@ -6096,8 +6010,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 } From 7e015258cd36381de13f4685e399a2a8b323add3 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 10 Jan 2019 12:23:57 +0100 Subject: [PATCH 6/8] lnwallet/channel test: add TestUpdateFeeConcurrentSig This tests make sure we don't reset our expected fee upate after signing our next commitment. This test would fail without the previous set of commits. --- lnwallet/channel_test.go | 82 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 5eba3bc4..7cdb3f7b 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -1999,6 +1999,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. From dadfcefc070422c9fde715fe82b511efc1bb1d93 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 10 Jan 2019 12:23:57 +0100 Subject: [PATCH 7/8] lnwallet/channel: don't return feePerKw from computeView Since the feerate is part of the computed view now, we don't have to pass the found feerate as a distinct parameter. --- lnwallet/channel.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 5d9f6dc9..103dc189 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -2245,8 +2245,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 := - lc.computeView(htlcView, remoteChain, true) + ourBalance, theirBalance, _, filteredHTLCView := lc.computeView( + 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. @@ -3543,7 +3545,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 @@ -3607,7 +3609,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 @@ -3638,9 +3640,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. @@ -5898,12 +5901,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) } From e6ee835bbe3238365a7e5054f4fd65b2dea05a92 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 10 Jan 2019 12:23:57 +0100 Subject: [PATCH 8/8] lnwallet/channel_test: assert FeeUpdate is removed from logs after lock-in --- lnwallet/channel_test.go | 49 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 7cdb3f7b..348a9e11 100644 --- a/lnwallet/channel_test.go +++ b/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