(0.5) Deprecate legacy pending HTLC ChannelManager persistence
#4359
+1,071
−534
Conversation
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
We recently added support for reconstructing ChannelManager::decode_update_add_htlcs on startup, using data present in the Channels. However, we failed to prune HTLCs from this rebuilt map if a given inbound HTLC was already forwarded to the outbound edge and in the outbound holding cell. Here we fix this bug that would have caused us to double-forward inbound HTLC forwards, which fortunately never shipped. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
We recently added support for reconstructing ChannelManager::decode_update_add_htlcs on startup, using data present in the Channels. However, we failed to prune HTLCs from this rebuilt map if a given HTLC was already forwarded+removed from the outbound edge and resolved in the inbound edge's holding cell. Here we fix this bug that would have caused us to double-forward inbound HTLC forwards, which fortunately was not shipped. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
In 0.3+, we are taking steps to remove the requirement of regularly persisting
the ChannelManager and instead rebuild the set of HTLC forwards (and the
manager generally) from Channel{Monitor} data.
We previously merged support for reconstructing the
ChannelManager::decode_update_add_htlcs map from channel data, using a new
HTLC onion field that will be present for inbound HTLCs received on 0.3+ only.
However, we now want to add support for pruning this field once it's no longer
needed so it doesn't get persisted every time the manager gets persisted. At
the same time, in a future LDK version we need to detect whether the field was
ever present to begin with to prevent upgrading with legacy HTLCs present.
We accomplish both by converting the plain update_add option that was
previously serialized to an enum that indicates whether the HTLC is from 0.2-
versus 0.3+-with-onion-pruned.
Actual pruning of the new update_add field is added in the next commit.
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.
In the last commit, we added support for pruning an inbound HTLC's persisted onion once the HTLC has been irrevocably forwarded to the outbound edge. Here, we add a check on startup that those inbound HTLCs were actually handled. Specifically, we check that the inbound HTLC is either (a) currently present in the outbound edge or (b) was removed via claim. If neither of those are true, we infer that the HTLC was removed from the outbound edge via fail and fail the inbound HTLC backwards. Tests for this code are added in a follow-up PR that will be merged in 0.5. We can't test this code right now because of reconstruct_manager_from_monitors logic is needed, and whether it runs during tests is chosen randomly.
In 0.3+, we are taking steps to remove the requirement of regularly persisting
the ChannelManager and instead rebuild the set of HTLC forwards (and the
manager generally) from Channel{Monitor} data.
We previously merged support for reconstructing the
ChannelManager::decode_update_add_htlcs map from channel data, using a new
HTLC onion field that will be present for inbound HTLCs received on 0.3+ only.
The plan is that in upcoming LDK versions, the manager will reconstruct this
map and the other forward/claimable/pending HTLC maps will automatically
repopulate themselves on the next call to process_pending_htlc_forwards.
As such, once we're in a future version that reconstructs the pending HTLC set,
we can stop persisting the legacy ChannelManager maps such as forward_htlcs,
pending_intercepted_htlcs since they will never be used.
For 0.3 to be compatible with this future version, in this commit we detect
that the manager was last written on a version of LDK that doesn't persist the
legacy maps. In that case, we don't try to read the old forwards map and run
the new reconstruction logic only.
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.
In a prior commit, we implemented a check during manager read where if we find an inbound HTLC that claims to have already been forwarded to the outbound edge, we double-check that either the HTLC itself or its preimage is present in the outbound edge. If not, we infer the HTLC was failed on the outbound edge and fail it backwards. Here we test that behavior. We couldn't test it previously because it relied on some behavior that only ran sometimes in tests, up until this commit's parent.
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.
|
👋 Hi! I see this is a draft PR. |
valentinewallace
changed the title
ChannelManager persistence
valentinewallace
added
the
blocked on next release
This was referenced Jan 29, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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