[sql-32] accounts: add migration code from kvdb to SQL #1047

ViktorTigerstrom · 2025-04-22T14:59:16Z

This PR introduces the migration logic for transitioning the accounts store from kvdb to SQL.

Note that as of this PR, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.

Part of #917

ellemouton

Soooooo excited to see this come through! I couldnt help but take a look even though review not requested - just leaving some drive by comments in the mean time :)

I'll wait till review requested before looking again :)

ellemouton · 2025-04-22T15:34:11Z

accounts/kv_sql_migration_test.go

+
+			t.Run("Postgres", func(t *testing.T) {
+				migrationTest(t, kvStore, testClock, false)
+			})
+
+			t.Run("SQLite", func(t *testing.T) {
+				migrationTest(t, kvStore, testClock, true)


in Lit, we've gone with the build flag approach. So can just use the existing NewTestDB methods no? and then if bbolt backend, just skip the test ?

Updated as discussed offline :)

ellemouton · 2025-04-22T15:34:42Z

accounts/store_sql.go

@@ -732,6 +733,79 @@ func (s *SQLStore) StoreLastIndexes(ctx context.Context, addIndex,
 	})
 }

+func makeInsertAccountParams(account *OffChainBalanceAccount) (


let's keep the migration code in the same file

Ok, yeah agree :)! Had it here previously to mimic lnd's migration implementation.

accounts/kv_sql_migration_test.go

accounts/sql_migration.go

ViktorTigerstrom · 2025-04-24T09:34:13Z

accounts/kv_sql_migration_test.go

+		{
+			"randomized accounts",
+			true,
+			randomizeAccounts,
+		},
+		{
+			"rapid randomized accounts",
+			true,
+			rapidRandomizeAccounts,
+		},


I added 2 tests here currently, where one uses rapid for the randomisation, and one uses the normal "golang.org/x/exp/rand" testing semi-randomisation.

The drawback with the rapid test, is that we can't really generate too many invoices and and payments per account, as you can't really limit the amount of times the rapid.Check runs in golang (i.e. the number of accounts we populate the db with), without limiting the amount of runs the Check function would run for the entire litd project. I.e. if we limited it for this test, that would impact any other tests in the future that uses the rapid.Check function. There is an open issue for this in the rapid lib, see:
flyingmutant/rapid#38

When running the migration locally, the test migration execution time really goes up if you have a lot of accounts with a lot of payments and invoices, and therefore I opted to not add such a test to not make the make unit execution time take too long locally.

Therefore, I also added a normal test that doesn't use rapid for randomization, so that test adds up to 1000 invoices and payments.

In the end though , I can delete one of the tests if you prefer any test over the other :)!

I think I prefer randomizeAccounts since it's more concise and performant, great you tested out both approaches 👍

Cool, thanks for the input! Would he interested to hear Elle's opinion here as well :)

bitromortac

Awesome work 🚀! Will certainly need another pass, since I haven't reviewed many migration PRs

accounts/kv_sql_migration_test.go

bitromortac · 2025-04-25T07:53:53Z

accounts/sql_migration.go

+		AccountID:      sqlID,
+		Hash:           hash[:],
+		Status:         int16(entry.Status),
+		FullAmountMsat: int64(entry.FullAmount),


an overflow is very unrealistic here, right?

Yeah I think we have to make a choice here, and for migrations in general:
We can either:

Assume that since this is a migration, the data we are migrating must have already been sanity checked and rule checked during the initial insertion into the kvdb, and therefore errors like that shouldn't be possible to occur during the migration.

We sanity check and check the rules again for all data during the migration.

I have opted for option 1 here and for upcoming migrations, as I do think that should be the case, and we'd only let the migrations use more resources during the migration if we chose option 2 instead.

But I'd very interested to hear from both of you if you think we should instead go with option 2 here.

Yes, I'm unsure, maybe we should be extra careful with amounts, but we can leave it as is.

I think we are already indirectly checking though: if we went with option 2, we'd check here and error out if it did overflow - but if we go with option 2, we will just fail later on when we do the reflect.DeapEqual check.

so i think for simplicity we dont need to add the redundant check

I agree that there's no big difference between 1 & 2 in ths migration. But there will be a difference for future migrations like sessions.

So for example in sessions:
To go with option 2, we'd also check when migrating a session that has linked sessions, that all prior sessions has been revoked. However I don't think that really makes sense as that's the type of rule that should have been checked when the entry was added to the kvdb, and would only add extra execution load during the migration IMO.

Would be interested to hear what both of your opinions is in regards to if we should do such sanity checking or not @bitromortac & @ellemouton!

I think even if a MaxUint64 would be present here, it wouldn't be a problem, because the binary format of a unit64 and int64 doesn't change with a conversion during the marshal/unmarshal operations.

I'm not sure if sanity checks should be done, but maybe we can decide case by case?

accounts/sql_migration.go

accounts/kv_sql_migration_test.go

bitromortac · 2025-04-25T09:21:36Z

accounts/kv_sql_migration_test.go

+		{
+			"randomized accounts",
+			true,
+			randomizeAccounts,
+		},
+		{
+			"rapid randomized accounts",
+			true,
+			rapidRandomizeAccounts,
+		},


I think I prefer randomizeAccounts since it's more concise and performant, great you tested out both approaches 👍

In preparation for upcoming migration tests from a kvdb to an SQL store, this commit updates the NewTestDB function to return the Store interface rather than a concrete store implementation. This change ensures that migration tests can call NewTestDB under any build tag while receiving a consistent return type.

ViktorTigerstrom · 2025-05-02T08:52:30Z

Thanks for the reviews! I've addressed feedback and left comments with the second last push. In the last push, I rebased on master to address a merge conflict.

bitromortac

LGTM 🔥🚀

accounts/sql_migration_test.go

bitromortac · 2025-05-05T13:42:56Z

accounts/sql_migration_test.go

+			},
+		},
+		{
+			"account with updated expiry",


I guess the intention here is to test the UpdatedAt field, right? If so that would be a better name for the test

Yeah your right, updated the name :).

bitromortac · 2025-05-05T13:44:41Z

accounts/sql_migration_test.go

+			},
+		},
+		{
+			"account with updated balance",


Is this test needed? it seems like it only tests CreditAccount/DebitAccount in addition. Also, maybe we can remove the HasExpired checks where applicable, as this adds a bit of noise?

Yeah I wanted to show the other type of update that you can do to an account, and that this didn't cause any issues to the migration. But you're right that it didn't add any real value, so I went ahead and removed the test :).

bitromortac · 2025-05-05T13:54:27Z

accounts/sql_migration_test.go

+		// randomize balance from 1,000 to 100,000,000
+		balance := lnwire.MilliSatoshi(
+			rand.Int63n(100000000-1000) + 1000,
+		)


Is it important that we have a lower bound in these tests? Nano-nit: the upper bound is not included, just to be aware

No not really, think I mainly wanted to just have some kind of balance, as I don't think the actual value should matter?

accounts/sql_migration_test.go

bitromortac · 2025-05-05T14:06:38Z

accounts/sql_migration.go

+		AccountID:      sqlID,
+		Hash:           hash[:],
+		Status:         int16(entry.Status),
+		FullAmountMsat: int64(entry.FullAmount),


Yes, I'm unsure, maybe we should be extra careful with amounts, but we can leave it as is.

ViktorTigerstrom · 2025-05-06T08:14:45Z

Thanks for the review @bitromortac! Will address feedback after next review round 🚀!

ellemouton

looking really great!

Note that as of this PR, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.

so reviewing this PR made me realise we should perhaps flip the order of things a bit: basically, i still want to be able to test this PR properly (ie, i want to run master branch - add some accounts, then switch to this PR and see that the accounts get migrated properly).

So this means that under the dev tag, we should first just add an option to point to an existing kvdb folder where we can migrate any existing accounts from (if we spin up in sql mode). Note: this probably means adding a set-up PR before we merge this one. But yeah - i think well worth it so that we can test these individually

accounts/sql_migration.go

ellemouton · 2025-05-06T11:39:23Z

accounts/sql_migration_test.go

+		populateDB      func(t *testing.T, kvStore *BoltStore)
+	}{
+		{
+			"empty",


let's always use named fields 🙏

reason being: right now, it's hard to do a quick check of things like "are there tests that cover expectedLastIndex=false" for example. if they are named, then it is very easy to check

Ah good point, updated!

accounts/sql_migration.go

ellemouton · 2025-05-06T12:26:19Z

accounts/sql_migration_test.go

+	}
+
+	for _, test := range tests {
+		tc := test


I thiiiink we dont need to do this anymore right?

Hmmm the sources I found seems to indicate that this is necessary, so if you know that something has changed, I'd really be interested to hear more about that 😃?

I can see that in lnd, this pattern is preset as soon as we use t.Parallel(), however in litd it seems like it varies. So I'm not fully sure if this is needed or not :)?

We should not need it anymore since we're on 1.23 (see https://tip.golang.org/wiki/LoopvarExperiment#does-this-mean-i-dont-have-to-write-x--x-in-my-loops-anymore).

yeah not needed

Thanks 🙏! Removed with the latest push!

accounts/sql_migration_test.go

ellemouton · 2025-05-06T12:34:29Z

accounts/sql_migration_test.go

+				require.NoError(t, kvStore.db.Close())
+			})
+
+			migrationTest(t, kvStore, testClock, tc.expectLastIndex)


not loving the flow here cause some of the setup is done here and the rest is done in migrationTest.

Thinking something like this instead:

// Create a new kvdb store to populate with test data. kvStore, err := NewBoltStore( t.TempDir(), DBFilename, clock, ) require.NoError(t, err) t.Cleanup(func() { require.NoError(t, kvStore.db.Close()) }) // Populate the kv store. test.populateDB(t, kvStore) // Create the SQL store that we will migrate the data // to. sqlStore, txEx := makeSQLDB(t) // Perform the migration. var opts sqldb.MigrationTxOptions err = txEx.ExecTx(ctx, &opts, func(tx SQLQueries) error { return MigrateAccountStoreToSQL( ctx, kvStore, tx, ) }) require.NoError(t, err) // Assert migration results. assertMigrationResults( t, kvStore, sqlStore, test.expectLastIndex, )

notice how the order of operations mimics that of what would actually happen on a real node.

Agree, thanks 🙏! I updated this, and also took #1047 (comment) into consideration. So therefore I changed your draft code to also fetch all accounts and indices prior to the migration.

accounts/sql_migration_test.go

lightninglabs-deploy · 2025-05-13T14:02:05Z

@ViktorTigerstrom, remember to re-request review from reviewers when ready

In preparation for the migration from kvdb to SQL, we update the results of the ListAllAccounts query. After the migration has been implemented, we will test that the result off all accounts in the SQL database is the same as the result of the fetching all accounts in the kvdb. By ordering the SQL query's result by account id, we ensure that all accounts are returned in the same order as they are returned by the kvdb.

ViktorTigerstrom · 2025-05-15T16:05:18Z

Re-requested a review from you @bitromortac as quite a bit of the testing code has changed, even though you previously ACKed. I hope that's ok

bitromortac

LGTM, the test code looks nicer now 🎉. Tested the migration manually as well, I ran a few commands for accounts (account creation/invoices/payments).

bitromortac · 2025-05-16T12:06:27Z

accounts/sql_migration_test.go

+	}
+
+	for _, test := range tests {
+		tc := test


We should not need it anymore since we're on 1.23 (see https://tip.golang.org/wiki/LoopvarExperiment#does-this-mean-i-dont-have-to-write-x--x-in-my-loops-anymore).

bitromortac · 2025-05-16T12:38:20Z

accounts/sql_migration.go

+		AccountID:      sqlID,
+		Hash:           hash[:],
+		Status:         int16(entry.Status),
+		FullAmountMsat: int64(entry.FullAmount),


I think even if a MaxUint64 would be present here, it wouldn't be a problem, because the binary format of a unit64 and int64 doesn't change with a conversion during the marshal/unmarshal operations.

I'm not sure if sanity checks should be done, but maybe we can decide case by case?

ellemouton

tACK, LGTM!

Couple of comments that came up during testing (but not for this PR):

currently we use BIGINT for account alias (8 byte) which becomes an int64 but then for session alias (4 bytes) we use a blob. should we make these the same type? iirc, we changed the account alias just for the space saving.
when we do the actual migration should we delete the old kvdb files? or perhaps we should leave them for the time being. and then in a future release we can do a clean up once we are more sure things are ok

ellemouton · 2025-05-19T11:06:08Z

accounts/sql_migration_test.go

+	}
+
+	for _, test := range tests {
+		tc := test


yeah not needed

accounts/sql_migration_test.go

This commit introduces the migration logic for transitioning the accounts store from kvdb to SQL. Note that as of this commit, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.

ViktorTigerstrom added the no-changelog This PR is does not require a release notes entry label Apr 22, 2025

ellemouton reviewed Apr 22, 2025

View reviewed changes

ViktorTigerstrom force-pushed the 2025-04-migrate-accounts branch from 3072e48 to b4cffd8 Compare April 24, 2025 09:12

ViktorTigerstrom commented Apr 24, 2025

View reviewed changes

ViktorTigerstrom requested review from ellemouton and bitromortac April 24, 2025 09:37

levmi assigned ViktorTigerstrom Apr 24, 2025

ViktorTigerstrom force-pushed the 2025-04-migrate-accounts branch from b4cffd8 to 8cd1631 Compare April 24, 2025 15:14

bitromortac reviewed Apr 25, 2025

View reviewed changes

ViktorTigerstrom force-pushed the 2025-04-migrate-accounts branch from 8cd1631 to 7ccdd7f Compare May 2, 2025 08:44

ViktorTigerstrom force-pushed the 2025-04-migrate-accounts branch from 7ccdd7f to 26b6809 Compare May 2, 2025 08:48

ViktorTigerstrom mentioned this pull request May 2, 2025

[sql-33] session: add migration code from kvdb to SQL #1051

Open

bitromortac approved these changes May 5, 2025

View reviewed changes

ellemouton reviewed May 6, 2025

View reviewed changes

ViktorTigerstrom force-pushed the 2025-04-migrate-accounts branch from 26b6809 to 81129d6 Compare May 15, 2025 15:29

ViktorTigerstrom requested review from ellemouton and bitromortac May 15, 2025 16:04

bitromortac approved these changes May 16, 2025

View reviewed changes

ellemouton approved these changes May 19, 2025

View reviewed changes

ViktorTigerstrom added 2 commits May 19, 2025 14:29

accounts: add migration code from kvdb to SQL

dbb5d73

This commit introduces the migration logic for transitioning the accounts store from kvdb to SQL. Note that as of this commit, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.

accounts: add randomized accounts migration tests

1e0e88b

ViktorTigerstrom force-pushed the 2025-04-migrate-accounts branch from 81129d6 to 1e0e88b Compare May 19, 2025 12:29

guggero merged commit 0015ca2 into lightninglabs:master May 19, 2025
22 checks passed

[sql-32] accounts: add migration code from kvdb to SQL #1047

[sql-32] accounts: add migration code from kvdb to SQL #1047

Conversation

ViktorTigerstrom commented Apr 22, 2025

ellemouton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitromortac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom commented May 2, 2025

bitromortac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom commented May 6, 2025

ellemouton left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightninglabs-deploy commented May 13, 2025

ViktorTigerstrom commented May 15, 2025

bitromortac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton left a comment •

edited

Loading