Add Custom Error msg and Prioritise replayed HTLCs #9454
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
guggero
reviewed
Jan 28, 2025
|
ziggie1984
force-pushed
the
add_custom_error_msg
branch
from
4383342 to
955facb
Compare
ziggie1984
added
amp
testing
ziggie1984
force-pushed
the
add_custom_error_msg
branch
4 times, most recently
from
5b53ddb to
7e0be90
Compare
Roasbeef
reviewed
Jan 29, 2025
yyforyongyu
reviewed
Jan 29, 2025
We make sure that HTLCs which have already been decided upon are resolved before before allowing the external interceptor to potentially cancel them back. This makes the implementation for the external HTLC interceptor more streamlined.
We introduce a new specific fail resolution error when the external HTLC interceptor denies the incoming HTLC. Moreover we introduce a new traffic shaper method which moves the implementation of asset HTLC to the external layers. Moreover itests are adopted to reflect this new change.
ziggie1984
force-pushed
the
add_custom_error_msg
branch
from
7e0be90 to
981a9d9
Compare
ziggie1984
force-pushed
the
add_custom_error_msg
branch
3 times, most recently
from
6ba3a46 to
4bfa305
Compare
|
hmm looks like the linter failed, plus this new flake, think it's related? |
The invoiceregistry test suite also includes unit tests for multi part payment especially also including payments to AMP invoices.
We always fetch the HTLCs for the specific setID, so there is no need to keep this code. In earlier versions we would call the UpdateInvoice method with `nil` for the setID therefore we had to lookup the AMPState. However this was error prune because in case one partial payment times-out the AMPState would change to cancelled and that could lead to not resolve HTLCs.
We need to make sure if we cancel an AMP invoice we also cancel all remaining HTLCs back.
ziggie1984
force-pushed
the
add_custom_error_msg
branch
from
4bfa305 to
91014ea
Compare
|
Yes it's related, I was relying on the kvdb.Update ordering on the Application level, however this is never gruanted. So I had to change the test a bit so it never runs in this scenario. |
yyforyongyu
approved these changes
Jan 30, 2025
| log.Debugf("Cancelling htlc (%v) of invoice(%v) with "+ | ||
| "resolution: %v", key, invoiceRef, result) | ||
| log.Debugf("Signaling htlc(%v) cancellation of invoice(%v) "+ | ||
| "with resolution(%v) to the link subsystem", key, |
There was a problem hiding this comment.
nit: it's not necessary the link system?
| log.Debugf("cancelSingleHtlc: invoice %v no longer "+ | ||
| "open", invoiceRef) | ||
| log.Debugf("CancelSingleHtlc: cannot cancel htlc %v "+ | ||
| "on invoice %v, invoice is no longer open", key, |
There was a problem hiding this comment.
nit: think we wanna be as concise as possible in the logging - so i would do,
"CancelSingleHtlc: cannot cancel htlc: invoice(%v) no longer open"
| @@ -234,6 +234,9 @@ config option](https://github.com/lightningnetwork/lnd/pull/9182) and introduce | |||
| a new option `channel-max-fee-exposure` which is unambiguous in its description. | |||
| The underlying functionality between those two options remain the same. | |||
|
|
|||
| * [Improved user experience](https://github.com/lightningnetwork/lnd/pull/9454) | |||
There was a problem hiding this comment.
Should be 0.18.5 now i guess?
There was a problem hiding this comment.
I pushed a commit to move it into 0.18.5.
Roasbeef
force-pushed
the
add_custom_error_msg
branch
from
91014ea to
f4e2f2a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change Description
We introduce a new method to the TrafficShaper to remove the logic needed for custom channels and move it to the outer layers. Moreover this PR also prioritises replayed HTLCs and will not allow the HTLCModifier to change their previous resolution result. It also increases the test coverage of the invoiceregistry especially for AMP multi-part payments.