Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 16 additions & 0 deletions accounts/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@ type AccountInvoices map[lntypes.Hash]struct{}
// AccountPayments is the set of payments that are associated with an account.
type AccountPayments map[lntypes.Hash]*PaymentEntry

// AccountPaymentEntry wraps a payment hash with its entry details.
type AccountPaymentEntry struct {
Hash lntypes.Hash
*PaymentEntry
}

// OffChainBalanceAccount holds all information that is needed to keep track of
// a user's off-chain account balance. This balance can only be spent by paying
// invoices.
Expand Down Expand Up @@ -279,6 +285,16 @@ type Store interface {
DeleteAccountPayment(_ context.Context, id AccountID,
hash lntypes.Hash) error

// ListAccountPayments returns a paginated list of payments
// associated with the given account, sorted in ascending
// lexicographical order of their payment hash.
ListAccountPayments(ctx context.Context, id AccountID, offset,
limit int32) ([]*AccountPaymentEntry, error)

// CountAccountPayments returns the total number of payments associated
// with the given account.
CountAccountPayments(ctx context.Context, id AccountID) (uint64, error)

// RemoveAccount finds an account by its ID and removes it from the¨
// store.
RemoveAccount(ctx context.Context, id AccountID) error
Expand Down
245 changes: 245 additions & 0 deletions accounts/rpcserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,43 @@ import (
"encoding/hex"
"errors"
"fmt"
"sync"
"time"

"github.com/btcsuite/btcd/btcutil"
"github.com/lightninglabs/lightning-terminal/litrpc"
litmac "github.com/lightninglabs/lightning-terminal/macaroons"
"github.com/lightningnetwork/lnd/lnrpc"
"github.com/lightningnetwork/lnd/lnrpc/routerrpc"
"github.com/lightningnetwork/lnd/lntypes"
"github.com/lightningnetwork/lnd/lnwire"
"github.com/lightningnetwork/lnd/macaroons"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"gopkg.in/macaroon-bakery.v2/bakery/checkers"
"gopkg.in/macaroon.v2"
)

const (
// DefaultMaxPayments is the default page size for listing payments.
DefaultMaxPayments = 20

// MaxPaymentsLimit is the maximum page size allowed for listing
// payments.
MaxPaymentsLimit = 50

// MaxIndexOffset is the maximum index offset allowed.
MaxIndexOffset = 0x7fffffff

// DefaultTrackPaymentTimeout is the timeout for track payment RPC
// calls.
DefaultTrackPaymentTimeout = 8 * time.Second

// MaxConcurrentTrackPayments is the maximum number of concurrent track
// payment requests.
MaxConcurrentTrackPayments = 10
)

var (
// ErrServerNotActive indicates that the server has started but hasn't
// fully finished the startup process.
Expand Down Expand Up @@ -371,3 +396,223 @@ func marshalAccount(acct *OffChainBalanceAccount) *litrpc.Account {

return rpcAccount
}

// AccountPayments returns the detailed payment history for the given account.
func (s *RPCServer) AccountPayments(ctx context.Context,
req *litrpc.AccountPaymentsRequest) (
*litrpc.AccountPaymentsResponse, error) {

if req.GetAccount() == nil {
return nil, fmt.Errorf("account param must be specified")
}

var id, label string
switch idType := req.Account.Identifier.(type) {
case *litrpc.AccountIdentifier_Id:
id = idType.Id
case *litrpc.AccountIdentifier_Label:
label = idType.Label
}

log.Infof("[accountpayments] id=%s, label=%v, max_payments=%d, "+
"index_offset=%d, count_total_payments=%v",
id, label, req.MaxPayments, req.IndexOffset,
req.CountTotalPayments)

accountID, err := s.findAccount(ctx, id, label)
if err != nil {
return nil, err
}

// Determine limits.
limit := req.MaxPayments
if limit == 0 {
limit = DefaultMaxPayments
} else if limit > MaxPaymentsLimit {
return nil, fmt.Errorf(
"max_payments cannot exceed %d", MaxPaymentsLimit,
)
}

if req.IndexOffset > MaxIndexOffset {
return nil, fmt.Errorf("index_offset out of range")
}
offset := req.IndexOffset
Comment thread
Cyberguru1 marked this conversation as resolved.
Comment thread
Cyberguru1 marked this conversation as resolved.

// Fetch the paginated payment entries from the store.
paymentsFromStore, err := s.service.store.ListAccountPayments(
ctx, accountID, int32(offset), int32(limit),
)
if err != nil {
return nil, fmt.Errorf(
"unable to list account payments: %w", err,
)
}

// Fetch total payment count if requested.
var totalNumPayments uint64
if req.CountTotalPayments {
total, err := s.service.store.CountAccountPayments(
ctx, accountID,
)
if err != nil {
return nil, fmt.Errorf(
"unable to count account payments: %w", err,
)
}

totalNumPayments = total
Comment thread
Cyberguru1 marked this conversation as resolved.
}

// Fetch the detailed payments concurrently from LND.
var (
wg sync.WaitGroup
mu sync.Mutex
payments = make(map[lntypes.Hash]*lnrpc.Payment)
errs []error
sem = make(chan struct{}, MaxConcurrentTrackPayments)
)
Comment thread
bitromortac marked this conversation as resolved.

rawCtx, _, client := s.service.routerClient.RawClientWithMacAuth(ctx)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The second return value of RawClientWithMacAuth is a cleanup/cancel function. Ignoring it with _ can lead to resource leaks (such as unreleased context resources or metadata). It should be deferred to ensure proper cleanup.

Suggested change
rawCtx, _, client := s.service.routerClient.RawClientWithMacAuth(ctx)
rawCtx, cleanup, client := s.service.routerClient.RawClientWithMacAuth(ctx)
defer cleanup()


for _, entry := range paymentsFromStore {
entry := entry

wg.Add(1)
go func() {
Comment thread
Cyberguru1 marked this conversation as resolved.
Comment thread
Cyberguru1 marked this conversation as resolved.
defer wg.Done()

// Bounded concurrency using semaphore.
select {
case sem <- struct{}{}:
case <-rawCtx.Done():
return
Comment thread
bitromortac marked this conversation as resolved.
}
Comment thread
bitromortac marked this conversation as resolved.
defer func() { <-sem }()

// Add a timeout to prevent hanging streaming calls.
trackCtx, trackCancel := context.WithTimeout(
rawCtx, DefaultTrackPaymentTimeout,
)
defer trackCancel()

stream, err := client.TrackPaymentV2(
trackCtx, &routerrpc.TrackPaymentRequest{
PaymentHash: entry.Hash[:],
NoInflightUpdates: false,
},
)
if err != nil {
// Skip error logging and recording if the
// parent context was cancelled or timed out.
if rawCtx.Err() != nil {
return
}

sErr, ok := status.FromError(err)
if ok && sErr.Code() == codes.NotFound {
log.Warnf("Payment %x not found in "+
"lnd, creating placeholder: %v",
entry.Hash[:], err)
Comment thread
Cyberguru1 marked this conversation as resolved.

mu.Lock()
payments[entry.Hash] =
notFoundPayment(entry.Hash)
mu.Unlock()

return
}

log.Errorf("Failed to track payment %x: %v",
entry.Hash[:], err)

mu.Lock()
errs = append(errs, err)
mu.Unlock()

return
}

payment, err := stream.Recv()
Comment thread
Cyberguru1 marked this conversation as resolved.
if err != nil {
// Skip error logging and recording if the
// parent context was cancelled or timed out.
if rawCtx.Err() != nil {
return
}

sErr, ok := status.FromError(err)
if ok && sErr.Code() == codes.NotFound {
log.Warnf("Payment %x not found in "+
"lnd, creating placeholder: %v",
entry.Hash[:], err)
Comment thread
Cyberguru1 marked this conversation as resolved.

mu.Lock()
payments[entry.Hash] =
notFoundPayment(entry.Hash)
mu.Unlock()

return
}

log.Errorf("Failed to receive payment "+
"update for %x: %v", entry.Hash[:], err)

mu.Lock()
errs = append(errs, err)
mu.Unlock()

return
}
Comment thread
bitromortac marked this conversation as resolved.

mu.Lock()
payments[entry.Hash] = payment
mu.Unlock()
}()
}

wg.Wait()

// Return immediately if the parent context was cancelled or timed out.
if err := rawCtx.Err(); err != nil {
return nil, err
}

// If there were any errors tracking the payments, return the
// error.
if len(errs) > 0 {
return nil, fmt.Errorf(
"failed to fetch payment details: %w", errs[0],
)
}
Comment thread
Cyberguru1 marked this conversation as resolved.
Comment on lines +583 to +587

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

When multiple concurrent operations fail, returning only the first error from a non-deterministically ordered slice can make debugging difficult. It would be more informative to log all the errors that occurred.

Suggested change
if len(errs) > 0 {
return nil, fmt.Errorf(
"failed to fetch payment details: %w", errs[0],
)
}
if len(errs) > 0 {
log.Errorf("Encountered %d errors fetching payment details: %v", len(errs), errs)
return nil, fmt.Errorf(
"failed to fetch payment details: %w", errs[0],
)
}


var finalPayments []*lnrpc.Payment
for _, entry := range paymentsFromStore {
if p, ok := payments[entry.Hash]; ok {
finalPayments = append(finalPayments, p)
}
}

var firstIndexOffset, lastIndexOffset uint64
if len(finalPayments) > 0 {
firstIndexOffset = offset
lastIndexOffset = offset + uint64(len(finalPayments))
}
Comment on lines +596 to +600

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The pagination offsets firstIndexOffset and lastIndexOffset should be based on the number of elements retrieved from the store (paymentsFromStore), rather than the number of successfully fetched payments from LND (finalPayments).

If any payments are skipped (e.g., because they are not found in LND), basing the offsets on finalPayments causes two issues:

  1. Duplicate/Overlapping Results: The next query's offset will overlap with the current page, causing already-returned payments to be fetched and returned again.
  2. Stuck Pagination: If an entire page of payments is missing in LND, lastIndexOffset will be 0, making it impossible for the client to paginate past the dead zone to fetch subsequent valid payments.

Using paymentsFromStore ensures that pagination correctly and deterministically advances through the store's index space.

Suggested change
var firstIndexOffset, lastIndexOffset uint64
if len(finalPayments) > 0 {
firstIndexOffset = offset
lastIndexOffset = offset + uint64(len(finalPayments))
}
var firstIndexOffset, lastIndexOffset uint64
if len(paymentsFromStore) > 0 {
firstIndexOffset = offset
lastIndexOffset = offset + uint64(len(paymentsFromStore))
}


return &litrpc.AccountPaymentsResponse{
Payments: finalPayments,
FirstIndexOffset: firstIndexOffset,
LastIndexOffset: lastIndexOffset,
TotalNumPayments: totalNumPayments,
}, nil
}

// notFoundPayment creates a placeholder payment for a desynced entry.
func notFoundPayment(hash lntypes.Hash) *lnrpc.Payment {
return &lnrpc.Payment{
PaymentHash: hex.EncodeToString(hash[:]),
Status: lnrpc.Payment_UNKNOWN,
FailureReason: lnrpc.
PaymentFailureReason_FAILURE_REASON_NONE,
}
}
55 changes: 55 additions & 0 deletions accounts/store_kvdb.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"math"
"os"
"path/filepath"
"sort"
"time"

"github.com/btcsuite/btcwallet/walletdb"
Expand Down Expand Up @@ -445,6 +446,60 @@ func (s *BoltStore) DeleteAccountPayment(_ context.Context, id AccountID,
return s.updateAccount(id, update)
}

// ListAccountPayments returns a paginated list of payments
// associated with the given account, sorted in ascending lexicographical
// order of their payment hash.
func (s *BoltStore) ListAccountPayments(ctx context.Context, id AccountID,
offset, limit int32) ([]*AccountPaymentEntry, error) {
Comment on lines +452 to +453

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Add a defensive check to ensure that offset is non-negative. If a negative offset is passed to ListAccountPayments, it could cause a runtime panic due to out-of-bounds slice indexing.

func (s *BoltStore) ListAccountPayments(ctx context.Context, id AccountID,
	offset, limit int32) ([]*AccountPaymentEntry, error) {

	if offset < 0 {
		return nil, fmt.Errorf("offset cannot be negative")
	}


account, err := s.Account(ctx, id)
if err != nil {
return nil, err
}

var matchedPayments []*AccountPaymentEntry
for hash, entry := range account.Payments {
matchedPayments = append(matchedPayments, &AccountPaymentEntry{
Hash: hash,
PaymentEntry: entry,
})
}

// Sort target hashes lexicographically to ensure pagination is
// deterministic across all store backends.
sort.Slice(matchedPayments, func(i, j int) bool {
hashI := matchedPayments[i].Hash[:]
hashJ := matchedPayments[j].Hash[:]
return bytes.Compare(hashI, hashJ) < 0
})

// Apply pagination limits and offsets.
total := int32(len(matchedPayments))
if offset >= total {
return nil, nil
}
Comment on lines +476 to +480

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Defensive programming: Ensure that offset is non-negative before using it for slicing matchedPayments. If a negative offset is passed to ListAccountPayments, it will cause a runtime panic due to out-of-bounds slice indexing.

Suggested change
// Apply pagination limits and offsets.
total := int32(len(matchedPayments))
if offset >= total {
return nil, nil
}
if offset < 0 {
offset = 0
}
total := int32(len(matchedPayments))
if offset >= total {
return nil, nil
}
References
  1. Ensure that invalid inputs or states are safely handled in all cases.

Comment thread
bitromortac marked this conversation as resolved.

end := offset + limit
if limit < 0 || end > total {
end = total
}
Comment on lines +482 to +485

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

If offset + limit overflows int32, end becomes negative, which will cause a panic when slicing matchedPayments[offset:end]. Although total is practically limited by memory and slice size, we can make this calculation completely overflow-safe by avoiding the addition when limit is positive.

Suggested change
end := offset + limit
if limit <= 0 || end > total {
end = total
}
end := total
if limit > 0 && total-offset > limit {
end = offset + limit
}

Comment on lines +483 to +485

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

This implementation handles a negative limit by effectively treating it as 'no limit' (fetching all records from the offset). However, the SQL implementation in store_sql.go passes the limit directly to the database, which will likely result in an error for a negative value. This creates an inconsistency between the two store implementations.

While the RPC layer currently ensures the limit is non-negative, it would be best to make the store implementations consistent. I'd suggest either documenting this behavior in the Store interface and replicating it in the SQL store, or adding a check for a negative limit here and in the SQL store to return an error, as it's an invalid argument.


return matchedPayments[offset:end], nil
}

// CountAccountPayments returns the total number of payments associated with
// the given account.
func (s *BoltStore) CountAccountPayments(ctx context.Context,
id AccountID) (uint64, error) {

account, err := s.Account(ctx, id)
if err != nil {
return 0, err
}

return uint64(len(account.Payments)), nil
}

func (s *BoltStore) updateAccount(id AccountID,
updateFn func(*OffChainBalanceAccount) error) error {

Expand Down
Loading
Loading