From e807639f8243f8eeffc6234822d06b975c025978 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 5 Jun 2024 10:48:10 -0400 Subject: [PATCH 01/11] accounts: start adding request ID to context In preperation for the trace logging we want to implement in an upcoming commit (where logs for requests & responsescan be linked via their request ID), we start adding the request ID to an intercepted account request/response in this commit. --- accounts/checkers_test.go | 7 ++++--- accounts/context.go | 32 +++++++++++++++++++++++++++++--- accounts/interceptor.go | 11 ++++++----- 3 files changed, 39 insertions(+), 11 deletions(-) diff --git a/accounts/checkers_test.go b/accounts/checkers_test.go index 2b37b9493..dfad5cb5f 100644 --- a/accounts/checkers_test.go +++ b/accounts/checkers_test.go @@ -112,6 +112,8 @@ func TestAccountChecker(t *testing.T) { func TestAccountCheckers(t *testing.T) { t.Parallel() + const reqID = uint64(55) + testCases := []struct { name string fullURI string @@ -416,9 +418,8 @@ func TestAccountCheckers(t *testing.T) { Invoices: make(AccountInvoices), Payments: make(AccountPayments), } - ctx := AddToContext( - context.Background(), KeyAccount, acct, - ) + ctx := AddAccountToContext(context.Background(), acct) + ctx = AddRequestIDToContext(ctx, reqID) // Is a setup call required to initialize initial // conditions? diff --git a/accounts/context.go b/accounts/context.go index 6bbcf3677..09ffa2a35 100644 --- a/accounts/context.go +++ b/accounts/context.go @@ -18,6 +18,10 @@ var ( // KeyAccount is the key under which we store the account in the request // context. KeyAccount = ContextKey{"account"} + + // KeyRequestID is the key under which we store the middleware request + // ID. + KeyRequestID = ContextKey{"request_id"} ) // FromContext tries to extract a value from the given context. @@ -25,11 +29,12 @@ func FromContext(ctx context.Context, key ContextKey) interface{} { return ctx.Value(key) } -// AddToContext adds the given value to the context for easy retrieval later on. -func AddToContext(ctx context.Context, key ContextKey, +// AddAccountToContext adds the given value to the context for easy retrieval +// later on. +func AddAccountToContext(ctx context.Context, value *OffChainBalanceAccount) context.Context { - return context.WithValue(ctx, key, value) + return context.WithValue(ctx, KeyAccount, value) } // AccountFromContext attempts to extract an account from the given context. @@ -46,3 +51,24 @@ func AccountFromContext(ctx context.Context) (*OffChainBalanceAccount, error) { return acct, nil } + +// AddRequestIDToContext adds the given request ID to the context for easy +// retrieval later on. +func AddRequestIDToContext(ctx context.Context, value uint64) context.Context { + return context.WithValue(ctx, KeyRequestID, value) +} + +// RequestIDFromContext attempts to extract a request ID from the given context. +func RequestIDFromContext(ctx context.Context) (uint64, error) { + val := FromContext(ctx, KeyRequestID) + if val == nil { + return 0, fmt.Errorf("no request ID found in context") + } + + reqID, ok := val.(uint64) + if !ok { + return 0, fmt.Errorf("invalid request ID value in context") + } + + return reqID, nil +} diff --git a/accounts/interceptor.go b/accounts/interceptor.go index 836c4a2fa..af026e3f2 100644 --- a/accounts/interceptor.go +++ b/accounts/interceptor.go @@ -101,9 +101,10 @@ func (s *InterceptorService) Intercept(ctx context.Context, ) } - // We now add the account to the incoming context to give each checker - // access to it if required. - ctxAccount := AddToContext(ctx, KeyAccount, acct) + // We now add the account and request ID to the incoming context to give + // each checker access to them if required. + ctx = AddAccountToContext(ctx, acct) + ctx = AddRequestIDToContext(ctx, req.RequestId) switch r := req.InterceptType.(type) { // In the authentication phase we just check that the account hasn't @@ -120,7 +121,7 @@ func (s *InterceptorService) Intercept(ctx context.Context, } return mid.RPCErr(req, s.checkers.checkIncomingRequest( - ctxAccount, r.Request.MethodFullUri, msg, + ctx, r.Request.MethodFullUri, msg, )) // Parse and possibly manipulate outgoing responses. @@ -131,7 +132,7 @@ func (s *InterceptorService) Intercept(ctx context.Context, } replacement, err := s.checkers.replaceOutgoingResponse( - ctxAccount, r.Response.MethodFullUri, msg, + ctx, r.Response.MethodFullUri, msg, ) if err != nil { return mid.RPCErr(req, err) From 18c819b442a1ba8c41ba2a0be3fe75cf9beaac1d Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 5 Jun 2024 15:30:54 -0400 Subject: [PATCH 02/11] accounts: add ReqIDToPaymentHashStore and implement In preparation for implementing trace logging that makes it easy to link state across requests and responses for more informative logs, we add a new ReqIDToPaymentHashStore interface along with an in-memory implementation. This can be expanded in future to contain other state but for now let's just go with the payment hash since that is useful to have and will provide the user useful information about the payment corresponding to a log. --- accounts/checkers_test.go | 9 +++-- accounts/interface.go | 29 +++++++++++++++ accounts/service.go | 75 ++++++++++++++++++++++++++++++++++----- 3 files changed, 102 insertions(+), 11 deletions(-) diff --git a/accounts/checkers_test.go b/accounts/checkers_test.go index dfad5cb5f..dc1ae1202 100644 --- a/accounts/checkers_test.go +++ b/accounts/checkers_test.go @@ -42,13 +42,16 @@ type mockService struct { trackedInvoices map[lntypes.Hash]AccountID trackedPayments AccountPayments + + *requestValuesStore } func newMockService() *mockService { return &mockService{ - acctBalanceMsat: 0, - trackedInvoices: make(map[lntypes.Hash]AccountID), - trackedPayments: make(AccountPayments), + acctBalanceMsat: 0, + trackedInvoices: make(map[lntypes.Hash]AccountID), + trackedPayments: make(AccountPayments), + requestValuesStore: newRequestValuesStore(), } } diff --git a/accounts/interface.go b/accounts/interface.go index 8fec489e2..f82979a5b 100644 --- a/accounts/interface.go +++ b/accounts/interface.go @@ -252,4 +252,33 @@ type Service interface { // restarted. AssociatePayment(id AccountID, paymentHash lntypes.Hash, fullAmt lnwire.MilliSatoshi) error + + RequestValuesStore +} + +// RequestValues holds various values associated with a specific request that +// we may want access to when handling the response. At the moment this only +// stores payment related data. +type RequestValues struct { + // PaymentHash is the hash of the payment that this request is + // associated with. + PaymentHash lntypes.Hash + + // PaymentAmount is the value of the payment being made. + PaymentAmount lnwire.MilliSatoshi +} + +// RequestValuesStore is a store that can be used to keep track of the mapping +// between a request ID and various values associated with that request which +// we may want access to when handling the request response. +type RequestValuesStore interface { + // RegisterValues stores values for the given request ID. + RegisterValues(reqID uint64, values *RequestValues) error + + // GetValues returns the corresponding request values for the given + // request ID if they exist. + GetValues(reqID uint64) (*RequestValues, bool) + + // DeleteValues deletes any values stored for the given request ID. + DeleteValues(reqID uint64) } diff --git a/accounts/service.go b/accounts/service.go index 208012fa9..8fc53e1a5 100644 --- a/accounts/service.go +++ b/accounts/service.go @@ -66,6 +66,8 @@ type InterceptorService struct { invoiceToAccount map[lntypes.Hash]AccountID pendingPayments map[lntypes.Hash]*trackedPayment + *requestValuesStore + mainErrCallback func(error) wg sync.WaitGroup quit chan struct{} @@ -86,14 +88,15 @@ func NewService(dir string, mainCtx, contextCancel := context.WithCancel(context.Background()) return &InterceptorService{ - store: accountStore, - mainCtx: mainCtx, - contextCancel: contextCancel, - invoiceToAccount: make(map[lntypes.Hash]AccountID), - pendingPayments: make(map[lntypes.Hash]*trackedPayment), - mainErrCallback: errCallback, - quit: make(chan struct{}), - isEnabled: false, + store: accountStore, + mainCtx: mainCtx, + contextCancel: contextCancel, + invoiceToAccount: make(map[lntypes.Hash]AccountID), + pendingPayments: make(map[lntypes.Hash]*trackedPayment), + requestValuesStore: newRequestValuesStore(), + mainErrCallback: errCallback, + quit: make(chan struct{}), + isEnabled: false, }, nil } @@ -830,3 +833,59 @@ func (s *InterceptorService) removePayment(hash lntypes.Hash, func successState(status lnrpc.Payment_PaymentStatus) bool { return status == lnrpc.Payment_SUCCEEDED } + +// requestValuesStore is an in-memory implementation of the +// RequestValuesStore interface. +type requestValuesStore struct { + m map[uint64]*RequestValues + + mu sync.Mutex +} + +// A compile-time check to ensure that requestValuesStore implements the +// RequestValuesStore interface. +var _ RequestValuesStore = (*requestValuesStore)(nil) + +// newRequestValuesStore constructs a new requestValuesStore which is an +// implementation of the RequestValuesStore interface. +func newRequestValuesStore() *requestValuesStore { + return &requestValuesStore{ + m: make(map[uint64]*RequestValues), + } +} + +// RegisterValues stores values for the given request ID. +func (s *requestValuesStore) RegisterValues(reqID uint64, + values *RequestValues) error { + + s.mu.Lock() + defer s.mu.Unlock() + + if _, ok := s.m[reqID]; ok { + return fmt.Errorf("values for request ID %d have already "+ + "been registered", reqID) + } + + s.m[reqID] = values + + return nil +} + +// GetValues returns the corresponding request values for the given request ID +// if they exist. +func (s *requestValuesStore) GetValues(reqID uint64) (*RequestValues, bool) { + s.mu.Lock() + defer s.mu.Unlock() + + values, ok := s.m[reqID] + + return values, ok +} + +// DeleteValues deletes any values stored for the given request ID. +func (s *requestValuesStore) DeleteValues(reqID uint64) { + s.mu.Lock() + defer s.mu.Unlock() + + delete(s.m, reqID) +} From 9ffd05a597115e1aed47f61c1c993bfbf418887d Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 5 Jun 2024 15:39:06 -0400 Subject: [PATCH 03/11] accounts: start writing to the new request ID scoped cache For all payment request handlers, start using the new ReqIDToPaymentHash store to store some state (just payment hash for now). --- accounts/checkers.go | 101 +++++++++++++++++++++++++++++++------- accounts/checkers_test.go | 10 +++- 2 files changed, 91 insertions(+), 20 deletions(-) diff --git a/accounts/checkers.go b/accounts/checkers.go index e4eff8935..279d6c7e7 100644 --- a/accounts/checkers.go +++ b/accounts/checkers.go @@ -63,6 +63,8 @@ var ( ListPeersEmptyRewriter = mid.NewResponseEmptier[ *lnrpc.ListPeersRequest, *lnrpc.ListPeersResponse, ]() + + emptyHash lntypes.Hash ) // CheckerMap is a type alias that maps gRPC request URIs to their @@ -181,7 +183,8 @@ func NewAccountChecker(service Service, func(ctx context.Context, r *lnrpc.SendRequest) error { return checkSend( ctx, chainParams, service, r.Amt, - r.AmtMsat, r.PaymentRequest, r.FeeLimit, + r.AmtMsat, r.PaymentRequest, + r.PaymentHash, r.FeeLimit, ) }, sendResponseHandler, mid.PassThroughErrorHandler, ), @@ -191,7 +194,8 @@ func NewAccountChecker(service Service, func(ctx context.Context, r *lnrpc.SendRequest) error { return checkSend( ctx, chainParams, service, r.Amt, - r.AmtMsat, r.PaymentRequest, r.FeeLimit, + r.AmtMsat, r.PaymentRequest, + r.PaymentHash, r.FeeLimit, ) }, sendResponseHandler, mid.PassThroughErrorHandler, ), @@ -210,6 +214,7 @@ func NewAccountChecker(service Service, return checkSend( ctx, chainParams, service, r.Amt, r.AmtMsat, r.PaymentRequest, + r.PaymentHash, &lnrpc.FeeLimit{ Limit: &lnrpc.FeeLimit_FixedMsat{ FixedMsat: feeLimitMsat, @@ -240,7 +245,9 @@ func NewAccountChecker(service Service, func(ctx context.Context, r *lnrpc.SendToRouteRequest) error { - return checkSendToRoute(ctx, service, r.Route) + return checkSendToRoute( + ctx, service, r.PaymentHash, r.Route, + ) }, sendResponseHandler, mid.PassThroughErrorHandler, ), "/lnrpc.Lightning/SendToRouteSync": mid.NewFullChecker( @@ -249,7 +256,9 @@ func NewAccountChecker(service Service, func(ctx context.Context, r *lnrpc.SendToRouteRequest) error { - return checkSendToRoute(ctx, service, r.Route) + return checkSendToRoute( + ctx, service, r.PaymentHash, r.Route, + ) }, sendResponseHandler, mid.PassThroughErrorHandler, ), // routerrpc.Router/SendToRoute is deprecated. @@ -259,7 +268,9 @@ func NewAccountChecker(service Service, func(ctx context.Context, r *routerrpc.SendToRouteRequest) error { - return checkSendToRoute(ctx, service, r.Route) + return checkSendToRoute( + ctx, service, r.PaymentHash, r.Route, + ) }, // We don't get the payment hash in the response to this // call. So we can't optimize things and need to rely on @@ -509,20 +520,29 @@ func filterPayments(ctx context.Context, // the context has enough balance to pay for it. func checkSend(ctx context.Context, chainParams *chaincfg.Params, service Service, amt, amtMsat int64, invoice string, - feeLimit *lnrpc.FeeLimit) error { + paymentHash []byte, feeLimit *lnrpc.FeeLimit) error { acct, err := AccountFromContext(ctx) if err != nil { return err } + reqID, err := RequestIDFromContext(ctx) + if err != nil { + return err + } + sendAmt := lnwire.NewMSatFromSatoshis(btcutil.Amount(amt)) if lnwire.MilliSatoshi(amtMsat) > sendAmt { sendAmt = lnwire.MilliSatoshi(amtMsat) } - // The invoice is optional. - var paymentHash lntypes.Hash + // We require that a payment hash is set, which we either read from the + // payment hash from the invoice or from the request. + var pHash lntypes.Hash + + // Check if an invoice was provided. If so, glean the payment hash from + // that. if len(invoice) > 0 { payReq, err := zpay32.Decode(invoice, chainParams) if err != nil { @@ -534,10 +554,36 @@ func checkSend(ctx context.Context, chainParams *chaincfg.Params, } if payReq.PaymentHash != nil { - paymentHash = *payReq.PaymentHash + pHash = *payReq.PaymentHash } } + // If a payment hash was separately provided in the request, then glean + // derive the hash from there. If the invoice was set then the two + // payment hashes must match. + if len(paymentHash) != 0 { + hash, err := lntypes.MakeHash(paymentHash) + if err != nil { + return err + } + + if pHash != emptyHash && hash != pHash { + return fmt.Errorf("two conflicting hashes provided") + } + + pHash = hash + } + + // If at this point we still have no payment hash, then we error out + // for now. + // TODO(elle): support key sends by: + // 1) allowing the user to set a pre-image + // 2) deriving a pre-image here. + // Then glean the payment hash from that. + if pHash == emptyHash { + return fmt.Errorf("a payment hash is required") + } + // We also add the max fee to the amount to check. This might mean that // not every single satoshi of an account can be used up. But it // prevents an account from going into a negative balance if we only @@ -554,15 +600,15 @@ func checkSend(ctx context.Context, chainParams *chaincfg.Params, return fmt.Errorf("error validating account balance: %w", err) } - emptyHash := lntypes.Hash{} - if paymentHash != emptyHash { - err = service.AssociatePayment(acct.ID, paymentHash, sendAmt) - if err != nil { - return fmt.Errorf("error associating payment: %w", err) - } + err = service.AssociatePayment(acct.ID, pHash, sendAmt) + if err != nil { + return fmt.Errorf("error associating payment: %w", err) } - return nil + return service.RegisterValues(reqID, &RequestValues{ + PaymentHash: pHash, + PaymentAmount: sendAmt, + }) } // checkSendResponse makes sure that a payment that is in flight is tracked @@ -593,7 +639,7 @@ func checkSendResponse(ctx context.Context, service Service, // checkSendToRoute checks if a payment can be sent to the route by making sure // the account in the context has enough balance to pay for it. -func checkSendToRoute(ctx context.Context, service Service, +func checkSendToRoute(ctx context.Context, service Service, paymentHash []byte, route *lnrpc.Route) error { acct, err := AccountFromContext(ctx) @@ -601,6 +647,16 @@ func checkSendToRoute(ctx context.Context, service Service, return err } + hash, err := lntypes.MakeHash(paymentHash) + if err != nil { + return err + } + + reqID, err := RequestIDFromContext(ctx) + if err != nil { + return err + } + if route == nil { return fmt.Errorf("invalid route") } @@ -625,5 +681,14 @@ func checkSendToRoute(ctx context.Context, service Service, return fmt.Errorf("error validating account balance: %w", err) } - return nil + err = service.AssociatePayment(acct.ID, hash, sendAmt) + if err != nil { + return fmt.Errorf("error associating payment with hash %s: %w", + hash, err) + } + + return service.RegisterValues(reqID, &RequestValues{ + PaymentHash: hash, + PaymentAmount: sendAmt, + }) } diff --git a/accounts/checkers_test.go b/accounts/checkers_test.go index dc1ae1202..4aa9cb14e 100644 --- a/accounts/checkers_test.go +++ b/accounts/checkers_test.go @@ -25,7 +25,10 @@ var ( } testID = AccountID{77, 88, 99} - testHash = lntypes.Hash{1, 2, 3, 4, 5} + testHash = lntypes.Hash{ + 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, + 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, + } testAmount = &lnrpc.Amount{ Sat: 456, @@ -207,7 +210,8 @@ func TestAccountCheckers(t *testing.T) { name: "send payment, not enough balance", fullURI: "/lnrpc.Lightning/SendPaymentSync", originalRequest: &lnrpc.SendRequest{ - AmtMsat: 5000, + AmtMsat: 5000, + PaymentHash: testHash[:], }, requestErr: "error validating account balance: invalid balance", }, { @@ -223,6 +227,7 @@ func TestAccountCheckers(t *testing.T) { Percent: 1, }, }, + PaymentHash: testHash[:], }, requestErr: "error validating account balance: invalid balance", }, { @@ -238,6 +243,7 @@ func TestAccountCheckers(t *testing.T) { FixedMsat: 123, }, }, + PaymentHash: testHash[:], }, originalResponse: &lnrpc.SendResponse{ PaymentHash: testHash[:], From 8b5a01f909b33a52e6c888e6cd848a603b181c1e Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 5 Jun 2024 15:44:26 -0400 Subject: [PATCH 04/11] accounts: start logging useful info from payment request handlers This commit adds a prefixed logger that is then used to log trace information about a payment. This will make it easy to see from the logs which account ID is making the request, what the request ID is and what the relavant payment hash is. --- accounts/checkers.go | 20 ++++++++------------ accounts/context.go | 25 +++++++++++++++++++++++++ accounts/interface.go | 5 +++++ accounts/log.go | 2 +- 4 files changed, 39 insertions(+), 13 deletions(-) diff --git a/accounts/checkers.go b/accounts/checkers.go index 279d6c7e7..b0fef5ea3 100644 --- a/accounts/checkers.go +++ b/accounts/checkers.go @@ -522,12 +522,7 @@ func checkSend(ctx context.Context, chainParams *chaincfg.Params, service Service, amt, amtMsat int64, invoice string, paymentHash []byte, feeLimit *lnrpc.FeeLimit) error { - acct, err := AccountFromContext(ctx) - if err != nil { - return err - } - - reqID, err := RequestIDFromContext(ctx) + log, acct, reqID, err := requestScopedValuesFromCtx(ctx) if err != nil { return err } @@ -584,6 +579,9 @@ func checkSend(ctx context.Context, chainParams *chaincfg.Params, return fmt.Errorf("a payment hash is required") } + log.Tracef("Handling send request for payment with hash: %s and "+ + "amount: %d", pHash, sendAmt) + // We also add the max fee to the amount to check. This might mean that // not every single satoshi of an account can be used up. But it // prevents an account from going into a negative balance if we only @@ -642,7 +640,7 @@ func checkSendResponse(ctx context.Context, service Service, func checkSendToRoute(ctx context.Context, service Service, paymentHash []byte, route *lnrpc.Route) error { - acct, err := AccountFromContext(ctx) + log, acct, reqID, err := requestScopedValuesFromCtx(ctx) if err != nil { return err } @@ -652,11 +650,6 @@ func checkSendToRoute(ctx context.Context, service Service, paymentHash []byte, return err } - reqID, err := RequestIDFromContext(ctx) - if err != nil { - return err - } - if route == nil { return fmt.Errorf("invalid route") } @@ -666,6 +659,9 @@ func checkSendToRoute(ctx context.Context, service Service, paymentHash []byte, sendAmt = lnwire.MilliSatoshi(route.TotalAmtMsat) } + log.Tracef("Handling send request for payment with hash: %s and "+ + "amount: %d", hash, sendAmt) + // We also add the max fee to the amount to check. This might mean that // not every single satoshi of an account can be used up. But it // prevents an account from going into a negative balance if we only diff --git a/accounts/context.go b/accounts/context.go index 09ffa2a35..6916a7cae 100644 --- a/accounts/context.go +++ b/accounts/context.go @@ -3,6 +3,9 @@ package accounts import ( "context" "fmt" + + "github.com/btcsuite/btclog" + "github.com/lightningnetwork/lnd/build" ) // ContextKey is the type that we use to identify account specific values in the @@ -72,3 +75,25 @@ func RequestIDFromContext(ctx context.Context) (uint64, error) { return reqID, nil } + +// requestScopedValuesFromCtx is a helper function that can be used to extract +// an account and requestID from the given context. It also creates a new +// prefixed logger that can be used by account request and response handlers. +// Each log line will be prefixed by the account ID and the request ID. +func requestScopedValuesFromCtx(ctx context.Context) (btclog.Logger, + *OffChainBalanceAccount, uint64, error) { + + acc, err := AccountFromContext(ctx) + if err != nil { + return nil, nil, 0, err + } + + reqID, err := RequestIDFromContext(ctx) + if err != nil { + return nil, nil, 0, err + } + + prefix := fmt.Sprintf("[account: %s, request: %d]", acc.ID, reqID) + + return build.NewPrefixLog(prefix, log), acc, reqID, nil +} diff --git a/accounts/interface.go b/accounts/interface.go index f82979a5b..038dfc9a2 100644 --- a/accounts/interface.go +++ b/accounts/interface.go @@ -53,6 +53,11 @@ func ParseAccountID(idStr string) (*AccountID, error) { return &id, nil } +// String returns the string representation of the AccountID. +func (a AccountID) String() string { + return hex.EncodeToString(a[:]) +} + // PaymentEntry is the data we track per payment that is associated with an // account. This basically includes all information required to make sure // in-flight payments don't exceed the total available account balance. diff --git a/accounts/log.go b/accounts/log.go index 81d822b8b..123bcfe55 100644 --- a/accounts/log.go +++ b/accounts/log.go @@ -5,7 +5,7 @@ import ( "github.com/lightningnetwork/lnd/build" ) -const Subsystem = "ACCT" +const Subsystem = "ACNT" // log is a logger that is initialized with no output filters. This // means the package will not perform any logging by default until the caller From 0bac0aacba82faa797c87b8a497ec09194e971d0 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 5 Jun 2024 12:44:26 -0400 Subject: [PATCH 05/11] accounts: start logging useful info from payment response handlers --- accounts/checkers.go | 105 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 94 insertions(+), 11 deletions(-) diff --git a/accounts/checkers.go b/accounts/checkers.go index b0fef5ea3..a0a791a97 100644 --- a/accounts/checkers.go +++ b/accounts/checkers.go @@ -262,7 +262,7 @@ func NewAccountChecker(service Service, }, sendResponseHandler, mid.PassThroughErrorHandler, ), // routerrpc.Router/SendToRoute is deprecated. - "/routerrpc.Router/SendToRouteV2": mid.NewRequestChecker( + "/routerrpc.Router/SendToRouteV2": mid.NewFullChecker( &routerrpc.SendToRouteRequest{}, &lnrpc.HTLCAttempt{}, func(ctx context.Context, @@ -272,10 +272,8 @@ func NewAccountChecker(service Service, ctx, service, r.PaymentHash, r.Route, ) }, - // We don't get the payment hash in the response to this - // call. So we can't optimize things and need to rely on - // the payment being tracked by the hash sent in the - // request. + sendToRouteHTLCResponseHandler(service), + mid.PassThroughErrorHandler, ), "/lnrpc.Lightning/DecodePayReq": DecodePayReqPassThrough, "/lnrpc.Lightning/ListPayments": mid.NewResponseRewriter( @@ -616,23 +614,42 @@ func checkSendResponse(ctx context.Context, service Service, status lnrpc.Payment_PaymentStatus, hash lntypes.Hash, fullAmt int64) (proto.Message, error) { - acct, err := AccountFromContext(ctx) + log, acct, reqID, err := requestScopedValuesFromCtx(ctx) if err != nil { return nil, err } + reqVals, ok := service.GetValues(reqID) + if !ok { + return nil, nil + } + + if hash != reqVals.PaymentHash { + return nil, fmt.Errorf("response hash did not match request " + + "hash") + } + + log.Tracef("Handling response payment with hash: %s", hash) + // If we directly observe a failure, make sure // we stop tracking the payment and then exit // early. if status == lnrpc.Payment_FAILED { + service.DeleteValues(reqID) + return nil, service.RemovePayment(hash) } - // If there is no immediate failure, make sure - // we track the payment. - return nil, service.TrackPayment( - acct.ID, hash, lnwire.MilliSatoshi(fullAmt), - ) + // If there is no immediate failure, make sure we track the payment. + err = service.TrackPayment(acct.ID, hash, lnwire.MilliSatoshi(fullAmt)) + if err != nil { + return nil, err + } + + // We can now delete the request values for this request ID. + service.DeleteValues(reqID) + + return nil, nil } // checkSendToRoute checks if a payment can be sent to the route by making sure @@ -688,3 +705,69 @@ func checkSendToRoute(ctx context.Context, service Service, paymentHash []byte, PaymentAmount: sendAmt, }) } + +// sendToRouteHTLCResponseHandler creates a response handler for the +// SendToRouteV2 endpoint which is a streaming endpoint that may return multiple +// HTLCAttempt updates. +func sendToRouteHTLCResponseHandler(service Service) func(ctx context.Context, + r *lnrpc.HTLCAttempt) (proto.Message, error) { + + return func(ctx context.Context, r *lnrpc.HTLCAttempt) (proto.Message, + error) { + + log, acct, reqID, err := requestScopedValuesFromCtx(ctx) + if err != nil { + return nil, err + } + + // Since SendToRouteV2 is a streaming endpoint, we may get + // multiple responses for it. If we have already handled it then + // we would have deleted the request ID to hash mapping, and so + // we can exit early here. + reqValues, ok := service.GetValues(reqID) + if !ok { + return nil, nil + } + + log.Tracef("Handling response for payment with hash: %s and "+ + "amount: %d", reqValues.PaymentHash, + reqValues.PaymentAmount) + + // If the pre-image is provided, do a sanity check to make sure + // that this does actually match the hash we stored for this + // payment. + if len(r.Preimage) != 0 { + preimage, err := lntypes.MakePreimage(r.Preimage) + if err != nil { + return nil, err + } + + if !preimage.Matches(reqValues.PaymentHash) { + return nil, fmt.Errorf("the preimage (%s) "+ + "returned by the response does not "+ + "match the payment hash (%s) of the "+ + "payment", preimage, + reqValues.PaymentHash) + } + } + + route := r.Route + totalAmount := int64(0) + if route != nil { + totalAmount = route.TotalAmtMsat + route.TotalFeesMsat + } + + err = service.TrackPayment( + acct.ID, reqValues.PaymentHash, + lnwire.MilliSatoshi(totalAmount), + ) + if err != nil { + return nil, err + } + + // We can now delete the request values for this ID. + service.DeleteValues(reqID) + + return nil, nil + } +} From fde86d31361c168e0e681b3e5e3616ca3f603526 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Tue, 4 Jun 2024 16:58:49 -0400 Subject: [PATCH 06/11] accounts+mid: support for error handlers in accounts We also want to be able to see via the trace logs if a request has errored. So first we need to update the framework to be ready for this. --- accounts/checkers.go | 24 +++++++++++++++ accounts/checkers_test.go | 4 +++ accounts/interceptor.go | 24 +++++++++++++++ accounts/interface.go | 5 ++++ accounts/service.go | 60 ++++++++++++++++++++++++++++++++++++-- rpcmiddleware/interface.go | 14 +++++---- 6 files changed, 123 insertions(+), 8 deletions(-) diff --git a/accounts/checkers.go b/accounts/checkers.go index a0a791a97..b44f160b9 100644 --- a/accounts/checkers.go +++ b/accounts/checkers.go @@ -398,6 +398,30 @@ func NewAccountChecker(service Service, } } +// handleErrorResponse passes an error to a checker if the checker has +// registered an error handler. +func (a *AccountChecker) handleErrorResponse(ctx context.Context, + fullUri string, parsedErr error) (error, error) { + + // If we don't have a handler for the URI, it means we don't support + // that RPC. + checker, ok := a.checkers[fullUri] + if !ok { + return nil, ErrNotSupportedWithAccounts + } + + newErr, err := checker.HandleErrorResponse(ctx, parsedErr) + if err != nil { + return nil, err + } + + if newErr != nil { + parsedErr = newErr + } + + return parsedErr, nil +} + // checkIncomingRequest makes sure the type of incoming call is supported and // if it is, that it is allowed with the current account balance. func (a *AccountChecker) checkIncomingRequest(ctx context.Context, diff --git a/accounts/checkers_test.go b/accounts/checkers_test.go index 4aa9cb14e..e482c1230 100644 --- a/accounts/checkers_test.go +++ b/accounts/checkers_test.go @@ -80,6 +80,10 @@ func (m *mockService) AssociatePayment(id AccountID, paymentHash lntypes.Hash, return nil } +func (m *mockService) PaymentErrored(id AccountID, hash lntypes.Hash) error { + return nil +} + func (m *mockService) TrackPayment(_ AccountID, hash lntypes.Hash, amt lnwire.MilliSatoshi) error { diff --git a/accounts/interceptor.go b/accounts/interceptor.go index af026e3f2..aa3d759f0 100644 --- a/accounts/interceptor.go +++ b/accounts/interceptor.go @@ -126,6 +126,30 @@ func (s *InterceptorService) Intercept(ctx context.Context, // Parse and possibly manipulate outgoing responses. case *lnrpc.RPCMiddlewareRequest_Response: + if r.Response.IsError { + parsedErr := mid.ParseResponseErr(r.Response.Serialized) + + replacementErr, err := s.checkers.handleErrorResponse( + ctx, r.Response.MethodFullUri, parsedErr, + ) + if err != nil { + return mid.RPCErr(req, err) + } + + // No error occurred but the response error should be + // replaced with the given custom error. Wrap it in the + // correct RPC response of the interceptor now. + if replacementErr != nil { + return mid.RPCErrReplacement( + req, replacementErr, + ) + } + + // No error and no replacement, just return an empty + // response of the correct type. + return mid.RPCOk(req) + } + msg, err := parseRPCMessage(r.Response) if err != nil { return mid.RPCErr(req, err) diff --git a/accounts/interface.go b/accounts/interface.go index 038dfc9a2..68bb5ad5c 100644 --- a/accounts/interface.go +++ b/accounts/interface.go @@ -258,6 +258,11 @@ type Service interface { AssociatePayment(id AccountID, paymentHash lntypes.Hash, fullAmt lnwire.MilliSatoshi) error + // PaymentErrored removes a pending payment from the accounts + // registered payment list. This should only ever be called if we are + // sure that the payment request errored out. + PaymentErrored(id AccountID, hash lntypes.Hash) error + RequestValuesStore } diff --git a/accounts/service.go b/accounts/service.go index 8fc53e1a5..98a58f249 100644 --- a/accounts/service.go +++ b/accounts/service.go @@ -437,6 +437,45 @@ func (s *InterceptorService) AssociateInvoice(id AccountID, return s.store.UpdateAccount(account) } +// PaymentErrored removes a pending payment from the account's registered +// payment list. This should only ever be called if we are sure that the payment +// request errored out. +func (s *InterceptorService) PaymentErrored(id AccountID, + hash lntypes.Hash) error { + + s.Lock() + defer s.Unlock() + + // If we have already started tracking this payment, then RemovePayment + // should have been called instead. + _, ok := s.pendingPayments[hash] + if ok { + return fmt.Errorf("cannot disassociate payment if tracking " + + "has already started") + } + + account, err := s.store.Account(id) + if err != nil { + return err + } + + // Check that this payment is actually associated with this account. + _, ok = account.Payments[hash] + if !ok { + return fmt.Errorf("payment with hash %s is not associated "+ + "with this account", hash) + } + + // Delete the payment and update the persisted account. + delete(account.Payments, hash) + + if err := s.store.UpdateAccount(account); err != nil { + return fmt.Errorf("error updating account: %w", err) + } + + return nil +} + // AssociatePayment associates a payment (hash) with the given account, // ensuring that the payment will be tracked for a user when LiT is // restarted. @@ -451,11 +490,26 @@ func (s *InterceptorService) AssociatePayment(id AccountID, return err } - // If the payment is already associated with the account, we don't need - // to associate it again. + // Check if this payment is associated with the account already. _, ok := account.Payments[paymentHash] if ok { - return nil + // We do not allow another payment to the same hash if the + // payment is already in-flight or succeeded. This mitigates a + // user being able to launch a second RPC-erring payment with + // the same hash that would remove the payment from being + // tracked. Note that this prevents launching multipart + // payments, but allows retrying a payment if it has failed. + if account.Payments[paymentHash].Status != + lnrpc.Payment_FAILED { + + return fmt.Errorf("payment with hash %s is already in "+ + "flight or succeeded (status %v)", paymentHash, + account.Payments[paymentHash].Status) + } + + // Otherwise, we fall through to correctly update the payment + // amount, in case we have a zero-amount invoice that is + // retried. } // Associate the payment with the account and store it. diff --git a/rpcmiddleware/interface.go b/rpcmiddleware/interface.go index 4f14c775d..159d7d8a2 100644 --- a/rpcmiddleware/interface.go +++ b/rpcmiddleware/interface.go @@ -35,7 +35,9 @@ var ( // PassThroughErrorHandler is an ErrorHandler that does not modify an // error and instead just passes it through. - PassThroughErrorHandler ErrorHandler = func(error) (error, error) { + PassThroughErrorHandler ErrorHandler = func(context.Context, error) ( + error, error) { + return nil, nil } @@ -81,7 +83,7 @@ type messageHandler func(context.Context, proto.Message) (proto.Message, error) // pass through the error unchanged (=return nil, nil), replace the error with // a different one (=return non-nil error, nil error) or abort by returning a // non-nil error. -type ErrorHandler func(respErr error) (error, error) +type ErrorHandler func(ctx context.Context, respErr error) (error, error) // RoundTripChecker is a type that represents a basic request/response round // trip checker. @@ -115,7 +117,7 @@ type RoundTripChecker interface { // The handler can pass through the error (=return nil, nil), replace // the response error with a new one (=return non-nil error, nil) or // abort by returning a non nil error (=return nil, non-nil error). - HandleErrorResponse(error) (error, error) + HandleErrorResponse(context.Context, error) (error, error) } // DefaultChecker is the default implementation of a round trip checker. @@ -171,8 +173,10 @@ func (r *DefaultChecker) HandleResponse(ctx context.Context, // The handler can pass through the error (=return nil, nil), replace // the response error with a new one (=return non-nil error, nil) or // abort by returning a non nil error (=return nil, non-nil error). -func (r *DefaultChecker) HandleErrorResponse(respErr error) (error, error) { - return r.errorHandler(respErr) +func (r *DefaultChecker) HandleErrorResponse(ctx context.Context, + respErr error) (error, error) { + + return r.errorHandler(ctx, respErr) } // NewPassThrough returns a round trip checker that allows the incoming request From 61f32f68fb5d0c9f8db643d304e8d5c6d20c0a1b Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 5 Jun 2024 12:51:40 -0400 Subject: [PATCH 07/11] accounts: start logging errored request info --- accounts/checkers.go | 42 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/accounts/checkers.go b/accounts/checkers.go index b44f160b9..00a41d393 100644 --- a/accounts/checkers.go +++ b/accounts/checkers.go @@ -186,7 +186,7 @@ func NewAccountChecker(service Service, r.AmtMsat, r.PaymentRequest, r.PaymentHash, r.FeeLimit, ) - }, sendResponseHandler, mid.PassThroughErrorHandler, + }, sendResponseHandler, erroredPaymentHandler(service), ), "/lnrpc.Lightning/SendPaymentSync": mid.NewFullChecker( &lnrpc.SendRequest{}, @@ -197,7 +197,7 @@ func NewAccountChecker(service Service, r.AmtMsat, r.PaymentRequest, r.PaymentHash, r.FeeLimit, ) - }, sendResponseHandler, mid.PassThroughErrorHandler, + }, sendResponseHandler, erroredPaymentHandler(service), ), // routerrpc.Router/SendPayment is deprecated. "/routerrpc.Router/SendPaymentV2": mid.NewFullChecker( @@ -237,7 +237,7 @@ func NewAccountChecker(service Service, return checkSendResponse( ctx, service, r.Status, hash, fullAmt, ) - }, mid.PassThroughErrorHandler, + }, erroredPaymentHandler(service), ), "/lnrpc.Lightning/SendToRoute": mid.NewFullChecker( &lnrpc.SendToRouteRequest{}, @@ -248,7 +248,7 @@ func NewAccountChecker(service Service, return checkSendToRoute( ctx, service, r.PaymentHash, r.Route, ) - }, sendResponseHandler, mid.PassThroughErrorHandler, + }, sendResponseHandler, erroredPaymentHandler(service), ), "/lnrpc.Lightning/SendToRouteSync": mid.NewFullChecker( &lnrpc.SendToRouteRequest{}, @@ -259,7 +259,7 @@ func NewAccountChecker(service Service, return checkSendToRoute( ctx, service, r.PaymentHash, r.Route, ) - }, sendResponseHandler, mid.PassThroughErrorHandler, + }, sendResponseHandler, erroredPaymentHandler(service), ), // routerrpc.Router/SendToRoute is deprecated. "/routerrpc.Router/SendToRouteV2": mid.NewFullChecker( @@ -273,7 +273,7 @@ func NewAccountChecker(service Service, ) }, sendToRouteHTLCResponseHandler(service), - mid.PassThroughErrorHandler, + erroredPaymentHandler(service), ), "/lnrpc.Lightning/DecodePayReq": DecodePayReqPassThrough, "/lnrpc.Lightning/ListPayments": mid.NewResponseRewriter( @@ -730,6 +730,36 @@ func checkSendToRoute(ctx context.Context, service Service, paymentHash []byte, }) } +// erroredPaymentHandler does some trace logging about the errored payment and +// clears up any state we may have had for the payment. +func erroredPaymentHandler(service Service) mid.ErrorHandler { + return func(ctx context.Context, respErr error) (error, error) { + log, acct, reqID, err := requestScopedValuesFromCtx(ctx) + if err != nil { + return nil, err + } + + reqVals, ok := service.GetValues(reqID) + if !ok { + return nil, fmt.Errorf("no request values found for "+ + "request: %d", reqID) + } + + log.Tracef("Handling payment request error for payment with "+ + "hash: %s and amount: %d", reqVals.PaymentHash, + reqVals.PaymentAmount) + + err = service.PaymentErrored(acct.ID, reqVals.PaymentHash) + if err != nil { + return nil, err + } + + service.DeleteValues(reqID) + + return nil, nil + } +} + // sendToRouteHTLCResponseHandler creates a response handler for the // SendToRouteV2 endpoint which is a streaming endpoint that may return multiple // HTLCAttempt updates. From 9d9d6585ab0e462e74f1cac56cc23b8428164ab9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20Tigerstr=C3=B6m?= Date: Wed, 5 Jun 2024 11:57:32 +0200 Subject: [PATCH 08/11] accounts: refactor in-flight check This commit refactors the in-flight check in the accounts service to its own function. The in-flight check is also inverted to only check the states that are not considered as in-flight. This makes the check forward-compatible with lnd `v0.18.0-beta` which introduces a new `lnrpc.Payment_INITIATED` state. --- accounts/service.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/accounts/service.go b/accounts/service.go index 98a58f249..111056bed 100644 --- a/accounts/service.go +++ b/accounts/service.go @@ -764,9 +764,7 @@ func (s *InterceptorService) paymentUpdate(hash lntypes.Hash, // Are we still in-flight? Then we don't have to do anything just yet. // The unknown state should never happen in practice but if it ever did // we couldn't handle it anyway, so let's also ignore it. - if status.State == lnrpc.Payment_IN_FLIGHT || - status.State == lnrpc.Payment_UNKNOWN { - + if inflightState(status.State) { return false, nil } @@ -888,6 +886,13 @@ func successState(status lnrpc.Payment_PaymentStatus) bool { return status == lnrpc.Payment_SUCCEEDED } +// inflightState returns true if a payment should be seen as in-flight by the +// accounts system. +func inflightState(status lnrpc.Payment_PaymentStatus) bool { + return status != lnrpc.Payment_SUCCEEDED && + status != lnrpc.Payment_FAILED +} + // requestValuesStore is an in-memory implementation of the // RequestValuesStore interface. type requestValuesStore struct { From fe333cb8f436db6caca37b6c058d21153bdb2711 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20Tigerstr=C3=B6m?= Date: Tue, 4 Jun 2024 16:21:01 -0400 Subject: [PATCH 09/11] accounts: deduct in-flight balance by account ID This commit ensures that a pending payment is associated with the specific account ID before deducting the in-flight balance from the available balance. --- accounts/service.go | 9 ++++++-- accounts/service_test.go | 46 +++++++++++++++++++++++++++++++++++----- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/accounts/service.go b/accounts/service.go index 111056bed..3cc4ace6e 100644 --- a/accounts/service.go +++ b/accounts/service.go @@ -406,8 +406,13 @@ func (s *InterceptorService) CheckBalance(id AccountID, } var inFlightAmt int64 - for _, pendingPayment := range s.pendingPayments { - inFlightAmt += int64(pendingPayment.fullAmount) + for _, payment := range account.Payments { + if inflightState(payment.Status) { + // If a payment is in-flight and associated with the + // account, the user should not be able to spend that + // amount while it's in-flight. + inFlightAmt += int64(payment.FullAmount) + } } availableAmount := account.CurrentBalance - inFlightAmt diff --git a/accounts/service_test.go b/accounts/service_test.go index dc169c3b8..8116d42ad 100644 --- a/accounts/service_test.go +++ b/accounts/service_test.go @@ -20,7 +20,10 @@ var ( testTimeout = time.Millisecond * 500 testInterval = time.Millisecond * 20 - testHash2 = lntypes.Hash{99, 88, 77} + testID2 = AccountID{22, 22, 22} + + testHash2 = lntypes.Hash{2, 2, 2, 2, 2} + testHash3 = lntypes.Hash{3, 3, 3, 3, 3} ) type mockLnd struct { @@ -623,8 +626,10 @@ func TestAccountService(t *testing.T) { }, { name: "in-flight payments", setup: func(t *testing.T, lnd *mockLnd, s *InterceptorService) { - // We set up our account with a balance of 5k msats and - // two in-flight payments with a total or 3k msats. + // We set up two accounts with a balance of 5k msats. + + // The first account has two in-flight payments, one of + // 2k msats and one of 1k msats, totaling 3k msats. acct := &OffChainBalanceAccount{ ID: testID, Type: TypeInitialBalance, @@ -646,12 +651,34 @@ func TestAccountService(t *testing.T) { err := s.store.UpdateAccount(acct) require.NoError(t, err) + + // The second account has one in-flight payment of 4k + // msats. + acct2 := &OffChainBalanceAccount{ + ID: testID2, + Type: TypeInitialBalance, + CurrentBalance: 5000, + Invoices: AccountInvoices{ + testHash: {}, + }, + Payments: AccountPayments{ + testHash3: { + Status: lnrpc.Payment_IN_FLIGHT, + FullAmount: 4000, + }, + }, + } + + err = s.store.UpdateAccount(acct2) + require.NoError(t, err) }, validate: func(t *testing.T, lnd *mockLnd, s *InterceptorService) { - // We should be able to initiate another payment with an - // amount smaller or equal to 2k msats. + // The first should be able to initiate another payment + // with an amount smaller or equal to 2k msats. This + // also asserts that the second accounts in-flight + // payment doesn't affect the first account. err := s.CheckBalance(testID, 2000) require.NoError(t, err) @@ -670,6 +697,15 @@ func TestAccountService(t *testing.T) { err = s.CheckBalance(testID, 4000) return err == nil }) + + // The second account should be able to initiate a + // payment of 1k msats. + err = s.CheckBalance(testID2, 1000) + require.NoError(t, err) + + // But exactly one sat over it should fail. + err = s.CheckBalance(testID2, 1001) + require.ErrorIs(t, err, ErrAccBalanceInsufficient) }, }} From 33d717a624bd07a2ab796983ece40f922adcfd81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20Tigerstr=C3=B6m?= Date: Tue, 4 Jun 2024 16:23:18 -0400 Subject: [PATCH 10/11] accounts: remove uninitiated payments If an unknown payment turns out to be a not initiated payment, we should remove it so that the pending balance is freed up for the user. --- accounts/checkers_test.go | 601 ++++++++++++++++++++++++++++++++++++++ accounts/service.go | 37 ++- accounts/service_test.go | 73 ++++- 3 files changed, 694 insertions(+), 17 deletions(-) diff --git a/accounts/checkers_test.go b/accounts/checkers_test.go index e482c1230..c2d503acf 100644 --- a/accounts/checkers_test.go +++ b/accounts/checkers_test.go @@ -5,6 +5,7 @@ import ( "encoding/hex" "fmt" "testing" + "time" "github.com/btcsuite/btcd/chaincfg" "github.com/lightningnetwork/lnd/lnrpc" @@ -29,6 +30,18 @@ var ( 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, } + testHash2 = lntypes.Hash{ + 2, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, + 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, + } + testHash3 = lntypes.Hash{ + 3, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, + 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, + } + testHash4 = lntypes.Hash{ + 4, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, + 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, + } testAmount = &lnrpc.Amount{ Sat: 456, @@ -472,6 +485,594 @@ func TestAccountCheckers(t *testing.T) { } } +// TestSendPaymentCalls performs test coverage on the SendPayment and +// SendPaymentSync checkers. +func TestSendPaymentCalls(t *testing.T) { + t.Run("SendPayment", func(t *testing.T) { + testSendPayment(t, "/lnrpc.Lightning/SendPayment") + }) + + t.Run("SendPaymentSync", func(t *testing.T) { + testSendPayment(t, "/lnrpc.Lightning/SendPaymentSync") + }) +} + +func testSendPayment(t *testing.T, uri string) { + var ( + parentCtx = context.Background() + zeroFee = &lnrpc.FeeLimit{Limit: &lnrpc.FeeLimit_Fixed{ + Fixed: 0, + }} + requestID uint64 + ) + + nextRequestID := func() uint64 { + requestID++ + + return requestID + } + + lndMock := newMockLnd() + errFunc := func(err error) { + lndMock.mainErrChan <- err + } + service, err := NewService(t.TempDir(), errFunc) + require.NoError(t, err) + + err = service.Start(lndMock, lndMock, chainParams) + require.NoError(t, err) + + assertBalance := func(id AccountID, expectedBalance int64) { + acct, err := service.Account(id) + require.NoError(t, err) + + require.Equal(t, expectedBalance, + calcAvailableAccountBalance(acct)) + } + + // This should error because there is no account in the context. + err = service.checkers.checkIncomingRequest( + parentCtx, uri, &lnrpc.SendRequest{}, + ) + require.ErrorContains(t, err, "no account found in context") + + // Create an account and add it to the context. + acct, err := service.NewAccount( + 5000, time.Now().Add(time.Hour), "test", + ) + require.NoError(t, err) + + ctxWithAcct := AddAccountToContext(parentCtx, acct) + + // This should error because there is no request ID in the context. + err = service.checkers.checkIncomingRequest( + ctxWithAcct, uri, &lnrpc.SendRequest{}, + ) + require.ErrorContains(t, err, "no request ID found in context") + + reqID1 := nextRequestID() + ctx := AddRequestIDToContext(ctxWithAcct, reqID1) + + // This should error because no payment hash is provided. + err = service.checkers.checkIncomingRequest( + ctx, uri, &lnrpc.SendRequest{}, + ) + require.ErrorContains(t, err, "a payment hash is required") + + // This should error because of an insufficient account balance. + err = service.checkers.checkIncomingRequest( + ctx, uri, &lnrpc.SendRequest{ + Amt: 1000, + PaymentHash: testHash[:], + }, + ) + require.ErrorContains(t, err, "account balance insufficient") + + // Assert that the balance of the account is still un-changed since none + // of the requests have gone through yet. + assertBalance(acct.ID, 5000) + + // This should work. + err = service.checkers.checkIncomingRequest( + ctx, uri, &lnrpc.SendRequest{ + AmtMsat: 1000, + PaymentHash: testHash[:], + FeeLimit: zeroFee, + }, + ) + require.NoError(t, err) + + // Alright, now assert that the pending amount has been accounted for. + assertBalance(acct.ID, 4000) + + // Try let the same request go through with the same payment hash. This + // should fail and the balance should remain unchanged. + err = service.checkers.checkIncomingRequest( + ctx, uri, &lnrpc.SendRequest{ + AmtMsat: 1000, + PaymentHash: testHash[:], + FeeLimit: zeroFee, + }, + ) + require.ErrorContains(t, err, "is already in flight") + assertBalance(acct.ID, 4000) + + // Now let the response come through for the first request. + _, err = service.checkers.replaceOutgoingResponse( + ctx, uri, &lnrpc.SendResponse{ + PaymentHash: testHash[:], + }, + ) + require.NoError(t, err) + assertBalance(acct.ID, 4000) + + // A repeated response should have no impact. + _, err = service.checkers.replaceOutgoingResponse( + ctx, uri, &lnrpc.SendResponse{ + PaymentHash: testHash[:], + }, + ) + require.NoError(t, err) + assertBalance(acct.ID, 4000) + + lndMock.assertPaymentRequests(t, map[lntypes.Hash]struct{}{ + testHash: {}, + }) + + nextRequestID() + + reqID2 := nextRequestID() + ctx = AddRequestIDToContext(ctxWithAcct, reqID2) + + // Ok now we will test an errored request. First send through a valid + // send request and assert that the available balance is reduced. + err = service.checkers.checkIncomingRequest( + ctx, uri, &lnrpc.SendRequest{ + AmtMsat: 1000, + PaymentHash: testHash2[:], + FeeLimit: zeroFee, + }, + ) + require.NoError(t, err) + assertBalance(acct.ID, 3000) + + // Now return an error response. + _, err = service.checkers.handleErrorResponse( + ctx, uri, nil, + ) + require.NoError(t, err) + + // The balance should have gone back to what it was before the payment + // was initiated. + assertBalance(acct.ID, 4000) + + lndMock.assertNoPaymentRequest(t) + + // The final test we will do is to have two send requests initiated + // before the response for the first one has been received. + reqID3 := nextRequestID() + ctx = AddRequestIDToContext(ctxWithAcct, reqID3) + + err = service.checkers.checkIncomingRequest( + ctx, uri, &lnrpc.SendRequest{ + AmtMsat: 2000, + PaymentHash: testHash3[:], + FeeLimit: zeroFee, + }, + ) + require.NoError(t, err) + assertBalance(acct.ID, 2000) + + reqID4 := nextRequestID() + ctx = AddRequestIDToContext(ctxWithAcct, reqID4) + err = service.checkers.checkIncomingRequest( + ctx, uri, &lnrpc.SendRequest{ + AmtMsat: 2000, + PaymentHash: testHash4[:], + FeeLimit: zeroFee, + }, + ) + require.NoError(t, err) + assertBalance(acct.ID, 0) + + // Ok, now let the response for the second request come through. + _, err = service.checkers.replaceOutgoingResponse( + ctx, uri, &lnrpc.SendResponse{ + PaymentHash: testHash4[:], + }, + ) + require.NoError(t, err) + assertBalance(acct.ID, 0) + + // Let the first request error. + ctx = AddRequestIDToContext(ctxWithAcct, reqID3) + _, err = service.checkers.handleErrorResponse( + ctx, uri, nil, + ) + require.NoError(t, err) + assertBalance(acct.ID, 2000) +} + +// TestSendPaymentV2 performs test coverage on the SendPaymentV2 checker. +func TestSendPaymentV2(t *testing.T) { + var ( + uri = "/routerrpc.Router/SendPaymentV2" + parentCtx = context.Background() + requestID uint64 + ) + + nextRequestID := func() uint64 { + requestID++ + + return requestID + } + + lndMock := newMockLnd() + errFunc := func(err error) { + lndMock.mainErrChan <- err + } + service, err := NewService(t.TempDir(), errFunc) + require.NoError(t, err) + + err = service.Start(lndMock, lndMock, chainParams) + require.NoError(t, err) + + assertBalance := func(id AccountID, expectedBalance int64) { + acct, err := service.Account(id) + require.NoError(t, err) + + require.Equal(t, expectedBalance, + calcAvailableAccountBalance(acct)) + } + + // This should error because there is no account in the context. + err = service.checkers.checkIncomingRequest( + parentCtx, uri, &routerrpc.SendPaymentRequest{}, + ) + require.ErrorContains(t, err, "no account found in context") + + // Create an account and add it to the context. + acct, err := service.NewAccount( + 5000, time.Now().Add(time.Hour), "test", + ) + require.NoError(t, err) + + ctxWithAcct := AddAccountToContext(parentCtx, acct) + + // This should error because there is no request ID in the context. + err = service.checkers.checkIncomingRequest( + ctxWithAcct, uri, &routerrpc.SendPaymentRequest{}, + ) + require.ErrorContains(t, err, "no request ID found in context") + + reqID1 := nextRequestID() + ctx := AddRequestIDToContext(ctxWithAcct, reqID1) + + // This should error because no payment hash is provided. + err = service.checkers.checkIncomingRequest( + ctx, uri, &routerrpc.SendPaymentRequest{}, + ) + require.ErrorContains(t, err, "a payment hash is required") + + // This should error because of an insufficient account balance. + err = service.checkers.checkIncomingRequest( + ctx, uri, &routerrpc.SendPaymentRequest{ + Amt: 1000, + PaymentHash: testHash[:], + }, + ) + require.ErrorContains(t, err, "account balance insufficient") + + // Assert that the balance of the account is still un-changed since none + // of the requests have gone through yet. + assertBalance(acct.ID, 5000) + + // This should work. + err = service.checkers.checkIncomingRequest( + ctx, uri, &routerrpc.SendPaymentRequest{ + AmtMsat: 1000, + PaymentHash: testHash[:], + }, + ) + require.NoError(t, err) + + // Alright, now assert that the pending amount has been accounted for. + assertBalance(acct.ID, 4000) + + // Try let the same request go through with the same payment hash. This + // should fail and the balance should remain unchanged. + err = service.checkers.checkIncomingRequest( + ctx, uri, &routerrpc.SendPaymentRequest{ + AmtMsat: 1000, + PaymentHash: testHash[:], + }, + ) + require.ErrorContains(t, err, "is already in flight") + assertBalance(acct.ID, 4000) + + // Now let the response come through for the first request. + _, err = service.checkers.replaceOutgoingResponse( + ctx, uri, &lnrpc.Payment{ + PaymentHash: hex.EncodeToString(testHash[:]), + }, + ) + require.NoError(t, err) + assertBalance(acct.ID, 4000) + + // A repeated response should have no impact. + _, err = service.checkers.replaceOutgoingResponse( + ctx, uri, &lnrpc.Payment{ + PaymentHash: hex.EncodeToString(testHash[:]), + }, + ) + require.NoError(t, err) + assertBalance(acct.ID, 4000) + + lndMock.assertPaymentRequests(t, map[lntypes.Hash]struct{}{ + testHash: {}, + }) + + nextRequestID() + + reqID2 := nextRequestID() + ctx = AddRequestIDToContext(ctxWithAcct, reqID2) + + // Ok now we will test an errored request. First send through a valid + // send request and assert that the available balance is reduced. + err = service.checkers.checkIncomingRequest( + ctx, uri, &routerrpc.SendPaymentRequest{ + AmtMsat: 1000, + PaymentHash: testHash2[:], + }, + ) + require.NoError(t, err) + assertBalance(acct.ID, 3000) + + // Now return an error response. + _, err = service.checkers.handleErrorResponse(ctx, uri, nil) + require.NoError(t, err) + + // The balance should have gone back to what it was before the payment + // was initiated. + assertBalance(acct.ID, 4000) + + lndMock.assertNoPaymentRequest(t) + + // The final test we will do is to have two send requests initiated + // before the response for the first one has been received. + reqID3 := nextRequestID() + ctx = AddRequestIDToContext(ctxWithAcct, reqID3) + + err = service.checkers.checkIncomingRequest( + ctx, uri, &routerrpc.SendPaymentRequest{ + AmtMsat: 2000, + PaymentHash: testHash3[:], + }, + ) + require.NoError(t, err) + assertBalance(acct.ID, 2000) + + reqID4 := nextRequestID() + ctx = AddRequestIDToContext(ctxWithAcct, reqID4) + err = service.checkers.checkIncomingRequest( + ctx, uri, &routerrpc.SendPaymentRequest{ + AmtMsat: 2000, + PaymentHash: testHash4[:], + }, + ) + require.NoError(t, err) + assertBalance(acct.ID, 0) + + // Ok, now let the response for the second request come through. + _, err = service.checkers.replaceOutgoingResponse( + ctx, uri, &lnrpc.Payment{ + PaymentHash: hex.EncodeToString(testHash4[:]), + }, + ) + require.NoError(t, err) + assertBalance(acct.ID, 0) + + // Let the first request error. + ctx = AddRequestIDToContext(ctxWithAcct, reqID3) + _, err = service.checkers.handleErrorResponse(ctx, uri, nil) + require.NoError(t, err) + assertBalance(acct.ID, 2000) +} + +// TestSendToRouteV2 performs test coverage on the SendToRouteV2 checker. +func TestSendToRouteV2(t *testing.T) { + var ( + uri = "/routerrpc.Router/SendToRouteV2" + parentCtx = context.Background() + requestID uint64 + ) + + nextRequestID := func() uint64 { + requestID++ + + return requestID + } + + lndMock := newMockLnd() + errFunc := func(err error) { + lndMock.mainErrChan <- err + } + service, err := NewService(t.TempDir(), errFunc) + require.NoError(t, err) + + err = service.Start(lndMock, lndMock, chainParams) + require.NoError(t, err) + + assertBalance := func(id AccountID, expectedBalance int64) { + acct, err := service.Account(id) + require.NoError(t, err) + + require.Equal(t, expectedBalance, + calcAvailableAccountBalance(acct)) + } + + // This should error because there is no account in the context. + err = service.checkers.checkIncomingRequest( + parentCtx, uri, &routerrpc.SendToRouteRequest{}, + ) + require.ErrorContains(t, err, "no account found in context") + + // Create an account and add it to the context. + acct, err := service.NewAccount( + 5000, time.Now().Add(time.Hour), "test", + ) + require.NoError(t, err) + + ctxWithAcct := AddAccountToContext(parentCtx, acct) + + // This should error because there is no request ID in the context. + err = service.checkers.checkIncomingRequest( + ctxWithAcct, uri, &routerrpc.SendToRouteRequest{}, + ) + require.ErrorContains(t, err, "no request ID found in context") + + reqID1 := nextRequestID() + ctx := AddRequestIDToContext(ctxWithAcct, reqID1) + + // This should error because no payment hash is provided. + err = service.checkers.checkIncomingRequest( + ctx, uri, &routerrpc.SendToRouteRequest{}, + ) + require.ErrorContains(t, err, "invalid hash length") + + // This should error because of an insufficient account balance. + err = service.checkers.checkIncomingRequest( + ctx, uri, &routerrpc.SendToRouteRequest{ + Route: &lnrpc.Route{ + TotalAmt: 1000, + }, + PaymentHash: testHash[:], + }, + ) + require.ErrorContains(t, err, "account balance insufficient") + + // Assert that the balance of the account is still un-changed since none + // of the requests have gone through yet. + assertBalance(acct.ID, 5000) + + // This should work. + err = service.checkers.checkIncomingRequest( + ctx, uri, &routerrpc.SendToRouteRequest{ + Route: &lnrpc.Route{ + TotalAmtMsat: 1000, + }, + PaymentHash: testHash[:], + }, + ) + require.NoError(t, err) + + // Alright, now assert that the pending amount has been accounted for. + assertBalance(acct.ID, 4000) + + // Try let the same request go through with the same payment hash. This + // should fail and the balance should remain unchanged. + err = service.checkers.checkIncomingRequest( + ctx, uri, &routerrpc.SendToRouteRequest{ + Route: &lnrpc.Route{ + TotalAmtMsat: 1000, + }, + PaymentHash: testHash[:], + }, + ) + require.ErrorContains(t, err, "is already in flight") + assertBalance(acct.ID, 4000) + + // Now let the response come through for the first request. Even though + // this response does not contain the payment hash, it should still be + // linked correctly since we track this in the request values store. + _, err = service.checkers.replaceOutgoingResponse( + ctx, uri, &lnrpc.HTLCAttempt{}, + ) + require.NoError(t, err) + assertBalance(acct.ID, 4000) + + // A repeated response should have no impact. + _, err = service.checkers.replaceOutgoingResponse( + ctx, uri, &lnrpc.HTLCAttempt{}, + ) + require.NoError(t, err) + assertBalance(acct.ID, 4000) + + lndMock.assertPaymentRequests(t, map[lntypes.Hash]struct{}{ + testHash: {}, + }) + + nextRequestID() + + reqID2 := nextRequestID() + ctx = AddRequestIDToContext(ctxWithAcct, reqID2) + + // Ok now we will test an errored request. First send through a valid + // send request and assert that the available balance is reduced. + err = service.checkers.checkIncomingRequest( + ctx, uri, &routerrpc.SendToRouteRequest{ + Route: &lnrpc.Route{ + TotalAmtMsat: 1000, + }, + PaymentHash: testHash2[:], + }, + ) + require.NoError(t, err) + assertBalance(acct.ID, 3000) + + // Now return an error response. + _, err = service.checkers.handleErrorResponse(ctx, uri, nil) + require.NoError(t, err) + + // The balance should have gone back to what it was before the payment + // was initiated. + assertBalance(acct.ID, 4000) + + lndMock.assertNoPaymentRequest(t) + + // The final test we will do is to have two send requests initiated + // before the response for the first one has been received. + reqID3 := nextRequestID() + ctx = AddRequestIDToContext(ctxWithAcct, reqID3) + + err = service.checkers.checkIncomingRequest( + ctx, uri, &routerrpc.SendToRouteRequest{ + Route: &lnrpc.Route{ + TotalAmtMsat: 2000, + }, + PaymentHash: testHash3[:], + }, + ) + require.NoError(t, err) + assertBalance(acct.ID, 2000) + + reqID4 := nextRequestID() + ctx = AddRequestIDToContext(ctxWithAcct, reqID4) + err = service.checkers.checkIncomingRequest( + ctx, uri, &routerrpc.SendToRouteRequest{ + Route: &lnrpc.Route{ + TotalAmtMsat: 2000, + }, + PaymentHash: testHash4[:], + }, + ) + require.NoError(t, err) + assertBalance(acct.ID, 0) + + // Ok, now let the response for the second request come through. + _, err = service.checkers.replaceOutgoingResponse( + ctx, uri, &lnrpc.HTLCAttempt{}, + ) + require.NoError(t, err) + assertBalance(acct.ID, 0) + + // Let the first request error. + ctx = AddRequestIDToContext(ctxWithAcct, reqID3) + _, err = service.checkers.handleErrorResponse(ctx, uri, nil) + require.NoError(t, err) + assertBalance(acct.ID, 2000) +} + // assertMessagesEqual makes sure two proto messages are equal by JSON // serializing them. func assertMessagesEqual(t *testing.T, expected, actual proto.Message) { diff --git a/accounts/service.go b/accounts/service.go index 3cc4ace6e..14f49decb 100644 --- a/accounts/service.go +++ b/accounts/service.go @@ -405,6 +405,15 @@ func (s *InterceptorService) CheckBalance(id AccountID, return ErrAccExpired } + availableAmount := calcAvailableAccountBalance(account) + if availableAmount < int64(requiredBalance) { + return ErrAccBalanceInsufficient + } + + return nil +} + +func calcAvailableAccountBalance(account *OffChainBalanceAccount) int64 { var inFlightAmt int64 for _, payment := range account.Payments { if inflightState(payment.Status) { @@ -415,12 +424,7 @@ func (s *InterceptorService) CheckBalance(id AccountID, } } - availableAmount := account.CurrentBalance - inFlightAmt - if availableAmount < int64(requiredBalance) { - return ErrAccBalanceInsufficient - } - - return nil + return account.CurrentBalance - inFlightAmt } // AssociateInvoice associates a generated invoice with the given account, @@ -641,6 +645,13 @@ func (s *InterceptorService) TrackPayment(id AccountID, hash lntypes.Hash, return nil } + // There is a case where the passed in fullAmt is zero but the pending + // amount is not. In that case, we should not overwrite the pending + // amount. + if fullAmt == 0 { + fullAmt = entry.FullAmount + } + account.Payments[hash] = &PaymentEntry{ Status: lnrpc.Payment_UNKNOWN, FullAmount: fullAmt, @@ -729,6 +740,20 @@ func (s *InterceptorService) TrackPayment(id AccountID, hash lntypes.Hash, log.Debugf("Payment %v not initiated, "+ "stopping tracking", hash) + // We also remove the payment from the + // account, so that the payment won't be + // seen as in-flight balance when + // calculating the account's available + // balance. + err := s.RemovePayment(hash) + if err != nil { + // We don't disable the service + // here, as the worst that can + // happen is that the payment is + // seen as still in-flight. + s.mainErrCallback(err) + } + return } diff --git a/accounts/service_test.go b/accounts/service_test.go index 8116d42ad..66fa5d266 100644 --- a/accounts/service_test.go +++ b/accounts/service_test.go @@ -7,6 +7,7 @@ import ( "time" "github.com/lightninglabs/lndclient" + "github.com/lightningnetwork/lnd/channeldb" invpkg "github.com/lightningnetwork/lnd/invoices" "github.com/lightningnetwork/lnd/lnrpc" "github.com/lightningnetwork/lnd/lntypes" @@ -21,9 +22,6 @@ var ( testInterval = time.Millisecond * 20 testID2 = AccountID{22, 22, 22} - - testHash2 = lntypes.Hash{2, 2, 2, 2, 2} - testHash3 = lntypes.Hash{3, 3, 3, 3, 3} ) type mockLnd struct { @@ -447,18 +445,22 @@ func TestAccountService(t *testing.T) { acct := &OffChainBalanceAccount{ ID: testID, Type: TypeInitialBalance, - CurrentBalance: 1234, + CurrentBalance: 5000, Invoices: AccountInvoices{ testHash: {}, }, Payments: AccountPayments{ testHash: { Status: lnrpc.Payment_IN_FLIGHT, - FullAmount: 1234, + FullAmount: 2000, }, testHash2: { Status: lnrpc.Payment_UNKNOWN, - FullAmount: 3456, + FullAmount: 1000, + }, + testHash3: { + Status: lnrpc.Payment_UNKNOWN, + FullAmount: 2000, }, }, } @@ -473,6 +475,7 @@ func TestAccountService(t *testing.T) { lnd.assertPaymentRequests(t, map[lntypes.Hash]struct{}{ testHash: {}, testHash2: {}, + testHash3: {}, }) lnd.assertInvoiceRequest(t, 0, 0) lnd.assertNoMainErr(t) @@ -481,15 +484,15 @@ func TestAccountService(t *testing.T) { // amount is debited from the account. lnd.paymentChans[testHash] <- lndclient.PaymentStatus{ State: lnrpc.Payment_SUCCEEDED, - Fee: 234, - Value: 1000, + Fee: 500, + Value: 1500, } assertEventually(t, func() bool { acct, err := s.store.Account(testID) require.NoError(t, err) - return acct.CurrentBalance == 0 + return acct.CurrentBalance == 3000 }) // Remove the other payment and make sure it disappears @@ -497,7 +500,7 @@ func TestAccountService(t *testing.T) { // correctly in the account store. lnd.paymentChans[testHash2] <- lndclient.PaymentStatus{ State: lnrpc.Payment_FAILED, - Fee: 234, + Fee: 0, Value: 1000, } @@ -505,7 +508,7 @@ func TestAccountService(t *testing.T) { acct, err := s.store.Account(testID) require.NoError(t, err) - if len(acct.Payments) != 2 { + if len(acct.Payments) != 3 { return false } @@ -518,6 +521,54 @@ func TestAccountService(t *testing.T) { }) require.NotContains(t, s.pendingPayments, testHash2) + + // Finally, if an unknown payment turns out to be + // a non-initiated payment, we should stop the tracking + // of the payment, fail it and remove it from the + // pendingPayments map. As the payment is failed, that + // will ensure that the payment is not considered when + // calculating the in-flight balance for the account. + // First check that the account has an available balance + // of 1000. That means that the payment with testHash3 + // and amount 2000 is still considered to be in-flight. + err := s.CheckBalance(testID, 1000) + require.NoError(t, err) + + err = s.CheckBalance(testID, 1001) + require.ErrorIs(t, err, ErrAccBalanceInsufficient) + + // Now signal that the payment was non-initiated. + lnd.paymentErrChan <- channeldb.ErrPaymentNotInitiated + + // Once the error is handled in the service.TrackPayment + // goroutine, and therefore free up the 2000 in-flight + // balance. + assertEventually(t, func() bool { + bal3000Err := s.CheckBalance(testID, 3000) + bal3001Err := s.CheckBalance(testID, 3001) + require.ErrorIs( + t, bal3001Err, + ErrAccBalanceInsufficient, + ) + + correctBalance := bal3000Err == nil + + // Ensure that the payment is also set to the + // failed status. + acct, err := s.store.Account(testID) + require.NoError(t, err) + + p, ok := acct.Payments[testHash3] + + correctStatus := ok && + p.Status == lnrpc.Payment_FAILED + + return correctBalance && correctStatus + }) + + // Ensure that the payment was removed from the pending + // payments. + require.NotContains(t, s.pendingPayments, testHash3) }, }, { name: "keep track of invoice indexes", From 5e07588b433ef6187acec1f5bf4a55818cd3c3ab Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Thu, 6 Jun 2024 12:49:35 -0400 Subject: [PATCH 11/11] firewalldb: address last few comments from #763 --- firewalldb/kvstores.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/firewalldb/kvstores.go b/firewalldb/kvstores.go index 5b653a200..26ced86e7 100644 --- a/firewalldb/kvstores.go +++ b/firewalldb/kvstores.go @@ -12,7 +12,7 @@ The KVStores are stored in the following structure in the KV db. Note that the `perm` and `temp` buckets are identical in structure. The only difference is that the `temp` bucket is cleared on restart of the db. The reason persisting the temporary store changes instead of just keeping an in-memory store is that -we can then guarantee idempotency if changes are made to both the permanent and +we can then guarantee atomicity if changes are made to both the permanent and temporary stores. rules -> perm -> rule-name -> global -> {k:v} @@ -86,14 +86,14 @@ type KVStoreTx interface { // GlobalTemp is similar to the Global store except that its contents // is cleared upon restart of the database. The reason persisting the // temporary store changes instead of just keeping an in-memory store is - // that we can then guarantee idempotency if changes are made to both + // that we can then guarantee atomicity if changes are made to both // the permanent and temporary stores. GlobalTemp() KVStore // LocalTemp is similar to the Local store except that its contents is // cleared upon restart of the database. The reason persisting the // temporary store changes instead of just keeping an in-memory store is - // that we can then guarantee idempotency if changes are made to both + // that we can then guarantee atomicity if changes are made to both // the permanent and temporary stores. LocalTemp() KVStore }