[sql-53] firewalldb: actions migration prep 2 - persist full macaroon root key ID in the SQL actions store #1146

ViktorTigerstrom · 2025-09-15T23:48:01Z

Part of #917

This PR addresses an issue I noticed we need to address prior to #1143:

When migrating the actions store from kvdb to sql, we will need to update the existing actions to include the full mac root key, instead of just the last 4 bytes (currently called MacaroonIdentifier). In order to do so, we need to change the sql implementation of the actions store to persist the full mac root key, instead of just the last 4 bytes. As no production data in the sql actions store exists for users yet, it's fine for us to change this without having to address old sql actions which only stored the last 4 bytes.

This PR adds this change.

Note though that this PR doesn't update the kvdb implementation, and the full macaroon root key will be ignored by the kvdb store even if set. Therefore, the rest of litd will still have to just expect the last 4 bytes of the mac root key when accessing an Action's MacaroonIdentifier. Therefore, we we currently never expose the rest of the mac root key outside of the sql actions store.

Once the kvdb store has been fully deprecated and removed, we can then update the rest of litd to also use the full mac root key, and change the Action struct's field to reflect this.

gemini-code-assist

Summary of Changes

Hello @ViktorTigerstrom, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request implements a foundational update to the SQL actions store, enabling it to record the complete 8-byte macaroon root key ID. This enhancement is a crucial prerequisite for the larger migration effort to transition the actions store from a KVDB to a SQL backend. By storing the full root key ID, the system gains improved accuracy and future-proofing for macaroon-related actions. The current design ensures backward compatibility by not altering the KVDB implementation, with plans to fully leverage the extended ID once the KVDB is phased out.

Highlights

Enhanced Macaroon ID Storage: The SQL actions store now persists the full 8-byte macaroon root key ID (uint64) instead of just the last 4 bytes, improving data fidelity for macaroon identification.
Migration Preparation: This change is a critical preparatory step for the ongoing migration of the actions store from KVDB to SQL, ensuring the SQL store is ready for more complete macaroon data.
Backward Compatibility: The KVDB implementation remains unchanged and will ignore the full root key ID, maintaining compatibility with existing systems until the KVDB store is fully deprecated.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request prepares for a database migration by changing the SQL actions store to persist the full 8-byte macaroon root key ID. The changes are mostly correct and well-tested, but I've found a critical issue in the data retrieval logic that would cause a panic. I've also suggested a minor refactoring in the test code to improve maintainability. Please see my detailed comments.

firewalldb/actions_test.go

The upcoming commit will update the `AddActionReq` struct to include an extra field which the `kvdb` actions store will ignore. Therefore the `assertEqualActions` for the `kvdb` version will need to be update to ignore this field. In preparation for that change, we also do another optimization of the `assertEqualActions` function under kvdb builds, to not mutate the passed action references.

When migrating the actions store from kvdb to sql, we will update the existing actions to include the full mac root key, instead of just the last 4 bytes (currently called `MacaroonIdentifier`). In order to do so, we change the sql implementation of the `actions` store to persist the full mac root key, instead of just the last 4 bytes. As no production data in the sql actions store exists for users yet, it's fine for us to change this without having to address old sql actions which only stored the last 4 bytes. Note though that we do not update the kvdb implementation, and the full macaroon root key will be ignored by the kvdb store even if set. Therefore, the rest of `litd` will still have to just expect the last 4 bytes of the mac root key when accessing an `Action`'s MacaroonIdentifier. Therefore, we we currently never expose the rest of the mac root key outside of the sql actions store. Once the kvdb store has been fully deprecated and removed, we can then update the rest of `litd` to also use the full mac root key, and change the `Action` struct's field to reflect this.

ellemouton · 2025-09-17T11:19:29Z

@ViktorTigerstrom - what's the main reasoning around storing the full thing?
iiuc, we just want to full thing at migration time to determine possible linked sessions/actions. But do we actually need the full thing to be persisted?

i guess no harm in doing so, but just checking understanding

ellemouton · 2025-09-17T11:26:00Z

firewalldb/test_kvdb.go

 	got.AccountID = expected.AccountID
 	require.Equal(t, expected, got)

-	got.AccountID = fn.None[accounts.AccountID]()
+	got.AccountID = actualAccountID


ellemouton · 2025-09-17T11:26:58Z

firewall/request_logger.go

@@ -7,6 +7,7 @@ import (
 	"sync"

 	"github.com/lightninglabs/lightning-terminal/firewalldb"
+	litd_macaroons "github.com/lightninglabs/lightning-terminal/macaroons"


suggest litmac?

ellemouton · 2025-09-17T11:28:02Z

firewall/request_logger.go

 		MacaroonIdentifier: macaroonID,
+		MacaroonRootKeyID:  rootKeyID,


ok and we need both cause kvdb still stores old identifier but now sql stores full rootkey id?

ellemouton · 2025-09-17T11:29:04Z

firewalldb/actions.go

 	// If no macaroon was used for the action, then this will not be set.
 	MacaroonIdentifier fn.Option[[4]byte]

+	// MacaroonRootKeyID is the uint64 / full 8 bytes of the root key ID of
+	// the macaroon used to perform the action.
+	// If no macaroon was used for the action, then this will not be set.
+	MacaroonRootKeyID fn.Option[uint64]
+
 	// SessionID holds the optional session ID of the session that this


related to above question, if one of these is stored in kvdb only and the other in sql only, lets add this warning in the comments

or actually, to keep the API intuitive, cant se instead only have the MacRootKeyID field here & remove the MacID field. Then extract the relevant format at the DB layer. ie, in the kvdb layer, take the root key ID and compute the mac ID.

ellemouton · 2025-09-17T11:31:18Z

firewalldb/actions_sql.go

-		macID = fn.Some([4]byte(dbAction.MacaroonIdentifier))
+	if len(dbAction.MacaroonIdentifier) >= 4 {
+		dbMacID := dbAction.MacaroonIdentifier
+		macID = fn.Some([4]byte(dbMacID[len(dbMacID)-4:]))
 	}


not populating the new field below? (see comment about only needing 1 field potentially anyways)

ViktorTigerstrom · 2025-09-17T17:00:28Z

Thanks for the review @ellemouton.

I'm answering this clarification first, as I believe that's the deciding factor regarding wether we want to go ahead of with this PR or not:

iiuc, we just want to full thing at migration time to determine possible linked sessions/actions. But do we actually need the full thing to be persisted?

So after working through the actions migration, it has become clear that the only real reason we'd need to persist the full macaroon root key id (8 bytes), compared to the short version (just the last 4 bytes), is to make it possible to link which exact macaroon was used to when the action was generated.
Otherwise, there can be the scenario that the user has generated 2 different macaroons where there's a collision for the last 4 bytes. In that scenario, the info is lost when looking at the action which macaroon was actually used to generate it. When we discussed this offline previously, we determined it to be worth it to persist the full macaroon root key id for this reason.

However, I'm not super firm that this protection is totally needed, and IMO it's worth doing for the the sake of it being more clean than opening up for the collision risk. If we do not care about that collision protection however, I don't think we actually need to determine the full root key ID during the migration time as it's not really needed to link possible linked sessions/actions. If so, we can drop the whole short mac key ID -> full root key mapping in the actions migration.

Let me know if you think we should proceed with this, or just not try to solve the collision issue :)

ViktorTigerstrom · 2025-09-18T11:07:50Z

I've realized one scenario where persisting the full root key ID miiiiight be useful is if the user deletes their lnd macaroon DB. Right now, we only store 4 bytes of the 8-byte root key ID for the action, which would mean the first 4 bytes of the root key id used for the action would be lost permanently for the user. If we persisted all 8 bytes, the user could still recover all 8 bytes. With the full ID, they’d be able to pass that full root_key_id into lncli bakemacaroon and generate a new macaroon.

Not sure if there are many scenarios where this would be useful though :)

ellemouton · 2025-09-19T05:10:42Z

Thanks for the info @ViktorTigerstrom - i think yes it makes sense to store the full thing 🙏

ViktorTigerstrom self-assigned this Sep 15, 2025

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

gemini-code-assist bot reviewed Sep 15, 2025

View reviewed changes

firewalldb/actions_test.go Outdated Show resolved Hide resolved

ViktorTigerstrom force-pushed the 2025-09-actions-sql-store-persits-mac-rootkey-id branch from 35ecb9c to 37fa599 Compare September 15, 2025 23:53

ViktorTigerstrom force-pushed the 2025-09-actions-sql-store-persits-mac-rootkey-id branch from 37fa599 to 87cf67e Compare September 16, 2025 09:44

ViktorTigerstrom requested review from ellemouton and bitromortac September 16, 2025 09:45

ellemouton reviewed Sep 17, 2025

View reviewed changes

ViktorTigerstrom mentioned this pull request Sep 18, 2025

[sql-52] firewalldb: add migration code for actions from kvdb to SQL #1143

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[sql-53] firewalldb: actions migration prep 2 - persist full macaroon root key ID in the SQL actions store #1146

[sql-53] firewalldb: actions migration prep 2 - persist full macaroon root key ID in the SQL actions store #1146

ViktorTigerstrom commented Sep 15, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

ellemouton commented Sep 17, 2025

Uh oh!

ellemouton Sep 17, 2025

Uh oh!

ellemouton Sep 17, 2025

Uh oh!

ellemouton Sep 17, 2025

Uh oh!

ellemouton Sep 17, 2025

Uh oh!

ellemouton Sep 17, 2025

Uh oh!

ellemouton Sep 17, 2025

Uh oh!

ViktorTigerstrom commented Sep 17, 2025

Uh oh!

ViktorTigerstrom commented Sep 18, 2025

Uh oh!

ellemouton commented Sep 19, 2025

Uh oh!

Uh oh!

		MacaroonIdentifier: macaroonID,
		MacaroonRootKeyID: rootKeyID,

[sql-53] firewalldb: actions migration prep 2 - persist full macaroon root key ID in the SQL actions store #1146

Are you sure you want to change the base?

[sql-53] firewalldb: actions migration prep 2 - persist full macaroon root key ID in the SQL actions store #1146

Conversation

ViktorTigerstrom commented Sep 15, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

ellemouton commented Sep 17, 2025

Uh oh!

ellemouton Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

ellemouton Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

ellemouton Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

ellemouton Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

ellemouton Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

ellemouton Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

ViktorTigerstrom commented Sep 17, 2025

Uh oh!

ViktorTigerstrom commented Sep 18, 2025

Uh oh!

ellemouton commented Sep 19, 2025

Uh oh!

Uh oh!