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.

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

Strict forwarding pt.2 #1244

Merged
merged 3 commits into from
Dec 13, 2024

Conversation

GeorgeTsagk
Copy link
Member

@GeorgeTsagk GeorgeTsagk commented Dec 6, 2024

Description

In a previous PR we added strict forwarding support, meaning that an invoice that asks for asset X may only be settled if the HTLCs are carrying asset X. We did not think of strict forwarding protection for btc invoices being paid for with asset HTLCs.

Solution

In this PR we extend the AuxInvoiceManager with an extra check, which cancels the HTLCs that are paying to this invoice if the invoice is a btc invoice, but the HTLCs are carrying assets.

Related Issues

Closes #1239

@GeorgeTsagk GeorgeTsagk self-assigned this Dec 6, 2024
@GeorgeTsagk
Copy link
Member Author

Will add some Litd itest coverage too

@coveralls
Copy link

coveralls commented Dec 6, 2024

Pull Request Test Coverage Report for Build 12313714722

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • 12 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.03%) to 40.722%

Files with Coverage Reduction New Missed Lines %
tapchannel/aux_leaf_signer.go 3 43.43%
tapgarden/caretaker.go 4 68.87%
universe/interface.go 5 50.65%
Totals Coverage Status
Change from base Build 12295795007: 0.03%
Covered Lines: 25831
Relevant Lines: 63433

💛 - Coveralls

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🏆

@Roasbeef
Copy link
Member

We'll merge this after the litd itests are more stable.

Previously we had strict-forwarding protection placed in the
AuxInvoiceManager, but that would only protect asset invoices against
pure-btc HTLCs. This commit adds the inverse complementary check which
prevents settling the invoice if btc ras requested but assets were
offered.
In this commit we add an extra test case that checks if the
AuxInvoiceManager will reject the asset HTLCs that pay to a btc invoice.
We also do some housekeeping (commonly used vars, comments) across the
other test cases.
In this commit we extend the property based tests to account for the new
piece of AuxInvoiceManager behavior. If the randomly generated invoice
is not an asset invoice, but the HTLCs do carry assets, we should expect
the manager to reject the HTLCs.
@ffranr ffranr removed their request for review December 12, 2024 15:49
@GeorgeTsagk
Copy link
Member Author

Since this is breaking the current expected behavior from LitD itests, we are not going to see a green Lit tests check

Refer to the respective LitD PR for the correct state of the itests
lightninglabs/lightning-terminal#916

@GeorgeTsagk
Copy link
Member Author

LitD itests are green, merging this

@GeorgeTsagk GeorgeTsagk added this pull request to the merge queue Dec 13, 2024
Merged via the queue into lightninglabs:main with commit 7358c1b Dec 13, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[bug]: Unexpected Payment Success: Invoicing Sats but Paying with Assets
5 participants