[sql-44] firewalldb: add migration code for privacy mapper from kvdb to SQL #1092

ViktorTigerstrom · 2025-06-17T16:07:02Z

Based on the branch of #1079, to only show the relevant commits.

This PR introduces the migration logic for transitioning the privacy mapper 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.

I will rebase this on the new version of #1079, once the current feedback has been addressed after we agree on the approach there.

As this PR is heavily dependent on #1079, this is blocked on the dependent PR until it has been merged. This is therefore not ready for review.

Part of #917

ellemouton · 2025-06-18T10:30:46Z

@ViktorTigerstrom - feel free to push a base branch to upstream & then target that so that this pr can just contain the commits that are relevant

ViktorTigerstrom · 2025-07-11T22:47:33Z

Rebased this on the latest version of #1079, as that PR feels good to go, and this PR is therefore close to not being blocked by that PR (and therefore ready to review).

ViktorTigerstrom · 2025-07-11T22:53:37Z

Requested a review by both of you @ellemouton & @bitromortac, even though #1079 isn't merged yet, just so you know that it is ready for review once that's been merged.

ellemouton

really really great work. Very clean 🔥

firewalldb/sql_migration_test.go

ellemouton · 2025-07-16T08:45:45Z

firewalldb/sql_migration.go

+	privPairs, err := collectPrivacyPairs(ctx, kvStore, sqlTx)
+	if err != nil {
+		return fmt.Errorf("error migrating privacy mapper store: %w",
+			err)
+	}
+
+	// 2) Insert all collected privacy pairs into the SQL database.
+	err = insertPrivacyPairs(ctx, sqlTx, privPairs)
+	if err != nil {


we may want to tests the performance of this one as i think unlike the kv-store db, this one could be quite populated.

So for this one it might make sense to migrate on the fly instead of collecting everything in memory first.

Good idea, I will benchmark this as a follow-up and see if it makes sense to process every privacy pair individually, or if this version works as well. I've prepared a separate branch that changes the code here to processes each pair one by one here:
https://github.com/ViktorTigerstrom/lightning-terminal/tree/2025-06-migrate-privacy-mapper-individual-pair-processing

That branch adds one additional commit that changes this code. If you think it already makes sense to change the code to that version already before benchmarking, let me know and I'll fixup that last commit into this PR.

ellemouton · 2025-07-16T08:49:10Z

firewalldb/sql_migration.go

+		_, err := sqlTx.GetPseudoForReal(
+			ctx, sqlc.GetPseudoForRealParams{
+				GroupID: groupID,
+				RealVal: realVal,
+			},
+		)
+		if err == nil {
+			return fmt.Errorf("duplicate privacy pair %s:%s: %w",
+				realVal, pseudoVal, ErrDuplicatePseudoValue)
+		} else if !errors.Is(err, sql.ErrNoRows) {
+			return err
+		}
+
+		_, err = sqlTx.GetRealForPseudo(
+			ctx, sqlc.GetRealForPseudoParams{
+				GroupID:   groupID,
+				PseudoVal: pseudoVal,
+			},
+		)
+		if err == nil {
+			return fmt.Errorf("duplicate privacy pair %s:%s: %w",
+				realVal, pseudoVal, ErrDuplicatePseudoValue)
+		} else if !errors.Is(err, sql.ErrNoRows) {
+			return err
+		}
+
+		err = sqlTx.InsertPrivacyPair(
+			ctx, sqlc.InsertPrivacyPairParams{
+				GroupID:   groupID,
+				RealVal:   realVal,
+				PseudoVal: pseudoVal,
+			},
+		)
+		if err != nil {


can we not potentially do this in one go with a single query that has a conflict rule?

Actually, I noticed that since the privacy_mapper SQL table already has unique constraints, those additional duplication checks becomes redundant. I included those as they were present in the sql_store.go insertion function. As those checks therefore also are redundant there, I included an extra commit 5506372 that removes them there as well. If you think we shouldn't remove them there as well, let me know and I'll drop that commit :)

I included an extra commit 5506372

Actually, I noticed that we have those checks explicitly in the kvdb stores as well, so I think this falls back to what we've discussed before: Don't change or optimize & change the definition in the SQL store before we've migrated, and instead mimic the kvdb store behaviour. Therefore I updated the PR again to remove the extra commit, as I believe we need to wait until after the migration has been shipped before we can change that.

ellemouton · 2025-07-16T08:51:19Z

firewalldb/sql_migration.go

+	if realBkt := bkt.Bucket(realToPseudoKey); realBkt != nil {
+		realToPseudoRes, err = collectPairs(realBkt)


just checking: do we have a test somewhere that makes sure that things dont fail on an empty kvdb?

I think the "empty" test in every migration does exactly that? Note that the real kvdb to SQL migration will also initiate the actual kvdb stores by calling accounts.NewBoltStore, session.NewDB & firewalldb.NewBoltDB before the migration starts, which initiates the base buckets.

ellemouton · 2025-07-16T08:53:25Z

firewalldb/sql_migration_test.go

 		kvEntries: fn.Some(insertedEntries),
+		// No privacy pairs are inserted in this test.
+		privPairs: fn.None[privacyPairs](),


we should have a test that includes both

Ah good idea! I added a randomFirewallDBEntries that combines the results of randomKVEntries & randomPrivacyPairs into one test :)

bitromortac

Looks great! Only have a couple of nits 🚀

firewalldb/sql_migration_test.go

firewalldb/sql_migration.go

bitromortac · 2025-07-17T12:33:31Z

firewalldb/sql_migration_test.go

+
+// multiSessionsOnePrivPairs inserts 1 session with 1 privacy pair into the
+// boltDB.
+func oneSessionAndPrivPair(t *testing.T, ctx context.Context,


nit: we could use createPrivacyPairs directly

I added these functions to avoid having to change the signature of the populateDB function, and to not to not have to define the populateDB function within the actual tests list definition. If you have an idea of how to do it in a cleaner way than having these separate functions, let me know :)

ah, I see, using anonymous functions like in the empty case would do as well, would have made it easier to spot the discrepancy in multiSessionsOnePrivPairs, but fine like it is

firewalldb/sql_migration_test.go

As the firewalldb migration will include more than just the migration of the kvstore data, we rename the migration tests that only migrate the kvstore data to make it clearer which tests only focus on migrating kv entries.

bitromortac

LGTM nice 🎉

bitromortac · 2025-07-29T12:11:59Z

firewalldb/sql_migration_test.go

+	sessSQLStore, ok := sessionStore.(*session.SQLStore)
+	require.True(t, ok)
+
+	for i := 0; i < numSessions; i++ {


nit: this and a case below can be simplified to the range int syntax, same in randomPrivacyPairs

we could also remove the random identifiers from the NewSession calls in these tests similarly like we did in the other PR, to remove a bit of noise

firewalldb/sql_migration_test.go

bitromortac · 2025-07-29T12:27:30Z

firewalldb/sql_migration_test.go

+
+// multiSessionsOnePrivPairs inserts 1 session with 1 privacy pair into the
+// boltDB.
+func oneSessionAndPrivPair(t *testing.T, ctx context.Context,


ah, I see, using anonymous functions like in the empty case would do as well, would have made it easier to spot the discrepancy in multiSessionsOnePrivPairs, but fine like it is

firewalldb/sql_migration_test.go

ellemouton

non-blocking nits only from my side!
Really beautiful piece of code 🤩

firewalldb/sql_migration_test.go

firewalldb/sql_migration.go

ellemouton · 2025-07-29T12:38:36Z

firewalldb/sql_migration.go

+func insertGroupPairs(ctx context.Context, sqlTx SQLQueries,
+	pairs map[string]string, groupID int64) error {


nit: paramater order. makes more sense to have static/dependent things first and then the variable thing. ie, have groupID before pairs

Ah yeah, addressed :)

firewalldb/sql_migration.go

ellemouton · 2025-07-29T12:39:47Z

firewalldb/sql_migration_test.go

+		// the expected results.
+		expRes.kvEntries.WhenSome(func(kvEntries []*kvEntry) {
+			assertKvStoreMigrationResults(t, sqlStore, kvEntries)
+		})


and when not some? should we not do an assertion then too?

I changed this, and by doing so I also changed to address @bitromortac's old comment here:
#1092 (comment)

Doing so also required me to update the assertPrivacyMapperMigrationResults slightly to make sure no privacy pairs exists in the SQL db which we do not expect to be there.

firewalldb/sql_migration.go

Currently, the migration tests for firewalldb only migrates the kv stores. In future commits, we will also migrate the privacy mapper and the actions in the firewalldb,. Before this commit, the expected results of the migrations tests could only be kv records, which will not be the case when we also migrate the privacy mapper and the actions. Therefore, we prepare the migration tests to expect more than just kv records. This commit introduces a new type of `expectedResult` type which the prep of the migration tests will use, which can specify more than just one type of expected result.

This commit removes speculative comments from the firewalldb migration docs that forecast future implementations. Such comments can create confusion for developers looking at the current code base without knowing our plans how the migration will be further developed.

This commit introduces the migration logic for transitioning the privacy mapper 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 Jun 17, 2025

ViktorTigerstrom self-assigned this Jun 17, 2025

ViktorTigerstrom force-pushed the 2025-06-migrate-privacy-mapper branch from bbe2b90 to 9715313 Compare July 11, 2025 22:45

ViktorTigerstrom changed the base branch from master to kvstores-migration-PR July 11, 2025 22:50

ViktorTigerstrom requested review from ellemouton and bitromortac July 11, 2025 22:52

ellemouton reviewed Jul 16, 2025

View reviewed changes

bitromortac reviewed Jul 17, 2025

View reviewed changes

firewalldb: rename kv stores migration tests

7a40615

As the firewalldb migration will include more than just the migration of the kvstore data, we rename the migration tests that only migrate the kvstore data to make it clearer which tests only focus on migrating kv entries.

ViktorTigerstrom force-pushed the 2025-06-migrate-privacy-mapper branch from 9715313 to d440c12 Compare July 23, 2025 14:31

ViktorTigerstrom changed the base branch from kvstores-migration-PR to master July 23, 2025 22:35

ViktorTigerstrom force-pushed the 2025-06-migrate-privacy-mapper branch 2 times, most recently from 5506372 to a4fc12e Compare July 23, 2025 23:11

ViktorTigerstrom requested review from ellemouton and bitromortac July 23, 2025 23:24

bitromortac approved these changes Jul 29, 2025

View reviewed changes

ellemouton approved these changes Jul 29, 2025

View reviewed changes

ViktorTigerstrom added 3 commits July 30, 2025 14:38

firewalldb: add privacy mapper SQL migration

205ec9e

This commit introduces the migration logic for transitioning the privacy mapper 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 force-pushed the 2025-06-migrate-privacy-mapper branch from a4fc12e to 205ec9e Compare July 30, 2025 12:44

ellemouton merged commit 5875147 into lightninglabs:master Jul 30, 2025
22 checks passed

		if realBkt := bkt.Bucket(realToPseudoKey); realBkt != nil {
		realToPseudoRes, err = collectPairs(realBkt)

		func insertGroupPairs(ctx context.Context, sqlTx SQLQueries,
		pairs map[string]string, groupID int64) error {

[sql-44] firewalldb: add migration code for privacy mapper from kvdb to SQL #1092

[sql-44] firewalldb: add migration code for privacy mapper from kvdb to SQL #1092

Uh oh!

Conversation

ViktorTigerstrom commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ellemouton commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorTigerstrom commented Jul 11, 2025

Uh oh!

ViktorTigerstrom commented Jul 11, 2025

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorTigerstrom Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitromortac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bitromortac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ViktorTigerstrom commented Jun 17, 2025 •

edited

Loading

ellemouton commented Jun 18, 2025 •

edited

Loading

ViktorTigerstrom Jul 23, 2025 •

edited

Loading