Follow-ups to #4227 (Part 2) #4303
Conversation
|
👋 I see @wpaulino was un-assigned. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4303 +/- ##
==========================================
+ Coverage 86.59% 86.60% +0.01%
==========================================
Files 158 158
Lines 102368 102331 -37
Branches 102368 102331 -37
==========================================
- Hits 88641 88628 -13
+ Misses 11313 11288 -25
- Partials 2414 2415 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
valentinewallace
force-pushed
the
2025-12-reconstruct-fwds-followup-2
branch
from
01aa4c0 to
4012864
Compare
valentinewallace
force-pushed
the
2025-12-reconstruct-fwds-followup-2
branch
from
4012864 to
e2ea1ed
Compare
| // | ||
| // TODO: Once this variant is removed, we should also clean up | ||
| // [`MonitorRestoreUpdates::accepted_htlcs`] as the path will be unreachable. | ||
| Resolved { pending_htlc_status: PendingHTLCStatus }, |
There was a problem hiding this comment.
Looking back at CHANGELOG I'm not sure the claim that these aren't created in recent versions is true? AFAICT we would still defaulted to creating these until 0.2, at which point we swapped to using Pending by default (#2933). The "you have to forward HTLCs before you upgrade to 0.1" thing was unrelated, the release notes point to #3355. Thus, I'm kinda uncomfortable dropping this so soon. It means we yet again have a "you have to clear all pending to-be-forwarded HTLCs" step and that you cannot upgrade directly from 0.1 to 0.3, which is in fact something we had been thinking we'd recommend to certain users carrying large patchsets on LDK!
There was a problem hiding this comment.
Discussed offline. Totally right, I will have to check that more closely next time.
I believe if we agree on the upgrade plan outlined here #4289 (comment) then one path forward could be to merge this PR's commits removing the ::Resolved variant in 0.5. That way users would have a few more versions to clear the deprecated pending-to-be-forwarded HTLCs? In 0.3 and 0.4 we still don't plan to use the new inbound HTLC's update_add_htlc field unless the user is downgrading from 0.5, so I think it shouldn't matter if the field isn't set for the purpose of upgrading to those versions.
Another option touched on was to fill in the InboundHTLCState::Committed { update_add_htlc_opt } field in Channel::revoke_and_ack by reconstructing the update_add from the PendingHTLCInfo that's present. That would let us avoid requiring deprecated ::Resolved HTLCs to be resolved before upgrading to 0.5. But it adds complexity, is hard to test, and involves wastefully decoding the onion extra times.
We store inbound committed HTLCs' onions in Channels for use in reconstructing the pending HTLC set on ChannelManager read. If an HTLC has been irrevocably forwarded to the outbound edge, we no longer need to persist the inbound edge's onion and can prune it here.
Used in the next commit when we log on some read errors.
We already pretty much deprecated handling of these HTLCs already in 0.1, see the 0.1 CHANGELOG.md entry. But as of 4f055ac, we really no longer handle these deprecated HTLCs properly (see comment in Channel::revoke_and_ack). Therefore, remove support for them more explicitly here.
In the previous commit, we removed the InboundHTLCResolution::Resolved enum variant, which caused us to never provide any HTLCs in this now-removed parameter.
Previously, several variants of InboundHTLCState contained an InboundHTLCResolution enum. Now that the resolution enum only has one variant, we can get rid of it and simplify the parent InboundHTLCState type.
We stopped adding any HTLCs to this vec a few commits ago when we removed support for HTLCs that were originally received on LDK 0.0.123-.
We stopped adding any HTLCs to this vec a few commits ago when we removed support for HTLCs that were originally received on LDK 0.0.123-.
We stopped using this struct a few commits ago when we removed support for HTLCs that were originally received on LDK 0.0.123-.
valentinewallace
force-pushed
the
2025-12-reconstruct-fwds-followup-2
branch
from
e2ea1ed to
c40741b
Compare
|
Pushed a rebase on |
3 participants
Closes #4280
Based on #4289
Channels in prod based on the serialized manager version Fix double-forward, prefer legacy forward maps #4289 (comment)