routing: fail attempt when no shard is found or circuit generation fails

htlcswitch/switch.go

@@ -1000,6 +1000,8 @@ func (s *Switch) extractResult(deobfuscator ErrorDecrypter, n *networkResult,
 	// We've received a fail update which means we can finalize the
 	// user payment and return fail response.
 	case *lnwire.UpdateFailHTLC:
+		// TODO(yy): construct deobfuscator here to avoid creating it
+		// in paymentLifecycle even for settled HTLCs.
 		paymentErr := s.parseFailedPayment(
 			deobfuscator, attemptID, paymentHash, n.unencrypted,
 			n.isResolution, htlc,

routing/payment_lifecycle.go

@@ -456,15 +456,27 @@ func (p *paymentLifecycle) collectResult(attempt *channeldb.HTLCAttempt) (
 	// below.
 	hash, err := p.shardTracker.GetHash(attempt.AttemptID)
 	if err != nil {
-		return nil, err
+		return p.failAttempt(attempt.AttemptID, err)
 	}
 
 	// Regenerate the circuit for this attempt.
 	_, circuit, err := generateSphinxPacket(
 		&attempt.Route, hash[:], attempt.SessionKey(),
 	)
+	// TODO(yy): We generate this circuit to create the error decryptor,
+	// which is then used in htlcswitch as the deobfuscator to decode the
+	// error from `UpdateFailHTLC`. However, suppose it's an
+	// `UpdateFulfillHTLC` message yet for some reason the sphinx packet is
+	// failed to be generated, we'd miss settling it. This means we should
+	// give it a second chance to try the settlement path in case
+	// `GetAttemptResult` gives us back the preimage. And move the circuit
+	// creation into htlcswitch so it's only constructed when there's a
+	// failure message we need to decode.
 	if err != nil {
-		return nil, err
+		log.Debugf("Unable to generate circuit for attempt %v: %v",
+			attempt.AttemptID, err)
+
+		return p.failAttempt(attempt.AttemptID, err)
 	}
 
 	// Using the created circuit, initialize the error decrypter so we can
@@ -476,6 +488,12 @@ func (p *paymentLifecycle) collectResult(attempt *channeldb.HTLCAttempt) (
 
 	// Now ask the switch to return the result of the payment when
 	// available.
+	//
+	// TODO(yy): consider using htlcswitch to create the `errorDecryptor`
+	// since the htlc is already in db. This will also make the interface
+	// `PaymentAttemptDispatcher` deeper and easier to use. Moreover, we'd
+	// only create the decryptor when received a failure, further saving us
+	// a few CPU cycles.
 	resultChan, err := p.router.cfg.Payer.GetAttemptResult(
 		attempt.AttemptID, p.identifier, errorDecryptor,
 	)
@@ -536,6 +554,9 @@ func (p *paymentLifecycle) collectResult(attempt *channeldb.HTLCAttempt) (
 	)
 	if err != nil {
 		log.Errorf("Unable to settle payment attempt: %v", err)
+
+		// We won't mark the attempt as failed since we already have
+		// the preimage.
 		return nil, err
 	}