channeldb: use RemoteCommitment in NextLocalHtlcIndex

This commit changes the fallback in NextLocalHtlcIndex to
RemoteCommitment since the LocalHtlcIndex field lags behind
on the LocalCommitment. Without this bug fix, open circuits
would get prematurely trimmed, resulting in more erroneous
logs. A test case is included to check that the fix works.

channeldb/channel.go

@@ -2171,7 +2171,7 @@ func (c *OpenChannel) AdvanceCommitChainTail(fwdPkg *FwdPkg) error {
 // NextLocalHtlcIndex returns the next unallocated local htlc index. To ensure
 // this always returns the next index that has been not been allocated, this
 // will first try to examine any pending commitments, before falling back to the
-// last locked-in local commitment.
+// last locked-in remote commitment.
 func (c *OpenChannel) NextLocalHtlcIndex() (uint64, error) {
 	// First, load the most recent commit diff that we initiated for the
 	// remote party. If no pending commit is found, this is not treated as
@@ -2187,8 +2187,8 @@ func (c *OpenChannel) NextLocalHtlcIndex() (uint64, error) {
 		return pendingRemoteCommit.Commitment.LocalHtlcIndex, nil
 	}
 
-	// Otherwise, fallback to using the local htlc index of our commitment.
-	return c.LocalCommitment.LocalHtlcIndex, nil
+	// Otherwise, fallback to using the local htlc index of their commitment.
+	return c.RemoteCommitment.LocalHtlcIndex, nil
 }
 
 // LoadFwdPkgs scans the forwarding log for any packages that haven't been

htlcswitch/link_test.go

@@ -3158,6 +3158,161 @@ func TestChannelLinkTrimCircuitsNoCommit(t *testing.T) {
 	assertLinkBandwidth(t, alice.link, aliceStartingBandwidth)
 }
 
+// TestChannelLinkTrimCircuitsRemoteCommit checks that the switch and link
+// don't trim circuits if the ADD is locked in on the remote commitment but
+// not on our local commitment.
+func TestChannelLinkTrimCircuitsRemoteCommit(t *testing.T) {
+	t.Parallel()
+
+	const (
+		chanAmt  = btcutil.SatoshiPerBitcoin * 5
+		numHtlcs = 2
+	)
+
+	// We'll start by creating a new link with our chanAmt (5 BTC).
+	aliceLink, bobChan, batchTicker, start, cleanUp, restore, err :=
+		newSingleLinkTestHarness(chanAmt, 0)
+	if err != nil {
+		t.Fatalf("unable to create link: %v", err)
+	}
+
+	if err := start(); err != nil {
+		t.Fatalf("unable to start test harness: %v", err)
+	}
+	defer cleanUp()
+
+	alice := newPersistentLinkHarness(
+		t, aliceLink, batchTicker, restore,
+	)
+
+	// Compute the static fees that will be used to determine the
+	// correctness of Alice's bandwidth when forwarding HTLCs.
+	estimator := chainfee.NewStaticEstimator(6000, 0)
+	feePerKw, err := estimator.EstimateFeePerKW(1)
+	if err != nil {
+		t.Fatalf("unable to query fee estimator: %v", err)
+	}
+
+	defaultCommitFee := alice.channel.StateSnapshot().CommitFee
+	htlcFee := lnwire.NewMSatFromSatoshis(
+		feePerKw.FeeForWeight(input.HTLCWeight),
+	)
+
+	// The starting bandwidth of the channel should be exactly the amount
+	// that we created the channel between her and Bob, minus the commitment
+	// fee and fee of adding an HTLC.
+	expectedBandwidth := lnwire.NewMSatFromSatoshis(
+		chanAmt-defaultCommitFee,
+	) - htlcFee
+	assertLinkBandwidth(t, alice.link, expectedBandwidth)
+
+	// Capture Alice's starting bandwidth to perform later, relative
+	// bandwidth assertions.
+	aliceStartingBandwidth := alice.link.Bandwidth()
+
+	// Next, we'll create an HTLC worth 1 BTC that will be used as a dummy
+	// message for the test.
+	var mockBlob [lnwire.OnionPacketSize]byte
+	htlcAmt := lnwire.NewMSatFromSatoshis(btcutil.SatoshiPerBitcoin)
+	_, htlc, _, err := generatePayment(htlcAmt, htlcAmt, 5, mockBlob)
+	if err != nil {
+		t.Fatalf("unable to create payment: %v", err)
+	}
+
+	// Create `numHtlc` htlcPackets and payment circuits that will be used
+	// to drive the test. All of the packets will use the same dummy HTLC.
+	addPkts, circuits := genAddsAndCircuits(numHtlcs, htlc)
+
+	// To begin the test, start by committing the circuits for our first two
+	// HTLCs.
+	fwdActions := alice.commitCircuits(circuits)
+
+	// Both of these circuits should have successfully added, as this is the
+	// first attempt to send them.
+	if len(fwdActions.Adds) != numHtlcs {
+		t.Fatalf("expected %d circuits to be added", numHtlcs)
+	}
+	alice.assertNumPendingNumOpenCircuits(2, 0)
+
+	// Since both were committed successfully, we will now deliver them to
+	// Alice's link.
+	for _, addPkt := range addPkts {
+		if err := alice.link.HandleSwitchPacket(addPkt); err != nil {
+			t.Fatalf("unable to handle switch packet: %v", err)
+		}
+	}
+
+	// Wait until Alice's link has sent both HTLCs via the peer.
+	alice.checkSent(addPkts)
+
+	// Pass both of the htlcs to Bob.
+	for i, addPkt := range addPkts {
+		pkt, ok := addPkt.htlc.(*lnwire.UpdateAddHTLC)
+		if !ok {
+			t.Fatalf("unable to add packet")
+		}
+
+		pkt.ID = uint64(i)
+
+		_, err := bobChan.ReceiveHTLC(pkt)
+		if err != nil {
+			t.Fatalf("unable to receive htlc: %v", err)
+		}
+	}
+
+	// The resulting bandwidth should reflect that Alice is paying both
+	// htlc amounts, in addition to both htlc fees.
+	assertLinkBandwidth(t, alice.link,
+		aliceStartingBandwidth-numHtlcs*(htlcAmt+htlcFee),
+	)
+
+	// Now, initiate a state transition by Alice so that the pending HTLCs
+	// are locked in.
+	alice.trySignNextCommitment()
+	alice.assertNumPendingNumOpenCircuits(2, 2)
+
+	select {
+	case aliceMsg := <-alice.msgs:
+		// Pass the commitment signature to Bob.
+		sig, ok := aliceMsg.(*lnwire.CommitSig)
+		if !ok {
+			t.Fatalf("alice did not send commitment signature")
+		}
+
+		err := bobChan.ReceiveNewCommitment(sig.CommitSig, sig.HtlcSigs)
+		if err != nil {
+			t.Fatalf("unable to receive new commitment: %v", err)
+		}
+	case <-time.After(time.Second):
+	}
+
+	// Next, revoke Bob's current commitment and send it to Alice so that we
+	// can test that Alice's circuits aren't trimmed.
+	rev, _, err := bobChan.RevokeCurrentCommitment()
+	if err != nil {
+		t.Fatalf("unable to revoke current commitment: %v", err)
+	}
+
+	_, _, _, _, err = alice.channel.ReceiveRevocation(rev)
+	if err != nil {
+		t.Fatalf("unable to receive revocation: %v", err)
+	}
+
+	// Restart Alice's link, which simulates a disconnection with the remote
+	// peer.
+	cleanUp = alice.restart(false)
+	defer cleanUp()
+
+	alice.assertNumPendingNumOpenCircuits(2, 2)
+
+	// Restart the link + switch and check that the number of open circuits
+	// doesn't change.
+	cleanUp = alice.restart(true)
+	defer cleanUp()
+
+	alice.assertNumPendingNumOpenCircuits(2, 2)
+}
+
 // TestChannelLinkBandwidthChanReserve checks that the bandwidth available
 // on the channel link reflects the channel reserve that must be kept
 // at all times.