Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

invoices: migrate KV invoices to native SQL for users of KV SQL backends #8831

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
02565bf
mod: temporarily replace sqldb with local version
bhandras Aug 14, 2024
0dc19ed
sqldb: add table to track custom SQL migrations
bhandras Nov 12, 2024
bdf2c68
sqldb: add support for custom in-code migrations
bhandras Nov 22, 2024
8ed2cad
sqldb: separate migration execution from construction
bhandras Nov 25, 2024
cd3be24
multi: add call to directly insert an AMP sub-invoice
bhandras Jun 11, 2024
cd28406
sqldb: set settled_at and settle_index on invocie insertion is set
bhandras Jun 11, 2024
a662470
sqldb: add a temporary index to store KV invoice hash to ID mapping
bhandras Sep 11, 2024
2ea0af2
sqldb: remove unused preimage query parameter
bhandras Dec 2, 2024
6ce0f8f
invoices: extract method to create invoice insertion params
bhandras Jun 11, 2024
f502910
invoices: add method to create payment hash index
bhandras Sep 11, 2024
da850b1
invoices: add migration code for a single invoice
bhandras Jun 11, 2024
1c2ee9e
invoices: add migration code that runs a full invoice DB SQL migration
bhandras Jun 12, 2024
7a60c30
sqldb+invoices: Optimize invoice fetching when the reference is only …
bhandras Dec 2, 2024
7bb57be
lnd: run invoice migration on startup
bhandras Sep 17, 2024
a9351c7
itest: add integration test for invoice migration
bhandras Sep 17, 2024
c4d14b5
itest: remove obsolete itest
bhandras Sep 18, 2024
7a5bc73
invoices+sql: use the stored AmtPaid value instead of recalculating
bhandras Jan 10, 2025
51045c4
invoices: allow migration test to work on kv sqlite channeldb
bhandras Jan 10, 2025
33b9112
docs: update release notes for 0.19.0
bhandras Sep 17, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 61 additions & 32 deletions config_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ import (
"github.com/lightningnetwork/lnd/rpcperms"
"github.com/lightningnetwork/lnd/signal"
"github.com/lightningnetwork/lnd/sqldb"
"github.com/lightningnetwork/lnd/sqldb/sqlc"
"github.com/lightningnetwork/lnd/sweep"
"github.com/lightningnetwork/lnd/walletunlocker"
"github.com/lightningnetwork/lnd/watchtower"
Expand All @@ -60,6 +61,16 @@ import (
"gopkg.in/macaroon-bakery.v2/bakery"
)

const (
// invoiceMigrationBatchSize is the number of invoices that will be
// migrated in a single batch.
invoiceMigrationBatchSize = 1000

// invoiceMigration is the version of the migration that will be used to
// migrate invoices from the kvdb to the sql database.
invoiceMigration = 6
)

// GrpcRegistrar is an interface that must be satisfied by an external subserver
// that wants to be able to register its own gRPC server onto lnd's main
// grpc.Server instance.
Expand Down Expand Up @@ -932,10 +943,10 @@ type DatabaseInstances struct {
// the btcwallet's loader.
WalletDB btcwallet.LoaderOption

// NativeSQLStore is a pointer to a native SQL store that can be used
// for native SQL queries for tables that already support it. This may
// be nil if the use-native-sql flag was not set.
NativeSQLStore *sqldb.BaseDB
// NativeSQLStore holds a reference to the native SQL store that can
// be used for native SQL queries for tables that already support it.
// This may be nil if the use-native-sql flag was not set.
NativeSQLStore sqldb.DB
}

// DefaultDatabaseBuilder is a type that builds the default database backends
Expand Down Expand Up @@ -1038,7 +1049,7 @@ func (d *DefaultDatabaseBuilder) BuildDatabase(
if err != nil {
cleanUp()

err := fmt.Errorf("unable to open graph DB: %w", err)
err = fmt.Errorf("unable to open graph DB: %w", err)
d.logger.Error(err)

return nil, nil, err
Expand Down Expand Up @@ -1072,51 +1083,69 @@ func (d *DefaultDatabaseBuilder) BuildDatabase(
case err != nil:
cleanUp()

err := fmt.Errorf("unable to open graph DB: %w", err)
err = fmt.Errorf("unable to open graph DB: %w", err)
d.logger.Error(err)
return nil, nil, err
}

// Instantiate a native SQL invoice store if the flag is set.
// Instantiate a native SQL store if the flag is set.
if d.cfg.DB.UseNativeSQL {
// KV invoice db resides in the same database as the channel
// state DB. Let's query the database to see if we have any
// invoices there. If we do, we won't allow the user to start
// lnd with native SQL enabled, as we don't currently migrate
// the invoices to the new database schema.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont we still want this check for anyone who has set d.cfg.DB.SkipSQLInvoiceMigration=true?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and to protect against users who had a bbolt invoice store before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont we still want this check for anyone who has set d.cfg.DB.SkipSQLInvoiceMigration=true

I don’t think it’s necessary, as this check was primarily intended to ensure that users with existing invoices in their database wouldn’t be able to start LND without the migration in place.

and to protect against users who had a bbolt invoice store before?

Since we don’t currently support mixed backends, the only scenario to consider is if the user is already using an SQL database but with the older KV schema.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes i see: if the user was pointing to a bbolt store and then changes config to sql store, there really isnt any way for us to know this so it is essentially a new node

invoiceSlice, err := dbs.ChanStateDB.QueryInvoices(
ctx, invoices.InvoiceQuery{
NumMaxInvoices: 1,
},
)
if err != nil {
cleanUp()
d.logger.Errorf("Unable to query KV invoice DB: %v",
err)
migrations := sqldb.GetMigrations()

// If the user has not explicitly disabled the SQL invoice
// migration, attach the custom migration function to invoice
// migration (version 6). Even if this custom migration is
// disabled, the regular native SQL store migrations will still
// run. If the database version is already above this custom
// migration's version (6), it will be skipped permanently,
// regardless of the flag.
if !d.cfg.DB.SkipSQLInvoiceMigration {
migrationFn := func(tx *sqlc.Queries) error {
return invoices.MigrateInvoicesToSQL(
ctx, dbs.ChanStateDB.Backend,
dbs.ChanStateDB, tx,
invoiceMigrationBatchSize,
)
}

return nil, nil, err
// Make sure we attach the custom migration function to
// the correct migration version.
for i := 0; i < len(migrations); i++ {
if migrations[i].Version != invoiceMigration {
continue
}

migrations[i].MigrationFn = migrationFn
}
}

if len(invoiceSlice.Invoices) > 0 {
// We need to apply all migrations to the native SQL store
// before we can use it.
err = dbs.NativeSQLStore.ApplyAllMigrations(ctx, migrations)
if err != nil {
cleanUp()
err := fmt.Errorf("found invoices in the KV invoice " +
"DB, migration to native SQL is not yet " +
"supported")
err = fmt.Errorf("faild to run migrations for the "+
"native SQL store: %w", err)
d.logger.Error(err)

return nil, nil, err
}

// With the DB ready and migrations applied, we can now create
// the base DB and transaction executor for the native SQL
// invoice store.
baseDB := dbs.NativeSQLStore.GetBaseDB()
executor := sqldb.NewTransactionExecutor(
dbs.NativeSQLStore,
func(tx *sql.Tx) invoices.SQLInvoiceQueries {
return dbs.NativeSQLStore.WithTx(tx)
baseDB, func(tx *sql.Tx) invoices.SQLInvoiceQueries {
return baseDB.WithTx(tx)
},
)

dbs.InvoiceDB = invoices.NewSQLStore(
sqlInvoiceDB := invoices.NewSQLStore(
executor, clock.NewDefaultClock(),
)

dbs.InvoiceDB = sqlInvoiceDB
} else {
dbs.InvoiceDB = dbs.ChanStateDB
}
Expand All @@ -1129,7 +1158,7 @@ func (d *DefaultDatabaseBuilder) BuildDatabase(
if err != nil {
cleanUp()

err := fmt.Errorf("unable to open %s database: %w",
err = fmt.Errorf("unable to open %s database: %w",
lncfg.NSTowerClientDB, err)
d.logger.Error(err)
return nil, nil, err
Expand All @@ -1144,7 +1173,7 @@ func (d *DefaultDatabaseBuilder) BuildDatabase(
if err != nil {
cleanUp()

err := fmt.Errorf("unable to open %s database: %w",
err = fmt.Errorf("unable to open %s database: %w",
lncfg.NSTowerServerDB, err)
d.logger.Error(err)
return nil, nil, err
Expand Down
6 changes: 6 additions & 0 deletions docs/release-notes/release-notes-0.19.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,11 @@ The underlying functionality between those two options remain the same.
transactions can run at once, increasing efficiency. Includes several bugfixes
to allow this to work properly.

* [Migrate KV invoices to
SQL](https://github.com/lightningnetwork/lnd/pull/8831) as part of a larger
effort to support SQL databases natively in LND.


## Code Health

* A code refactor that [moves all the graph related DB code out of the
Expand All @@ -265,6 +270,7 @@ The underlying functionality between those two options remain the same.

* Abdullahi Yunus
* Alex Akselrod
* Andras Banki-Horvath
* Animesh Bilthare
* Boris Nagaev
* Carla Kirk-Cohen
Expand Down
6 changes: 5 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ require (
github.com/opencontainers/image-spec v1.0.2 // indirect
github.com/opencontainers/runc v1.1.12 // indirect
github.com/ory/dockertest/v3 v3.10.0 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/pmezard/go-difflib v1.0.0
github.com/prometheus/client_model v0.2.0 // indirect
github.com/prometheus/common v0.26.0 // indirect
github.com/prometheus/procfs v0.6.0 // indirect
Expand Down Expand Up @@ -207,6 +207,10 @@ replace github.com/gogo/protobuf => github.com/gogo/protobuf v1.3.2
// allows us to specify that as an option.
replace google.golang.org/protobuf => github.com/lightninglabs/protobuf-go-hex-display v1.30.0-hex-display

// Temporary replace until https://github.com/lightningnetwork/lnd/pull/8831 is
// merged.
replace github.com/lightningnetwork/lnd/sqldb => ./sqldb
Comment on lines +210 to +212
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this commit should be dropped.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is still needed as we modify the sqldb package in this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is close to merge - should we not split this up into 2 PRs? 1) that updates sqldb and 2) that points to that one? so that we dont merge with this replacement. or is the plan just to do an immediate follow up removing the replace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both works imo, I'm happy to split it up once we get all approves :)


// If you change this please also update docs/INSTALL.md and GO_VERSION in
// Makefile (then run `make lint` to see where else it needs to be updated as
// well).
Expand Down
2 changes: 0 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -464,8 +464,6 @@ github.com/lightningnetwork/lnd/kvdb v1.4.12 h1:Y0WY5Tbjyjn6eCYh068qkWur5oFtioJl
github.com/lightningnetwork/lnd/kvdb v1.4.12/go.mod h1:hx9buNcxsZpZwh8m1sjTQwy2SOeBoWWOZ3RnOQkMsxI=
github.com/lightningnetwork/lnd/queue v1.1.1 h1:99ovBlpM9B0FRCGYJo6RSFDlt8/vOkQQZznVb18iNMI=
github.com/lightningnetwork/lnd/queue v1.1.1/go.mod h1:7A6nC1Qrm32FHuhx/mi1cieAiBZo5O6l8IBIoQxvkz4=
github.com/lightningnetwork/lnd/sqldb v1.0.6 h1:LJdDSVdN33bVBIefsaJlPW9PDAm6GrXlyFucmzSJ3Ts=
github.com/lightningnetwork/lnd/sqldb v1.0.6/go.mod h1:OG09zL/PHPaBJefp4HsPz2YLUJ+zIQHbpgCtLnOx8I4=
github.com/lightningnetwork/lnd/ticker v1.1.1 h1:J/b6N2hibFtC7JLV77ULQp++QLtCwT6ijJlbdiZFbSM=
github.com/lightningnetwork/lnd/ticker v1.1.1/go.mod h1:waPTRAAcwtu7Ji3+3k+u/xH5GHovTsCoSVpho0KDvdA=
github.com/lightningnetwork/lnd/tlv v1.3.0 h1:exS/KCPEgpOgviIttfiXAPaUqw2rHQrnUOpP7HPBPiY=
Expand Down
5 changes: 5 additions & 0 deletions invoices/invoices.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,11 @@ func (r InvoiceRef) Modifier() RefModifier {
return r.refModifier
}

// IsHashOnly returns true if the invoice ref only contains a payment hash.
func (r InvoiceRef) IsHashOnly() bool {
return r.payHash != nil && r.payAddr == nil && r.setID == nil
}

// String returns a human-readable representation of an InvoiceRef.
func (r InvoiceRef) String() string {
var ids []string
Expand Down
Loading
Loading