Merge pull request #4107 from Crypt-iQ/switch_err_0220

htlcswitch: log fixes

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.go

@@ -2060,6 +2060,10 @@ func (l *channelLink) updateCommitTx() error {
 		return err
 	}
 
+	if err := l.ackDownStreamPackets(); err != nil {
+		return err
+	}
+
 	// The remote party now has a new pending commitment, so we'll update
 	// the contract court to be aware of this new set (the prior old remote
 	// pending).
@@ -2069,11 +2073,7 @@ func (l *channelLink) updateCommitTx() error {
 		Htlcs:   pendingHTLCs,
 	}:
 	case <-l.quit:
-		return nil
-	}
-
-	if err := l.ackDownStreamPackets(); err != nil {
-		return err
+		return ErrLinkShuttingDown
 	}
 
 	commitSig := &lnwire.CommitSig{

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.

htlcswitch/switch.go

@@ -1164,6 +1164,12 @@ func (s *Switch) handlePacketForward(packet *htlcPacket) error {
 			return err
 		}
 
+		// closeCircuit returns a nil circuit when a settle packet returns an
+		// ErrUnknownCircuit error upon the inner call to CloseCircuit.
+		if circuit == nil {
+			return nil
+		}
+
 		fail, isFail := htlc.(*lnwire.UpdateFailHTLC)
 		if isFail && !packet.hasSource {
 			switch {
@@ -1394,19 +1400,28 @@ func (s *Switch) closeCircuit(pkt *htlcPacket) (*PaymentCircuit, error) {
 	// Failed to close circuit because it does not exist. This is likely
 	// because the circuit was already successfully closed.
 	case ErrUnknownCircuit:
-		err := fmt.Errorf("Unable to find target channel "+
-			"for HTLC settle/fail: channel ID = %s, "+
-			"HTLC ID = %d", pkt.outgoingChanID,
-			pkt.outgoingHTLCID)
-		log.Error(err)
-
 		if pkt.destRef != nil {
 			// Add this SettleFailRef to the set of pending settle/fail entries
 			// awaiting acknowledgement.
 			s.pendingSettleFails = append(s.pendingSettleFails, *pkt.destRef)
 		}
 
-		return nil, err
+		// If this is a settle, we will not log an error message as settles
+		// are expected to hit the ErrUnknownCircuit case. The only way fails
+		// can hit this case if the link restarts after having just sent a fail
+		// to the switch.
+		_, isSettle := pkt.htlc.(*lnwire.UpdateFulfillHTLC)
+		if !isSettle {
+			err := fmt.Errorf("unable to find target channel "+
+				"for HTLC fail: channel ID = %s, "+
+				"HTLC ID = %d", pkt.outgoingChanID,
+				pkt.outgoingHTLCID)
+			log.Error(err)
+
+			return nil, err
+		}
+
+		return nil, nil
 
 	// Unexpected error.
 	default:

htlcswitch/switch_test.go

@@ -761,8 +761,8 @@ func TestSwitchForwardSettleAfterFullAdd(t *testing.T) {
 	}
 
 	// Send the settle packet again, which should fail.
-	if err := s2.forward(settle); err == nil {
-		t.Fatalf("expected failure when sending duplicate settle " +
+	if err := s2.forward(settle); err != nil {
+		t.Fatalf("expected success when sending duplicate settle " +
 			"with no pending circuit")
 	}
 }