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

ViktorT-11 · 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

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

firewalldb/test_kvdb.go

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

firewall/request_logger.go

 	"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?

Correct, the kvdb store will only store the 4 byte MacaroonID, and not the full 8 bytes. Therefore all callsites that uses the persisted AddActionReq will still only use the MacaroonIdentifier still.

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.

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.

Hmmm I'm not sure if I misunderstand you here, but as this is also used as the return value for an action, we would need to set the MacRootKeyID when returning an kvdb action if we were to remove the MacaroonIdentifier field. However, since we can't regenerated the full MacRootKeyID from just the MacaroonIdentifier that wouldn't be possible. OR am I misunderstanding your suggestion?

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

Good idea. I added a warning to the MacaroonRootKeyID that it will not be set for kvdb database backends.

Potentially, we could make the struct returned from a persisted action, different from the struct used when persisting a new action. I.e. not use the AddActionReq struct for both.

Perhaps you could move MacaroonIdentifier from AddActionReq to Action and keep the full root key unpopulated on retrieval?

we would need to set the MacRootKeyID when returning an kvdb action if we were to remove the MacaroonIdentifier field

but we only ever use 4 of the bytes right? and i think we only ever use it in ListActions for the RPC?
so could potentially just let the bbolt impl append 4 zero bytes to whatever it has stored in order to construct the full rootkeyId?.

bitromortac's idea is also good - we can also just completely remove the Req struct from being embedded in the action. We could rather just have an ActionsInfo struct which both the ActionsReq and Actions structs can embed. then it makes things clearer and doesnt require that we store everything that we needed to construct the ActionsReq.

Anyways, gonna leave this up to you - but i do think it makes things easier to read. So leaving as non-blocking comment

so could potentially just let the bbolt impl append 4 zero bytes to whatever it has stored in order to construct the full rootkeyId?.

Ah good idea! I actually liked that version most, so I went a head and implemented that. I therefore also added a last commit that removes the MacaroonIdentifier field. If we want to further change the structure of the Action & AddActionReq struct, I'd suggest we take that in a future follow-up once we've decided how we'd like it structured :)

Those changes look good! Just a small suggestion to add a method to cut down on code repetition: patch.txt.

Cool thanks 🙏! Applied the patch as an extra helper function commit :)

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

firewalldb/actions_sql.go

+	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)

Idid not set the field previously to currently only have one field set when reading a persisted action, to make it as little confusing as possible at callsites, and ensure no callsites used the MacaroonRootKeyID field yet. But I agree that this potentially even makes it more confusing. So therefore changed so that we set the MacaroonRootKeyID field when reading persisted actions on a an sql databasebackend :)

ViktorT-11 · 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 :)

ViktorT-11 · 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 🙏

bitromortac · 2025-09-19T10:23:53Z

firewall/request_logger.go

 		var err error
-		macID, err := session.IDFromMacaroon(ri.Macaroon)
+
+		fullRootKeyID, err := litd_macaroons.RootKeyIDFromMacaroon(


Still a bit unsure why it's needed, looking at buildSession, we derive the full root key id from the last four bytes of the local pubkey and a common prefix, see NewSuperMacaroonRootKeyID. So it seems like we can always reconstruct the full macaroon root key id from the four bytes that are stored?

Thanks a lot for the deep investigation! Unfortunately though, s also communicated offline we an have actions listed in litd which have a different prefix than the SuperMacaroonRootKeyPrefix.
If you run litd with the firewall.request-logger.level=all config option set, any calls using the pure lnd admin.macaroon, will log actions that start with bytes of just zeros.
Additionally, if users have created a macaroon with lncli bakemacaroon command and set a value for the root_key_id param that uses another prefix than the SuperMacaroonRootKeyPrefix, calls using that macaroon will also use another prefix than the SuperMacaroonRootKeyPrefix.
I do however think that no macaroons that were used for litd sessions nor accounts should ever start with any other prefix than the SuperMacaroonRootKeyPrefix.

We therefore decided to go ahead with this PR.

bitromortac · 2025-09-22T15:53:38Z

firewalldb/actions_test.go

 	acct1, err := accountsDB.NewAccount(ctx, 0, time.Time{}, "foo")
 	require.NoError(t, err)

+	sess1RootKeyID := macIDToRootKeyID(sess1.ID)


so here we could use NewSuperMacaroonRootKeyID instead?

Thanks, updated to use your suggestion!

ViktorT-11 · 2025-09-26T19:59:17Z

Thanks a lot for the reviews @ellemouton & @bitromortac! I've addressed your feedback in the best way I could. I do agree that it's a bit confusing that there's both a MacaroonIdentifier & a MacaroonRootKeyID in the AddActionReq struct. I did not find a way to remove one of them though, so if you have feedback on how we'd do so, that'd be much appreciated :)

ellemouton

🙏

ViktorT-11 · 2025-10-01T13:51:50Z

Thanks for the review @ellemouton! Addressed your latest feedback with:
#1146 (comment)

bitromortac

LGTM 🎉 (just have a minor suggestion to cut down code repetition a bit)

bitromortac · 2025-10-01T14:38:26Z

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


Those changes look good! Just a small suggestion to add a method to cut down on code repetition: patch.txt.

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 since old actions stored in the kvdb implementation only have the last 4 bytes of the mac root key persisted, we will only ever persist the last 4 byte of the mac root key ID for kvdb actions. When the actions are later read back from the kvdb store, the first 4 bytes of the mac root key ID will be padded with zeroes to make up the full 8 bytes. As no call site currently utilizes the full 8 bytes of the mac root key ID, this is okay for now. When we later deprecate and remove the kvdb implementation, we can then update the rest of `litd` to also use the full mac root key ID.

As the `MacaroonRootKeyID` field of the `AddActionReq` struct also contains the 4 bytes of the `MacaroonIdentifier`, we change all call sites to instead use the last 4 bytes of the `MacaroonRootKeyID` field. As the `MacaroonIdentifier` field therefore becomes redundant, we also remove it.

Add helper method to `AddActionReq` returns the 4 byte macaroon ID that is derived from the MacaroonRootKeyID. Using the helper removes some code repetition at call sites, and makes the intended usage clearer.

ViktorT-11 self-assigned this Sep 15, 2025

ViktorT-11 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

ViktorT-11 force-pushed the 2025-09-actions-sql-store-persits-mac-rootkey-id branch 2 times, most recently from 37fa599 to 87cf67e Compare September 16, 2025 09:44

ViktorT-11 requested review from bitromortac and ellemouton September 16, 2025 09:45

ellemouton reviewed Sep 17, 2025

View reviewed changes

ViktorT-11 mentioned this pull request Sep 18, 2025

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

Merged

bitromortac reviewed Sep 19, 2025

View reviewed changes

bitromortac reviewed Sep 22, 2025

View reviewed changes

ViktorT-11 force-pushed the 2025-09-actions-sql-store-persits-mac-rootkey-id branch 2 times, most recently from b805fb4 to df09427 Compare September 26, 2025 19:56

ViktorT-11 requested review from bitromortac and ellemouton September 26, 2025 19:59

ellemouton approved these changes Sep 29, 2025

View reviewed changes

ViktorT-11 force-pushed the 2025-09-actions-sql-store-persits-mac-rootkey-id branch from df09427 to 0c9ef5a Compare October 1, 2025 13:49

bitromortac approved these changes Oct 1, 2025

View reviewed changes

ViktorT-11 added 4 commits October 3, 2025 11:43

multi: add AddActionReq MacaroonId helper func

bd59605

Add helper method to `AddActionReq` returns the 4 byte macaroon ID that is derived from the MacaroonRootKeyID. Using the helper removes some code repetition at call sites, and makes the intended usage clearer.

ViktorT-11 force-pushed the 2025-09-actions-sql-store-persits-mac-rootkey-id branch from 0c9ef5a to bd59605 Compare October 3, 2025 09:44

ViktorT-11 merged commit c2c63cc into lightninglabs:master Oct 3, 2025
21 of 22 checks passed

		MacaroonIdentifier: macaroonID,
		MacaroonRootKeyID: rootKeyID,

[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

Conversation

ViktorT-11 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!

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!

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!

ViktorT-11 commented Sep 17, 2025

Uh oh!

ViktorT-11 commented Sep 18, 2025

Uh oh!

ellemouton commented Sep 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorT-11 Sep 26, 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!

ViktorT-11 commented Sep 26, 2025

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

ViktorT-11 commented Oct 1, 2025

Uh oh!

bitromortac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

ViktorT-11 Sep 26, 2025 •

edited

Loading