diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 8d5bba990..4d54c1d14 100644 --- a/.github/workflows/main.yml +++ b/.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 ######################################### diff --git a/docs/release-notes/release-notes-0.21.0.md b/docs/release-notes/release-notes-0.21.0.md index 5c456d2c4..d6ecb3703 100644 --- a/docs/release-notes/release-notes-0.21.0.md +++ b/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** diff --git a/invoices/invoiceregistry_test.go b/invoices/invoiceregistry_test.go index 5e13f8735..785da115e 100644 --- a/invoices/invoiceregistry_test.go +++ b/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") } diff --git a/invoices/sql_store.go b/invoices/sql_store.go index a31d354fc..0628a5d54 100644 --- a/invoices/sql_store.go +++ b/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 - // by the payment hash which is faster. - if ref.IsHashOnly() { + // If the reference contains a payment hash we can look up the invoice + // directly by hash using the unique index, avoiding a full table scan. + // 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 - // assemble the query parameters based on the fields that are set. - var params sqlc.GetInvoiceParams + // If the reference contains a payment address (AMP payments), look up + // directly by payment address using the unique index. + // + // 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 { - params.Hash = ref.PayHash()[:] + return invoice, nil } - // Newer invoices (0.11 and up) are indexed by payment address in - // 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 only the set ID is given, look up via the AMP sub-invoice index. 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 ( - 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 + return sqlc.Invoice{}, ErrInvoiceNotFound } // fetchInvoice fetches the common invoice data and the AMP state for the diff --git a/scripts/bw-compatibility-test/docker-compose.override.yaml b/scripts/bw-compatibility-test/docker-compose.override.yaml index 51ca758b9..59dc9830f 100644 --- a/scripts/bw-compatibility-test/docker-compose.override.yaml +++ b/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 diff --git a/scripts/bw-compatibility-test/docker-compose.yaml b/scripts/bw-compatibility-test/docker-compose.yaml index 7860b4415..ea901e699 100644 --- a/scripts/bw-compatibility-test/docker-compose.yaml +++ b/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" diff --git a/scripts/bw-compatibility-test/network.sh b/scripts/bw-compatibility-test/network.sh index 98eca675b..c712381f8 100644 --- a/scripts/bw-compatibility-test/network.sh +++ b/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() { diff --git a/scripts/bw-compatibility-test/test.sh b/scripts/bw-compatibility-test/test.sh index d8bb6660e..7b9f2f7bf 100755 --- a/scripts/bw-compatibility-test/test.sh +++ b/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 -trap compose_down EXIT +# regardless of success. Logs are collected first so they are +# available for CI artifact upload after the cluster is torn down. +trap 'collect_logs; compose_down' EXIT # Set up the network. setup_network diff --git a/sqldb/migrations_dev.go b/sqldb/migrations_dev.go index 7b02b151f..dca536531 100644 --- a/sqldb/migrations_dev.go +++ b/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, + }, } diff --git a/sqldb/sqlc/invoices.sql.go b/sqldb/sqlc/invoices.sql.go index 911c4e1d1..9743f2b64 100644 --- a/sqldb/sqlc/invoices.sql.go +++ b/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 -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 +WHERE i.payment_addr = $1 ` -type GetInvoiceParams struct { - SetID []byte - AddIndex sql.NullInt64 - Hash []byte - PaymentAddr []byte -} - -// 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. -func (q *Queries) GetInvoice(ctx context.Context, arg GetInvoiceParams) ([]Invoice, error) { - rows, err := q.db.QueryContext(ctx, getInvoice, - arg.SetID, - arg.AddIndex, - arg.Hash, - arg.PaymentAddr, +func (q *Queries) GetInvoiceByAddr(ctx context.Context, paymentAddr []byte) (Invoice, error) { + row := q.db.QueryRowContext(ctx, getInvoiceByAddr, paymentAddr) + var i Invoice + err := row.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, ) - if err != nil { - 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 + return i, err } 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 { diff --git a/sqldb/sqlc/migrations/000012_drop_redundant_invoice_indexes.down.sql b/sqldb/sqlc/migrations/000012_drop_redundant_invoice_indexes.down.sql new file mode 100644 index 000000000..92e8599e0 --- /dev/null +++ b/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); diff --git a/sqldb/sqlc/migrations/000012_drop_redundant_invoice_indexes.up.sql b/sqldb/sqlc/migrations/000012_drop_redundant_invoice_indexes.up.sql new file mode 100644 index 000000000..fa6a0e2f7 --- /dev/null +++ b/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; diff --git a/sqldb/sqlc/querier.go b/sqldb/sqlc/querier.go index 947237300..c2d9c8187 100644 --- a/sqldb/sqlc/querier.go +++ b/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 - // 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) + GetInvoiceByAddr(ctx context.Context, paymentAddr []byte) (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) diff --git a/sqldb/sqlc/queries/invoices.sql b/sqldb/sqlc/queries/invoices.sql index f9b92388d..1ec96acf1 100644 --- a/sqldb/sqlc/queries/invoices.sql +++ b/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