Repository: lightningnetwork/lnd
Pull request #5305 (2021-05-17 12:32:35)

channeldb: read HtlcAttemptInfo session key raw bytes
When running on a node with a lot of payments (+900k), we see a significant performance slowdown from the ec ops in `btcec.PrivKeyFromBytes` used to deserialize the session key for the attempt. 

Part of profile for `ListPayments` (can't take full screenshot at high enough res): 
![Screenshot 2021-05-17 at 14 27 51](https://user-images.githubusercontent.com/42311294/118488614-34ef5700-b71c-11eb-80de-45a3a35ed024.png)


We don't actually need this value after it's been read off disk, except for when we resume an in-flight payment and [regenerate its sphinx packet](https://github.com/lightningnetwork/lnd/blob/d771ed761678f8074bd396db1d77c7b7b859abdc/routing/payment_lifecycle.go#L501) so we can lazily read the raw bytes off disk, and perform the expensive EC ops only when required. Since the serialized session key is just `[32]byte`, we can just swap out the field in the `HtlcAttemptInfo` without a migration. 

PR is can probably be squashed down to 1 commit after review, thought it was more readable this way. 

joostjager: The whole key parsing and construction of the sphinx packet only needs to be done when a payment fails and there is a failure message to decode. The sphinx packet construction itself is also an expensive operation, but it can be done lazily on failure only. Probably out of scope for this PR, but wanted to mention anyway because it is a significant factor in high-throughput scenarios.



Patch:
diff --git a/channeldb/duplicate_payments.go b/channeldb/duplicate_payments.go
index c26f185ad..7dad7df70 100644
--- a/channeldb/duplicate_payments.go
+++ b/channeldb/duplicate_payments.go
@@ -53,7 +53,7 @@ type duplicateHTLCAttemptInfo struct {
 	attemptID uint64
 
 	// sessionKey is the ephemeral key used for this attempt.
-	sessionKey *btcec.PrivateKey
+	sessionKey [btcec.PrivKeyBytesLen]byte
 
 	// route is the route attempted to send the HTLC.
 	route route.Route
@@ -181,7 +181,7 @@ func fetchDuplicatePayment(bucket kvdb.RBucket) (*MPPayment, error) {
 			HTLCAttemptInfo: HTLCAttemptInfo{
 				AttemptID:  attempt.attemptID,
 				Route:      attempt.route,
-				SessionKey: attempt.sessionKey,
+				sessionKey: attempt.sessionKey,
 			},
 		}
 
diff --git a/channeldb/mp_payment.go b/channeldb/mp_payment.go
index 4d14d5047..49a4b4df7 100644
--- a/channeldb/mp_payment.go
+++ b/channeldb/mp_payment.go
@@ -21,8 +21,15 @@ type HTLCAttemptInfo struct {
 	// AttemptID is the unique ID used for this attempt.
 	AttemptID uint64
 
-	// SessionKey is the ephemeral key used for this attempt.
-	SessionKey *btcec.PrivateKey
+	// sessionKey is the raw bytes ephemeral key used for this attempt.
+	// These bytes are lazily read off disk to save ourselves the expensive
+	// EC operations used by btcec.PrivKeyFromBytes.
+	sessionKey [btcec.PrivKeyBytesLen]byte
+
+	// cachedSessionKey is our fully deserialized sesionKey. This value
+	// may be nil if the attempt has just been read from disk and its
+	// session key has not been used yet.
+	cachedSessionKey *btcec.PrivateKey
 
 	// Route is the route attempted to send the HTLC.
 	Route route.Route
@@ -38,6 +45,36 @@ type HTLCAttemptInfo struct {
 	Hash *lntypes.Hash
 }
 
+// NewHtlcAttemptInfo creates a htlc attempt.
+func NewHtlcAttemptInfo(attemptID uint64, sessionKey *btcec.PrivateKey,
+	route route.Route, attemptTime time.Time,
+	hash *lntypes.Hash) *HTLCAttemptInfo {
+
+	var scratch [btcec.PrivKeyBytesLen]byte
+	copy(scratch[:], sessionKey.Serialize())
+
+	return &HTLCAttemptInfo{
+		AttemptID:        attemptID,
+		sessionKey:       scratch,
+		cachedSessionKey: sessionKey,
+		Route:            route,
+		AttemptTime:      attemptTime,
+		Hash:             hash,
+	}
+}
+
+// SessionKey returns the ephemeral key used for a htlc attempt. This function
+// performs expensive ec-ops to obtain the session key if it is not cached.
+func (h *HTLCAttemptInfo) SessionKey() *btcec.PrivateKey {
+	if h.cachedSessionKey == nil {
+		h.cachedSessionKey, _ = btcec.PrivKeyFromBytes(
+			btcec.S256(), h.sessionKey[:],
+		)
+	}
+
+	return h.cachedSessionKey
+}
+
 // HTLCAttempt contains information about a specific HTLC attempt for a given
 // payment. It contains the HTLCAttemptInfo used to send the HTLC, as well
 // as a timestamp and any known outcome of the attempt.
diff --git a/channeldb/payments.go b/channeldb/payments.go
index 72b15ffb7..1abe54a57 100644
--- a/channeldb/payments.go
+++ b/channeldb/payments.go
@@ -919,7 +919,7 @@ func deserializePaymentCreationInfo(r io.Reader) (*PaymentCreationInfo, error) {
 }
 
 func serializeHTLCAttemptInfo(w io.Writer, a *HTLCAttemptInfo) error {
-	if err := WriteElements(w, a.SessionKey); err != nil {
+	if err := WriteElements(w, a.sessionKey); err != nil {
 		return err
 	}
 
@@ -945,7 +945,7 @@ func serializeHTLCAttemptInfo(w io.Writer, a *HTLCAttemptInfo) error {
 
 func deserializeHTLCAttemptInfo(r io.Reader) (*HTLCAttemptInfo, error) {
 	a := &HTLCAttemptInfo{}
-	err := ReadElements(r, &a.SessionKey)
+	err := ReadElements(r, &a.sessionKey)
 	if err != nil {
 		return nil, err
 	}
diff --git a/routing/payment_lifecycle.go b/routing/payment_lifecycle.go
index 90dd684b6..4703a6272 100644
--- a/routing/payment_lifecycle.go
+++ b/routing/payment_lifecycle.go
@@ -499,7 +499,7 @@ func (p *shardHandler) collectResult(attempt *channeldb.HTLCAttemptInfo) (
 
 	// Regenerate the circuit for this attempt.
 	_, circuit, err := generateSphinxPacket(
-		&attempt.Route, hash[:], attempt.SessionKey,
+		&attempt.Route, hash[:], attempt.SessionKey(),
 	)
 	if err != nil {
 		return nil, err
@@ -677,15 +677,11 @@ func (p *shardHandler) createNewPaymentAttempt(rt *route.Route, lastShard bool)
 		rt.Hops[0].ChannelID,
 	)
 
-	// We now have all the information needed to populate
-	// the current attempt information.
-	attempt := &channeldb.HTLCAttemptInfo{
-		AttemptID:   attemptID,
-		AttemptTime: p.router.cfg.Clock.Now(),
-		SessionKey:  sessionKey,
-		Route:       *rt,
-		Hash:        &hash,
-	}
+	// We now have all the information needed to populate the current
+	// attempt information.
+	attempt := channeldb.NewHtlcAttemptInfo(
+		attemptID, sessionKey, *rt, p.router.cfg.Clock.Now(), &hash,
+	)
 
 	return firstHop, htlcAdd, attempt, nil
 }


Test patch:
diff --git a/channeldb/mp_payment_test.go b/channeldb/mp_payment_test.go
new file mode 100644
index 000000000..eb5f0f48c
--- /dev/null
+++ b/channeldb/mp_payment_test.go
@@ -0,0 +1,27 @@
+package channeldb
+
+import (
+	"bytes"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestLazySessionKeyDeserialize tests that we can read htlc attempt session
+// keys that were previously serialized as a private key as raw bytes.
+func TestLazySessionKeyDeserialize(t *testing.T) {
+	var b bytes.Buffer
+
+	// Serialize as a private key.
+	err := WriteElements(&b, priv)
+	require.NoError(t, err)
+
+	// Deserialize into [btcec.PrivKeyBytesLen]byte.
+	attempt := HTLCAttemptInfo{}
+	err = ReadElements(&b, &attempt.sessionKey)
+	require.NoError(t, err)
+	require.Zero(t, b.Len())
+
+	sessionKey := attempt.SessionKey()
+	require.Equal(t, priv, sessionKey)
+}
diff --git a/channeldb/payment_control_test.go b/channeldb/payment_control_test.go
index a524fb0e7..4c9fcf00c 100644
--- a/channeldb/payment_control_test.go
+++ b/channeldb/payment_control_test.go
@@ -37,17 +37,15 @@ func genInfo() (*PaymentCreationInfo, *HTLCAttemptInfo,
 	}
 
 	rhash := sha256.Sum256(preimage[:])
+	attempt := NewHtlcAttemptInfo(
+		0, priv, *testRoute.Copy(), time.Time{}, nil,
+	)
 	return &PaymentCreationInfo{
-			PaymentIdentifier: rhash,
-			Value:             testRoute.ReceiverAmt(),
-			CreationTime:      time.Unix(time.Now().Unix(), 0),
-			PaymentRequest:    []byte("hola"),
-		},
-		&HTLCAttemptInfo{
-			AttemptID:  0,
-			SessionKey: priv,
-			Route:      *testRoute.Copy(),
-		}, preimage, nil
+		PaymentIdentifier: rhash,
+		Value:             testRoute.ReceiverAmt(),
+		CreationTime:      time.Unix(time.Now().Unix(), 0),
+		PaymentRequest:    []byte("hola"),
+	}, attempt, preimage, nil
 }
 
 // TestPaymentControlSwitchFail checks that payment status returns to Failed
diff --git a/channeldb/payments_test.go b/channeldb/payments_test.go
index 9fab9a1fe..370a1d9c5 100644
--- a/channeldb/payments_test.go
+++ b/channeldb/payments_test.go
@@ -68,13 +68,10 @@ func makeFakeInfo() (*PaymentCreationInfo, *HTLCAttemptInfo) {
 		PaymentRequest: []byte(""),
 	}
 
-	a := &HTLCAttemptInfo{
-		AttemptID:   44,
-		SessionKey:  priv,
-		Route:       testRoute,
-		AttemptTime: time.Unix(100, 0),
-		Hash:        &hash,
-	}
+	a := NewHtlcAttemptInfo(
+		44, priv, testRoute, time.Unix(100, 0), &hash,
+	)
+
 	return c, a
 }
 
@@ -123,9 +120,11 @@ func TestSentPaymentSerialization(t *testing.T) {
 	newWireInfo.Route = route.Route{}
 	s.Route = route.Route{}
 
+	// Call session key method to set our cached session key so we can use
+	// DeepEqual, and assert that our key equals the original key.
+	require.Equal(t, s.cachedSessionKey, newWireInfo.SessionKey())
+
 	if !reflect.DeepEqual(s, newWireInfo) {
-		s.SessionKey.Curve = nil
-		newWireInfo.SessionKey.Curve = nil
 		t.Fatalf("Payments do not match after "+
 			"serialization/deserialization %v vs %v",
 			spew.Sdump(s), spew.Sdump(newWireInfo),
diff --git a/routing/control_tower_test.go b/routing/control_tower_test.go
index 15706eafd..8b7082db4 100644
--- a/routing/control_tower_test.go
+++ b/routing/control_tower_test.go
@@ -331,11 +331,9 @@ func genInfo() (*channeldb.PaymentCreationInfo, *channeldb.HTLCAttemptInfo,
 			CreationTime:      time.Unix(time.Now().Unix(), 0),
 			PaymentRequest:    []byte("hola"),
 		},
-		&channeldb.HTLCAttemptInfo{
-			AttemptID:  1,
-			SessionKey: priv,
-			Route:      testRoute,
-		}, preimage, nil
+		channeldb.NewHtlcAttemptInfo(
+			1, priv, testRoute, time.Time{}, nil,
+		), preimage, nil
 }
 
 func genPreimage() ([32]byte, error) {