(0.5) Deprecate legacy pending HTLC ChannelManager persistence#4359
Draft
valentinewallace wants to merge 11 commits intolightningdevkit:mainfrom
Draft
(0.5) Deprecate legacy pending HTLC ChannelManager persistence#4359valentinewallace wants to merge 11 commits intolightningdevkit:mainfrom
ChannelManager persistence#4359valentinewallace wants to merge 11 commits intolightningdevkit:mainfrom
Conversation
|
👋 Hi! I see this is a draft PR. |
ChannelManager persistence
This was referenced Jan 29, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4359 +/- ##
==========================================
+ Coverage 86.09% 86.13% +0.03%
==========================================
Files 156 156
Lines 103623 103456 -167
Branches 103623 103456 -167
==========================================
- Hits 89211 89108 -103
+ Misses 11895 11854 -41
+ Partials 2517 2494 -23
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:
|
0350632 to
2170910
Compare
2170910 to
eb547f2
Compare
In LDK 0.3 we added support for reconstructing the ChannelManager's pending forward HTLCs maps from channel data as part of removing the requirement to regularly persist the manager, but til now we still would write/read those maps within the manager to maintain compat with 0.2-. Also 0.3 added a new field to Channel that allowed the map reconstruction. Now that a few versions have passed we have more confidence that the new field is being written, here we deprecate persistence of the old legacy maps and only attempt to read them if the manager serialized version indicates that they were written. Upcoming commits will ensure we error if the new field is missing. XXX clean up claude tests
See previous commit, but now in prod reconstruct_manager_from_monitors will always be set, so there's no need to have an option to set it in tests.
Used in the next commit when we log on some read errors.
In 0.3, we added a new field to Channel as part of adding support for
reconstructing the ChannelManager's pending forward HTLC set from
Channel{Monitor} data, moving towards removing the requirement of regularly
persisting the ChannelManager.
This new field cannot be set for HTLCs received pre-0.1 that are using this
legacy ::Resolved variant.
In this version, we fully rely on the new Channel field being set and cease
handling legacy HTLCs entirely, and thus must fully deprecate the ::Resolved
variant and require all inbound HTLCs be in state
InboundHTLCResolution::Pending, which we do here. Further cleanups are coming
in upcoming commits.
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.
This version of LDK no longer supports HTLCs created before 0.3, which allows us to remove this variant that will only be present if an HTLC was received on a pre-0.3 LDK version. See the past few commits.
In the previous commit, we removed the InboundHTLCResolution::Resolved enum variant, which caused us to never provide any HTLCs in this now-removed parameter.
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.1.
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.1
We stopped using this struct a few commits ago when we removed support for HTLCs that were originally received on LDK 0.1.
eb547f2 to
492f5db
Compare
This file contains hidden or 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
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.
In LDK 0.3 we're adding support for reconstructing the
ChannelManager's pendingforward HTLCs maps from channel data as part of removing the requirement to
regularly persist the manager, but til 0.5 we still would write/read those maps
within the manager to maintain compat with 0.2-. Also 0.3 adds a new field to
Channelthat allowed the map reconstruction.Once a few versions have passed, we have more confidence that the new field
is being written, so here we deprecate persistence of the old legacy maps and only
attempt to read them if the manager serialized version indicates that they were
written. We can also deprecate some other old codepaths, see commit messages.
Partially addresses #4286
Based on #4303
upgrade_downgrade_teststhat currently upgrade from 0.2- tomain, to upgrade to 0.3/0.4 instead (since 0.2- to 0.5 is unsupported)upgrade_downgrade_teststo not point to my branch once 0.3 is released