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 1 commit
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: 54 additions & 39 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 @@ -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,65 +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 {
// We need to apply all migrations to the native SQL store
// before we can use it.
err := dbs.NativeSQLStore.ApplyAllMigrations(
ctx, sqldb.GetMigrations(),
)
if err != nil {
cleanUp()
err := fmt.Errorf("unable to apply migrations: %w", err)
d.logger.Error(err)

return nil, nil, 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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm so if the user disables this, and we add a new migration in the future (version 7) he will not be able to migrate the invoice ?

if !d.cfg.DB.SkipSQLInvoiceMigration {
migrationFn := func(tx *sqlc.Queries) error {
return invoices.MigrateInvoicesToSQL(
ctx, dbs.ChanStateDB.Backend,
dbs.ChanStateDB, tx,
invoiceMigrationBatchSize,
)
}

// 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)
// 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
}

return nil, nil, err
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(
baseDB,
func(tx *sql.Tx) invoices.SQLInvoiceQueries {
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 @@ -1143,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 @@ -1158,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
Binary file modified invoices/testdata/channel.db
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure we want to commit this?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it can be generated at runtime if we don't want to lug this blob around?

Copy link
Collaborator

Choose a reason for hiding this comment

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

cc @bhandras - thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

would it be difficult to inject in at runtime @bhandras

Binary file not shown.
5 changes: 4 additions & 1 deletion lncfg/db.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ type DB struct {

UseNativeSQL bool `long:"use-native-sql" description:"Use native SQL for tables that already support it."`

SkipSQLInvoiceMigration bool `long:"skip-sql-invoice-migration" description:"Do not migrate invoices stored in our key-value database to native SQL."`

NoGraphCache bool `long:"no-graph-cache" description:"Don't use the in-memory graph cache for path finding. Much slower but uses less RAM. Can only be used with a bolt database backend."`

PruneRevocation bool `long:"prune-revocation" description:"Run the optional migration that prunes the revocation logs to save disk space."`
Expand Down Expand Up @@ -115,7 +117,8 @@ func DefaultDB() *DB {
MaxConnections: defaultSqliteMaxConnections,
BusyTimeout: defaultSqliteBusyTimeout,
},
UseNativeSQL: false,
UseNativeSQL: false,
SkipSQLInvoiceMigration: false,
}
}

Expand Down
3 changes: 3 additions & 0 deletions sample-lnd.conf
Original file line number Diff line number Diff line change
Expand Up @@ -1472,6 +1472,9 @@
; own risk.
; db.use-native-sql=false

; If set to true, native SQL invoice migration will be skipped. Note that this
; option is intended for users who experience non-resolvable migration errors.
; db.skip-sql-invoice-migration=false
ellemouton marked this conversation as resolved.
Show resolved Hide resolved

[etcd]

Expand Down
4 changes: 4 additions & 0 deletions sqldb/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ var (
Name: "000006_invoice_migration",
Version: 6,
SchemaVersion: 6,
// A migration function is may be attached to this
// migration to migrate KV invoices to the native SQL
// schema. This is optional and can be disabled by the
// user.
},
}
)
Expand Down