htlcswitch: Fix failure error handling on outgoing adds.

htlcswitch/link.go

@@ -662,22 +662,18 @@ func (l *channelLink) handleDownStreamPkt(pkt *htlcPacket, isReProcess bool) {
 			// machine, as a result, we'll signal the switch to
 			// cancel the pending payment.
 			default:
+				log.Warnf("Unable to handle downstream add HTLC: %v", err)
+
 				var (
-					isObfuscated bool
+					localFailure = false
 					reason       lnwire.OpaqueReason
 				)
 
-				// We'll parse the sphinx packet enclosed so we
-				// can obtain the shared secret required to
-				// encrypt the error back to the source.
 				failure := lnwire.NewTemporaryChannelFailure(nil)
-				onionReader := bytes.NewReader(htlc.OnionBlob[:])
-				obfuscator, failCode := l.cfg.DecodeOnionObfuscator(onionReader)
 
-				switch {
-				// If we were unable to parse the onion blob,
-				// then we'll send an error back to the source.
-				case failCode != lnwire.CodeNone:
+				// Encrypt the error back to the source unless the payment was
+				// generated locally.
+				if pkt.obfuscator == nil {
 					var b bytes.Buffer
 					err := lnwire.EncodeFailure(&b, failure, 0)
 					if err != nil {
@@ -685,24 +681,22 @@ func (l *channelLink) handleDownStreamPkt(pkt *htlcPacket, isReProcess bool) {
 						return
 					}
 					reason = lnwire.OpaqueReason(b.Bytes())
-					isObfuscated = false
-
-				// Otherwise, we'll send back a proper failure
-				// message.
-				default:
-					reason, err = obfuscator.EncryptFirstHop(failure)
+					localFailure = true
+				} else {
+					var err error
+					reason, err = pkt.obfuscator.EncryptFirstHop(failure)
 					if err != nil {
 						log.Errorf("unable to obfuscate error: %v", err)
 						return
 					}
-					isObfuscated = true
 				}
 
 				failPkt := &htlcPacket{
 					incomingChanID: pkt.incomingChanID,
 					incomingHTLCID: pkt.incomingHTLCID,
 					amount:         htlc.Amount,
-					isObfuscated:   isObfuscated,
+					isRouted:       true,
+					localFailure:   localFailure,
 					htlc: &lnwire.UpdateFailHTLC{
 						Reason: reason,
 					},
@@ -711,7 +705,6 @@ func (l *channelLink) handleDownStreamPkt(pkt *htlcPacket, isReProcess bool) {
 				// TODO(roasbeef): need to identify if sent
 				// from switch so don't need to obfuscate
 				go l.cfg.Switch.forward(failPkt)
-				log.Infof("Unable to handle downstream add HTLC: %v", err)
 				return
 			}
 		}
@@ -1214,7 +1207,6 @@ func (l *channelLink) processLockedInHtlcs(
 				outgoingChanID: l.ShortChanID(),
 				outgoingHTLCID: pd.ParentIndex,
 				amount:         pd.Amount,
-				isObfuscated:   false,
 				htlc: &lnwire.UpdateFailHTLC{
 					Reason: lnwire.OpaqueReason(pd.FailReason),
 				},

htlcswitch/mailbox_test.go

@@ -33,7 +33,6 @@ func TestMailBoxCouriers(t *testing.T) {
 			outgoingChanID: lnwire.NewShortChanIDFromInt(uint64(prand.Int63())),
 			incomingChanID: lnwire.NewShortChanIDFromInt(uint64(prand.Int63())),
 			amount:         lnwire.MilliSatoshi(prand.Int63()),
-			isObfuscated:   i%2 == 0,
 		}
 		sentPackets[i] = pkt
 

htlcswitch/packet.go

@@ -37,11 +37,13 @@ type htlcPacket struct {
 	// any forwarded errors in an additional layer of encryption.
 	obfuscator ErrorEncrypter
 
-	// isObfuscated is set to true if an error occurs as soon as the switch
-	// forwards a packet to the link. If so, and this is an error packet,
-	// then this allows the switch to avoid doubly encrypting the error.
-	//
-	// TODO(andrew.shvv) revisit after refactoring the way of returning
-	// errors inside the htlcswitch packet.
-	isObfuscated bool
+	// localFailure is set to true if an HTLC fails for a local payment before
+	// the first hop. In this case, the failure reason is simply encoded, not
+	// encrypted with any shared secret.
+	localFailure bool
+
+	// isRouted is set to true if the incomingChanID and incomingHTLCID fields
+	// of a forwarded fail packet are already set and do not need to be looked
+	// up in the circuit map.
+	isRouted bool
 }

htlcswitch/switch.go

@@ -1,6 +1,7 @@
 package htlcswitch
 
 import (
+	"bytes"
 	"fmt"
 	"sync"
 	"sync/atomic"
@@ -435,23 +436,39 @@ func (s *Switch) handleLocalDispatch(packet *htlcPacket) error {
 	// We've just received a fail update which means we can finalize the
 	// user payment and return fail response.
 	case *lnwire.UpdateFailHTLC:
-		// We'll attempt to fully decrypt the onion encrypted error. If
-		// we're unable to then we'll bail early.
-		failure, err := payment.deobfuscator.DecryptError(htlc.Reason)
-		if err != nil {
-			userErr := fmt.Sprintf("unable to de-obfuscate "+
-				"onion failure, htlc with hash(%x): %v",
-				payment.paymentHash[:], err)
-			log.Error(userErr)
-			payment.err <- &ForwardingError{
+		var failure *ForwardingError
+		if packet.localFailure {
+			var userErr string
+			r := bytes.NewReader(htlc.Reason)
+			failureMsg, err := lnwire.DecodeFailure(r, 0)
+			if err != nil {
+				userErr = fmt.Sprintf("unable to decode onion failure, "+
+					"htlc with hash(%x): %v", payment.paymentHash[:], err)
+				log.Error(userErr)
+				failureMsg = lnwire.NewTemporaryChannelFailure(nil)
+			}
+			failure = &ForwardingError{
 				ErrorSource:    s.cfg.SelfKey,
 				ExtraMsg:       userErr,
-				FailureMessage: lnwire.NewTemporaryChannelFailure(nil),
+				FailureMessage: failureMsg,
 			}
 		} else {
-			payment.err <- failure
+			// We'll attempt to fully decrypt the onion encrypted error. If
+			// we're unable to then we'll bail early.
+			failure, err = payment.deobfuscator.DecryptError(htlc.Reason)
+			if err != nil {
+				userErr := fmt.Sprintf("unable to de-obfuscate onion failure, "+
+					"htlc with hash(%x): %v", payment.paymentHash[:], err)
+				log.Error(userErr)
+				failure = &ForwardingError{
+					ErrorSource:    s.cfg.SelfKey,
+					ExtraMsg:       userErr,
+					FailureMessage: lnwire.NewTemporaryChannelFailure(nil),
+				}
+			}
 		}
 
+		payment.err <- failure
 		payment.preimage <- zeroPreimage
 		s.removePendingPayment(packet.incomingHTLCID)
 
@@ -503,7 +520,7 @@ func (s *Switch) handlePacketForward(packet *htlcPacket) error {
 			source.HandleSwitchPacket(&htlcPacket{
 				incomingChanID: packet.incomingChanID,
 				incomingHTLCID: packet.incomingHTLCID,
-				isObfuscated:   true,
+				isRouted:       true,
 				htlc: &lnwire.UpdateFailHTLC{
 					Reason: reason,
 				},
@@ -551,7 +568,7 @@ func (s *Switch) handlePacketForward(packet *htlcPacket) error {
 			source.HandleSwitchPacket(&htlcPacket{
 				incomingChanID: packet.incomingChanID,
 				incomingHTLCID: packet.incomingHTLCID,
-				isObfuscated:   true,
+				isRouted:       true,
 				htlc: &lnwire.UpdateFailHTLC{
 					Reason: reason,
 				},
@@ -573,7 +590,7 @@ func (s *Switch) handlePacketForward(packet *htlcPacket) error {
 	// payment circuit by forwarding the settle msg to the channel from
 	// which htlc add packet was initially received.
 	case *lnwire.UpdateFufillHTLC, *lnwire.UpdateFailHTLC:
-		if packet.incomingChanID == (lnwire.ShortChannelID{}) {
+		if !packet.isRouted {
 			// Use circuit map to find the link to forward settle/fail to.
 			circuit := s.circuits.LookupByHTLC(packet.outgoingChanID,
 				packet.outgoingHTLCID)
@@ -603,20 +620,22 @@ func (s *Switch) handlePacketForward(packet *htlcPacket) error {
 			packet.incomingChanID = circuit.IncomingChanID
 			packet.incomingHTLCID = circuit.IncomingHTLCID
 
-			// A blank IncomingChanID in a circuit indicates that it is a
-			// pending user-initiated payment.
-			if circuit.IncomingChanID == (lnwire.ShortChannelID{}) {
-				return s.handleLocalDispatch(packet)
-			}
-
 			// Obfuscate the error message for fail updates before sending back
-			// through the circuit.
-			if htlc, ok := htlc.(*lnwire.UpdateFailHTLC); ok && !packet.isObfuscated {
-				htlc.Reason = circuit.ErrorEncrypter.IntermediateEncrypt(
-					htlc.Reason)
+			// through the circuit unless the payment was generated locally.
+			if circuit.ErrorEncrypter != nil {
+				if htlc, ok := htlc.(*lnwire.UpdateFailHTLC); ok {
+					htlc.Reason = circuit.ErrorEncrypter.IntermediateEncrypt(
+						htlc.Reason)
+				}
 			}
 		}
 
+		// A blank IncomingChanID in a circuit indicates that it is a pending
+		// user-initiated payment.
+		if packet.incomingChanID == (lnwire.ShortChannelID{}) {
+			return s.handleLocalDispatch(packet)
+		}
+
 		source, err := s.getLinkByShortID(packet.incomingChanID)
 		if err != nil {
 			err := errors.Errorf("Unable to get source channel link to "+

htlcswitch/switch_test.go

@@ -270,7 +270,6 @@ func TestSwitchCancel(t *testing.T) {
 		outgoingChanID: bobChannelLink.ShortChanID(),
 		outgoingHTLCID: 0,
 		amount:         1,
-		isObfuscated:   true,
 		htlc:           &lnwire.UpdateFailHTLC{},
 	}
 
@@ -373,7 +372,6 @@ func TestSwitchAddSamePayment(t *testing.T) {
 		outgoingChanID: bobChannelLink.ShortChanID(),
 		outgoingHTLCID: 0,
 		amount:         1,
-		isObfuscated:   true,
 		htlc:           &lnwire.UpdateFailHTLC{},
 	}
 
@@ -397,7 +395,6 @@ func TestSwitchAddSamePayment(t *testing.T) {
 		outgoingChanID: bobChannelLink.ShortChanID(),
 		outgoingHTLCID: 1,
 		amount:         1,
-		isObfuscated:   true,
 		htlc:           &lnwire.UpdateFailHTLC{},
 	}
 
@@ -500,7 +497,6 @@ func TestSwitchSendPayment(t *testing.T) {
 		outgoingChanID: aliceChannelLink.ShortChanID(),
 		outgoingHTLCID: 0,
 		amount:         1,
-		isObfuscated:   true,
 		htlc: &lnwire.UpdateFailHTLC{
 			Reason: reason,
 		},
@@ -522,7 +518,6 @@ func TestSwitchSendPayment(t *testing.T) {
 	packet = &htlcPacket{
 		outgoingChanID: aliceChannelLink.ShortChanID(),
 		outgoingHTLCID: 1,
-		isObfuscated:   true,
 		htlc: &lnwire.UpdateFailHTLC{
 			Reason: reason,
 		},

lnd_test.go

@@ -4043,7 +4043,8 @@ func testAsyncPayments(net *networkHarness, t *harnessTest) {
 	// Open up a payment stream to Alice that we'll use to send payment to
 	// Bob. We also create a small helper function to send payments to Bob,
 	// consuming the payment hashes we generated above.
-	alicePayStream, err := net.Alice.SendPayment(ctxb)
+	ctxt, _ = context.WithTimeout(ctxb, time.Minute)
+	alicePayStream, err := net.Alice.SendPayment(ctxt)
 	if err != nil {
 		t.Fatalf("unable to create payment stream for alice: %v", err)
 	}