From 819239c5c86201180fe8e49ae0c9dd386893d709 Mon Sep 17 00:00:00 2001 From: Keagan McClelland Date: Thu, 18 Jul 2024 17:14:13 -0700 Subject: [PATCH] lnwallet: inline processUpdateFee and remove the function entirely In this commit we observe that the previous commit reduced the role of this function to a single assignment statement with numerous newly irrelevant parameters. This commit makes the choice of inlining it at the two call-sites within evaluateHTLCView and removing the funciton definition entirely. This also allows us to drop a huge portion of newly irrelevant test code. --- lnwallet/channel.go | 28 ++--- lnwallet/channel_test.go | 217 --------------------------------------- 2 files changed, 11 insertions(+), 234 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 2157afe03..6b6f44431 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -2932,9 +2932,11 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance, ) if h == 0 { - processFeeUpdate( - entry, &newView.FeePerKw, nextHeight, - whoseCommitChain, + // If the update wasn't already locked in, + // update the current fee rate to reflect this + // update. + newView.FeePerKw = chainfee.SatPerKWeight( + entry.Amount.ToSatoshis(), ) if mutateState { @@ -2996,11 +2998,14 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, ourBalance, ) if h == 0 { - processFeeUpdate( - entry, &newView.FeePerKw, nextHeight, - whoseCommitChain, + // If the update wasn't already locked in, + // update the current fee rate to reflect this + // update. + newView.FeePerKw = chainfee.SatPerKWeight( + entry.Amount.ToSatoshis(), ) + if mutateState { entry.addCommitHeights.SetForParty( whoseCommitChain, nextHeight, @@ -3206,17 +3211,6 @@ func processRemoveEntry(htlc *paymentDescriptor, ourBalance, } } -// processFeeUpdate processes a log update that updates the current commitment -// fee. -func processFeeUpdate(feeUpdate *paymentDescriptor, - feeRef *chainfee.SatPerKWeight, nextHeight uint64, - whoseCommitChain lntypes.ChannelParty) { - - // If the update wasn't already locked in, update the current fee rate - // to reflect this update. - *feeRef = chainfee.SatPerKWeight(feeUpdate.Amount.ToSatoshis()) -} - // 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 77630d57a..50eb24663 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -9021,223 +9021,6 @@ type heights struct { remoteRemove uint64 } -// TestProcessFeeUpdate tests the applying of fee updates and mutation of -// local and remote add and remove heights on update messages. -func TestProcessFeeUpdate(t *testing.T) { - const ( - // height is a non-zero height that can be used for htlcs - // heights. - height = 200 - - // nextHeight is a constant that we use for the next height in - // all unit tests. - nextHeight = 400 - - // feePerKw is the fee we start all of our unit tests with. - feePerKw = 1 - - // ourFeeUpdateAmt is an amount that we update fees to expressed - // in msat. - ourFeeUpdateAmt = 20000 - - // ourFeeUpdatePerSat is the fee rate *in satoshis* that we - // expect if we update to ourFeeUpdateAmt. - ourFeeUpdatePerSat = chainfee.SatPerKWeight(20) - ) - - tests := []struct { - name string - startHeights heights - expectedHeights heights - whoseCommitChain lntypes.ChannelParty - mutate bool - expectedFee chainfee.SatPerKWeight - }{ - { - // Looking at local chain, local add is non-zero so - // the update has been applied already; no fee change. - name: "non-zero local height, fee unchanged", - startHeights: heights{ - localAdd: height, - localRemove: 0, - remoteAdd: 0, - remoteRemove: height, - }, - expectedHeights: heights{ - localAdd: height, - localRemove: 0, - remoteAdd: 0, - remoteRemove: height, - }, - whoseCommitChain: lntypes.Local, - mutate: false, - expectedFee: feePerKw, - }, - { - // Looking at local chain, local add is zero so the - // update has not been applied yet; we expect a fee - // update. - name: "zero local height, fee changed", - startHeights: heights{ - localAdd: 0, - localRemove: 0, - remoteAdd: height, - remoteRemove: 0, - }, - expectedHeights: heights{ - localAdd: 0, - localRemove: 0, - remoteAdd: height, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Local, - mutate: false, - expectedFee: ourFeeUpdatePerSat, - }, - { - // Looking at remote chain, the remote add height is - // zero, so the update has not been applied so we expect - // a fee change. - name: "zero remote height, fee changed", - startHeights: heights{ - localAdd: height, - localRemove: 0, - remoteAdd: 0, - remoteRemove: 0, - }, - expectedHeights: heights{ - localAdd: height, - localRemove: 0, - remoteAdd: 0, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Remote, - mutate: false, - expectedFee: ourFeeUpdatePerSat, - }, - { - // Looking at remote chain, the remote add height is - // non-zero, so the update has been applied so we expect - // no fee change. - name: "non-zero remote height, no fee change", - startHeights: heights{ - localAdd: height, - localRemove: 0, - remoteAdd: height, - remoteRemove: 0, - }, - expectedHeights: heights{ - localAdd: height, - localRemove: 0, - remoteAdd: height, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Remote, - mutate: false, - expectedFee: feePerKw, - }, - { - // Local add height is non-zero, so the update has - // already been applied; we do not expect fee to - // change or any mutations to be applied. - name: "non-zero local height, mutation not applied", - startHeights: heights{ - localAdd: height, - localRemove: 0, - remoteAdd: 0, - remoteRemove: height, - }, - expectedHeights: heights{ - localAdd: height, - localRemove: 0, - remoteAdd: 0, - remoteRemove: height, - }, - whoseCommitChain: lntypes.Local, - mutate: true, - expectedFee: feePerKw, - }, - { - // Local add is zero and we are looking at our local - // chain, so the update has not been applied yet. We - // expect the local add and remote heights to be - // mutated. - name: "zero height, fee changed, mutation applied", - startHeights: heights{ - localAdd: 0, - localRemove: 0, - remoteAdd: 0, - remoteRemove: 0, - }, - expectedHeights: heights{ - localAdd: nextHeight, - localRemove: nextHeight, - remoteAdd: 0, - remoteRemove: 0, - }, - whoseCommitChain: lntypes.Local, - mutate: true, - expectedFee: ourFeeUpdatePerSat, - }, - } - - for _, test := range tests { - test := test - - t.Run(test.name, func(t *testing.T) { - // Create a fee update with add and remove heights as - // set in the test. - heights := test.startHeights - update := &paymentDescriptor{ - Amount: ourFeeUpdateAmt, - addCommitHeights: lntypes.Dual[uint64]{ - Local: heights.localAdd, - Remote: heights.remoteAdd, - }, - removeCommitHeights: lntypes.Dual[uint64]{ - Local: heights.localRemove, - Remote: heights.remoteRemove, - }, - EntryType: FeeUpdate, - } - - view := &HtlcView{ - FeePerKw: chainfee.SatPerKWeight(feePerKw), - } - - h := update.addCommitHeights.GetForParty( - test.whoseCommitChain, - ) - - if h == 0 { - processFeeUpdate( - update, &view.FeePerKw, nextHeight, - test.whoseCommitChain, - ) - - if test.mutate { - update.addCommitHeights.SetForParty( - test.whoseCommitChain, - nextHeight, - ) - - update.removeCommitHeights.SetForParty( - test.whoseCommitChain, - nextHeight, - ) - } - } - - if view.FeePerKw != test.expectedFee { - t.Fatalf("expected fee: %v, got: %v", - test.expectedFee, feePerKw) - } - - checkHeights(t, update, test.expectedHeights) - }) - } -} - func checkHeights(t *testing.T, update *paymentDescriptor, expected heights) { updateHeights := heights{ localAdd: update.addCommitHeights.Local,