lightningdevkit
Enable decoding HTLC onions when fully committed
This PR ensures all new incoming HTLCs going forward will have their onion decoded when they become fully committed to decide how we should proceed with each one. As a result, we'll obtain HTLCHandlingFailed events for any failed HTLC that comes across a channel.
We will now start writing channels with the new serialization version (4), and we will still be able to downgrade back to the commit that introduced it since reading version 4 is supported.
Note that existing pending inbound HTLCs may already have their resolution if they were received in a previous version of LDK. We must support those until we no longer allow downgrading beyond this commit.
Codecov Report
Attention: Patch coverage is 98.41772% with 5 lines in your changes missing coverage. Please review.
Project coverage is 88.44%. Comparing base (
ad462bd) to head (d8d9dc7). Report is 62 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| lightning/src/ln/channel.rs | 88.46% | 0 Missing and 3 partials :warning: |
| lightning/src/ln/payment_tests.rs | 98.09% | 2 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #2933 +/- ##
==========================================
+ Coverage 88.42% 88.44% +0.02%
==========================================
Files 149 149
Lines 112959 113363 +404
Branches 112959 113363 +404
==========================================
+ Hits 99883 100268 +385
Misses 10605 10605
- Partials 2471 2490 +19
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Tagging 0.0.122 not because we need to land this for 0.0.122, but because we have to at least be roughly confident in it working and the tests in it being sufficient.
As discussed offline, we'll want to land a cfg-flagged version of this PR :)
Okay, sounds like @valentinewallace took a glance at this, and I did too. Nothing too wild here, tests pass, we can do this in the next release (or whenever), but should cfg-flag the changes and land it sooner so that we don't have this branch get toooo stale.
Before I go down the cfg flag path, does it make sense to merge this now as is if the project is doing branched releases going forward?
Yea, I don't see why not? Just cause we're branching off for future releases doesn't mean we can't/shouldn't have future code behind cfg flags on the working tip.
Chatted with @wpaulino today. His quiescence branch is based on this PR and IIUC requires it. He agreed to rebase it soon. @TheBlueMatt @wpaulino Do we still want a cfg flag for this?
I think we're probably okay without now. @valentinewallace checked today and said support was added in 0.0.123, which means if we ship this we'll support downgrading two versions of LDK but not more, which is pretty normal for us, I think.