[abhayguptas]

Change Description

This PR fixes a bug in the invoice registry where AMP sub-invoices that are already in a terminal state (e.g., canceled due to MPP timeout) incorrectly accept new HTLC shards.

Because AMP invoices allow multiple concurrent sub-invoices (tracked via setID), the parent invoice remains ContractOpen. Previously, the updateMpp handler only checked the parent invoice state, which caused late-arriving HTLCs for canceled setIDs to be accepted instead of failed back. This led to stuck funds because the sub-invoice was already terminal.

The fix moves the AMPState validation after the paymentAddr check in updateMpp, guaranteeing that HTLCs belonging to canceled sub-invoices are immediately rejected with ResultInvoiceAlreadyCanceled. The regression test testFailPartialAMPPayment was also updated to explicitly verify this rejection behavior and remove its previous buggy workaround, and a check for ResultAddressMismatch on canceled sub-invoices was added.

Resolves an inline TODO(ziggie) from the test suite acknowledging the bug.

Steps to Test

1. Run go test -v -run "TestInvoiceRegistry/FailPartialAMPPayment" ./invoices/... to verify the new test logic passes on the KV backend.
2. Review the testFailPartialAMPPayment test in invoices/invoiceregistry_test.go to confirm that late-arriving HTLCs are successfully rejected instead of added.

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change is not insubstantial. Typo fixes are not accepted to fight bot spam.
• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.
• Any new lncli commands have appropriate tags in the comments for the rpc in the proto file.
• There is a change description in the release notes, or [skip ci] in the commit message for small changes.

fixes #11176

[github-actions]

🟠 PR Severity: HIGH

lines changed | 3 files | 69 additions, 41 deletions

🟠 High (1 file)

• invoices/update.go - modifies AMP sub-invoice HTLC acceptance/rejection logic in updateMpp, affecting invoice settlement correctness

🟢 Low (2 files)

• invoices/invoiceregistry_test.go - test-only change updating regression coverage for the fix
• docs/release-notes/release-notes-0.22.0.md - release notes entry

Analysis

The highest-severity file is invoices/update.go, which falls under invoices/* (invoice management and settlement) — a HIGH-severity package per the classification rules. The change reorders validation in updateMpp so that HTLCs arriving for an already-canceled AMP sub-invoice (identified by setID) are rejected with ResultInvoiceAlreadyCanceled instead of being incorrectly accepted, fixing a stuck-funds bug. The diff is small (69/41 lines across 3 files, 1 non-test source file) so no severity bump applies. The accompanying test and release-note changes are LOW severity and don't affect the overall classification.

---

_{To override, add a severity-override-{critical,high,medium,low} label.}

[Lrifton92]

Reviewed at 5be3c73.

The placement of the new AMPState check looks right to me: it sits after the ContractOpen and payment-address checks, so a wrong payment_addr still fails with ResultAddressMismatch before any per-setID state is revealed, and the new test pins that ordering. ResultInvoiceAlreadyCanceled is not in IsSetFailure(), so the registry won't re-fan-out fail resolutions to the already-canceled HTLCs of that set (invoiceregistry.go ~L1237), which is what we want here since those were resolved when the set was canceled. Dropping the require.Eventually is also fine now that the rejection is a direct resolution.

One question on scope. The comment says "terminal state (e.g. canceled)" but the check only handles HtlcStateCanceled. InvoiceStateAMP.State can also be HtlcStateSettled, and a late shard for an already-settled setID still reaches the accept path below: inv.HTLCSet(setID, HtlcStateAccepted) is empty for a settled set, so newSetTotal is just the new shard, and it is either recorded as a partial (and then sits until the MPP timeout) or, if a single shard covers totalAmt, goes through reconstructAMPPreimages on a one-element set. I did not find a guard for that in updateMpp or in the registry (ErrDuplicateSetID is about a setID indexed by another invoice, not about a settled set), but I have not traced the store layer, so it may be handled there.

Is leaving the settled case out intentional? If yes, narrowing the comment to "canceled" would avoid implying settled is covered. If not, HtlcStateSettled could fail with ResultInvoiceAlreadySettled in the same branch, with a mirror of the new test asserting the shard is rejected and len(inv.Htlcs) is unchanged.

[abhayguptas]

Thanks for the review. I’ve extended the fix to also reject HTLCs for already-settled AMP sub-invoices, with ResultInvoiceAlreadySettled, and added regression coverage for that case. I also kept the payment-address check before the AMP state check as suggested.

[Lrifton92]

Re-reviewed at 9550bd9.

The extension to the settled state looks correct to me:

• updateMpp now switches on ampState.State and fails fast with ResultInvoiceAlreadyCanceled / ResultInvoiceAlreadySettled for a terminal sub-invoice. The check still sits after the ContractOpen and payment-address checks, so a wrong payment_addr keeps failing with ResultAddressMismatch before any per-setID state is revealed (same ordering I asked about on 5be3c73).
• The new block in testFailPartialAMPPayment settles a fresh setID with a full-amount HTLC, then sends a second HTLC on the same setID and asserts ResultInvoiceAlreadySettled, that the HTLC is not persisted (len(inv.Htlcs) == 3), and that the parent invoice stays ContractOpen. That pins both the outcome and the "no state leak" property.

ResultInvoiceAlreadySettled already exists in resolution_result.go with the matching String() case, so no new outcome to wire up elsewhere.

LGTM.

[saubyk]

Thanks for opening this, @abhayguptas.

Our contribution guidelines note that PRs from new contributors aren't prioritized for review at the moment. With the backlog we have, that means closing this instead of leaving it in limbo.

If you hit an actual failure that motivated this change, an issue with the reproduction details (lnd version, backend, logs, expected vs. observed behavior) would be much more useful to us than the patch on its own. We'll triage it from there.

The way to get future PRs reviewed promptly is to build some history first: PR reviews and issue triage are where that usually starts. Once there's a track record, new code from you becomes much easier to prioritize.