From 49add0f57b3af7b47a800caa143a644499d0f0a1 Mon Sep 17 00:00:00 2001 From: Keagan McClelland Date: Wed, 17 Jul 2024 13:09:04 -0700 Subject: [PATCH] lnwallet: eliminate inner-most layer of evil mutateState nonsense This commit begins the process of moving towards a more principled means of state tracking. We eliminate the mutateState argument from processAddEntry and processRemoveEntry and move the responsibility of mutating said state to the call-sites. The current call-sites of these functions still have their *own* mutateState argument which will be eliminated during upcoming commits. However, following the principle of micro-commits I opted to break these changes up to make review simpler. --- lnwallet/channel.go | 119 +++++++++++++++++++++------------------ lnwallet/channel_test.go | 20 +++++-- 2 files changed, 79 insertions(+), 60 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 6b6799227..13a421458 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -2953,10 +2953,19 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance, skipThem[addEntry.HtlcIndex] = struct{}{} - processRemoveEntry( - entry, ourBalance, theirBalance, nextHeight, - whoseCommitChain, true, mutateState, - ) + rmvHeights := &entry.removeCommitHeights + rmvHeight := rmvHeights.GetForParty(whoseCommitChain) + if rmvHeight == 0 { + processRemoveEntry( + entry, ourBalance, theirBalance, true, + ) + + if mutateState { + rmvHeights.SetForParty( + whoseCommitChain, nextHeight, + ) + } + } } for _, entry := range view.TheirUpdates { switch entry.EntryType { @@ -2993,10 +3002,19 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance, skipUs[addEntry.HtlcIndex] = struct{}{} - processRemoveEntry( - entry, ourBalance, theirBalance, nextHeight, - whoseCommitChain, false, mutateState, - ) + rmvHeights := &entry.removeCommitHeights + rmvHeight := rmvHeights.GetForParty(whoseCommitChain) + if rmvHeight == 0 { + processRemoveEntry( + entry, ourBalance, theirBalance, false, + ) + + if mutateState { + rmvHeights.SetForParty( + whoseCommitChain, nextHeight, + ) + } + } } // Next we take a second pass through all the log entries, skipping any @@ -3008,10 +3026,24 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance, continue } - processAddEntry( - entry, ourBalance, theirBalance, nextHeight, - whoseCommitChain, false, mutateState, - ) + // Skip the entries that have already had their add commit + // height set for this commit chain. + addHeights := &entry.addCommitHeights + addHeight := addHeights.GetForParty(whoseCommitChain) + if addHeight == 0 { + processAddEntry( + entry, ourBalance, theirBalance, false, + ) + + // If we are mutating the state, then set the add + // height for the appropriate commitment chain to the + // next height. + if mutateState { + addHeights.SetForParty( + whoseCommitChain, nextHeight, + ) + } + } newView.OurUpdates = append(newView.OurUpdates, entry) } @@ -3021,10 +3053,24 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance, continue } - processAddEntry( - entry, ourBalance, theirBalance, nextHeight, - whoseCommitChain, true, mutateState, - ) + // Skip the entries that have already had their add commit + // height set for this commit chain. + addHeights := &entry.addCommitHeights + addHeight := addHeights.GetForParty(whoseCommitChain) + if addHeight == 0 { + processAddEntry( + entry, ourBalance, theirBalance, true, + ) + + // If we are mutating the state, then set the add + // height for the appropriate commitment chain to the + // next height. + if mutateState { + addHeights.SetForParty( + whoseCommitChain, nextHeight, + ) + } + } newView.TheirUpdates = append(newView.TheirUpdates, entry) } @@ -3081,23 +3127,7 @@ func (lc *LightningChannel) fetchParent(entry *paymentDescriptor, // 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, nextHeight uint64, - whoseCommitChain lntypes.ChannelParty, isIncoming, mutateState bool) { - - // If we're evaluating this entry for the remote chain (to create/view - // a new commitment), then we'll may be updating the height this entry - // was added to the chain. Otherwise, we may be updating the entry's - // height w.r.t the local chain. - var addHeight *uint64 - if whoseCommitChain.IsRemote() { - addHeight = &htlc.addCommitHeights.Remote - } else { - addHeight = &htlc.addCommitHeights.Local - } - - if *addHeight != 0 { - return - } + theirBalance *lnwire.MilliSatoshi, isIncoming bool) { if isIncoming { // If this is a new incoming (un-committed) HTLC, then we need @@ -3109,30 +3139,13 @@ func processAddEntry(htlc *paymentDescriptor, ourBalance, // going HTLC to reflect the pending balance. *ourBalance -= htlc.Amount } - - if mutateState { - *addHeight = nextHeight - } } // 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, nextHeight uint64, - whoseCommitChain lntypes.ChannelParty, isIncoming, mutateState bool) { - - var removeHeight *uint64 - if whoseCommitChain.IsRemote() { - removeHeight = &htlc.removeCommitHeights.Remote - } else { - removeHeight = &htlc.removeCommitHeights.Local - } - - // Ignore any removal entries which have already been processed. - if *removeHeight != 0 { - return - } + theirBalance *lnwire.MilliSatoshi, isIncoming bool) { switch { // If an incoming HTLC is being settled, then this means that we've @@ -3159,10 +3172,6 @@ func processRemoveEntry(htlc *paymentDescriptor, ourBalance, case !isIncoming && (htlc.EntryType == Fail || htlc.EntryType == MalformedFail): *ourBalance += htlc.Amount } - - if mutateState { - *removeHeight = nextHeight - } } // processFeeUpdate processes a log update that updates the current commitment diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 82b5d5554..997f0c22d 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -9613,15 +9613,25 @@ func TestProcessAddRemoveEntry(t *testing.T) { // 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 } - process( - update, &ourBalance, &theirBalance, nextHeight, - test.whoseCommitChain, test.isIncoming, - test.mutateState, - ) + if heightDual.GetForParty(test.whoseCommitChain) == 0 { + process( + update, &ourBalance, &theirBalance, + test.isIncoming, + ) + + if test.mutateState { + heightDual.SetForParty( + test.whoseCommitChain, + nextHeight, + ) + } + } // Check that balances were updated as expected. if ourBalance != test.ourExpectedBalance {