Merge pull request #9972 from ellemouton/graphSQL17-complete-unit-tests

[17] multi: run all graph unit tests against SQL backends & run itest suite against SQL graph backend

Author: guggero

.github/workflows/main.yml

@@ -368,10 +368,14 @@ jobs:
             args: backend=bitcoind dbbackend=sqlite
           - name: bitcoind-sqlite-nativesql
             args: backend=bitcoind dbbackend=sqlite nativesql=true
+          - name: bitcoind-sqlite=nativesql-experiment
+            args: backend=bitcoind dbbackend=sqlite nativesql=true tags=test_native_sql
           - name: bitcoind-postgres
             args: backend=bitcoind dbbackend=postgres
           - name: bitcoind-postgres-nativesql
             args: backend=bitcoind dbbackend=postgres nativesql=true
+          - name: bitcoind-postgres-nativesql-experiment
+            args: backend=bitcoind dbbackend=postgres nativesql=true tags=test_native_sql
     steps:
       - name: git checkout
         uses: actions/checkout@v4

config_builder.go

@@ -1046,23 +1046,6 @@ func (d *DefaultDatabaseBuilder) BuildDatabase(
 		)
 	}
 
-	graphStore, err := graphdb.NewKVStore(
-		databaseBackends.GraphDB, graphDBOptions...,
-	)
-	if err != nil {
-		return nil, nil, err
-	}
-
-	dbs.GraphDB, err = graphdb.NewChannelGraph(graphStore, chanGraphOpts...)
-	if err != nil {
-		cleanUp()
-
-		err = fmt.Errorf("unable to open graph DB: %w", err)
-		d.logger.Error(err)
-
-		return nil, nil, err
-	}
-
 	dbOptions := []channeldb.OptionModifier{
 		channeldb.OptionDryRunMigration(cfg.DryRunMigration),
 		channeldb.OptionKeepFailedPaymentAttempts(
@@ -1098,6 +1081,10 @@ func (d *DefaultDatabaseBuilder) BuildDatabase(
 		return nil, nil, err
 	}
 
+	// The graph store implementation we will use depends on whether
+	// native SQL is enabled or not.
+	var graphStore graphdb.V1Store
+
 	// Instantiate a native SQL store if the flag is set.
 	if d.cfg.DB.UseNativeSQL {
 		migrations := sqldb.GetMigrations()
@@ -1156,17 +1143,27 @@ func (d *DefaultDatabaseBuilder) BuildDatabase(
 		// the base DB and transaction executor for the native SQL
 		// invoice store.
 		baseDB := dbs.NativeSQLStore.GetBaseDB()
-		executor := sqldb.NewTransactionExecutor(
+		invoiceExecutor := sqldb.NewTransactionExecutor(
 			baseDB, func(tx *sql.Tx) invoices.SQLInvoiceQueries {
 				return baseDB.WithTx(tx)
 			},
 		)
 
 		sqlInvoiceDB := invoices.NewSQLStore(
-			executor, clock.NewDefaultClock(),
+			invoiceExecutor, clock.NewDefaultClock(),
 		)
 
 		dbs.InvoiceDB = sqlInvoiceDB
+
+		graphStore, err = d.getGraphStore(
+			baseDB, databaseBackends.GraphDB, graphDBOptions...,
+		)
+		if err != nil {
+			err = fmt.Errorf("unable to get graph store: %w", err)
+			d.logger.Error(err)
+
+			return nil, nil, err
+		}
 	} else {
 		// Check if the invoice bucket tombstone is set. If it is, we
 		// need to return and ask the user switch back to using the
@@ -1188,6 +1185,23 @@ func (d *DefaultDatabaseBuilder) BuildDatabase(
 		}
 
 		dbs.InvoiceDB = dbs.ChanStateDB
+
+		graphStore, err = graphdb.NewKVStore(
+			databaseBackends.GraphDB, graphDBOptions...,
+		)
+		if err != nil {
+			return nil, nil, err
+		}
+	}
+
+	dbs.GraphDB, err = graphdb.NewChannelGraph(graphStore, chanGraphOpts...)
+	if err != nil {
+		cleanUp()
+
+		err = fmt.Errorf("unable to open channel graph DB: %w", err)
+		d.logger.Error(err)
+
+		return nil, nil, err
 	}
 
 	// Wrap the watchtower client DB and make sure we clean up.

config_prod.go

@@ -0,0 +1,18 @@
+//go:build !test_native_sql
+
+package lnd
+
+import (
+	graphdb "github.com/lightningnetwork/lnd/graph/db"
+	"github.com/lightningnetwork/lnd/kvdb"
+	"github.com/lightningnetwork/lnd/sqldb"
+)
+
+// getGraphStore returns a graphdb.V1Store backed by a graphdb.KVStore
+// implementation.
+func (d *DefaultDatabaseBuilder) getGraphStore(_ *sqldb.BaseDB,
+	kvBackend kvdb.Backend,
+	opts ...graphdb.StoreOptionModifier) (graphdb.V1Store, error) {
+
+	return graphdb.NewKVStore(kvBackend, opts...)
+}

config_test_native_sql.go

@@ -0,0 +1,31 @@
+//go:build test_native_sql
+
+package lnd
+
+import (
+	"database/sql"
+
+	graphdb "github.com/lightningnetwork/lnd/graph/db"
+	"github.com/lightningnetwork/lnd/kvdb"
+	"github.com/lightningnetwork/lnd/sqldb"
+)
+
+// getGraphStore returns a graphdb.V1Store backed by a graphdb.SQLStore
+// implementation.
+func (d *DefaultDatabaseBuilder) getGraphStore(baseDB *sqldb.BaseDB,
+	_ kvdb.Backend, opts ...graphdb.StoreOptionModifier) (graphdb.V1Store,
+	error) {
+
+	graphExecutor := sqldb.NewTransactionExecutor(
+		baseDB, func(tx *sql.Tx) graphdb.SQLQueries {
+			return baseDB.WithTx(tx)
+		},
+	)
+
+	return graphdb.NewSQLStore(
+		&graphdb.SQLStoreConfig{
+			ChainHash: *d.cfg.ActiveNetParams.GenesisHash,
+		},
+		graphExecutor, opts...,
+	)
+}

discovery/bootstrapper.go

@@ -236,7 +236,7 @@ func (c *ChannelGraphBootstrapper) SampleNodeAddrs(_ context.Context,
 
 			return errFound
 		})
-		if err != nil && err != errFound {
+		if err != nil && !errors.Is(err, errFound) {
 			return nil, err
 		}
 

docs/release-notes/release-notes-0.20.0.md

@@ -91,6 +91,7 @@ circuit. The indices are only available for forwarding events saved after v0.20.
     * [8](https://github.com/lightningnetwork/lnd/pull/9938)
     * [9](https://github.com/lightningnetwork/lnd/pull/9939)
     * [10](https://github.com/lightningnetwork/lnd/pull/9971)
+    * [11](https://github.com/lightningnetwork/lnd/pull/9972)
 
 ## RPC Updates
 

graph/builder_test.go

@@ -892,15 +892,27 @@ func TestPruneChannelGraphDoubleDisabled(t *testing.T) {
 }
 
 func testPruneChannelGraphDoubleDisabled(t *testing.T, assumeValid bool) {
+	timestamp := time.Now()
+
+	// nextTimeStamp is a helper closure that will return a new
+	// timestamp each time it's called, this helps us create channel updates
+	// with new timestamps so that we don't run into our SQL DB constraint
+	// which only allows an update to a channel edge if the last update
+	// timestamp is greater than the previous one.
+	nextTimeStamp := func() time.Time {
+		timestamp = timestamp.Add(time.Second)
+
+		return timestamp
+	}
+
 	// We'll create the following test graph so that only the last channel
 	// is pruned. We'll use a fresh timestamp to ensure they're not pruned
 	// according to that heuristic.
-	timestamp := time.Now()
 	testChannels := []*testChannel{
 		// Channel from self shouldn't be pruned.
 		symmetricTestChannel(
 			"self", "a", 100000, &testChannelPolicy{
-				LastUpdate: timestamp,
+				LastUpdate: nextTimeStamp(),
 				Disabled:   true,
 			}, 99,
 		),
@@ -918,7 +930,7 @@ func testPruneChannelGraphDoubleDisabled(t *testing.T, assumeValid bool) {
 			Node1: &testChannelEnd{
 				Alias: "a",
 				testChannelPolicy: &testChannelPolicy{
-					LastUpdate: timestamp,
+					LastUpdate: nextTimeStamp(),
 					Disabled:   true,
 				},
 			},
@@ -932,7 +944,7 @@ func testPruneChannelGraphDoubleDisabled(t *testing.T, assumeValid bool) {
 			Node1: &testChannelEnd{
 				Alias: "a",
 				testChannelPolicy: &testChannelPolicy{
-					LastUpdate: timestamp,
+					LastUpdate: nextTimeStamp(),
 					Disabled:   false,
 				},
 			},
@@ -946,14 +958,14 @@ func testPruneChannelGraphDoubleDisabled(t *testing.T, assumeValid bool) {
 			Node1: &testChannelEnd{
 				Alias: "a",
 				testChannelPolicy: &testChannelPolicy{
-					LastUpdate: timestamp,
+					LastUpdate: nextTimeStamp(),
 					Disabled:   true,
 				},
 			},
 			Node2: &testChannelEnd{
 				Alias: "b",
 				testChannelPolicy: &testChannelPolicy{
-					LastUpdate: timestamp,
+					LastUpdate: nextTimeStamp(),
 					Disabled:   false,
 				},
 			},
@@ -963,13 +975,13 @@ func testPruneChannelGraphDoubleDisabled(t *testing.T, assumeValid bool) {
 
 		// Both edges enabled.
 		symmetricTestChannel("c", "d", 100000, &testChannelPolicy{
-			LastUpdate: timestamp,
+			LastUpdate: nextTimeStamp(),
 			Disabled:   false,
 		}, 2),
 
 		// Both edges disabled, only one pruned.
 		symmetricTestChannel("e", "f", 100000, &testChannelPolicy{
-			LastUpdate: timestamp,
+			LastUpdate: nextTimeStamp(),
 			Disabled:   true,
 		}, 3),
 	}
@@ -1363,7 +1375,9 @@ func parseTestGraph(t *testing.T, useCache bool, path string) (
 	testAddrs = append(testAddrs, testAddr)
 
 	// Next, create a temporary graph database for usage within the test.
-	graph := graphdb.MakeTestGraph(t, graphdb.WithUseGraphCache(useCache))
+	graph := graphdb.MakeTestGraph(
+		t, graphdb.WithUseGraphCache(useCache),
+	)
 
 	aliasMap := make(map[string]route.Vertex)
 	privKeyMap := make(map[string]*btcec.PrivateKey)
@@ -1441,6 +1455,13 @@ func parseTestGraph(t *testing.T, useCache bool, path string) (
 			}
 
 			source = dbNode
+
+			// Set the selected source node.
+			if err := graph.SetSourceNode(ctx, source); err != nil {
+				return nil, err
+			}
+
+			continue
 		}
 
 		// With the node fully parsed, add it as a vertex within the
@@ -1450,13 +1471,6 @@ func parseTestGraph(t *testing.T, useCache bool, path string) (
 		}
 	}
 
-	if source != nil {
-		// Set the selected source node
-		if err := graph.SetSourceNode(ctx, source); err != nil {
-			return nil, err
-		}
-	}
-
 	// With all the vertexes inserted, we can now insert the edges into the
 	// test graph.
 	for _, edge := range g.Edges {
@@ -1739,14 +1753,16 @@ func createTestGraphFromChannels(t *testing.T, useCache bool,
 	testAddrs = append(testAddrs, testAddr)
 
 	// Next, create a temporary graph database for usage within the test.
-	graph := graphdb.MakeTestGraph(t, graphdb.WithUseGraphCache(useCache))
+	graph := graphdb.MakeTestGraph(
+		t, graphdb.WithUseGraphCache(useCache),
+	)
 
 	aliasMap := make(map[string]route.Vertex)
 	privKeyMap := make(map[string]*btcec.PrivateKey)
 
 	nodeIndex := byte(0)
-	addNodeWithAlias := func(alias string, features *lnwire.FeatureVector) (
-		*models.LightningNode, error) {
+	addNodeWithAlias := func(alias string,
+		features *lnwire.FeatureVector) error {
 
 		keyBytes := []byte{
 			0, 0, 0, 0, 0, 0, 0, 0,
@@ -1776,26 +1792,26 @@ func createTestGraphFromChannels(t *testing.T, useCache bool,
 
 		// With the node fully parsed, add it as a vertex within the
 		// graph.
-		if err := graph.AddLightningNode(ctx, dbNode); err != nil {
-			return nil, err
+		if alias == source {
+			err = graph.SetSourceNode(ctx, dbNode)
+			require.NoError(t, err)
+		} else {
+			err := graph.AddLightningNode(ctx, dbNode)
+			require.NoError(t, err)
 		}
 
 		aliasMap[alias] = dbNode.PubKeyBytes
 		nodeIndex++
 
-		return dbNode, nil
+		return nil
 	}
 
 	// Add the source node.
-	dbNode, err := addNodeWithAlias(source, lnwire.EmptyFeatureVector())
+	err = addNodeWithAlias(source, lnwire.EmptyFeatureVector())
 	if err != nil {
 		return nil, err
 	}
 
-	if err = graph.SetSourceNode(ctx, dbNode); err != nil {
-		return nil, err
-	}
-
 	// Initialize variable that keeps track of the next channel id to assign
 	// if none is specified.
 	nextUnassignedChannelID := uint64(100000)
@@ -1813,7 +1829,7 @@ func createTestGraphFromChannels(t *testing.T, useCache bool,
 					features =
 						node.testChannelPolicy.Features
 				}
-				_, err := addNodeWithAlias(
+				err := addNodeWithAlias(
 					node.Alias, features,
 				)
 				if err != nil {

graph/db/errors.go

@@ -66,6 +66,11 @@ var (
 	// ErrUnknownAddressType is returned when a node's addressType is not
 	// an expected value.
 	ErrUnknownAddressType = fmt.Errorf("address type cannot be resolved")
+
+	// ErrCantCheckIfZombieEdgeStr is an error returned when we
+	// attempt to check if an edge is a zombie but encounter an error.
+	ErrCantCheckIfZombieEdgeStr = fmt.Errorf("unable to check if edge " +
+		"is a zombie")
 )
 
 // ErrTooManyExtraOpaqueBytes creates an error which should be returned if the

graph/db/graph.go

@@ -594,7 +594,7 @@ func (c *ChannelGraph) UpdateEdgePolicy(ctx context.Context,
 	return nil
 }
 
-// MakeTestGraphNew creates a new instance of the ChannelGraph for testing
+// MakeTestGraph creates a new instance of the ChannelGraph for testing
 // purposes. The backing V1Store implementation depends on the version of
 // NewTestDB included in the current build.
 //
@@ -603,7 +603,7 @@ func (c *ChannelGraph) UpdateEdgePolicy(ctx context.Context,
 // implemented, unit tests will be switched to use this function instead of
 // the existing MakeTestGraph helper. Once only this function is used, the
 // existing MakeTestGraph function will be removed and this one will be renamed.
-func MakeTestGraphNew(t testing.TB,
+func MakeTestGraph(t testing.TB,
 	opts ...ChanGraphOption) *ChannelGraph {
 
 	t.Helper()