Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

invoices: amp invoices bugfix. #9459

Merged
merged 2 commits into from
Jan 31, 2025
Merged

invoices: amp invoices bugfix. #9459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
118261a
invoices+channeldb: Fix AMP invoices behaviour.
ziggie1984 Jan 30, 2025
715cafa
docs: add release-notes.
ziggie1984 Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 11 additions & 13 deletions channeldb/invoices.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,18 +650,13 @@ func (d *DB) UpdateInvoice(_ context.Context, ref invpkg.InvoiceRef,
return err
}

// If the set ID hint is non-nil, then we'll use that to filter
// out the HTLCs for AMP invoice so we don't need to read them
// all out to satisfy the invoice callback below. If it's nil,
// then we pass in the zero set ID which means no HTLCs will be
// read out.
var invSetID invpkg.SetID

if setIDHint != nil {
invSetID = *setIDHint
}
// setIDHint can also be nil here, which means all the HTLCs
// for AMP invoices are fetched. If the blank setID is passed
// in, then no HTLCs are fetched for the AMP invoice. If a
// specific setID is passed in, then only the HTLCs for that
// setID are fetched for a particular sub-AMP invoice.
invoice, err := fetchInvoice(
invoiceNum, invoices, []*invpkg.SetID{&invSetID}, false,
invoiceNum, invoices, []*invpkg.SetID{setIDHint}, false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that this changes behavior slightly as previously we'd pass in a pointer to an empty set id if the hint was nil whereas now we pass in the nil which isn't considered the same as an empty set id. PTAL here on how it is used within this package:

lnd/channeldb/invoices.go

Line 1579 in 32cdbb4

if len(setIDs) != 0 && setIDs[0] != nil &&

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I know this is a change to the logic however I think it was a bug in the first place, for the UpdateInvoice functionality. Because UpdateInvoice will always update a state. Normally we should always have the setID set for AMP invoices. However now that we cancel also the AMP invoice we need to make sure we consider all HTLCs according all setIDs if we cancel the invoice (in case multiple attempts are in the accepted state across multiple setIDs).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moreover if it is not a AMP invoice in the first place this setting is irrelevant anyways:

We already prevent fetching it for non-AMP invoices here in the kv store:

lnd/channeldb/invoices.go

Lines 1541 to 1543 in f4bf99b

if !invoice.IsAMP() {
return invoice, nil
}

And also in the sql store:

lnd/invoices/sql_store.go

Lines 1556 to 1569 in f4bf99b

if invoice.IsAMP() {
invoiceID := int64(invoice.AddIndex)
ampState, ampHtlcs, err := fetchAmpState(
ctx, db, invoiceID, setID, fetchAmpHtlcs,
)
if err != nil {
return nil, nil, err
}
invoice.AMPState = ampState
invoice.Htlcs = ampHtlcs
return hash, invoice, nil
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Discussed this offline too. It's probably the best if we remove the HtlcSetBlankModifier altogether as it is not used anywhere.

)
if err != nil {
return err
Expand Down Expand Up @@ -691,7 +686,7 @@ func (d *DB) UpdateInvoice(_ context.Context, ref invpkg.InvoiceRef,
// If this is an AMP update, then limit the returned AMP state
// to only the requested set ID.
if setIDHint != nil {
filterInvoiceAMPState(updatedInvoice, &invSetID)
filterInvoiceAMPState(updatedInvoice, setIDHint)
}

return nil
Expand Down Expand Up @@ -848,7 +843,10 @@ func (k *kvInvoiceUpdater) Finalize(updateType invpkg.UpdateType) error {
return k.storeSettleHodlInvoiceUpdate()

case invpkg.CancelInvoiceUpdate:
return k.serializeAndStoreInvoice()
// Persist all changes which where made when cancelling the
// invoice. All HTLCs which were accepted are now canceled, so
// we persist this state.
return k.storeCancelHtlcsUpdate()
}

return fmt.Errorf("unknown update type: %v", updateType)
Expand Down
54 changes: 52 additions & 2 deletions docs/release-notes/release-notes-0.18.5.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,58 @@
## Bug Fixes
# Release Notes
- [Bug Fixes](#bug-fixes)
- [New Features](#new-features)
- [Functional Enhancements](#functional-enhancements)
- [RPC Additions](#rpc-additions)
- [lncli Additions](#lncli-additions)
- [Improvements](#improvements)
- [Functional Updates](#functional-updates)
- [RPC Updates](#rpc-updates)
- [lncli Updates](#lncli-updates)
- [Breaking Changes](#breaking-changes)
- [Performance Improvements](#performance-improvements)
- [Technical and Architectural Updates](#technical-and-architectural-updates)
- [BOLT Spec Updates](#bolt-spec-updates)
- [Testing](#testing)
- [Database](#database)
- [Code Health](#code-health)
- [Tooling and Documentation](#tooling-and-documentation)

# Bug Fixes

* [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/9459) where we
would not cancel accepted HTLCs on AMP invoices if the whole invoice was
canceled.

# New Features

## Functional Enhancements

## RPC Additions

## lncli Additions


# Improvements
## Functional Updates
## RPC Updates

## lncli Updates
## Code Health
## Breaking Changes
## Performance Improvements

# Technical and Architectural Updates
## BOLT Spec Updates

## Testing
## Database
## Code Health

* [Improved user experience](https://github.com/lightningnetwork/lnd/pull/9454)
by returning a custom error code when HTLC carries incorrect custom records.

## Tooling and Documentation

# Contributors (Alphabetical Order)

* Ziggie
* Ziggie
5 changes: 5 additions & 0 deletions invoices/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ type InvoiceDB interface {
// passed payment hash. If an invoice matching the passed payment hash
// doesn't exist within the database, then the action will fail with a
// "not found" error.
// The setIDHint is used to signal whether AMP HTLCs should be fetched
// for the invoice. If a blank setID is passed no HTLCs will be fetched
// in case of an AMP invoice. Nil means all HTLCs for all sub AMP
// invoices will be fetched and if a specific setID is supplied only
// HTLCs for that setID will be fetched.
//
// The update is performed inside the same database transaction that
// fetches the invoice and is therefore atomic. The fields to update
Expand Down
31 changes: 25 additions & 6 deletions invoices/invoiceregistry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,10 @@ func (i *InvoiceRegistry) cancelSingleHtlc(invoiceRef InvoiceRef,
// Try to mark the specified htlc as canceled in the invoice database.
// Intercept the update descriptor to set the local updated variable. If
// no invoice update is performed, we can return early.
// setID is only set for AMP HTLCs, so it can be nil and it is expected
// to be nil for non-AMP HTLCs.
setID := (*SetID)(invoiceRef.SetID())

var updated bool
invoice, err := i.idb.UpdateInvoice(
context.Background(), invoiceRef, setID,
Expand Down Expand Up @@ -1014,6 +1017,9 @@ func (i *InvoiceRegistry) notifyExitHopHtlcLocked(
HtlcResolution, invoiceExpiry, error) {

invoiceRef := ctx.invoiceRef()

// This setID is only set for AMP HTLCs, so it can be nil and it is
// also expected to be nil for non-AMP HTLCs.
setID := (*SetID)(ctx.setID())

// We need to look up the current state of the invoice in order to send
Expand Down Expand Up @@ -1374,7 +1380,15 @@ func (i *InvoiceRegistry) SettleHodlInvoice(ctx context.Context,

hash := preimage.Hash()
invoiceRef := InvoiceRefByHash(hash)
invoice, err := i.idb.UpdateInvoice(ctx, invoiceRef, nil, updateInvoice)

// AMP hold invoices are not supported so we set the setID to nil.
// For non-AMP invoices this parameter is ignored during the fetching
// of the database state.
setID := (*SetID)(nil)

invoice, err := i.idb.UpdateInvoice(
ctx, invoiceRef, setID, updateInvoice,
)
if err != nil {
log.Errorf("SettleHodlInvoice with preimage %v: %v",
preimage, err)
Expand Down Expand Up @@ -1458,10 +1472,14 @@ func (i *InvoiceRegistry) cancelInvoiceImpl(ctx context.Context,
}, nil
}

// If it's an AMP invoice we need to fetch all AMP HTLCs here so that
// we can cancel all of HTLCs which are in the accepted state across
// different setIDs.
setID := (*SetID)(nil)
invoiceRef := InvoiceRefByHash(payHash)

// We pass a nil setID which means no HTLCs will be read out.
invoice, err := i.idb.UpdateInvoice(ctx, invoiceRef, nil, updateInvoice)
invoice, err := i.idb.UpdateInvoice(
ctx, invoiceRef, setID, updateInvoice,
)

// Implement idempotency by returning success if the invoice was already
// canceled.
Expand All @@ -1487,8 +1505,8 @@ func (i *InvoiceRegistry) cancelInvoiceImpl(ctx context.Context,
// that are waiting for resolution. Any htlcs that were already canceled
// before, will be notified again. This isn't necessary but doesn't hurt
// either.
//
// TODO(ziggie): Also consider AMP HTLCs here.
// For AMP invoices we fetched all AMP HTLCs for all sub AMP invoices
// here so we can clean up all of them.
for key, htlc := range invoice.Htlcs {
if htlc.State != HtlcStateCanceled {
continue
Expand All @@ -1500,6 +1518,7 @@ func (i *InvoiceRegistry) cancelInvoiceImpl(ctx context.Context,
),
)
}

i.notifyClients(payHash, invoice, nil)

// Attempt to also delete the invoice if requested through the registry
Expand Down
131 changes: 131 additions & 0 deletions invoices/invoiceregistry_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,10 @@ func TestInvoiceRegistry(t *testing.T) {
name: "FailPartialAMPPayment",
test: testFailPartialAMPPayment,
},
{
name: "CancelAMPInvoicePendingHTLCs",
test: testCancelAMPInvoicePendingHTLCs,
},
}

makeKeyValueDB := func(t *testing.T) (invpkg.InvoiceDB,
Expand Down Expand Up @@ -2441,3 +2445,130 @@ func testFailPartialAMPPayment(t *testing.T,
"expected HTLC to be canceled")
}
}

// testCancelAMPInvoicePendingHTLCs tests the case where an AMP invoice is
// canceled and the remaining HTLCs are also canceled so that no HTLCs are left
// in the accepted state.
func testCancelAMPInvoicePendingHTLCs(t *testing.T,
makeDB func(t *testing.T) (invpkg.InvoiceDB, *clock.TestClock)) {

t.Parallel()

ctx := newTestContext(t, nil, makeDB)
ctxb := context.Background()

const (
expiry = uint32(testCurrentHeight + 20)
numShards = 4
)

var (
shardAmt = testInvoiceAmount / lnwire.MilliSatoshi(numShards)
payAddr [32]byte
)
_, err := rand.Read(payAddr[:])
require.NoError(t, err)

// Create an AMP invoice we are going to pay via a multi-part payment.
ampInvoice := newInvoice(t, false, true)

// An AMP invoice is referenced by the payment address.
ampInvoice.Terms.PaymentAddr = payAddr

_, err = ctx.registry.AddInvoice(
ctxb, ampInvoice, testInvoicePaymentHash,
)
require.NoError(t, err)

htlcPayloadSet1 := &mockPayload{
mpp: record.NewMPP(testInvoiceAmount, payAddr),
// We are not interested in settling the AMP HTLC so we don't
// use valid shares.
amp: record.NewAMP([32]byte{1}, [32]byte{1}, 1),
}

// Send first HTLC which pays part of the invoice.
hodlChan1 := make(chan interface{}, 1)
resolution, err := ctx.registry.NotifyExitHopHtlc(
lntypes.Hash{1}, shardAmt, expiry, testCurrentHeight,
getCircuitKey(1), hodlChan1, nil, htlcPayloadSet1,
)
require.NoError(t, err)
require.Nil(t, resolution, "did not expect direct resolution")

htlcPayloadSet2 := &mockPayload{
mpp: record.NewMPP(testInvoiceAmount, payAddr),
// We are not interested in settling the AMP HTLC so we don't
// use valid shares.
amp: record.NewAMP([32]byte{2}, [32]byte{2}, 1),
}

// Send htlc 2 which should be added to the invoice as expected.
hodlChan2 := make(chan interface{}, 1)
resolution, err = ctx.registry.NotifyExitHopHtlc(
lntypes.Hash{2}, shardAmt, expiry, testCurrentHeight,
getCircuitKey(2), hodlChan2, nil, htlcPayloadSet2,
)
require.NoError(t, err)
require.Nil(t, resolution, "did not expect direct resolution")

require.Eventuallyf(t, func() bool {
inv, err := ctx.registry.LookupInvoice(
ctxb, testInvoicePaymentHash,
)
require.NoError(t, err)

return len(inv.Htlcs) == 2
}, testTimeout, time.Millisecond*100, "HTLCs not added to invoice")

// expire the invoice here.
ctx.clock.SetTime(testTime.Add(65 * time.Minute))

// Expect HLTC 1 to be canceled via the MPPTimeout fail resolution.
select {
case resolution := <-hodlChan1:
htlcResolution, _ := resolution.(invpkg.HtlcResolution)
_, ok := htlcResolution.(*invpkg.HtlcFailResolution)
require.True(
t, ok, "expected fail resolution, got: %T", resolution,
)

case <-time.After(testTimeout):
t.Fatal("timeout waiting for HTLC resolution")
}

// Expect HLTC 2 to be canceled via the MPPTimeout fail resolution.
select {
case resolution := <-hodlChan2:
htlcResolution, _ := resolution.(invpkg.HtlcResolution)
_, ok := htlcResolution.(*invpkg.HtlcFailResolution)
require.True(
t, ok, "expected fail resolution, got: %T", resolution,
)

case <-time.After(testTimeout):
t.Fatal("timeout waiting for HTLC resolution")
}

require.Eventuallyf(t, func() bool {
inv, err := ctx.registry.LookupInvoice(
ctxb, testInvoicePaymentHash,
)
require.NoError(t, err)

return inv.State == invpkg.ContractCanceled
}, testTimeout, time.Millisecond*100, "invoice not canceled")

// Fetch the invoice again and compare the number of cancelled HTLCs.
inv, err := ctx.registry.LookupInvoice(
ctxb, testInvoicePaymentHash,
)
require.NoError(t, err)

// Make sure all HTLCs are in the cancelled state.
require.Len(t, inv.Htlcs, 2)
for _, htlc := range inv.Htlcs {
require.Equal(t, invpkg.HtlcStateCanceled, htlc.State,
"expected HTLC to be canceled")
}
}
20 changes: 16 additions & 4 deletions invoices/sql_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1406,14 +1406,26 @@ func (i *SQLStore) UpdateInvoice(ctx context.Context, ref InvoiceRef,

txOpt := SQLInvoiceQueriesTxOptions{readOnly: false}
txErr := i.db.ExecTx(ctx, &txOpt, func(db SQLInvoiceQueries) error {
if setID != nil {
// Make sure to use the set ID if this is an AMP update.
switch {
// For the default case we fetch all HTLCs.
case setID == nil:
ref.refModifier = DefaultModifier

// If the setID is the blank but NOT nil, we set the
// refModifier to HtlcSetBlankModifier to fetch no HTLC for the
// AMP invoice.
case *setID == BlankPayAddr:
ref.refModifier = HtlcSetBlankModifier

// A setID is provided, we use the refModifier to fetch only
// the HTLCs for the given setID and also make sure we add the
// setID to the ref.
default:
var setIDBytes [32]byte
copy(setIDBytes[:], setID[:])
ref.setID = &setIDBytes

// If we're updating an AMP invoice, we'll also only
// need to fetch the HTLCs for the given set ID.
// We only fetch the HTLCs for the given setID.
ref.refModifier = HtlcSetOnlyModifier
}

Expand Down
Loading
Loading