Merge pull request #10010 from ellemouton/sqlGraphUpdates

graph/db: various misc updates

graph/db/graph_test.go

@@ -97,6 +97,7 @@ func createTestVertex(t testing.TB) *models.LightningNode {
 	return createLightningNode(priv)
 }
 
+// TestNodeInsertionAndDeletion tests the CRUD operations for a LightningNode.
 func TestNodeInsertionAndDeletion(t *testing.T) {
 	t.Parallel()
 	ctx := context.Background()
@@ -124,9 +125,7 @@ func TestNodeInsertionAndDeletion(t *testing.T) {
 	// First, insert the node into the graph DB. This should succeed
 	// without any errors.
 	node := nodeWithAddrs(testAddrs)
-	if err := graph.AddLightningNode(ctx, node); err != nil {
-		t.Fatalf("unable to add node: %v", err)
-	}
+	require.NoError(t, graph.AddLightningNode(ctx, node))
 	assertNodeInCache(t, graph, node, testFeatures)
 
 	// Next, fetch the node from the database to ensure everything was
@@ -135,11 +134,8 @@ func TestNodeInsertionAndDeletion(t *testing.T) {
 	require.NoError(t, err, "unable to locate node")
 
 	_, exists, err := graph.HasLightningNode(ctx, dbNode.PubKeyBytes)
-	if err != nil {
-		t.Fatalf("unable to query for node: %v", err)
-	} else if !exists {
-		t.Fatalf("node should be found but wasn't")
-	}
+	require.NoError(t, err)
+	require.True(t, exists)
 
 	// The two nodes should match exactly!
 	compareNodes(t, node, dbNode)
@@ -194,7 +190,7 @@ func TestNodeInsertionAndDeletion(t *testing.T) {
 	// Fetch the node and assert the empty addresses.
 	dbNode, err = graph.FetchLightningNode(ctx, testPub)
 	require.NoError(t, err)
-	require.Empty(t, dbNode.Addresses)
+	compareNodes(t, node, dbNode)
 
 	known, addrs, err = graph.AddrsForNode(ctx, pub)
 	require.NoError(t, err)

graph/db/sql_store.go

@@ -64,7 +64,7 @@ type SQLQueries interface {
 	ListNodesPaginated(ctx context.Context, arg sqlc.ListNodesPaginatedParams) ([]sqlc.Node, error)
 	ListNodeIDsAndPubKeys(ctx context.Context, arg sqlc.ListNodeIDsAndPubKeysParams) ([]sqlc.ListNodeIDsAndPubKeysRow, error)
 	IsPublicV1Node(ctx context.Context, pubKey []byte) (bool, error)
-	GetUnconnectedNodes(ctx context.Context) ([]sqlc.GetUnconnectedNodesRow, error)
+	DeleteUnconnectedNodes(ctx context.Context) ([][]byte, error)
 	DeleteNodeByPubKey(ctx context.Context, arg sqlc.DeleteNodeByPubKeyParams) (sql.Result, error)
 	DeleteNode(ctx context.Context, id int64) error
 
@@ -154,11 +154,6 @@ type BatchedSQLQueries interface {
 
 // SQLStore is an implementation of the V1Store interface that uses a SQL
 // database as the backend.
-//
-// NOTE: currently, this temporarily embeds the KVStore struct so that we can
-// implement the V1Store interface incrementally. For any method not
-// implemented,  things will fall back to the KVStore. This is ONLY the case
-// for the time being while this struct is purely used in unit tests only.
 type SQLStore struct {
 	cfg *SQLStoreConfig
 	db  BatchedSQLQueries
@@ -2568,30 +2563,21 @@ func (s *SQLStore) PruneTip() (*chainhash.Hash, uint32, error) {
 func (s *SQLStore) pruneGraphNodes(ctx context.Context,
 	db SQLQueries) ([]route.Vertex, error) {
 
-	// Fetch all un-connected nodes from the database.
-	// NOTE: this will not include any nodes that are listed in the
-	// source table.
-	nodes, err := db.GetUnconnectedNodes(ctx)
+	nodeKeys, err := db.DeleteUnconnectedNodes(ctx)
 	if err != nil {
-		return nil, fmt.Errorf("unable to fetch unconnected nodes: %w",
-			err)
+		return nil, fmt.Errorf("unable to delete unconnected "+
+			"nodes: %w", err)
 	}
 
-	prunedNodes := make([]route.Vertex, 0, len(nodes))
-	for _, node := range nodes {
-		// TODO(elle): update to use sqlc.slice() once that works.
-		if err = db.DeleteNode(ctx, node.ID); err != nil {
-			return nil, fmt.Errorf("unable to delete "+
-				"node(id=%d): %w", node.ID, err)
-		}
-
-		pubKey, err := route.NewVertexFromBytes(node.PubKey)
+	prunedNodes := make([]route.Vertex, len(nodeKeys))
+	for i, nodeKey := range nodeKeys {
+		pub, err := route.NewVertexFromBytes(nodeKey)
 		if err != nil {
 			return nil, fmt.Errorf("unable to parse pubkey "+
-				"for node(id=%d): %w", node.ID, err)
+				"from bytes: %w", err)
 		}
 
-		prunedNodes = append(prunedNodes, pubKey)
+		prunedNodes[i] = pub
 	}
 
 	return prunedNodes, nil
@@ -3638,6 +3624,12 @@ func getNodeAddresses(ctx context.Context, db SQLQueries, nodePub []byte) (bool,
 		}
 	}
 
+	// If we have no addresses, then we'll return nil instead of an
+	// empty slice.
+	if len(addresses) == 0 {
+		addresses = nil
+	}
+
 	return true, addresses, nil
 }
 

sqldb/sqlc/graph.sql.go

@@ -244,6 +244,46 @@ func (q *Queries) DeletePruneLogEntriesInRange(ctx context.Context, arg DeletePr
 	return err
 }
 
+const deleteUnconnectedNodes = `-- name: DeleteUnconnectedNodes :many
+DELETE FROM nodes
+WHERE
+    -- Ignore any of our source nodes.
+    NOT EXISTS (
+        SELECT 1
+        FROM source_nodes sn
+        WHERE sn.node_id = nodes.id
+    )
+    -- Select all nodes that do not have any channels.
+    AND NOT EXISTS (
+        SELECT 1
+        FROM channels c
+        WHERE c.node_id_1 = nodes.id OR c.node_id_2 = nodes.id
+) RETURNING pub_key
+`
+
+func (q *Queries) DeleteUnconnectedNodes(ctx context.Context) ([][]byte, error) {
+	rows, err := q.db.QueryContext(ctx, deleteUnconnectedNodes)
+	if err != nil {
+		return nil, err
+	}
+	defer rows.Close()
+	var items [][]byte
+	for rows.Next() {
+		var pub_key []byte
+		if err := rows.Scan(&pub_key); err != nil {
+			return nil, err
+		}
+		items = append(items, pub_key)
+	}
+	if err := rows.Close(); err != nil {
+		return nil, err
+	}
+	if err := rows.Err(); err != nil {
+		return nil, err
+	}
+	return items, nil
+}
+
 const deleteZombieChannel = `-- name: DeleteZombieChannel :execresult
 DELETE FROM zombie_channels
 WHERE scid = $1
@@ -1426,51 +1466,6 @@ func (q *Queries) GetSourceNodesByVersion(ctx context.Context, version int16) ([
 	return items, nil
 }
 
-const getUnconnectedNodes = `-- name: GetUnconnectedNodes :many
-SELECT n.id, n.pub_key
-FROM nodes n
-WHERE NOT EXISTS (
-    SELECT 1
-    FROM channels c
-    WHERE c.node_id_1 = n.id OR c.node_id_2 = n.id
-)
-AND NOT EXISTS (
-    SELECT 1
-    FROM source_nodes sn
-    WHERE sn.node_id = n.id
-)
-`
-
-type GetUnconnectedNodesRow struct {
-	ID     int64
-	PubKey []byte
-}
-
-// Select all nodes that do not have any channels.
-// Ignore any of our source nodes.
-func (q *Queries) GetUnconnectedNodes(ctx context.Context) ([]GetUnconnectedNodesRow, error) {
-	rows, err := q.db.QueryContext(ctx, getUnconnectedNodes)
-	if err != nil {
-		return nil, err
-	}
-	defer rows.Close()
-	var items []GetUnconnectedNodesRow
-	for rows.Next() {
-		var i GetUnconnectedNodesRow
-		if err := rows.Scan(&i.ID, &i.PubKey); err != nil {
-			return nil, err
-		}
-		items = append(items, i)
-	}
-	if err := rows.Close(); err != nil {
-		return nil, err
-	}
-	if err := rows.Err(); err != nil {
-		return nil, err
-	}
-	return items, nil
-}
-
 const getV1DisabledSCIDs = `-- name: GetV1DisabledSCIDs :many
 SELECT c.scid
 FROM channels c

sqldb/sqlc/querier.go

@@ -27,6 +27,7 @@ type Querier interface {
 	DeleteNodeByPubKey(ctx context.Context, arg DeleteNodeByPubKeyParams) (sql.Result, error)
 	DeleteNodeFeature(ctx context.Context, arg DeleteNodeFeatureParams) error
 	DeletePruneLogEntriesInRange(ctx context.Context, arg DeletePruneLogEntriesInRangeParams) error
+	DeleteUnconnectedNodes(ctx context.Context) ([][]byte, error)
 	DeleteZombieChannel(ctx context.Context, arg DeleteZombieChannelParams) (sql.Result, error)
 	FetchAMPSubInvoiceHTLCs(ctx context.Context, arg FetchAMPSubInvoiceHTLCsParams) ([]FetchAMPSubInvoiceHTLCsRow, error)
 	FetchAMPSubInvoices(ctx context.Context, arg FetchAMPSubInvoicesParams) ([]AmpSubInvoice, error)
@@ -66,9 +67,6 @@ type Querier interface {
 	GetPublicV1ChannelsBySCID(ctx context.Context, arg GetPublicV1ChannelsBySCIDParams) ([]Channel, error)
 	GetSCIDByOutpoint(ctx context.Context, arg GetSCIDByOutpointParams) ([]byte, error)
 	GetSourceNodesByVersion(ctx context.Context, version int16) ([]GetSourceNodesByVersionRow, error)
-	// Select all nodes that do not have any channels.
-	// Ignore any of our source nodes.
-	GetUnconnectedNodes(ctx context.Context) ([]GetUnconnectedNodesRow, error)
 	// NOTE: this is V1 specific since for V1, disabled is a
 	// simple, single boolean. The proposed V2 policy
 	// structure will have a more complex disabled bit vector

sqldb/sqlc/queries/graph.sql

@@ -65,21 +65,21 @@ SELECT EXISTS (
       AND n.pub_key = $1
 );
 
--- name: GetUnconnectedNodes :many
-SELECT n.id, n.pub_key
-FROM nodes n
--- Select all nodes that do not have any channels.
-WHERE NOT EXISTS (
-    SELECT 1
-    FROM channels c
-    WHERE c.node_id_1 = n.id OR c.node_id_2 = n.id
-)
--- Ignore any of our source nodes.
-AND NOT EXISTS (
-    SELECT 1
-    FROM source_nodes sn
-    WHERE sn.node_id = n.id
-);
+-- name: DeleteUnconnectedNodes :many
+DELETE FROM nodes
+WHERE
+    -- Ignore any of our source nodes.
+    NOT EXISTS (
+        SELECT 1
+        FROM source_nodes sn
+        WHERE sn.node_id = nodes.id
+    )
+    -- Select all nodes that do not have any channels.
+    AND NOT EXISTS (
+        SELECT 1
+        FROM channels c
+        WHERE c.node_id_1 = nodes.id OR c.node_id_2 = nodes.id
+) RETURNING pub_key;
 
 -- name: DeleteNodeByPubKey :execresult
 DELETE FROM nodes