[sql-41] firewalldb: add migration code for kvstores from kvdb to SQL #1079

ViktorTigerstrom · 2025-05-27T15:55:25Z

Based on #1051

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

Once #1051 is merged, I will rebase this PR and request reviews.

Part of #917

ellemouton

i think this doesnt need to be based on the sessions pr right? yes order of merge matters, but i think we dont need to base this one off of that one or at least can change the base branch here so that we can review both migrations in parallel

firewalldb/sql_migration.go

ViktorTigerstrom · 2025-06-03T23:01:13Z

Thanks for the swift review @ellemouton 🙏! I've rebased this to be based on master and not the sessions migration PR. Note that I've kept 2 commits from the sessions migration PR in this PR (the 2 first), as they are required. So feel free to ignore reviewing the first to commits of this PR.
Also note that since this is not based on the sessions migrations PR, we can't test this migration locally as we need the sessions to be migrated prior to the kvstores (so that we can fetch the sessions SQL IDs). Let me know if you feel a need to test this prior to merging the sessions PR though, and I'll send you a branch which adds local testing that's based on both PRs.

ellemouton

looking great! a couple of comments. I'll do a final round once the session PR is in 👍

ellemouton · 2025-06-04T12:36:12Z

firewalldb/interface.go

@@ -141,4 +141,7 @@ type FirewallDBs interface {
 	RulesDB
 	PrivacyMapper
 	ActionDB
+
+	// Close closes the underlying store.
+	Close() error


dont think we need this

just call t.Cleanup sooner

(again commit message could explain more as it took me a sec to figure out why you're adding this)

Thanks 🙏! Addressed with a new commit in the latest push :)

ellemouton · 2025-06-04T12:39:42Z

firewalldb/db.go

@@ -14,29 +14,21 @@ var (
 	ErrNoSuchKeyFound = fmt.Errorf("no such key found")
 )



your commit messages could be fleshed out a bit more pls. it isnt clear why this needs to be exported (same for other commits in this PR)

Sorry, completely forgot updating the commit messages in the latest push. I've updated them in the latest push though, so I hope the commits should be understandable now!

firewalldb/sql_migration.go

ellemouton · 2025-06-04T12:42:13Z

firewalldb/sql_migration.go

+	// SQL database during the migration.
+	var allParams []kvParams
+
+	err := kvStore.Update(func(kvTx *bbolt.Tx) error {


why Update?

It was due to the migration utilising the getMainBucket helper function, which with the previous version created the main bucket if it didn't exist. However I realised that if the bucket doesn't exist, it just means that we can skip migrating the records, so I updated the PR to do that instead with the latest push.

ellemouton · 2025-06-04T12:45:49Z

firewalldb/sql_migration.go

+		return err
+	}
+
+	// After the migration is done, we validate that all inserted kvParams


cool yeah thanks for separating this 👍 i think it is especially important for the kv stores since there is a higher chance of overwriting 👍

ellemouton · 2025-06-04T12:48:32Z

firewalldb/sql_migration_test.go

+		},
+		{
+			name:       "random records",
+			populateDB: randomKVRecords,


i think it's worth having a case that does some explicit entries that write to all the store types. currently we only do that for the random one.

Good idea! Added an extra test with the latest push.

it still doesnt have a deterministic test that stores in 2 feature tables/ 2 rule tables etc right?

like, currently globalRecords only inserts under a single rule name, for example

also, looks like for this new test, all the key-value pairs inserted at all the levels are the same... so again it doesnt properly test that the correct things mapping is taking place

Ah yeah, good point! I updated this test quite a bit (now renamed to allEntryCombinations) to hopefully cover all different types of entries in a deterministic way. I think it should now cover all of those cases.

ViktorTigerstrom · 2025-06-09T12:35:30Z

Thanks for the review @ellemouton 🙏! Addressed the latest feedback and rebased this once again on the important commits from #1051.

bitromortac

Great work 🚀, took an initial look, the structure looks great!

firewalldb/kvstores_kvdb.go

firewalldb/sql_migration.go

firewalldb/sql_migration_test.go

ellemouton · 2025-06-13T07:22:58Z

firewalldb/kvstores_kvdb.go

-              -> sessions -> group ID  -> session-kv-store  -> {k:v}
-			               -> feature-kv-stores -> feature-name -> {k:v}
+"rules" -> "perm" -> rule-name -> "global" -> {k:v}
+				  "session-kv-store" -> group ID -> {k:v}


missing the -> here

ellemouton · 2025-06-13T07:30:16Z

firewalldb/kvstores_kvdb.go

-rules -> perm -> rule-name -> global   -> {k:v}
-              -> sessions -> group ID  -> session-kv-store  -> {k:v}
-			               -> feature-kv-stores -> feature-name -> {k:v}
+"rules" -> "perm" -> rule-name -> "global" -> {k:v}


while we are here, can we add < & > around keys that vary? like <rule-name>

and <group ID>& <feature name>

Good idea! done :)

firewalldb/kvstores_kvdb.go

ellemouton · 2025-06-13T07:38:08Z

firewalldb/sql_migration.go

+				continue
+			}
+
+			err = mainBucket.ForEach(func(k, v []byte) error {


suggest adding a short comment: "Loop over each rule-name in the bucket"

firewalldb/sql_migration.go

ellemouton · 2025-06-13T09:51:24Z

firewalldb/sql_migration_test.go

+	tempKvRecord := kvStoreRecord{
+		RuleName:    ruleName,
+		GroupID:     &sess.GroupID,
+		FeatureName: featureNameOpt,
+		EntryKey:    entryKey,
+		Value:       entryVal,
+		Perm:        false,
+		Global:      global,
+	}
+
+	insertKvRecord(t, ctx, boltDB, tempKvRecord)
+
+	permKvRecord := kvStoreRecord{
+		RuleName:    ruleName,
+		GroupID:     &sess.GroupID,
+		FeatureName: featureNameOpt,
+		EntryKey:    entryKey,
+		Value:       entryVal,
+		Perm:        true,
+		Global:      global,
+	}


if you are inserting the same key-value pair in both, then things are not properly being tested since you are not testing that the one namespace doesnt overwrite the other

I'm not fully sure that I got exactly what this feedback was referring to here, but I added additional inserts to the allEntryCombinations as mentioned in #1079 (comment). I hope that covers this feedback, else let me know!

ellemouton · 2025-06-13T09:53:36Z

firewalldb/sql_migration_test.go

+		},
+		{
+			name:       "random records",
+			populateDB: randomKVRecords,


also, looks like for this new test, all the key-value pairs inserted at all the levels are the same... so again it doesnt properly test that the correct things mapping is taking place

ellemouton · 2025-06-13T09:58:12Z

firewalldb/sql_migration_test.go

+	// Create a session that we can reference.
+	sess, err := sessionStore.NewSession(
+		ctx, "test", session.TypeAutopilot,
+		time.Unix(1000, 0), "something",
+	)
+	require.NoError(t, err)
+


only need to create session if !global

firewalldb/sql_migration_test.go

firewalldb/sql_migration.go

lightninglabs-deploy · 2025-06-27T11:02:07Z

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

ViktorTigerstrom · 2025-07-02T00:01:10Z

Thanks a lot for the reviews and the great suggestions @ellemouton & @bitromortac 🙏🎉!!! I've overhauled this PR quite a bit since the last push to address that feedback just FYI, so expect the PR to contain quite a lot of new changes.

Also, apologies for the linting error. I'll address that one after the next review round, in order to not waste extra CI resources just for that.

bitromortac

Very nice changes 🔥! I think the general structure looks good, I have a suggestion that we could make our store consistent such that if []byte{} is passed in we would receive a []byte{}. If we would need to be able to store nil we could remove the NOT NULL constraint, but I'm not sure it's needed. If this works, we could remove uncertainty and test code.

bitromortac · 2025-07-02T08:43:43Z

db/sqlc/queries/kvstores.sql

  AND feature_id IS NULL;

-- name: GetSessionKVStoreRecord :one
+-- name: GetSessionGroupKVStoreRecord :one


nit: we could shorten SessionGroup to just Group as I think it isn't that ambiguous

Addressed :)

bitromortac · 2025-07-02T08:47:35Z

firewalldb/kvstores_kvdb.go

-			               -> feature-kv-stores -> feature-name -> {k:v}
+"rules" -> "perm" -> <rule-name> -> "global" -> {k:v}
+				 -> "session-kv-store" -> <group-ID> -> {k:v}
+							             -> "feature-kv-stores" -> <feature-name> -> {k:v}


tiny nit: I would prefer to use spaces only instead of tabs

@ViktorTigerstrom - you should be able to set your IDE up to auto replace tabs as spaces

just wanted to note that the whitespace still consists of tabs, but it's ok, as it was just small nit

Ah snap! Thanks again, this latest push should really have addressed this now :)

firewalldb/sql_migration.go

bitromortac · 2025-07-02T09:20:40Z

firewalldb/sql_migration.go

+
+	var pairs []*kvEntry
+
+	// 1) Collect all key-value pairs from the KV store.


We don't anticipate any memory concerns here I guess since value objects seem to be quite small right? I think for actions we may need to consider a chunked approach

yeah this is the sort of thing we should write benchmarks for. perhaps a good candidate for a follow up: vitkor, you can create a TODO in the epic description & then it can be done in a follow up i think (for all the migrations)

Good idea with the benchmarking! Added that to the epic.

We don't anticipate any memory concerns here I guess since value objects seem to be quite small right? I think for actions we may need to consider a chunked approach

Yeah agree, we discussed this a bit offline previously, and for kvstores we deemed that this shouldn't be an issue as there shouldn't be much data stored in the kvstores for our users. But definitely agree that this becomes much more important for the actions migration, and I'd argue that it might be worth it to insert and validate the migration of one action at a time there just to ensure that this can't be an issue there either.

bitromortac · 2025-07-02T09:40:48Z

firewalldb/sql_migration.go

+		// Compare the value of the migrated entry with the original
+		// value from the KV store.
+		// NOTE: if the insert a []byte{} value into the sqldb as the
+		// entry value, and then retrieve it, the value will be
+		// returned as nil. The bytes.Equal will pass in that case,
+		// and therefore such cases won't error out. The kvdb instance
+		// can store []byte{} values.


I'd favor the store to return []byte{} if it internally stores it as nil when passing in []byte{}, with a conversion at the lowest layer. Can the kv database currently have []byte(nil) stored via some code path? If that's the case we may want to remove NOT NULL from value BLOB.

Can the kv database currently have []byte(nil) stored via some code path? If

Should make sure to cover in a unit test

I'd favor the store to return []byte{} if it internally stores it as nil when passing in []byte{}

I agree that this would be the ideal solution, but I've tested and unfortunately I can't get the SQLLite DB to return it as a []byte{}, even when removing the NOT NULL constraint. For Postgres DBs, I can get it to work. So I believe this is a differentiation of how the DB implementations handles that type of data, and therefore IMO the bytes.equals solution is the best we can do.

Can the kv database currently have []byte(nil) stored via some code path?

In a prod env, there should be no code path that leads to a []byte(nil) value being inserted from my research.

Should make sure to cover in a unit test

The allEntryCombinations & randomKVEntries unit tests already covered that by inserting nil values, which are equivalent, but just to explicitly also insert a value set to exactly []byte(nil), I added an extra nilSliceValue = []byte(nil) to the allEntryCombinations unit test :)

I can't get the SQLLite DB to return it as a []byte{}

Right, I tested that as well, and it doesn't seem to work. I meant another solution, to update sqlKVStore.Get to convert []byte(nil) to []byte{}, I think that could resolve this.

Do you mean that we'd implement a conversion in the actual sql store layer (the SQLDB struct, i.e. the layer between the SQLQueries and the rest of the application), which converts nil values fetched from the db to []byte{}?
We could potentially get that to work in the sql_migration_test.go file, but not in the sql_migration.go file, as we need to use the pure SQLQueries object in the sql_migration.go file (this becomes very important when implementing migrations using the sqldb/v2 library). Eventually I think we may want to go for the using the pure SQLQueries object in the test assertions as well, but I think that's out of scope for this PR.

Potentially we could manually update the query implementation to always return []byte{} on nil values, but that would mean that we wouldn't be able to use sqlc to generate this query, which I'm not sure we'd want to do due to the downsides? Would be appreciate your opinions on that as well :)!

I see, one workaround could be to apply a patch with gen_sqlc_docker.sh like here to the respective queries.

Hmmm yeah cool idea! I can look into if it'd be possible to replace nil results with []byte{} for all queries in the sql db by a adding some kind of script there, if you think it's worth it. Do you think we should do that in this PR, or as a follow-up?

firewalldb/sql_migration_test.go

bitromortac · 2025-07-04T08:46:56Z

firewalldb/sql_migration_test.go

+		ctx, "initial-session", session.Type(uint8(rand.Intn(5))),
+		time.Unix(1000, 0), randomString(rand.Intn(10)+1),


nit: I suggest to remove randomness here, since we are not testing session functionality, to have clearer code

firewalldb/sql_migration_test.go

ellemouton

great work!

I think g2g on next push 🛩️

ellemouton · 2025-07-09T09:02:38Z

firewalldb/kvstores_kvdb.go

-			               -> feature-kv-stores -> feature-name -> {k:v}
+"rules" -> "perm" -> <rule-name> -> "global" -> {k:v}
+				 -> "session-kv-store" -> <group-ID> -> {k:v}
+							             -> "feature-kv-stores" -> <feature-name> -> {k:v}


@ViktorTigerstrom - you should be able to set your IDE up to auto replace tabs as spaces

ellemouton · 2025-07-09T09:04:17Z

firewalldb/sql_migration_test.go

+		var store KVStore
+		switch {
+		case entry.groupAlias.IsNone() && entry.perm:
+			store = tx.Global()
+		case entry.groupAlias.IsNone() && !entry.perm:
+			store = tx.GlobalTemp()
+		case entry.groupAlias.IsSome() && !entry.perm:
+			store = tx.LocalTemp()
+		case entry.groupAlias.IsSome() && entry.perm:
+			store = tx.Local()
+		}


note that in the example i posted before, we could not possibly end up with a nil store. So if you want to have these 4 cases and not pre assigne a default, the rather have a default case to catch that none of the cases match

Ok I changed to your previous example :). The reason I went for the above, was that I liked that it was very explicit what each possible logical combination would lead to, and that this was in a test file (which is why I avoided the extra unreachable default case).

But addressed this with your previous suggestion :)

firewalldb/sql_migration.go

ellemouton · 2025-07-09T09:08:28Z

firewalldb/sql_migration.go

+
+	var pairs []*kvEntry
+
+	// 1) Collect all key-value pairs from the KV store.


yeah this is the sort of thing we should write benchmarks for. perhaps a good candidate for a follow up: vitkor, you can create a TODO in the epic description & then it can be done in a follow up i think (for all the migrations)

ellemouton · 2025-07-09T09:09:12Z

firewalldb/sql_migration.go

+		// Compare the value of the migrated entry with the original
+		// value from the KV store.
+		// NOTE: if the insert a []byte{} value into the sqldb as the
+		// entry value, and then retrieve it, the value will be
+		// returned as nil. The bytes.Equal will pass in that case,
+		// and therefore such cases won't error out. The kvdb instance
+		// can store []byte{} values.


Can the kv database currently have []byte(nil) stored via some code path? If

Should make sure to cover in a unit test

firewalldb/sql_migration.go

ViktorTigerstrom · 2025-07-09T21:51:21Z

Thanks a lot for the in depth reviews @bitromortac & @ellemouton 🙏🔥! Addressed your feedback with the latest push.

ellemouton

LGTM ✅ great work!

So we are following up with the []byte{}/nil thing yeah? @ViktorTigerstrom can you create an issue or add it to the epic's TODO list just so that it is being tracked and we dont forget?

ViktorTigerstrom · 2025-07-11T11:56:23Z

Thanks for the review @ellemouton 🎉! Added that + the sqldb v2 follow-up to the epic.

bitromortac

LGTM 🎉🎉, great work!

bitromortac · 2025-07-15T13:38:36Z

firewalldb/sql_migration_test.go

+		nilValue      []byte = nil
+		nilSliceValue        = []byte(nil)


nit: wanted to point out that those are equivalent (can be addressed later)

Yeah, I included this to address #1079 (comment), to clarify that this was really covered by the unit tests. I added an extra comment in the tests just to make that clearer :)

bitromortac · 2025-07-15T13:41:52Z

firewalldb/kvstores_kvdb.go

-			               -> feature-kv-stores -> feature-name -> {k:v}
+"rules" -> "perm" -> <rule-name> -> "global" -> {k:v}
+				 -> "session-kv-store" -> <group-ID> -> {k:v}
+							             -> "feature-kv-stores" -> <feature-name> -> {k:v}


just wanted to note that the whitespace still consists of tabs, but it's ok, as it was just small nit

We add a helper function to the functions that creates the test SQL stores, in order to ensure that the store is properly closed when the test is cleaned up.

In the upcoming migration of the firewall database to SQL, the helper functions that creates the test databases of different types, need to return a unified interface in order to not have to control the migration tests file by build tags. Therefore, we export the unified interface FirewallDBs, so that it can be returned public test DB creation functions

In the upcoming migration of the firewall database to SQL, the helper functions that creates the test databases of different types, need to return a unified interface in order to not have to control the migration tests file by build tags. Therefore, we update the `NewTestDB` functions to return the `FirewallDBs` interface instead of the specific store implementation type.

During the upcoming upcoming migration of the firewall database to SQL, we need to be able to check all kvstores records in the SQL database, to validate that the migration is successful in tests. This commits adds a query to list all kvstores records, which enables that functionality.

Rename the session_id to group_id in kvstores table in the SQL store, to better represent how the field is actually used. Note that this is a breaking change, and would normally require a new migration. But as the SQL store is not used in production, and only enabled under the dev build flag, we can rename it without a new migration, as there's no users of the SQL store in production.

During the migration of the kvstores to SQL, we'll iterate over the buckets in the bbolt database, which holds all kvstores records. In order to understand why the migration iterates over the buckets in the specific order, we need to clarify the bbolt kvstores illustration docs, so that it correctly reflects how the records are actually stored in the bbolt database.

This commit introduces the migration logic for transitioning the kvstores 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 · 2025-07-21T12:48:12Z

Rebased on master and changed sqldb.MigrationTxOptions to sqldb.WriteTxOpt() to make the PR compatible with the new lnd version.

ViktorTigerstrom changed the title ~~2025 05 migrate kvstores~~ [sql-41] firewalldb: add migration code for kvstores from kvdb to SQL May 27, 2025

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

ViktorTigerstrom self-assigned this May 27, 2025

ellemouton reviewed May 30, 2025

View reviewed changes

ViktorTigerstrom force-pushed the 2025-05-migrate-kvstores branch 2 times, most recently from 4151b93 to 7a517ad Compare June 3, 2025 18:22

ViktorTigerstrom requested a review from ellemouton June 3, 2025 23:01

ellemouton reviewed Jun 4, 2025

View reviewed changes

ViktorTigerstrom force-pushed the 2025-05-migrate-kvstores branch from 7a517ad to 10550d4 Compare June 9, 2025 12:20

ViktorTigerstrom requested a review from ellemouton June 9, 2025 12:35

ellemouton requested a review from bitromortac June 10, 2025 07:50

ViktorTigerstrom mentioned this pull request Jun 10, 2025

[sql-43] Add support for sqldb/v2 in litd #1085

Draft

bitromortac reviewed Jun 12, 2025

View reviewed changes

ellemouton reviewed Jun 13, 2025

View reviewed changes

ViktorTigerstrom mentioned this pull request Jun 17, 2025

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

Open

ViktorTigerstrom force-pushed the 2025-05-migrate-kvstores branch from 10550d4 to 7bb668e Compare July 1, 2025 23:29

ViktorTigerstrom requested review from ellemouton and bitromortac July 2, 2025 00:01

bitromortac reviewed Jul 4, 2025

View reviewed changes

ellemouton reviewed Jul 9, 2025

View reviewed changes

ViktorTigerstrom force-pushed the 2025-05-migrate-kvstores branch 2 times, most recently from 36e0f1c to c3d8ecf Compare July 9, 2025 21:27

ViktorTigerstrom requested review from ellemouton and bitromortac July 9, 2025 21:50

ellemouton approved these changes Jul 11, 2025

View reviewed changes

ViktorTigerstrom mentioned this pull request Jul 11, 2025

db: use native SQL #917

Open

45 tasks

bitromortac approved these changes Jul 15, 2025

View reviewed changes

ViktorTigerstrom force-pushed the 2025-05-migrate-kvstores branch from c3d8ecf to 3141357 Compare July 21, 2025 12:34

ViktorTigerstrom added 7 commits July 21, 2025 14:45

firewalldb: ensure that test SQL store is closed

e4f678b

We add a helper function to the functions that creates the test SQL stores, in order to ensure that the store is properly closed when the test is cleaned up.

firewalldb: add kvstores kvdb to SQL migration

1553888

This commit introduces the migration logic for transitioning the kvstores 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-05-migrate-kvstores branch from 3141357 to 1553888 Compare July 21, 2025 12:46

ViktorTigerstrom merged commit 55f5682 into lightninglabs:master Jul 21, 2025
21 of 22 checks passed

		@@ -14,29 +14,21 @@ var (
		ErrNoSuchKeyFound = fmt.Errorf("no such key found")
		)


		var pairs []*kvEntry

		// 1) Collect all key-value pairs from the KV store.

		ctx, "initial-session", session.Type(uint8(rand.Intn(5))),
		time.Unix(1000, 0), randomString(rand.Intn(10)+1),

[sql-41] firewalldb: add migration code for kvstores from kvdb to SQL #1079

[sql-41] firewalldb: add migration code for kvstores from kvdb to SQL #1079

Uh oh!

Conversation

ViktorTigerstrom commented May 27, 2025

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ViktorTigerstrom commented Jun 3, 2025

Uh oh!

ellemouton 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorTigerstrom commented Jun 9, 2025 •

edited

Loading