Skip to content

Commit

Permalink
lnwallet: inline processUpdateFee and remove the function entirely
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@ProofOfKeags
ProofOfKeags committed Oct 11, 2024
1 parent 05347c8 commit 819239c
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 234 deletions.
28 changes: 11 additions & 17 deletions lnwallet/channel.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
217 changes: 0 additions & 217 deletions lnwallet/channel_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 819239c

Please sign in to comment.