From 9fea5380abde7eba36048f76f45f2748346c1e47 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Thu, 22 May 2025 13:40:05 +0200 Subject: [PATCH 1/4] firewalldb: fix missing `ctx` param in actions test --- firewalldb/actions_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firewalldb/actions_test.go b/firewalldb/actions_test.go index 0f93ed2fe..fb0cfc414 100644 --- a/firewalldb/actions_test.go +++ b/firewalldb/actions_test.go @@ -161,7 +161,7 @@ func TestActionStorage(t *testing.T) { // Check that providing no session id and no filter function returns // all the actions. - actions, _, _, err = db.ListActions(nil, &ListActionsQuery{ + actions, _, _, err = db.ListActions(ctx, &ListActionsQuery{ IndexOffset: 0, MaxNum: 100, Reversed: false, From b99a4f8fa2ffee8945d725e0c086d4c552376b31 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Tue, 27 May 2025 09:26:09 +0200 Subject: [PATCH 2/4] firewall+firewalldb: move mac ID serialisation to kvdb impl For our kvdb firewalldb, we use an empty 4 byte array as the macaroon identifier even if no macaroon was used to create the action. This is so that we have some sort of "session ID" bucket to store these set of actions under. For our SQL impl, however, this is not needed and we will likely just use a nullable field for the macaroon ID. So in preparation for this, we move the kvdb specific logic to the kvdb impl. --- firewall/request_logger.go | 10 +++++----- firewalldb/actions.go | 3 ++- firewalldb/actions_kvdb.go | 13 ++++++++++--- firewalldb/actions_test.go | 16 +++++++++------- session_rpcserver.go | 7 ++++++- 5 files changed, 32 insertions(+), 17 deletions(-) diff --git a/firewall/request_logger.go b/firewall/request_logger.go index 8b038a6b6..b28b1092a 100644 --- a/firewall/request_logger.go +++ b/firewall/request_logger.go @@ -9,6 +9,7 @@ import ( "github.com/lightninglabs/lightning-terminal/firewalldb" mid "github.com/lightninglabs/lightning-terminal/rpcmiddleware" "github.com/lightninglabs/lightning-terminal/session" + "github.com/lightningnetwork/lnd/fn" "github.com/lightningnetwork/lnd/lnrpc" "github.com/lightningnetwork/lnd/macaroons" ) @@ -181,16 +182,15 @@ func (r *RequestLogger) Intercept(ctx context.Context, func (r *RequestLogger) addNewAction(ctx context.Context, ri *RequestInfo, withPayloadData bool) error { - // If no macaroon is provided, then an empty 4-byte array is used as the - // macaroon ID. Otherwise, the last 4 bytes of the macaroon's root key - // ID are used. - var macaroonID [4]byte + var macaroonID fn.Option[[4]byte] if ri.Macaroon != nil { var err error - macaroonID, err = session.IDFromMacaroon(ri.Macaroon) + macID, err := session.IDFromMacaroon(ri.Macaroon) if err != nil { return fmt.Errorf("could not extract ID from macaroon") } + + macaroonID = fn.Some([4]byte(macID)) } actionReq := &firewalldb.AddActionReq{ diff --git a/firewalldb/actions.go b/firewalldb/actions.go index 88172e31b..1d0c8c36f 100644 --- a/firewalldb/actions.go +++ b/firewalldb/actions.go @@ -36,7 +36,8 @@ const ( type AddActionReq struct { // MacaroonIdentifier is a 4 byte identifier created from the last 4 // bytes of the root key ID of the macaroon used to perform the action. - MacaroonIdentifier [4]byte + // If no macaroon was used for the action, then this will not be set. + MacaroonIdentifier fn.Option[[4]byte] // SessionID holds the optional session ID of the session that this // action was performed with. diff --git a/firewalldb/actions_kvdb.go b/firewalldb/actions_kvdb.go index 7a8ae3e15..dbe3f7b26 100644 --- a/firewalldb/actions_kvdb.go +++ b/firewalldb/actions_kvdb.go @@ -58,6 +58,13 @@ var ( func (db *BoltDB) AddAction(ctx context.Context, req *AddActionReq) (ActionLocator, error) { + // If no macaroon is provided, then an empty 4-byte array is used as the + // macaroon ID. + var macaroonID [4]byte + req.MacaroonIdentifier.WhenSome(func(id [4]byte) { + macaroonID = id + }) + // If the new action links to a session, the session must exist. // For the bbolt impl of the store, this is our best effort attempt // at ensuring each action links to a session. If the session is @@ -105,7 +112,7 @@ func (db *BoltDB) AddAction(ctx context.Context, } sessBucket, err := actionsBucket.CreateBucketIfNotExists( - action.MacaroonIdentifier[:], + macaroonID[:], ) if err != nil { return err @@ -134,7 +141,7 @@ func (db *BoltDB) AddAction(ctx context.Context, } locator = kvdbActionLocator{ - sessionID: action.MacaroonIdentifier, + sessionID: macaroonID, actionID: nextActionIndex, } @@ -574,7 +581,7 @@ func DeserializeAction(r io.Reader, sessionID session.ID) (*Action, error) { return nil, err } - action.MacaroonIdentifier = sessionID + action.MacaroonIdentifier = fn.Some([4]byte(sessionID)) action.SessionID = fn.Some(sessionID) action.ActorName = string(actor) action.FeatureName = string(featureName) diff --git a/firewalldb/actions_test.go b/firewalldb/actions_test.go index fb0cfc414..62a628c85 100644 --- a/firewalldb/actions_test.go +++ b/firewalldb/actions_test.go @@ -67,7 +67,7 @@ func TestActionStorage(t *testing.T) { action1Req := &AddActionReq{ SessionID: fn.Some(sess1.ID), AccountID: fn.Some(acct1.ID), - MacaroonIdentifier: sess1.ID, + MacaroonIdentifier: fn.Some([4]byte(sess1.ID)), ActorName: "Autopilot", FeatureName: "auto-fees", Trigger: "fee too low", @@ -85,7 +85,7 @@ func TestActionStorage(t *testing.T) { action2Req := &AddActionReq{ SessionID: fn.Some(sess2.ID), - MacaroonIdentifier: sess2.ID, + MacaroonIdentifier: fn.Some([4]byte(sess2.ID)), ActorName: "Autopilot", FeatureName: "rebalancer", Trigger: "channels not balanced", @@ -223,7 +223,7 @@ func TestListActions(t *testing.T) { actionIds++ actionReq := &AddActionReq{ - MacaroonIdentifier: sessionID, + MacaroonIdentifier: fn.Some(sessionID), ActorName: "Autopilot", FeatureName: fmt.Sprintf("%d", actionIds), Trigger: "fee too low", @@ -245,9 +245,11 @@ func TestListActions(t *testing.T) { assertActions := func(dbActions []*Action, al []*action) { require.Len(t, dbActions, len(al)) for i, a := range al { - require.EqualValues( - t, a.sessionID, dbActions[i].MacaroonIdentifier, + mID, err := dbActions[i].MacaroonIdentifier.UnwrapOrErr( + fmt.Errorf("macaroon identifier is none"), ) + require.NoError(t, err) + require.EqualValues(t, a.sessionID, mID) require.Equal(t, a.actionID, dbActions[i].FeatureName) } } @@ -433,7 +435,7 @@ func TestListGroupActions(t *testing.T) { action1Req := &AddActionReq{ SessionID: fn.Some(sess1.ID), - MacaroonIdentifier: sess1.ID, + MacaroonIdentifier: fn.Some([4]byte(sess1.ID)), ActorName: "Autopilot", FeatureName: "auto-fees", Trigger: "fee too low", @@ -451,7 +453,7 @@ func TestListGroupActions(t *testing.T) { action2Req := &AddActionReq{ SessionID: fn.Some(sess2.ID), - MacaroonIdentifier: sess2.ID, + MacaroonIdentifier: fn.Some([4]byte(sess2.ID)), ActorName: "Autopilot", FeatureName: "rebalancer", Trigger: "channels not balanced", diff --git a/session_rpcserver.go b/session_rpcserver.go index 9baf4aa5c..cb57b847b 100644 --- a/session_rpcserver.go +++ b/session_rpcserver.go @@ -806,9 +806,14 @@ func (s *sessionRpcServer) ListActions(ctx context.Context, sessionID = id }) + var macID [4]byte + a.MacaroonIdentifier.WhenSome(func(id [4]byte) { + macID = id + }) + resp[i] = &litrpc.Action{ SessionId: sessionID[:], - MacaroonIdentifier: a.MacaroonIdentifier[:], + MacaroonIdentifier: macID[:], ActorName: a.ActorName, FeatureName: a.FeatureName, Trigger: a.Trigger, From e40328c931ca06bb16d8de96ebc29170d9e68ed1 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Tue, 27 May 2025 14:52:54 +0200 Subject: [PATCH 3/4] firewalldb: let test times have intuitive order testTime1 should be before testTime2 otherwise it can lead to confusion. --- firewalldb/actions_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/firewalldb/actions_test.go b/firewalldb/actions_test.go index 62a628c85..ea47d3985 100644 --- a/firewalldb/actions_test.go +++ b/firewalldb/actions_test.go @@ -14,8 +14,8 @@ import ( ) var ( - testTime1 = time.Unix(32100, 0) - testTime2 = time.Unix(12300, 0) + testTime1 = time.Unix(12300, 0) + testTime2 = time.Unix(32100, 0) ) // TestActionStorage tests that the ActionsListDB CRUD logic. From ed96f9356e7f7af41c7ffbd40050bdd1360b6e89 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Tue, 27 May 2025 14:56:30 +0200 Subject: [PATCH 4/4] firewalldb: fix BBolt ListGroupActions when Reversed is set Add a test covering a call to ListActions with group ID set and Reversed set and assert that the actions are returned in the correct order. This reveals a bug in the BoltDB implementation which was not making use of the query.Reversed parameter when group ID is set. NOTE: it still wont do pagination correctly (ie the pagination params still dont get used) but we at least here can easily let it use the Reversed param. --- firewalldb/actions_kvdb.go | 25 ++++++++++++++++++++----- firewalldb/actions_test.go | 12 +++++++++++- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/firewalldb/actions_kvdb.go b/firewalldb/actions_kvdb.go index dbe3f7b26..b2dddd36c 100644 --- a/firewalldb/actions_kvdb.go +++ b/firewalldb/actions_kvdb.go @@ -330,7 +330,13 @@ func (db *BoltDB) ListActions(ctx context.Context, query *ListActionsQuery, ) } if opts.groupID != session.EmptyID { - actions, err := db.listGroupActions(ctx, opts.groupID, filterFn) + var reversed bool + if query != nil { + reversed = query.Reversed + } + actions, err := db.listGroupActions( + ctx, opts.groupID, filterFn, reversed, + ) if err != nil { return nil, 0, 0, err } @@ -439,11 +445,11 @@ func (db *BoltDB) listSessionActions(sessionID session.ID, // // TODO: update to allow for pagination. func (db *BoltDB) listGroupActions(ctx context.Context, groupID session.ID, - filterFn listActionsFilterFn) ([]*Action, error) { + filterFn listActionsFilterFn, reversed bool) ([]*Action, error) { if filterFn == nil { filterFn = func(a *Action, reversed bool) (bool, bool) { - return true, true + return true, reversed } } @@ -482,9 +488,18 @@ func (db *BoltDB) listGroupActions(ctx context.Context, groupID session.ID, return err } - include, cont := filterFn(action, false) + include, cont := filterFn(action, reversed) if include { - actions = append(actions, action) + if !reversed { + actions = append( + actions, action, + ) + } else { + actions = append( + []*Action{action}, + actions..., + ) + } } if !cont { diff --git a/firewalldb/actions_test.go b/firewalldb/actions_test.go index ea47d3985..a355bede0 100644 --- a/firewalldb/actions_test.go +++ b/firewalldb/actions_test.go @@ -497,12 +497,22 @@ func TestListGroupActions(t *testing.T) { _, err = db.AddAction(ctx, action2Req) require.NoError(t, err) - // There should now be actions in the group. + // There should now be two actions in the group. al, _, _, err = db.ListActions(ctx, nil, WithActionGroupID(group1)) require.NoError(t, err) require.Len(t, al, 2) assertEqualActions(t, action1, al[0]) assertEqualActions(t, action2, al[1]) + + // Try the reversed query too. + al, _, _, err = db.ListActions( + ctx, &ListActionsQuery{Reversed: true}, + WithActionGroupID(group1), + ) + require.NoError(t, err) + require.Len(t, al, 2) + assertEqualActions(t, action2, al[0]) + assertEqualActions(t, action1, al[1]) } func assertEqualActions(t *testing.T, expected, got *Action) {