invoices: cancel only the failing AMP set on reconstruction failure
What changed, and why it matters
This commit fixes a bug in LND's invoice handling for reusable AMP (Atomic Multi-Path) invoices. Previously, if one payment set failed to reconstruct its preimage, the entire invoice was canceled, even though other valid payment sets on the same invoice were unaffected. Now, only the failing set is canceled, and the invoice stays open so other payers can still complete their payments. This is a correctness and availability fix for a specific Lightning Network payment feature.
Review and merge the patch; ensure CancelHTLCsUpdate correctly handles all edge cases for reusable AMP invoices and does not leave orphaned HTLCs in an unexpected state.
Security signals we found
Denial-of-service-like availability impact: a malicious or buggy payer could cause a whole reusable AMP invoice to be canceled, blocking legitimate concurrent payers
Set-local failure incorrectly escalated to invoice-global cancellation
Regression test added for concurrent-set survival after reconstruction failure
Evidence from the diff
In invoices/update.go, updateMpp previously issued a CancelInvoiceUpdate with ContractCanceled when AMP preimage reconstruction failed. For reusable static AMP invoices, this was overly broad: it canceled the whole invoice and any accepted HTLCs from unrelated sets. The patch changes the failure path to issue a CancelHTLCsUpdate scoped to the failing set’s HTLCs, sets AddHtlcs to nil, and keeps the invoice in ContractOpen. A regression test verifies that a concurrently accepted set on the same invoice survives the failure and can still settle.
Changed components
invoices/update.goinvoices/invoiceregistry_test.goLND AMP invoice settlement logicInspect captured patch +141 / −4
### invoices/invoiceregistry_test.go
@@ -118,6 +118,10 @@ func TestInvoiceRegistry(t *testing.T) {
name: "CancelAMPInvoicePendingHTLCs",
test: testCancelAMPInvoicePendingHTLCs,
},
+ {
+ name: "AmpReconstructionFailCancelsSetOnly",
+ test: testAmpReconstructionFailCancelsSetOnly,
+ },
}
makeKeyValueDB := func(t *testing.T) (invpkg.InvoiceDB,
@@ -2607,3 +2611,122 @@ func testCancelAMPInvoicePendingHTLCs(t *testing.T,
"expected HTLC to be canceled")
}
}
+
+// testAmpReconstructionFailCancelsSetOnly tests that an AMP set which fails
+// reconstruction only cancels that set: the invoice stays open and
+// concurrently accepted sets from other payers remain intact and settleable.
+func testAmpReconstructionFailCancelsSetOnly(t *testing.T,
+ makeDB func(t *testing.T) (invpkg.InvoiceDB, *clock.TestClock)) {
+
+ t.Parallel()
+ defer timeout()()
+
+ ctx := newTestContext(t, nil, makeDB)
+ ctxb := t.Context()
+
+ const expiry = uint32(testCurrentHeight + 20)
+
+ var payAddr [32]byte
+ _, err := rand.Read(payAddr[:])
+ require.NoError(t, err)
+
+ // Create a reusable static AMP invoice.
+ ampInvoice := newInvoice(t, false, true)
+ ampInvoice.Terms.PaymentAddr = payAddr
+
+ _, err = ctx.registry.AddInvoice(
+ ctxb, ampInvoice, testInvoicePaymentHash,
+ )
+ require.NoError(t, err)
+
+ // Payer A starts a two-shard payment under setID A with valid AMP
+ // shares.
+ var sharer amp.Sharer
+ sharer, err = amp.NewSeedSharer()
+ require.NoError(t, err)
+
+ left, sharer, err := sharer.Split()
+ require.NoError(t, err)
+
+ var setIDA [32]byte
+ _, err = rand.Read(setIDA[:])
+ require.NoError(t, err)
+
+ childA0 := left.Child(0)
+ childA1 := sharer.Child(1)
+
+ hodlChanA0 := make(chan interface{}, 1)
+ payloadA0 := &mockPayload{
+ mpp: record.NewMPP(testInvoiceAmount, payAddr),
+ amp: record.NewAMP(childA0.Share, setIDA, 0),
+ }
+
+ // The first shard is incomplete, so it is accepted and hodl'd without
+ // any reconstruction attempt.
+ res, err := ctx.registry.NotifyExitHopHtlc(
+ childA0.Hash, testInvoiceAmount/2, expiry, testCurrentHeight,
+ getCircuitKey(1), hodlChanA0, nil, payloadA0,
+ )
+ require.NoError(t, err)
+ require.Nil(t, res, "payer A partial HTLC should be hodl'd")
+
+ // Payer B sends a complete-value set under a different setID with a
+ // blank root share, which fails reconstruction. This must only fail
+ // that set, not cancel the invoice.
+ var setIDB [32]byte
+ _, err = rand.Read(setIDB[:])
+ require.NoError(t, err)
+
+ payloadB := &mockPayload{
+ mpp: record.NewMPP(testInvoiceAmount, payAddr),
+ amp: record.NewAMP([32]byte{}, setIDB, 0),
+ }
+
+ res, err = ctx.registry.NotifyExitHopHtlc(
+ lntypes.Hash{2}, testInvoiceAmount, expiry, testCurrentHeight,
+ getCircuitKey(2), nil, nil, payloadB,
+ )
+ require.NoError(t, err)
+ require.NotNil(t, res, "invalid HTLC should fail directly")
+ checkFailResolution(t, res, invpkg.ResultAmpReconstruction)
+
+ // The invoice must remain open, and payer A's accepted HTLC must be
+ // untouched.
+ inv, err := ctx.registry.LookupInvoice(ctxb, testInvoicePaymentHash)
+ require.NoError(t, err)
+ require.Equal(t, invpkg.ContractOpen, inv.State,
+ "invoice must stay open on set-local reconstruction failure")
+
+ htlcA0, ok := inv.Htlcs[getCircuitKey(1)]
+ require.True(t, ok)
+ require.Equal(t, invpkg.HtlcStateAccepted, htlcA0.State,
+ "concurrent set's HTLC must remain accepted")
+
+ // Payer A's set can still complete and settle.
+ payloadA1 := &mockPayload{
+ mpp: record.NewMPP(testInvoiceAmount, payAddr),
+ amp: record.NewAMP(childA1.Share, setIDA, 1),
+ }
+
+ res, err = ctx.registry.NotifyExitHopHtlc(
+ childA1.Hash, testInvoiceAmount/2, expiry, testCurrentHeight,
+ getCircuitKey(3), nil, nil, payloadA1,
+ )
+ require.NoError(t, err)
+ require.NotNil(t, res)
+ checkSettleResolution(t, res, childA1.Preimage)
+
+ // The first shard of payer A's set is settled as well.
+ resolution, ok := (<-hodlChanA0).(invpkg.HtlcResolution)
+ require.True(t, ok)
+ require.NotNil(t, resolution)
+ checkSettleResolution(t, resolution, childA0.Preimage)
+
+ inv, err = ctx.registry.LookupInvoice(ctxb, testInvoicePaymentHash)
+ require.NoError(t, err)
+ require.Equal(t, invpkg.ContractOpen, inv.State,
+ "AMP invoice remains open after settling a set")
+ require.Equal(
+ t, invpkg.HtlcStateSettled, inv.AMPState[setIDA].State,
+ )
+}
### invoices/update.go
@@ -335,11 +335,25 @@ func updateMpp(ctx *invoiceUpdateCtx, inv *Invoice) (*InvoiceUpdateDesc,
var failRes *HtlcFailResolution
htlcPreimages, failRes = reconstructAMPPreimages(ctx, htlcSet)
if failRes != nil {
- update.UpdateType = CancelInvoiceUpdate
- update.State = &InvoiceStateUpdateDesc{
- NewState: ContractCanceled,
- SetID: setID,
+ // Reconstruction failure is a set-local condition: only
+ // the HTLCs of this set can never settle. Cancel just
+ // this set and keep the invoice open, since reusable
+ // static AMP invoices may carry other accepted sets
+ // that are unaffected by the failure. The current HTLC
+ // is failed directly via failRes and was never added
+ // to the invoice, so it is not part of the cancel set.
+ cancelHtlcs := make(
+ map[CircuitKey]struct{}, len(htlcSet),
+ )
+ for key := range htlcSet {
+ cancelHtlcs[key] = struct{}{}
}
+
+ update.UpdateType = CancelHTLCsUpdate
+ update.CancelHtlcs = cancelHtlcs
+ update.AddHtlcs = nil
+ update.SetID = (*SetID)(setID)
+
return &update, failRes, nil
}
Why this scored 60/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.