diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 60c7faa21..f5666dabf 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -2957,10 +2957,44 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance, whoseCommitChain, ) if rmvHeight == 0 { - processRemoveEntry( - entry, ourBalance, theirBalance, - party.CounterParty(), - ) + switch { + // If an incoming HTLC is being settled, then + // this means that we've received the preimage + // either from another subsystem, or the + // upstream peer in the route. Therefore, we + // increase our balance by the HTLC amount. + case party.CounterParty() == lntypes.Remote && + entry.EntryType == Settle: + + *ourBalance += entry.Amount + + // Otherwise, this HTLC is being failed out, + // therefore the value of the HTLC should + // return to the remote party. + case party.CounterParty() == lntypes.Remote && + entry.EntryType != Settle: + + *theirBalance += entry.Amount + + // If an outgoing HTLC is being settled, then + // this means that the downstream party + // resented the preimage or learned of it via a + // downstream peer. In either case, we credit + // their settled value with the value of the + // HTLC. + case party.CounterParty() == lntypes.Local && + entry.EntryType == Settle: + + *theirBalance += entry.Amount + + // Otherwise, one of our outgoing HTLC's has + // timed out, so the value of the HTLC should + // be returned to our settled balance. + case party.CounterParty() == lntypes.Local && + entry.EntryType != Settle: + + *ourBalance += entry.Amount + } } } } @@ -2981,9 +3015,19 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance, whoseCommitChain, ) if addHeight == 0 { - processAddEntry( - entry, ourBalance, theirBalance, party, - ) + if party == lntypes.Remote { + // If this is a new incoming + // (un-committed) HTLC, then we need + // to update their balance accordingly + // by subtracting the amount of the + // HTLC that are funds pending. + *theirBalance -= entry.Amount + } else { + // Similarly, we need to debit our + // balance if this is an out going HTLC + // to reflect the pending balance. + *ourBalance -= entry.Amount + } } } @@ -3071,62 +3115,6 @@ func (lc *LightningChannel) fetchParent(entry *paymentDescriptor, return addEntry, nil } -// processAddEntry evaluates the effect of an add entry within the HTLC log. -// If the HTLC hasn't yet been committed in either chain, then the height it -// was committed is updated. Keeping track of this inclusion height allows us to -// later compact the log once the change is fully committed in both chains. -func processAddEntry(htlc *paymentDescriptor, ourBalance, - theirBalance *lnwire.MilliSatoshi, originator lntypes.ChannelParty) { - - if originator == lntypes.Remote { - // If this is a new incoming (un-committed) HTLC, then we need - // to update their balance accordingly by subtracting the - // amount of the HTLC that are funds pending. - *theirBalance -= htlc.Amount - } else { - // Similarly, we need to debit our balance if this is an out - // going HTLC to reflect the pending balance. - *ourBalance -= htlc.Amount - } -} - -// processRemoveEntry processes a log entry which settles or times out a -// previously added HTLC. If the removal entry has already been processed, it -// is skipped. -func processRemoveEntry(htlc *paymentDescriptor, ourBalance, - theirBalance *lnwire.MilliSatoshi, originator lntypes.ChannelParty) { - - switch { - // If an incoming HTLC is being settled, then this means that we've - // received the preimage either from another subsystem, or the - // upstream peer in the route. Therefore, we increase our balance by - // the HTLC amount. - case originator == lntypes.Remote && htlc.EntryType == Settle: - *ourBalance += htlc.Amount - - // Otherwise, this HTLC is being failed out, therefore the value of the - // HTLC should return to the remote party. - case originator == lntypes.Remote && - (htlc.EntryType == Fail || htlc.EntryType == MalformedFail): - - *theirBalance += htlc.Amount - - // If an outgoing HTLC is being settled, then this means that the - // downstream party resented the preimage or learned of it via a - // downstream peer. In either case, we credit their settled value with - // the value of the HTLC. - case originator == lntypes.Local && htlc.EntryType == Settle: - *theirBalance += htlc.Amount - - // Otherwise, one of our outgoing HTLC's has timed out, so the value of - // the HTLC should be returned to our settled balance. - case originator == lntypes.Local && - (htlc.EntryType == Fail || htlc.EntryType == MalformedFail): - - *ourBalance += htlc.Amount - } -} - // 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 diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 62eb01e3e..f99fb141d 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -9055,437 +9055,6 @@ type heights struct { remoteRemove uint64 } -func checkHeights(t *testing.T, update *paymentDescriptor, expected heights) { - updateHeights := heights{ - localAdd: update.addCommitHeights.Local, - localRemove: update.removeCommitHeights.Local, - remoteAdd: update.addCommitHeights.Remote, - remoteRemove: update.removeCommitHeights.Remote, - } - - if !reflect.DeepEqual(updateHeights, expected) { - t.Fatalf("expected: %v, got: %v", expected, updateHeights) - } -} - -// TestProcessAddRemoveEntry tests the updating of our and their balances when -// we process adds, settles and fails. It also tests the mutating of add and -// remove heights. -func TestProcessAddRemoveEntry(t *testing.T) { - const ( - // addHeight is a non-zero addHeight that is used for htlc - // add heights. - addHeight = 100 - - // removeHeight is a non-zero removeHeight that is used for - // htlc remove heights. - removeHeight = 200 - - // nextHeight is a constant that we use for the nextHeight in - // all unit tests. - nextHeight = 400 - - // updateAmount is the amount that the update is set to. - updateAmount = lnwire.MilliSatoshi(10) - - // startBalance is a balance we start both sides out with - // so that balances can be incremented. - startBalance = lnwire.MilliSatoshi(100) - ) - - tests := []struct { - name string - startHeights heights - whoseCommitChain lntypes.ChannelParty - originator lntypes.ChannelParty - mutateState bool - ourExpectedBalance lnwire.MilliSatoshi - theirExpectedBalance lnwire.MilliSatoshi - expectedHeights heights - updateType updateType - }{ - { - name: "add, remote chain, already processed", - startHeights: heights{ - localAdd: 0, - remoteAdd: addHeight, - localRemove: 0, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Remote, - originator: lntypes.Local, - mutateState: false, - ourExpectedBalance: startBalance, - theirExpectedBalance: startBalance, - expectedHeights: heights{ - localAdd: 0, - remoteAdd: addHeight, - localRemove: 0, - remoteRemove: 0, - }, - updateType: Add, - }, - { - name: "add, local chain, already processed", - startHeights: heights{ - localAdd: addHeight, - remoteAdd: 0, - localRemove: 0, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Local, - originator: lntypes.Local, - mutateState: false, - ourExpectedBalance: startBalance, - theirExpectedBalance: startBalance, - expectedHeights: heights{ - localAdd: addHeight, - remoteAdd: 0, - localRemove: 0, - remoteRemove: 0, - }, - updateType: Add, - }, - { - name: "incoming add, local chain, not mutated", - startHeights: heights{ - localAdd: 0, - remoteAdd: 0, - localRemove: 0, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Local, - originator: lntypes.Remote, - mutateState: false, - ourExpectedBalance: startBalance, - theirExpectedBalance: startBalance - updateAmount, - expectedHeights: heights{ - localAdd: 0, - remoteAdd: 0, - localRemove: 0, - remoteRemove: 0, - }, - updateType: Add, - }, - { - name: "incoming add, local chain, mutated", - startHeights: heights{ - localAdd: 0, - remoteAdd: 0, - localRemove: 0, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Local, - originator: lntypes.Remote, - mutateState: true, - ourExpectedBalance: startBalance, - theirExpectedBalance: startBalance - updateAmount, - expectedHeights: heights{ - localAdd: nextHeight, - remoteAdd: 0, - localRemove: 0, - remoteRemove: 0, - }, - updateType: Add, - }, - - { - name: "outgoing add, remote chain, not mutated", - startHeights: heights{ - localAdd: 0, - remoteAdd: 0, - localRemove: 0, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Remote, - originator: lntypes.Local, - mutateState: false, - ourExpectedBalance: startBalance - updateAmount, - theirExpectedBalance: startBalance, - expectedHeights: heights{ - localAdd: 0, - remoteAdd: 0, - localRemove: 0, - remoteRemove: 0, - }, - updateType: Add, - }, - { - name: "outgoing add, remote chain, mutated", - startHeights: heights{ - localAdd: 0, - remoteAdd: 0, - localRemove: 0, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Remote, - originator: lntypes.Local, - mutateState: true, - ourExpectedBalance: startBalance - updateAmount, - theirExpectedBalance: startBalance, - expectedHeights: heights{ - localAdd: 0, - remoteAdd: nextHeight, - localRemove: 0, - remoteRemove: 0, - }, - updateType: Add, - }, - { - name: "settle, remote chain, already processed", - startHeights: heights{ - localAdd: addHeight, - remoteAdd: addHeight, - localRemove: 0, - remoteRemove: removeHeight, - }, - whoseCommitChain: lntypes.Remote, - originator: lntypes.Local, - mutateState: false, - ourExpectedBalance: startBalance, - theirExpectedBalance: startBalance, - expectedHeights: heights{ - localAdd: addHeight, - remoteAdd: addHeight, - localRemove: 0, - remoteRemove: removeHeight, - }, - updateType: Settle, - }, - { - name: "settle, local chain, already processed", - startHeights: heights{ - localAdd: addHeight, - remoteAdd: addHeight, - localRemove: removeHeight, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Local, - originator: lntypes.Local, - mutateState: false, - ourExpectedBalance: startBalance, - theirExpectedBalance: startBalance, - expectedHeights: heights{ - localAdd: addHeight, - remoteAdd: addHeight, - localRemove: removeHeight, - remoteRemove: 0, - }, - updateType: Settle, - }, - { - // Remote chain, and not processed yet. Incoming settle, - // so we expect our balance to increase. - name: "incoming settle", - startHeights: heights{ - localAdd: addHeight, - remoteAdd: addHeight, - localRemove: 0, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Remote, - originator: lntypes.Remote, - mutateState: false, - ourExpectedBalance: startBalance + updateAmount, - theirExpectedBalance: startBalance, - expectedHeights: heights{ - localAdd: addHeight, - remoteAdd: addHeight, - localRemove: 0, - remoteRemove: 0, - }, - updateType: Settle, - }, - { - // Remote chain, and not processed yet. Incoming settle, - // so we expect our balance to increase. - name: "outgoing settle", - startHeights: heights{ - localAdd: addHeight, - remoteAdd: addHeight, - localRemove: 0, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Remote, - originator: lntypes.Local, - mutateState: false, - ourExpectedBalance: startBalance, - theirExpectedBalance: startBalance + updateAmount, - expectedHeights: heights{ - localAdd: addHeight, - remoteAdd: addHeight, - localRemove: 0, - remoteRemove: 0, - }, - updateType: Settle, - }, - { - // Remote chain, and not processed yet. Incoming fail, - // so we expect their balance to increase. - name: "incoming fail", - startHeights: heights{ - localAdd: addHeight, - remoteAdd: addHeight, - localRemove: 0, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Remote, - originator: lntypes.Remote, - mutateState: false, - ourExpectedBalance: startBalance, - theirExpectedBalance: startBalance + updateAmount, - expectedHeights: heights{ - localAdd: addHeight, - remoteAdd: addHeight, - localRemove: 0, - remoteRemove: 0, - }, - updateType: Fail, - }, - { - // Remote chain, and not processed yet. Outgoing fail, - // so we expect our balance to increase. - name: "outgoing fail", - startHeights: heights{ - localAdd: addHeight, - remoteAdd: addHeight, - localRemove: 0, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Remote, - originator: lntypes.Local, - mutateState: false, - ourExpectedBalance: startBalance + updateAmount, - theirExpectedBalance: startBalance, - expectedHeights: heights{ - localAdd: addHeight, - remoteAdd: addHeight, - localRemove: 0, - remoteRemove: 0, - }, - updateType: Fail, - }, - { - // Local chain, and not processed yet. Incoming settle, - // so we expect our balance to increase. Mutate is - // true, so we expect our remove removeHeight to have - // changed. - name: "fail, our remove height mutated", - startHeights: heights{ - localAdd: addHeight, - remoteAdd: addHeight, - localRemove: 0, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Local, - originator: lntypes.Remote, - mutateState: true, - ourExpectedBalance: startBalance + updateAmount, - theirExpectedBalance: startBalance, - expectedHeights: heights{ - localAdd: addHeight, - remoteAdd: addHeight, - localRemove: nextHeight, - remoteRemove: 0, - }, - updateType: Settle, - }, - { - // Remote chain, and not processed yet. Incoming settle, - // so we expect our balance to increase. Mutate is - // true, so we expect their remove removeHeight to have - // changed. - name: "fail, their remove height mutated", - startHeights: heights{ - localAdd: addHeight, - remoteAdd: addHeight, - localRemove: 0, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Remote, - originator: lntypes.Remote, - mutateState: true, - ourExpectedBalance: startBalance + updateAmount, - theirExpectedBalance: startBalance, - expectedHeights: heights{ - localAdd: addHeight, - remoteAdd: addHeight, - localRemove: 0, - remoteRemove: nextHeight, - }, - updateType: Settle, - }, - } - - for _, test := range tests { - test := test - - t.Run(test.name, func(t *testing.T) { - t.Parallel() - - heights := test.startHeights - update := &paymentDescriptor{ - Amount: updateAmount, - addCommitHeights: lntypes.Dual[uint64]{ - Local: heights.localAdd, - Remote: heights.remoteAdd, - }, - removeCommitHeights: lntypes.Dual[uint64]{ - Local: heights.localRemove, - Remote: heights.remoteRemove, - }, - EntryType: test.updateType, - } - - var ( - // Start both parties off with an initial - // balance. Copy by value here so that we do - // not mutate the startBalance constant. - ourBalance, theirBalance = startBalance, - startBalance - ) - - // Choose the processing function we need based on the - // update type. Process remove is used for settles, - // fails and malformed htlcs. - process := processRemoveEntry - heightDual := &update.removeCommitHeights - if test.updateType == Add { - process = processAddEntry - heightDual = &update.addCommitHeights - } - - if heightDual.GetForParty(test.whoseCommitChain) == 0 { - process( - update, &ourBalance, &theirBalance, - test.originator, - ) - - if test.mutateState { - heightDual.SetForParty( - test.whoseCommitChain, - nextHeight, - ) - } - } - - // Check that balances were updated as expected. - if ourBalance != test.ourExpectedBalance { - t.Fatalf("expected our balance: %v, got: %v", - test.ourExpectedBalance, ourBalance) - } - - if theirBalance != test.theirExpectedBalance { - t.Fatalf("expected their balance: %v, got: %v", - test.theirExpectedBalance, theirBalance) - } - - // Check that heights on the update are as expected. - checkHeights(t, update, test.expectedHeights) - }) - } -} - // TestChannelUnsignedAckedFailure tests that unsigned acked updates are // properly restored after signing for them and disconnecting. //