Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+83 −9
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f7c6fc
firewalldb: update kvdb `assertEqualActions`
ViktorTigerstrom Sep 10, 2025
87cf67e
multi: persist full mac root key in sql actions db
ViktorTigerstrom Sep 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions firewall/request_logger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"sync"

"github.com/lightninglabs/lightning-terminal/firewalldb"
litd_macaroons "github.com/lightninglabs/lightning-terminal/macaroons"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest litmac?

mid "github.com/lightninglabs/lightning-terminal/rpcmiddleware"
"github.com/lightninglabs/lightning-terminal/session"
"github.com/lightningnetwork/lnd/fn"
Expand Down Expand Up @@ -182,21 +183,33 @@ func (r *RequestLogger) Intercept(ctx context.Context,
func (r *RequestLogger) addNewAction(ctx context.Context, ri *RequestInfo,
withPayloadData bool) error {

var macaroonID fn.Option[[4]byte]
var (
rootKeyID fn.Option[uint64]
macaroonID fn.Option[[4]byte]
)

if ri.Macaroon != nil {
var err error
macID, err := session.IDFromMacaroon(ri.Macaroon)

fullRootKeyID, err := litd_macaroons.RootKeyIDFromMacaroon(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

ri.Macaroon,
)
if err != nil {
return fmt.Errorf("could not extract ID from macaroon")
return fmt.Errorf("could not extract root key ID from "+
"macaroon: %w", err)
}

macID := session.IDFromMacRootKeyID(fullRootKeyID)

rootKeyID = fn.Some(fullRootKeyID)
macaroonID = fn.Some([4]byte(macID))
}

actionReq := &firewalldb.AddActionReq{
SessionID: ri.SessionID,
AccountID: ri.AccountID,
MacaroonIdentifier: macaroonID,
MacaroonRootKeyID: rootKeyID,
Comment on lines 211 to +212
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

RPCMethod: ri.URI,
}

Expand Down
5 changes: 5 additions & 0 deletions firewalldb/actions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ type AddActionReq struct {
// 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
Comment on lines 39 to 47
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// action was performed with.
//
Expand Down
20 changes: 16 additions & 4 deletions firewalldb/actions_sql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package firewalldb
import (
"context"
"database/sql"
"encoding/binary"
"errors"
"fmt"
"math"
Expand Down Expand Up @@ -140,8 +141,11 @@ func (s *SQLDB) AddAction(ctx context.Context,
}

var macID []byte
req.MacaroonIdentifier.WhenSome(func(id [4]byte) {
macID = id[:]
req.MacaroonRootKeyID.WhenSome(func(rootKeyID uint64) {
rootKeyBytes := make([]byte, 8)
binary.BigEndian.PutUint64(rootKeyBytes[:], rootKeyID)

macID = rootKeyBytes
})

id, err := db.InsertAction(ctx, sqlc.InsertActionParams{
Expand Down Expand Up @@ -393,9 +397,17 @@ func unmarshalAction(ctx context.Context, db SQLActionQueries,
legacyAcctID = fn.Some(acctID)
}

// While we store the full 8 byte macaroon root key ID in the sql
// actions DB, the kvdb version only stored the last 4 bytes. So
// we'll only return that here to maintain compatibility with any
// existing callers.
//
// TODO(viktor): Remove this when we no longer need to be compatible
// with the kvdb version.
var macID fn.Option[[4]byte]
if len(dbAction.MacaroonIdentifier) > 0 {
macID = fn.Some([4]byte(dbAction.MacaroonIdentifier))
if len(dbAction.MacaroonIdentifier) >= 4 {
dbMacID := dbAction.MacaroonIdentifier
macID = fn.Some([4]byte(dbMacID[len(dbMacID)-4:]))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return &Action{
Expand Down
27 changes: 27 additions & 0 deletions firewalldb/actions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package firewalldb

import (
"context"
"encoding/binary"
"fmt"
"testing"
"time"
Expand Down Expand Up @@ -60,10 +61,13 @@ func TestActionStorage(t *testing.T) {
acct1, err := accountsDB.NewAccount(ctx, 0, time.Time{}, "foo")
require.NoError(t, err)

sess1RootKeyID := macIDToRootKeyID(sess1.ID)

action1Req := &AddActionReq{
SessionID: fn.Some(sess1.ID),
AccountID: fn.Some(acct1.ID),
MacaroonIdentifier: fn.Some([4]byte(sess1.ID)),
MacaroonRootKeyID: fn.Some(sess1RootKeyID),
ActorName: "Autopilot",
FeatureName: "auto-fees",
Trigger: "fee too low",
Expand All @@ -79,9 +83,12 @@ func TestActionStorage(t *testing.T) {
State: ActionStateDone,
}

sess2RootKeyID := macIDToRootKeyID(sess2.ID)

action2Req := &AddActionReq{
SessionID: fn.Some(sess2.ID),
MacaroonIdentifier: fn.Some([4]byte(sess2.ID)),
MacaroonRootKeyID: fn.Some(sess2RootKeyID),
ActorName: "Autopilot",
FeatureName: "rebalancer",
Trigger: "channels not balanced",
Expand Down Expand Up @@ -213,8 +220,11 @@ func TestListActions(t *testing.T) {
addAction := func(sessionID [4]byte) {
actionIds++

sessRootKeyID := macIDToRootKeyID(sessionID)

actionReq := &AddActionReq{
MacaroonIdentifier: fn.Some(sessionID),
MacaroonRootKeyID: fn.Some(sessRootKeyID),
ActorName: "Autopilot",
FeatureName: fmt.Sprintf("%d", actionIds),
Trigger: "fee too low",
Expand Down Expand Up @@ -424,9 +434,12 @@ func TestListGroupActions(t *testing.T) {
)
require.NoError(t, err)

sess1RootKeyID := macIDToRootKeyID(sess1.ID)

action1Req := &AddActionReq{
SessionID: fn.Some(sess1.ID),
MacaroonIdentifier: fn.Some([4]byte(sess1.ID)),
MacaroonRootKeyID: fn.Some(sess1RootKeyID),
ActorName: "Autopilot",
FeatureName: "auto-fees",
Trigger: "fee too low",
Expand All @@ -442,9 +455,12 @@ func TestListGroupActions(t *testing.T) {
State: ActionStateDone,
}

sess2RootKeyID := macIDToRootKeyID(sess2.ID)

action2Req := &AddActionReq{
SessionID: fn.Some(sess2.ID),
MacaroonIdentifier: fn.Some([4]byte(sess2.ID)),
MacaroonRootKeyID: fn.Some(sess2RootKeyID),
ActorName: "Autopilot",
FeatureName: "rebalancer",
Trigger: "channels not balanced",
Expand Down Expand Up @@ -501,3 +517,14 @@ func TestListGroupActions(t *testing.T) {
assertEqualActions(t, action2, al[0])
assertEqualActions(t, action1, al[1])
}

// macIDToRootKeyID is a helper function for tests that converts a 4 byte
// macaroon ID to a full macaroon root key ID by padding it to 8 bytes.
// Note that the first 4 bytes of the returned root key ID will be zeros,
// followed by the 4 bytes passed in.
func macIDToRootKeyID(macID [4]byte) uint64 {
rootKeyIDBytes := make([]byte, 8)
copy(rootKeyIDBytes[4:], macID[:])

return binary.BigEndian.Uint64(rootKeyIDBytes)
}
11 changes: 9 additions & 2 deletions firewalldb/test_kvdb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package firewalldb
import (
"testing"

"github.com/lightninglabs/lightning-terminal/accounts"
"github.com/lightninglabs/lightning-terminal/session"
"github.com/lightningnetwork/lnd/clock"
"github.com/lightningnetwork/lnd/fn"
Expand Down Expand Up @@ -59,8 +58,16 @@ func newDBFromPathWithSessions(t *testing.T, dbPath string,

func assertEqualActions(t *testing.T, expected, got *Action) {
// Accounts are not explicitly linked in our bbolt DB implementation.
actualAccountID := got.AccountID
got.AccountID = expected.AccountID

// As the kvdb implementation doesn't store the Macaroon Root Key ID,
// we clear the expected value before comparison, and restore it after.
expectedMacRootKey := expected.MacaroonRootKeyID
expected.MacaroonRootKeyID = fn.None[uint64]()

require.Equal(t, expected, got)

got.AccountID = fn.None[accounts.AccountID]()
got.AccountID = actualAccountID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

expected.MacaroonRootKeyID = expectedMacRootKey
}
10 changes: 10 additions & 0 deletions firewalldb/test_sql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/lightninglabs/lightning-terminal/db"
"github.com/lightninglabs/lightning-terminal/session"
"github.com/lightningnetwork/lnd/clock"
"github.com/lightningnetwork/lnd/fn"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -46,11 +47,20 @@ func assertEqualActions(t *testing.T, expected, got *Action) {
expected.AttemptedAt = time.Time{}
got.AttemptedAt = time.Time{}

// As the kvdb implementation doesn't store the Macaroon Root Key ID,
// we don't yet expose this for the sql version, until the kvdb version
// has been deprecated and removed. Therefore, we ignore this field in
// our comparison here, and clear the expected value before comparison,
// and restore it after.
expectedMacRootKey := expected.MacaroonRootKeyID
expected.MacaroonRootKeyID = fn.None[uint64]()

require.Equal(t, expected, got)
require.Equal(t, expectedAttemptedAt.Unix(), actualAttemptedAt.Unix())

expected.AttemptedAt = expectedAttemptedAt
got.AttemptedAt = actualAttemptedAt
expected.MacaroonRootKeyID = expectedMacRootKey
}

// createStore is a helper function that creates a new SQLDB and ensure that
Expand Down
Loading