Merge pull request #10433 from ellemouton/backport-removePubKeyCaching

backport: graph: fix various races

Author: Roasbeef

docs/release-notes/release-notes-0.20.1.md

@@ -47,6 +47,14 @@
   or key) existed. The manager now correctly regenerates both files when either
   is missing, preventing "file not found" errors on startup.
 
+* [Fixed race conditions](https://github.com/lightningnetwork/lnd/pull/10433) in
+  the channel graph database. The `Node.PubKey()` and
+  `ChannelEdgeInfo.NodeKey1/NodeKey2()` methods had check-then-act races when
+  caching parsed public keys. Additionally, `DisconnectBlockAtHeight` was
+  accessing the reject and channel caches without proper locking. The caching
+  has been removed from the public key parsing methods, and proper mutex
+  protection has been added to the cache access in `DisconnectBlockAtHeight`.
+
 # New Features
 
 ## Functional Enhancements

graph/db/models/channel_edge_info.go

@@ -33,11 +33,9 @@ type ChannelEdgeInfo struct {
 
 	// NodeKey1Bytes is the raw public key of the first node.
 	NodeKey1Bytes [33]byte
-	nodeKey1      *btcec.PublicKey
 
 	// NodeKey2Bytes is the raw public key of the first node.
 	NodeKey2Bytes [33]byte
-	nodeKey2      *btcec.PublicKey
 
 	// BitcoinKey1Bytes is the raw public key of the first node.
 	BitcoinKey1Bytes [33]byte
@@ -84,10 +82,8 @@ type ChannelEdgeInfo struct {
 func (c *ChannelEdgeInfo) AddNodeKeys(nodeKey1, nodeKey2, bitcoinKey1,
 	bitcoinKey2 *btcec.PublicKey) {
 
-	c.nodeKey1 = nodeKey1
-	copy(c.NodeKey1Bytes[:], c.nodeKey1.SerializeCompressed())
+	copy(c.NodeKey1Bytes[:], nodeKey1.SerializeCompressed())
 
-	c.nodeKey2 = nodeKey2
 	copy(c.NodeKey2Bytes[:], nodeKey2.SerializeCompressed())
 
 	c.bitcoinKey1 = bitcoinKey1
@@ -101,42 +97,16 @@ func (c *ChannelEdgeInfo) AddNodeKeys(nodeKey1, nodeKey2, bitcoinKey1,
 // the creation of this channel. A node is considered "first" if the
 // lexicographical ordering the its serialized public key is "smaller" than
 // that of the other node involved in channel creation.
-//
-// NOTE: By having this method to access an attribute, we ensure we only need
-// to fully deserialize the pubkey if absolutely necessary.
 func (c *ChannelEdgeInfo) NodeKey1() (*btcec.PublicKey, error) {
-	if c.nodeKey1 != nil {
-		return c.nodeKey1, nil
-	}
-
-	key, err := btcec.ParsePubKey(c.NodeKey1Bytes[:])
-	if err != nil {
-		return nil, err
-	}
-	c.nodeKey1 = key
-
-	return key, nil
+	return btcec.ParsePubKey(c.NodeKey1Bytes[:])
 }
 
 // NodeKey2 is the identity public key of the "second" node that was involved in
 // the creation of this channel. A node is considered "second" if the
 // lexicographical ordering the its serialized public key is "larger" than that
 // of the other node involved in channel creation.
-//
-// NOTE: By having this method to access an attribute, we ensure we only need
-// to fully deserialize the pubkey if absolutely necessary.
 func (c *ChannelEdgeInfo) NodeKey2() (*btcec.PublicKey, error) {
-	if c.nodeKey2 != nil {
-		return c.nodeKey2, nil
-	}
-
-	key, err := btcec.ParsePubKey(c.NodeKey2Bytes[:])
-	if err != nil {
-		return nil, err
-	}
-	c.nodeKey2 = key
-
-	return key, nil
+	return btcec.ParsePubKey(c.NodeKey2Bytes[:])
 }
 
 // BitcoinKey1 is the Bitcoin multi-sig key belonging to the first node, that

graph/db/models/node.go

@@ -18,7 +18,6 @@ import (
 type Node struct {
 	// PubKeyBytes is the raw bytes of the public key of the target node.
 	PubKeyBytes [33]byte
-	pubKey      *btcec.PublicKey
 
 	// HaveNodeAnnouncement indicates whether we received a node
 	// announcement for this particular node. If true, the remaining fields
@@ -62,67 +61,53 @@ type Node struct {
 
 // PubKey is the node's long-term identity public key. This key will be used to
 // authenticated any advertisements/updates sent by the node.
-//
-// NOTE: By having this method to access an attribute, we ensure we only need
-// to fully deserialize the pubkey if absolutely necessary.
-func (l *Node) PubKey() (*btcec.PublicKey, error) {
-	if l.pubKey != nil {
-		return l.pubKey, nil
-	}
-
-	key, err := btcec.ParsePubKey(l.PubKeyBytes[:])
-	if err != nil {
-		return nil, err
-	}
-	l.pubKey = key
-
-	return key, nil
+func (n *Node) PubKey() (*btcec.PublicKey, error) {
+	return btcec.ParsePubKey(n.PubKeyBytes[:])
 }
 
 // AuthSig is a signature under the advertised public key which serves to
 // authenticate the attributes announced by this node.
 //
 // NOTE: By having this method to access an attribute, we ensure we only need
 // to fully deserialize the signature if absolutely necessary.
-func (l *Node) AuthSig() (*ecdsa.Signature, error) {
-	return ecdsa.ParseSignature(l.AuthSigBytes)
+func (n *Node) AuthSig() (*ecdsa.Signature, error) {
+	return ecdsa.ParseSignature(n.AuthSigBytes)
 }
 
 // AddPubKey is a setter-link method that can be used to swap out the public
 // key for a node.
-func (l *Node) AddPubKey(key *btcec.PublicKey) {
-	l.pubKey = key
-	copy(l.PubKeyBytes[:], key.SerializeCompressed())
+func (n *Node) AddPubKey(key *btcec.PublicKey) {
+	copy(n.PubKeyBytes[:], key.SerializeCompressed())
 }
 
 // NodeAnnouncement retrieves the latest node announcement of the node.
-func (l *Node) NodeAnnouncement(signed bool) (*lnwire.NodeAnnouncement1,
+func (n *Node) NodeAnnouncement(signed bool) (*lnwire.NodeAnnouncement1,
 	error) {
 
-	if !l.HaveNodeAnnouncement {
+	if !n.HaveNodeAnnouncement {
 		return nil, fmt.Errorf("node does not have node announcement")
 	}
 
-	alias, err := lnwire.NewNodeAlias(l.Alias)
+	alias, err := lnwire.NewNodeAlias(n.Alias)
 	if err != nil {
 		return nil, err
 	}
 
 	nodeAnn := &lnwire.NodeAnnouncement1{
-		Features:        l.Features.RawFeatureVector,
-		NodeID:          l.PubKeyBytes,
-		RGBColor:        l.Color,
+		Features:        n.Features.RawFeatureVector,
+		NodeID:          n.PubKeyBytes,
+		RGBColor:        n.Color,
 		Alias:           alias,
-		Addresses:       l.Addresses,
-		Timestamp:       uint32(l.LastUpdate.Unix()),
-		ExtraOpaqueData: l.ExtraOpaqueData,
+		Addresses:       n.Addresses,
+		Timestamp:       uint32(n.LastUpdate.Unix()),
+		ExtraOpaqueData: n.ExtraOpaqueData,
 	}
 
 	if !signed {
 		return nodeAnn, nil
 	}
 
-	sig, err := lnwire.NewSigFromECDSARawSignature(l.AuthSigBytes)
+	sig, err := lnwire.NewSigFromECDSARawSignature(n.AuthSigBytes)
 	if err != nil {
 		return nil, err
 	}

graph/db/sql_store.go

@@ -2916,10 +2916,12 @@ func (s *SQLStore) DisconnectBlockAtHeight(height uint32) (
 			"height: %w", err)
 	}
 
+	s.cacheMu.Lock()
 	for _, channel := range removedChans {
 		s.rejectCache.remove(channel.ChannelID)
 		s.chanCache.remove(channel.ChannelID)
 	}
+	s.cacheMu.Unlock()
 
 	return removedChans, nil
 }