Merge pull request #10420 from ellemouton/removePubKeyCaching

graph: fix various races

Author: yyforyongyu

docs/release-notes/release-notes-0.21.0.md

@@ -31,6 +31,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/10420) 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
 
 - Basic Support for [onion messaging forwarding](https://github.com/lightningnetwork/lnd/pull/9868) 

graph/db/models/channel_edge_info.go

@@ -30,11 +30,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
@@ -78,42 +76,16 @@ type ChannelEdgeInfo struct {
 // 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[:])
 }
 
 // OtherNodeKeyBytes returns the node key bytes of the other end of the channel.

graph/db/models/node.go

@@ -22,7 +22,6 @@ type Node struct {
 
 	// PubKeyBytes is the raw bytes of the public key of the target node.
 	PubKeyBytes [33]byte
-	pubKey      *btcec.PublicKey
 
 	// LastUpdate is the last time the vertex information for this node has
 	// been updated.
@@ -129,21 +128,8 @@ func (n *Node) HaveAnnouncement() bool {
 
 // 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 (n *Node) PubKey() (*btcec.PublicKey, error) {
-	if n.pubKey != nil {
-		return n.pubKey, nil
-	}
-
-	key, err := btcec.ParsePubKey(n.PubKeyBytes[:])
-	if err != nil {
-		return nil, err
-	}
-	n.pubKey = key
-
-	return key, nil
+	return btcec.ParsePubKey(n.PubKeyBytes[:])
 }
 
 // NodeAnnouncement retrieves the latest node announcement of the node.

graph/db/sql_store.go

@@ -2911,10 +2911,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
 }