Merge pull request #10619 from ziggie1984/increase-settling-speed

invoices/sql: fix full table scans on HTLC settlement hot path

.github/workflows/main.yml

@@ -602,6 +602,14 @@ jobs:
       - name: 🛡️ Backwards compatibility test
         run: make backwards-compat-test
 
+      - name: 📋 Upload node logs on failure
+        if: failure()
+        uses: actions/upload-artifact@v4
+        with:
+          name: bw-compat-logs
+          path: scripts/bw-compatibility-test/logs/
+          retention-days: 7
+
   #########################################
   # Auto Cache Cleanup on Pull Requests
   #########################################

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

@@ -149,6 +149,13 @@
   only the parameters it actually needs and uses a direct `ORDER BY`, allowing
   the planner to perform efficient index range scans on the invoice table.
 
+*  [Fix full table scans on the HTLC settlement 
+    hot path](https://github.com/lightningnetwork/lnd/pull/10619).
+    Replace the catch-all `GetInvoice` query (which used `OR $1 IS NULL`
+    predicates that forced full table scans) with three dedicated queries
+    targeting uniquely-constrained columns. Also drop four redundant indexes 
+    that duplicated UNIQUE constraints or were never used as query filters.
+
 ## Deprecations
 
 ### ⚠️ **Warning:** The deprecated fee rate option `--sat_per_byte` will be removed in release version **0.22**

invoices/invoiceregistry_test.go

@@ -2322,7 +2322,7 @@ func testFailPartialAMPPayment(t *testing.T,
 			"expected MPPTimeout, got: %v", failRes.Outcome,
 		)
 
-	case <-time.After(testTimeout):
+	case <-time.After(testTimeoutLong):
 		t.Fatal("timeout waiting for HTLC resolution")
 	}
 
@@ -2339,7 +2339,7 @@ func testFailPartialAMPPayment(t *testing.T,
 			"expected MPPTimeout, got: %v", failRes.Outcome,
 		)
 
-	case <-time.After(testTimeout):
+	case <-time.After(testTimeoutLong):
 		t.Fatal("timeout waiting for HTLC resolution")
 	}
 
@@ -2408,7 +2408,7 @@ func testFailPartialAMPPayment(t *testing.T,
 			"expected MPPTimeout, got: %v", failRes.Outcome,
 		)
 
-	case <-time.After(testTimeout):
+	case <-time.After(testTimeoutLong):
 		t.Fatal("timeout waiting for HTLC resolution")
 	}
 

invoices/sql_store.go

@@ -1,6 +1,7 @@
 package invoices
 
 import (
+	"bytes"
 	"context"
 	"crypto/sha256"
 	"database/sql"
@@ -13,7 +14,6 @@ import (
 	"github.com/lightningnetwork/lnd/clock"
 	"github.com/lightningnetwork/lnd/graph/db/models"
 	"github.com/lightningnetwork/lnd/lntypes"
-	"github.com/lightningnetwork/lnd/lnutils"
 	"github.com/lightningnetwork/lnd/lnwire"
 	"github.com/lightningnetwork/lnd/record"
 	"github.com/lightningnetwork/lnd/sqldb"
@@ -100,12 +100,12 @@ type SQLInvoiceQueries interface { //nolint:interfacebloat
 	FilterInvoicesReverse(ctx context.Context,
 		arg sqlc.FilterInvoicesReverseParams) ([]sqlc.Invoice, error)
 
-	GetInvoice(ctx context.Context,
-		arg sqlc.GetInvoiceParams) ([]sqlc.Invoice, error)
-
 	GetInvoiceByHash(ctx context.Context, hash []byte) (sqlc.Invoice,
 		error)
 
+	GetInvoiceByAddr(ctx context.Context,
+		paymentAddr []byte) (sqlc.Invoice, error)
+
 	GetInvoiceBySetID(ctx context.Context, setID []byte) ([]sqlc.Invoice,
 		error)
 
@@ -398,73 +398,76 @@ func getInvoiceByRef(ctx context.Context,
 		return sqlc.Invoice{}, ErrInvoiceNotFound
 	}
 
-	// If the reference is a hash only, we can look up the invoice directly
+	// If the reference contains a payment hash we can look up the invoice
-	// by the payment hash which is faster.
+	// directly by hash using the unique index, avoiding a full table scan.
-	if ref.IsHashOnly() {
+	// The hash alone uniquely identifies any invoice so additional fields
+	// in the ref (payment address, set ID) are not needed for the lookup.
+	if ref.PayHash() != nil {
 		invoice, err := db.GetInvoiceByHash(ctx, ref.PayHash()[:])
 		if errors.Is(err, sql.ErrNoRows) {
 			return sqlc.Invoice{}, ErrInvoiceNotFound
 		}
+		if err != nil {
+			return sqlc.Invoice{}, fmt.Errorf("unable to fetch "+
+				"invoice by hash: %w", err)
+		}
 
-		return invoice, err
+		// If the ref also specifies a payment address, verify it
+		// matches the invoice found by hash. A mismatch means the ref
+		// is equivocating — the hash points to one invoice and the
+		// address points to another.
+		payAddr := ref.PayAddr()
+		if payAddr != nil && *payAddr != BlankPayAddr {
+			if !bytes.Equal(invoice.PaymentAddr, payAddr[:]) {
+				return sqlc.Invoice{}, ErrInvRefEquivocation
+			}
+		}
+
+		return invoice, nil
 	}
 
-	// Otherwise the reference may include more fields, so we'll need to
+	// If the reference contains a payment address (AMP payments), look up
-	// assemble the query parameters based on the fields that are set.
+	// directly by payment address using the unique index.
-	var params sqlc.GetInvoiceParams
+	//
+	// NOTE: Pre-0.8 invoices do not have a payment address, and blank
+	// payment addresses are a special case for legacy keysend invoices.
+	// Those are handled by the hash fast path above.
+	payAddr := ref.PayAddr()
+	if payAddr != nil && *payAddr != BlankPayAddr {
+		invoice, err := db.GetInvoiceByAddr(ctx, payAddr[:])
+		if errors.Is(err, sql.ErrNoRows) {
+			return sqlc.Invoice{}, ErrInvoiceNotFound
+		}
+		if err != nil {
+			return sqlc.Invoice{}, fmt.Errorf("unable to fetch "+
+				"invoice by payment address: %w", err)
+		}
 
-	if ref.PayHash() != nil {
+		return invoice, nil
-		params.Hash = ref.PayHash()[:]
 	}
 
-	// Newer invoices (0.11 and up) are indexed by payment address in
+	// If only the set ID is given, look up via the AMP sub-invoice index.
-	// addition to payment hash, but pre 0.8 invoices do not have one at
-	// all. Only allow lookups for payment address if it is not a blank
-	// payment address, which is a special-cased value for legacy keysend
-	// invoices.
-	if ref.PayAddr() != nil && *ref.PayAddr() != BlankPayAddr {
-		params.PaymentAddr = ref.PayAddr()[:]
-	}
-
-	// If the reference has a set ID we'll fetch the invoice which has the
-	// corresponding AMP sub invoice.
 	if ref.SetID() != nil {
-		params.SetID = ref.SetID()[:]
+		rows, err := db.GetInvoiceBySetID(ctx, ref.SetID()[:])
+		if err != nil {
+			return sqlc.Invoice{}, fmt.Errorf("unable to fetch "+
+				"invoice: %w", err)
+		}
+
+		if len(rows) == 0 {
+			return sqlc.Invoice{}, ErrInvoiceNotFound
+		}
+
+		if len(rows) > 1 {
+			return sqlc.Invoice{}, fmt.Errorf("ambiguous "+
+				"invoice ref: set_id=%x matches %d invoices",
+				ref.SetID(), len(rows))
+		}
+
+		return rows[0], nil
 	}
 
-	var (
+	return sqlc.Invoice{}, ErrInvoiceNotFound
-		rows []sqlc.Invoice
-		err  error
-	)
-
-	// We need to split the query based on how we intend to look up the
-	// invoice. If only the set ID is given then we want to have an exact
-	// match on the set ID. If other fields are given, we want to match on
-	// those fields and the set ID but with a less strict join condition.
-	if params.Hash == nil && params.PaymentAddr == nil &&
-		params.SetID != nil {
-
-		rows, err = db.GetInvoiceBySetID(ctx, params.SetID)
-	} else {
-		rows, err = db.GetInvoice(ctx, params)
-	}
-
-	switch {
-	case len(rows) == 0:
-		return sqlc.Invoice{}, ErrInvoiceNotFound
-
-	case len(rows) > 1:
-		// In case the reference is ambiguous, meaning it matches more
-		// than	one invoice, we'll return an error.
-		return sqlc.Invoice{}, fmt.Errorf("ambiguous invoice ref: "+
-			"%s: %s", ref.String(), lnutils.SpewLogClosure(rows))
-
-	case err != nil:
-		return sqlc.Invoice{}, fmt.Errorf("unable to fetch invoice: %w",
-			err)
-	}
-
-	return rows[0], nil
 }
 
 // fetchInvoice fetches the common invoice data and the AMP state for the

scripts/bw-compatibility-test/docker-compose.override.yaml

@@ -42,6 +42,7 @@ services:
       --protocol.zero-conf
       --protocol.simple-taproot-chans
       --trickledelay=50
+      --historicalsyncinterval=10s
 
   dave-pr:
     image: lnd-dev:backward-compat-test-build
@@ -83,5 +84,6 @@ services:
       --protocol.zero-conf
       --protocol.simple-taproot-chans
       --trickledelay=50
+      --historicalsyncinterval=10s
       --db.backend=sqlite
       --db.use-native-sql

scripts/bw-compatibility-test/docker-compose.yaml

@@ -71,6 +71,7 @@ services:
       - "--protocol.zero-conf"
       - "--protocol.simple-taproot-chans"
       - "--trickledelay=50"
+      - "--historicalsyncinterval=10s"
 
   bob:
     image: lightninglabs/lnd:${LND_LATEST_VERSION}
@@ -111,6 +112,7 @@ services:
       - "--protocol.zero-conf"
       - "--protocol.simple-taproot-chans"
       - "--trickledelay=50"
+      - "--historicalsyncinterval=10s"
 
   charlie:
     image: lightninglabs/lnd:${LND_LATEST_VERSION}
@@ -148,6 +150,7 @@ services:
       - "--tlsextradomain=charlie"
       - "--accept-keysend"
       - "--trickledelay=50"
+      - "--historicalsyncinterval=10s"
 
   dave:
     image: lightninglabs/lnd:${LND_LATEST_VERSION}
@@ -185,6 +188,7 @@ services:
       - "--tlsextradomain=dave"
       - "--accept-keysend"
       - "--trickledelay=50"
+      - "--historicalsyncinterval=10s"
       - "--db.backend=sqlite"
       - "--db.use-native-sql"
 

scripts/bw-compatibility-test/network.sh

@@ -298,6 +298,24 @@ wait_for_active_chans() {
   echo "🟢 $node now has exactly $expected_channels active channels!"
 }
 
+# collect_logs copies the lnd log file from each running container into a
+# local ./logs directory. Call this before compose_down so logs are available
+# for CI artifact upload even after the cluster is torn down.
+function collect_logs() {
+  local log_dir="$DIR/logs"
+  mkdir -p "$log_dir"
+
+  for node in alice bob charlie dave bob-pr dave-pr; do
+    if docker ps -a --format '{{.Names}}' | grep -q "^${node}$"; then
+      mkdir -p "$log_dir/$node"
+      docker cp "$node:/root/.lnd/logs/bitcoin/regtest/lnd.log" \
+        "$log_dir/$node/lnd.log" 2>/dev/null || true
+    fi
+  done
+
+  echo "📋 Logs collected in $log_dir"
+}
+
 # mine mines a number of blocks on the regtest network. If no
 # argument is provided, it defaults to 6 blocks.
 function mine() {

scripts/bw-compatibility-test/test.sh

@@ -16,8 +16,9 @@ cd $DIR
 compose_up
 
 # Ensure that the cluster is shut down when the script exits
-# regardless of success
+# regardless of success. Logs are collected first so they are
-trap compose_down EXIT
+# available for CI artifact upload after the cluster is torn down.
+trap 'collect_logs; compose_down' EXIT
 
 # Set up the network.
 setup_network

sqldb/migrations_dev.go

@@ -22,4 +22,9 @@ var migrationAdditions = []MigrationConfig{
 		// schema. This is optional and can be disabled by the
 		// user if necessary.
 	},
+	{
+		Name:          "000012_drop_redundant_invoice_indexes",
+		Version:       15,
+		SchemaVersion: 12,
+	},
 }

sqldb/sqlc/invoices.sql.go

@@ -396,84 +396,36 @@ func (q *Queries) FilterInvoicesReverse(ctx context.Context, arg FilterInvoicesR
 	return items, nil
 }
 
-const getInvoice = `-- name: GetInvoice :many
+const getInvoiceByAddr = `-- name: GetInvoiceByAddr :one
-
 SELECT i.id, i.hash, i.preimage, i.settle_index, i.settled_at, i.memo, i.amount_msat, i.cltv_delta, i.expiry, i.payment_addr, i.payment_request, i.payment_request_hash, i.state, i.amount_paid_msat, i.is_amp, i.is_hodl, i.is_keysend, i.created_at
 FROM invoices i
-LEFT JOIN amp_sub_invoices a 
+WHERE i.payment_addr = $1
-ON i.id = a.invoice_id
-AND (
-    a.set_id = $1 OR $1 IS NULL
-)
-WHERE (
-    i.id = $2 OR 
-    $2 IS NULL
-) AND (
-    i.hash = $3 OR 
-    $3 IS NULL
-) AND (
-    i.payment_addr = $4 OR 
-    $4 IS NULL
-)
-GROUP BY i.id
-LIMIT 2
 `
 
-type GetInvoiceParams struct {
+func (q *Queries) GetInvoiceByAddr(ctx context.Context, paymentAddr []byte) (Invoice, error) {
-	SetID       []byte
+	row := q.db.QueryRowContext(ctx, getInvoiceByAddr, paymentAddr)
-	AddIndex    sql.NullInt64
+	var i Invoice
-	Hash        []byte
+	err := row.Scan(
-	PaymentAddr []byte
+		&i.ID,
-}
+		&i.Hash,
-
+		&i.Preimage,
-// This method may return more than one invoice if filter using multiple fields
+		&i.SettleIndex,
-// from different invoices. It is the caller's responsibility to ensure that
+		&i.SettledAt,
-// we bubble up an error in those cases.
+		&i.Memo,
-func (q *Queries) GetInvoice(ctx context.Context, arg GetInvoiceParams) ([]Invoice, error) {
+		&i.AmountMsat,
-	rows, err := q.db.QueryContext(ctx, getInvoice,
+		&i.CltvDelta,
-		arg.SetID,
+		&i.Expiry,
-		arg.AddIndex,
+		&i.PaymentAddr,
-		arg.Hash,
+		&i.PaymentRequest,
-		arg.PaymentAddr,
+		&i.PaymentRequestHash,
+		&i.State,
+		&i.AmountPaidMsat,
+		&i.IsAmp,
+		&i.IsHodl,
+		&i.IsKeysend,
+		&i.CreatedAt,
 	)
-	if err != nil {
+	return i, err
-		return nil, err
-	}
-	defer rows.Close()
-	var items []Invoice
-	for rows.Next() {
-		var i Invoice
-		if err := rows.Scan(
-			&i.ID,
-			&i.Hash,
-			&i.Preimage,
-			&i.SettleIndex,
-			&i.SettledAt,
-			&i.Memo,
-			&i.AmountMsat,
-			&i.CltvDelta,
-			&i.Expiry,
-			&i.PaymentAddr,
-			&i.PaymentRequest,
-			&i.PaymentRequestHash,
-			&i.State,
-			&i.AmountPaidMsat,
-			&i.IsAmp,
-			&i.IsHodl,
-			&i.IsKeysend,
-			&i.CreatedAt,
-		); 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 getInvoiceByHash = `-- name: GetInvoiceByHash :one
@@ -515,6 +467,8 @@ INNER JOIN amp_sub_invoices a
 ON i.id = a.invoice_id AND a.set_id = $1
 `
 
+// TODO(ziggie): This query can only return one invoice if the set_id is
+// the primary key of amp_sub_invoices table.
 func (q *Queries) GetInvoiceBySetID(ctx context.Context, setID []byte) ([]Invoice, error) {
 	rows, err := q.db.QueryContext(ctx, getInvoiceBySetID, setID)
 	if err != nil {

sqldb/sqlc/migrations/000012_drop_redundant_invoice_indexes.down.sql

@@ -0,0 +1,4 @@
+CREATE INDEX IF NOT EXISTS invoices_hash_idx ON invoices(hash);
+CREATE INDEX IF NOT EXISTS invoices_payment_addr_idx ON invoices(payment_addr);
+CREATE INDEX IF NOT EXISTS invoices_preimage_idx ON invoices(preimage);
+CREATE INDEX IF NOT EXISTS invoices_settled_at_idx ON invoices(settled_at);

sqldb/sqlc/migrations/000012_drop_redundant_invoice_indexes.up.sql

@@ -0,0 +1,16 @@
+-- invoices_hash_idx is redundant: The UNIQUE constraint on invoices(hash)
+-- already creates an implicit index.
+DROP INDEX IF EXISTS invoices_hash_idx;
+
+-- invoices_payment_addr_idx is redundant: The UNIQUE constraint on
+-- invoices(payment_addr) already creates an implicit index.
+DROP INDEX IF EXISTS invoices_payment_addr_idx;
+
+-- invoices_preimage_idx is useless: There are no queries that filter on 
+-- preimage so there is no need to index it.
+DROP INDEX IF EXISTS invoices_preimage_idx;
+
+-- invoices_settled_at_idx is useless: settled_at is NULL for all pending
+-- invoices and is never used as a filter in any query (settle_index is used
+-- instead).
+DROP INDEX IF EXISTS invoices_settled_at_idx;

sqldb/sqlc/querier.go

@@ -107,11 +107,10 @@ type Querier interface {
 	GetClosedChannelsSCIDs(ctx context.Context, scids [][]byte) ([][]byte, error)
 	GetDatabaseVersion(ctx context.Context) (int32, error)
 	GetExtraNodeTypes(ctx context.Context, nodeID int64) ([]GraphNodeExtraType, error)
-	// This method may return more than one invoice if filter using multiple fields
+	GetInvoiceByAddr(ctx context.Context, paymentAddr []byte) (Invoice, error)
-	// from different invoices. It is the caller's responsibility to ensure that
-	// we bubble up an error in those cases.
-	GetInvoice(ctx context.Context, arg GetInvoiceParams) ([]Invoice, error)
 	GetInvoiceByHash(ctx context.Context, hash []byte) (Invoice, error)
+	// TODO(ziggie): This query can only return one invoice if the set_id is
+	// the primary key of amp_sub_invoices table.
 	GetInvoiceBySetID(ctx context.Context, setID []byte) ([]Invoice, error)
 	GetInvoiceFeatures(ctx context.Context, invoiceID int64) ([]InvoiceFeature, error)
 	GetInvoiceHTLCCustomRecords(ctx context.Context, invoiceID int64) ([]GetInvoiceHTLCCustomRecordsRow, error)

sqldb/sqlc/queries/invoices.sql

@@ -29,37 +29,19 @@ SELECT *
 FROM invoice_features
 WHERE invoice_id = $1;
 
--- This method may return more than one invoice if filter using multiple fields
--- from different invoices. It is the caller's responsibility to ensure that 
--- we bubble up an error in those cases.
-
--- name: GetInvoice :many
-SELECT i.*
-FROM invoices i
-LEFT JOIN amp_sub_invoices a 
-ON i.id = a.invoice_id
-AND (
-    a.set_id = sqlc.narg('set_id') OR sqlc.narg('set_id') IS NULL
-)
-WHERE (
-    i.id = sqlc.narg('add_index') OR 
-    sqlc.narg('add_index') IS NULL
-) AND (
-    i.hash = sqlc.narg('hash') OR 
-    sqlc.narg('hash') IS NULL
-) AND (
-    i.payment_addr = sqlc.narg('payment_addr') OR 
-    sqlc.narg('payment_addr') IS NULL
-)
-GROUP BY i.id
-LIMIT 2;
-
 -- name: GetInvoiceByHash :one
 SELECT i.*
 FROM invoices i
 WHERE i.hash = $1;
 
+-- name: GetInvoiceByAddr :one
+SELECT i.*
+FROM invoices i
+WHERE i.payment_addr = $1;
+
 -- name: GetInvoiceBySetID :many
+-- TODO(ziggie): This query can only return one invoice if the set_id is 
+-- the primary key of amp_sub_invoices table.
 SELECT i.*
 FROM invoices i
 INNER JOIN amp_sub_invoices a