Merge pull request #9097 from ProofOfKeags/refactor/evaluate-htlc-view

[KILO]: DynComms Prefactor: Refactor/evaluate htlc view

go.mod

@@ -35,7 +35,7 @@ require (
 	github.com/lightningnetwork/lightning-onion v1.2.1-0.20240712235311-98bd56499dfb
 	github.com/lightningnetwork/lnd/cert v1.2.2
 	github.com/lightningnetwork/lnd/clock v1.1.1
-	github.com/lightningnetwork/lnd/fn v1.2.1
+	github.com/lightningnetwork/lnd/fn v1.2.2
 	github.com/lightningnetwork/lnd/healthcheck v1.2.5
 	github.com/lightningnetwork/lnd/kvdb v1.4.10
 	github.com/lightningnetwork/lnd/queue v1.1.1

go.sum

@@ -453,8 +453,8 @@ github.com/lightningnetwork/lnd/cert v1.2.2 h1:71YK6hogeJtxSxw2teq3eGeuy4rHGKcFf
 github.com/lightningnetwork/lnd/cert v1.2.2/go.mod h1:jQmFn/Ez4zhDgq2hnYSw8r35bqGVxViXhX6Cd7HXM6U=
 github.com/lightningnetwork/lnd/clock v1.1.1 h1:OfR3/zcJd2RhH0RU+zX/77c0ZiOnIMsDIBjgjWdZgA0=
 github.com/lightningnetwork/lnd/clock v1.1.1/go.mod h1:mGnAhPyjYZQJmebS7aevElXKTFDuO+uNFFfMXK1W8xQ=
-github.com/lightningnetwork/lnd/fn v1.2.1 h1:pPsVGrwi9QBwdLJzaEGK33wmiVKOxs/zc8H7+MamFf0=
+github.com/lightningnetwork/lnd/fn v1.2.2 h1:rVtmGW1cQTmYce2XdUbRcc5qLDxqu+aQ6IGRpyspakk=
-github.com/lightningnetwork/lnd/fn v1.2.1/go.mod h1:SyFohpVrARPKH3XVAJZlXdVe+IwMYc4OMAvrDY32kw0=
+github.com/lightningnetwork/lnd/fn v1.2.2/go.mod h1:SyFohpVrARPKH3XVAJZlXdVe+IwMYc4OMAvrDY32kw0=
 github.com/lightningnetwork/lnd/healthcheck v1.2.5 h1:aTJy5xeBpcWgRtW/PGBDe+LMQEmNm/HQewlQx2jt7OA=
 github.com/lightningnetwork/lnd/healthcheck v1.2.5/go.mod h1:G7Tst2tVvWo7cx6mSBEToQC5L1XOGxzZTPB29g9Rv2I=
 github.com/lightningnetwork/lnd/kvdb v1.4.10 h1:vK89IVv1oVH9ubQWU+EmoCQFeVRaC8kfmOrqHbY5zoY=

lntypes/channel_party.go

@@ -117,3 +117,5 @@ func MapDual[A, B any](d Dual[A], f func(A) B) Dual[B] {
 		Remote: f(d.Remote),
 	}
 }
+
+var BothParties []ChannelParty = []ChannelParty{Local, Remote}

lnwallet/aux_signer.go

@@ -109,10 +109,10 @@ func newAuxHtlcDescriptor(p *paymentDescriptor) AuxHtlcDescriptor {
 		ParentIndex:              p.ParentIndex,
 		EntryType:                p.EntryType,
 		CustomRecords:            p.CustomRecords.Copy(),
-		addCommitHeightRemote:    p.addCommitHeightRemote,
+		addCommitHeightRemote:    p.addCommitHeights.Remote,
-		addCommitHeightLocal:     p.addCommitHeightLocal,
+		addCommitHeightLocal:     p.addCommitHeights.Local,
-		removeCommitHeightRemote: p.removeCommitHeightRemote,
+		removeCommitHeightRemote: p.removeCommitHeights.Remote,
-		removeCommitHeightLocal:  p.removeCommitHeightLocal,
+		removeCommitHeightLocal:  p.removeCommitHeights.Local,
 	}
 }
 

lnwallet/channel_test.go

@@ -7614,13 +7614,13 @@ func TestChannelRestoreCommitHeight(t *testing.T) {
 			t.Fatalf("htlc not found in log")
 		}
 
-		if pd.addCommitHeightLocal != expLocal {
+		if pd.addCommitHeights.Local != expLocal {
 			t.Fatalf("expected local add height to be %d, was %d",
-				expLocal, pd.addCommitHeightLocal)
+				expLocal, pd.addCommitHeights.Local)
 		}
-		if pd.addCommitHeightRemote != expRemote {
+		if pd.addCommitHeights.Remote != expRemote {
 			t.Fatalf("expected remote add height to be %d, was %d",
-				expRemote, pd.addCommitHeightRemote)
+				expRemote, pd.addCommitHeights.Remote)
 		}
 		return newChannel
 	}
@@ -8402,16 +8402,20 @@ func TestFetchParent(t *testing.T) {
 			remoteEntries: []*paymentDescriptor{
 				// This entry will be added at log index =0.
 				{
-					HtlcIndex:             1,
+					HtlcIndex: 1,
-					addCommitHeightLocal:  100,
+					addCommitHeights: lntypes.Dual[uint64]{
-					addCommitHeightRemote: 100,
+						Local:  100,
+						Remote: 100,
+					},
 				},
 				// This entry will be added at log index =1, it
 				// is the parent entry we are looking for.
 				{
-					HtlcIndex:             2,
+					HtlcIndex: 2,
-					addCommitHeightLocal:  100,
+					addCommitHeights: lntypes.Dual[uint64]{
-					addCommitHeightRemote: 0,
+						Local:  100,
+						Remote: 0,
+					},
 				},
 			},
 			whoseCommitChain: lntypes.Remote,
@@ -8424,16 +8428,20 @@ func TestFetchParent(t *testing.T) {
 			remoteEntries: []*paymentDescriptor{
 				// This entry will be added at log index =0.
 				{
-					HtlcIndex:             1,
+					HtlcIndex: 1,
-					addCommitHeightLocal:  100,
+					addCommitHeights: lntypes.Dual[uint64]{
-					addCommitHeightRemote: 100,
+						Local:  100,
+						Remote: 100,
+					},
 				},
 				// This entry will be added at log index =1, it
 				// is the parent entry we are looking for.
 				{
-					HtlcIndex:             2,
+					HtlcIndex: 2,
-					addCommitHeightLocal:  0,
+					addCommitHeights: lntypes.Dual[uint64]{
-					addCommitHeightRemote: 100,
+						Local:  0,
+						Remote: 100,
+					},
 				},
 			},
 			localEntries:     nil,
@@ -8447,16 +8455,20 @@ func TestFetchParent(t *testing.T) {
 			localEntries: []*paymentDescriptor{
 				// This entry will be added at log index =0.
 				{
-					HtlcIndex:             1,
+					HtlcIndex: 1,
-					addCommitHeightLocal:  100,
+					addCommitHeights: lntypes.Dual[uint64]{
-					addCommitHeightRemote: 100,
+						Local:  100,
+						Remote: 100,
+					},
 				},
 				// This entry will be added at log index =1, it
 				// is the parent entry we are looking for.
 				{
-					HtlcIndex:             2,
+					HtlcIndex: 2,
-					addCommitHeightLocal:  0,
+					addCommitHeights: lntypes.Dual[uint64]{
-					addCommitHeightRemote: 100,
+						Local:  0,
+						Remote: 100,
+					},
 				},
 			},
 			remoteEntries:    nil,
@@ -8471,16 +8483,20 @@ func TestFetchParent(t *testing.T) {
 			localEntries: []*paymentDescriptor{
 				// This entry will be added at log index =0.
 				{
-					HtlcIndex:             1,
+					HtlcIndex: 1,
-					addCommitHeightLocal:  100,
+					addCommitHeights: lntypes.Dual[uint64]{
-					addCommitHeightRemote: 100,
+						Local:  100,
+						Remote: 100,
+					},
 				},
 				// This entry will be added at log index =1, it
 				// is the parent entry we are looking for.
 				{
-					HtlcIndex:             2,
+					HtlcIndex: 2,
-					addCommitHeightLocal:  100,
+					addCommitHeights: lntypes.Dual[uint64]{
-					addCommitHeightRemote: 0,
+						Local:  100,
+						Remote: 0,
+					},
 				},
 			},
 			remoteEntries:    nil,
@@ -8495,16 +8511,20 @@ func TestFetchParent(t *testing.T) {
 			remoteEntries: []*paymentDescriptor{
 				// This entry will be added at log index =0.
 				{
-					HtlcIndex:             1,
+					HtlcIndex: 1,
-					addCommitHeightLocal:  100,
+					addCommitHeights: lntypes.Dual[uint64]{
-					addCommitHeightRemote: 0,
+						Local:  100,
+						Remote: 0,
+					},
 				},
 				// This entry will be added at log index =1, it
 				// is the parent entry we are looking for.
 				{
-					HtlcIndex:             2,
+					HtlcIndex: 2,
-					addCommitHeightLocal:  100,
+					addCommitHeights: lntypes.Dual[uint64]{
-					addCommitHeightRemote: 100,
+						Local:  100,
+						Remote: 100,
+					},
 				},
 			},
 			whoseCommitChain: lntypes.Remote,
@@ -8518,16 +8538,20 @@ func TestFetchParent(t *testing.T) {
 			localEntries: []*paymentDescriptor{
 				// This entry will be added at log index =0.
 				{
-					HtlcIndex:             1,
+					HtlcIndex: 1,
-					addCommitHeightLocal:  0,
+					addCommitHeights: lntypes.Dual[uint64]{
-					addCommitHeightRemote: 100,
+						Local:  0,
+						Remote: 100,
+					},
 				},
 				// This entry will be added at log index =1, it
 				// is the parent entry we are looking for.
 				{
-					HtlcIndex:             2,
+					HtlcIndex: 2,
-					addCommitHeightLocal:  100,
+					addCommitHeights: lntypes.Dual[uint64]{
-					addCommitHeightRemote: 100,
+						Local:  100,
+						Remote: 100,
+					},
 				},
 			},
 			remoteEntries:    nil,
@@ -8626,6 +8650,7 @@ func TestEvaluateView(t *testing.T) {
 		name             string
 		ourHtlcs         []*paymentDescriptor
 		theirHtlcs       []*paymentDescriptor
+		channelInitiator lntypes.ChannelParty
 		whoseCommitChain lntypes.ChannelParty
 		mutateState      bool
 
@@ -8655,6 +8680,7 @@ func TestEvaluateView(t *testing.T) {
 	}{
 		{
 			name:             "our fee update is applied",
+			channelInitiator: lntypes.Local,
 			whoseCommitChain: lntypes.Local,
 			mutateState:      false,
 			ourHtlcs: []*paymentDescriptor{
@@ -8672,6 +8698,7 @@ func TestEvaluateView(t *testing.T) {
 		},
 		{
 			name:             "their fee update is applied",
+			channelInitiator: lntypes.Remote,
 			whoseCommitChain: lntypes.Local,
 			mutateState:      false,
 			ourHtlcs:         []*paymentDescriptor{},
@@ -8728,10 +8755,12 @@ func TestEvaluateView(t *testing.T) {
 			mutateState:      true,
 			ourHtlcs: []*paymentDescriptor{
 				{
-					HtlcIndex:            0,
+					HtlcIndex: 0,
-					Amount:               htlcAddAmount,
+					Amount:    htlcAddAmount,
-					EntryType:            Add,
+					EntryType: Add,
-					addCommitHeightLocal: addHeight,
+					addCommitHeights: lntypes.Dual[uint64]{
+						Local: addHeight,
+					},
 				},
 			},
 			theirHtlcs: []*paymentDescriptor{
@@ -8763,10 +8792,12 @@ func TestEvaluateView(t *testing.T) {
 			mutateState:      false,
 			ourHtlcs: []*paymentDescriptor{
 				{
-					HtlcIndex:            0,
+					HtlcIndex: 0,
-					Amount:               htlcAddAmount,
+					Amount:    htlcAddAmount,
-					EntryType:            Add,
+					EntryType: Add,
-					addCommitHeightLocal: addHeight,
+					addCommitHeights: lntypes.Dual[uint64]{
+						Local: addHeight,
+					},
 				},
 			},
 			theirHtlcs: []*paymentDescriptor{
@@ -8813,16 +8844,20 @@ func TestEvaluateView(t *testing.T) {
 			},
 			theirHtlcs: []*paymentDescriptor{
 				{
-					HtlcIndex:            0,
+					HtlcIndex: 0,
-					Amount:               htlcAddAmount,
+					Amount:    htlcAddAmount,
-					EntryType:            Add,
+					EntryType: Add,
-					addCommitHeightLocal: addHeight,
+					addCommitHeights: lntypes.Dual[uint64]{
+						Local: addHeight,
+					},
 				},
 				{
-					HtlcIndex:            1,
+					HtlcIndex: 1,
-					Amount:               htlcAddAmount,
+					Amount:    htlcAddAmount,
-					EntryType:            Add,
+					EntryType: Add,
-					addCommitHeightLocal: addHeight,
+					addCommitHeights: lntypes.Dual[uint64]{
+						Local: addHeight,
+					},
 				},
 			},
 			expectedFee: feePerKw,
@@ -8857,10 +8892,12 @@ func TestEvaluateView(t *testing.T) {
 			},
 			theirHtlcs: []*paymentDescriptor{
 				{
-					HtlcIndex:            0,
+					HtlcIndex: 0,
-					Amount:               htlcAddAmount,
+					Amount:    htlcAddAmount,
-					EntryType:            Add,
+					EntryType: Add,
-					addCommitHeightLocal: addHeight,
+					addCommitHeights: lntypes.Dual[uint64]{
+						Local: addHeight,
+					},
 				},
 			},
 			expectedFee: feePerKw,
@@ -8877,8 +8914,10 @@ func TestEvaluateView(t *testing.T) {
 		test := test
 
 		t.Run(test.name, func(t *testing.T) {
+			isInitiator := test.channelInitiator == lntypes.Local
 			lc := LightningChannel{
 				channelState: &channeldb.OpenChannel{
+					IsInitiator:       isInitiator,
 					TotalMSatSent:     0,
 					TotalMSatReceived: 0,
 				},
@@ -8907,40 +8946,62 @@ func TestEvaluateView(t *testing.T) {
 			}
 
 			view := &HtlcView{
-				OurUpdates:   test.ourHtlcs,
+				Updates: lntypes.Dual[[]*paymentDescriptor]{
-				TheirUpdates: test.theirHtlcs,
+					Local:  test.ourHtlcs,
-				FeePerKw:     feePerKw,
+					Remote: test.theirHtlcs,
+				},
+				FeePerKw: feePerKw,
 			}
 
-			var (
+			// Evaluate the htlc view, mutate as test expects.
-				// Create vars to store balance changes. We do
+			// We do not check the balance deltas in this test
-				// not check these values in this test because
+			// because balance modification happens on the htlc
-				// balance modification happens on the htlc
+			// processing level.
-				// processing level.
+			result, uncommitted, _, err := lc.evaluateHTLCView(
-				ourBalance   lnwire.MilliSatoshi
+				view, test.whoseCommitChain, nextHeight,
-				theirBalance lnwire.MilliSatoshi
 			)
 
-			// Evaluate the htlc view, mutate as test expects.
-			result, err := lc.evaluateHTLCView(
-				view, &ourBalance, &theirBalance, nextHeight,
-				test.whoseCommitChain, test.mutateState,
-			)
 			if err != nil {
 				t.Fatalf("unexpected error: %v", err)
 			}
 
+			// TODO(proofofkeags): This block is here because we
+			// extracted this code from a previous implementation
+			// of evaluateHTLCView, due to a reduced scope of
+			// responsibility of that function. Consider removing
+			// it from the test altogether.
+			if test.mutateState {
+				for _, party := range lntypes.BothParties {
+					us := uncommitted.GetForParty(party)
+					for _, u := range us {
+						u.setCommitHeight(
+							test.whoseCommitChain,
+							nextHeight,
+						)
+						if test.whoseCommitChain ==
+							lntypes.Local &&
+							u.EntryType == Settle {
+
+							lc.recordSettlement(
+								party, u.Amount,
+							)
+						}
+					}
+				}
+			}
+
 			if result.FeePerKw != test.expectedFee {
 				t.Fatalf("expected fee: %v, got: %v",
 					test.expectedFee, result.FeePerKw)
 			}
 
 			checkExpectedHtlcs(
-				t, result.OurUpdates, test.ourExpectedHtlcs,
+				t, result.Updates.Local, test.ourExpectedHtlcs,
 			)
 
 			checkExpectedHtlcs(
-				t, result.TheirUpdates, test.theirExpectedHtlcs,
+				t, result.Updates.Remote,
+				test.theirExpectedHtlcs,
 			)
 
 			if lc.channelState.TotalMSatSent != test.expectSent {
@@ -8987,617 +9048,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,
-				addCommitHeightRemote:    heights.remoteAdd,
-				addCommitHeightLocal:     heights.localAdd,
-				removeCommitHeightRemote: heights.remoteRemove,
-				removeCommitHeightLocal:  heights.localRemove,
-				EntryType:                FeeUpdate,
-			}
-
-			view := &HtlcView{
-				FeePerKw: chainfee.SatPerKWeight(feePerKw),
-			}
-			processFeeUpdate(
-				update, nextHeight, test.whoseCommitChain,
-				test.mutate, view,
-			)
-
-			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.addCommitHeightLocal,
-		localRemove:  update.removeCommitHeightLocal,
-		remoteAdd:    update.addCommitHeightRemote,
-		remoteRemove: update.removeCommitHeightRemote,
-	}
-
-	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
-		isIncoming           bool
-		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,
-			isIncoming:           false,
-			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,
-			isIncoming:           false,
-			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,
-			isIncoming:           true,
-			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,
-			isIncoming:           true,
-			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,
-			isIncoming:           false,
-			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,
-			isIncoming:           false,
-			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,
-			isIncoming:           false,
-			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,
-			isIncoming:           false,
-			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,
-			isIncoming:           true,
-			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,
-			isIncoming:           false,
-			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,
-			isIncoming:           true,
-			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,
-			isIncoming:           false,
-			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,
-			isIncoming:           true,
-			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,
-			isIncoming:           true,
-			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,
-				addCommitHeightLocal:     heights.localAdd,
-				addCommitHeightRemote:    heights.remoteAdd,
-				removeCommitHeightLocal:  heights.localRemove,
-				removeCommitHeightRemote: 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
-			if test.updateType == Add {
-				process = processAddEntry
-			}
-
-			process(
-				update, &ourBalance, &theirBalance, nextHeight,
-				test.whoseCommitChain, test.isIncoming,
-				test.mutateState,
-			)
-
-			// 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.
 //

lnwallet/commitment.go

@@ -702,7 +702,7 @@ func (cb *CommitmentBuilder) createUnsignedCommitmentTx(ourBalance,
 	}
 
 	numHTLCs := int64(0)
-	for _, htlc := range filteredHTLCView.OurUpdates {
+	for _, htlc := range filteredHTLCView.Updates.Local {
 		if HtlcIsDust(
 			cb.chanState.ChanType, false, whoseCommit, feePerKw,
 			htlc.Amount.ToSatoshis(), dustLimit,
@@ -713,7 +713,7 @@ func (cb *CommitmentBuilder) createUnsignedCommitmentTx(ourBalance,
 
 		numHTLCs++
 	}
-	for _, htlc := range filteredHTLCView.TheirUpdates {
+	for _, htlc := range filteredHTLCView.Updates.Remote {
 		if HtlcIsDust(
 			cb.chanState.ChanType, true, whoseCommit, feePerKw,
 			htlc.Amount.ToSatoshis(), dustLimit,
@@ -827,7 +827,7 @@ func (cb *CommitmentBuilder) createUnsignedCommitmentTx(ourBalance,
 	// purposes of sorting.
 	cltvs := make([]uint32, len(commitTx.TxOut))
 	htlcIndexes := make([]input.HtlcIndex, len(commitTx.TxOut))
-	for _, htlc := range filteredHTLCView.OurUpdates {
+	for _, htlc := range filteredHTLCView.Updates.Local {
 		if HtlcIsDust(
 			cb.chanState.ChanType, false, whoseCommit, feePerKw,
 			htlc.Amount.ToSatoshis(), dustLimit,
@@ -855,7 +855,7 @@ func (cb *CommitmentBuilder) createUnsignedCommitmentTx(ourBalance,
 		cltvs = append(cltvs, htlc.Timeout)               //nolint
 		htlcIndexes = append(htlcIndexes, htlc.HtlcIndex) //nolint
 	}
-	for _, htlc := range filteredHTLCView.TheirUpdates {
+	for _, htlc := range filteredHTLCView.Updates.Remote {
 		if HtlcIsDust(
 			cb.chanState.ChanType, true, whoseCommit, feePerKw,
 			htlc.Amount.ToSatoshis(), dustLimit,

lnwallet/payment_descriptor.go

@@ -6,6 +6,7 @@ import (
 	"github.com/lightningnetwork/lnd/channeldb"
 	"github.com/lightningnetwork/lnd/channeldb/models"
 	"github.com/lightningnetwork/lnd/input"
+	"github.com/lightningnetwork/lnd/lntypes"
 	"github.com/lightningnetwork/lnd/lnwire"
 )
 
@@ -165,16 +166,14 @@ type paymentDescriptor struct {
 	// which included this HTLC on either the remote or local commitment
 	// chain. This value is used to determine when an HTLC is fully
 	// "locked-in".
-	addCommitHeightRemote uint64
+	addCommitHeights lntypes.Dual[uint64]
-	addCommitHeightLocal  uint64
 
 	// removeCommitHeight[Remote|Local] encodes the height of the
 	// commitment which removed the parent pointer of this
 	// paymentDescriptor either due to a timeout or a settle. Once both
 	// these heights are below the tail of both chains, the log entries can
 	// safely be removed.
-	removeCommitHeightRemote uint64
+	removeCommitHeights lntypes.Dual[uint64]
-	removeCommitHeightLocal  uint64
 
 	// OnionBlob is an opaque blob which is used to complete multi-hop
 	// routing.
@@ -284,3 +283,31 @@ func (pd *paymentDescriptor) toLogUpdate() channeldb.LogUpdate {
 		UpdateMsg: msg,
 	}
 }
+
+// setCommitHeight updates the appropriate addCommitHeight and/or
+// removeCommitHeight for whoseCommitChain and locks it in at nextHeight.
+func (pd *paymentDescriptor) setCommitHeight(
+	whoseCommitChain lntypes.ChannelParty, nextHeight uint64) {
+
+	switch pd.EntryType {
+	case Add:
+		pd.addCommitHeights.SetForParty(
+			whoseCommitChain, nextHeight,
+		)
+	case Settle, Fail, MalformedFail:
+		pd.removeCommitHeights.SetForParty(
+			whoseCommitChain, nextHeight,
+		)
+	case FeeUpdate:
+		// Fee updates are applied for all commitments
+		// after they are sent/received, so we consider
+		// them being added and removed at the same
+		// height.
+		pd.addCommitHeights.SetForParty(
+			whoseCommitChain, nextHeight,
+		)
+		pd.removeCommitHeights.SetForParty(
+			whoseCommitChain, nextHeight,
+		)
+	}
+}

lnwallet/update_log.go

@@ -153,6 +153,7 @@ func compactLogs(ourLog, theirLog *updateLog,
 			nextA = e.Next()
 
 			htlc := e.Value
+			rmvHeights := htlc.removeCommitHeights
 
 			// We skip Adds, as they will be removed along with the
 			// fail/settles below.
@@ -162,9 +163,7 @@ func compactLogs(ourLog, theirLog *updateLog,
 
 			// If the HTLC hasn't yet been removed from either
 			// chain, the skip it.
-			if htlc.removeCommitHeightRemote == 0 ||
+			if rmvHeights.Remote == 0 || rmvHeights.Local == 0 {
-				htlc.removeCommitHeightLocal == 0 {
-
 				continue
 			}
 
@@ -172,8 +171,8 @@ func compactLogs(ourLog, theirLog *updateLog,
 			// is at least the height in which the HTLC was
 			// removed, then evict the settle/timeout entry along
 			// with the original add entry.
-			if remoteChainTail >= htlc.removeCommitHeightRemote &&
+			if remoteChainTail >= rmvHeights.Remote &&
-				localChainTail >= htlc.removeCommitHeightLocal {
+				localChainTail >= rmvHeights.Local {
 
 				// Fee updates have no parent htlcs, so we only
 				// remove the update itself.