Skip to content

Add a method to avoid re-persisting monitors during startup #3996

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

TheBlueMatt
Copy link
Collaborator

Prior to LDK 0.1, in rare cases we could replay payment claims to ChannelMonitors on startup, which we then expected to be persisted prior to normal node operation. This required re-persisting ChannelMonitors after deserializing the ChannelManager, delaying startup in some cases substantially.

In 0.1 we fixed this, moving claim replays to the background to run after the ChannelManager starts operating (and only updating/persisting changes to the ChannelMonitors which need it). However, we didn't actually enable this meaningfully in our API - nearly all users use our ChainMonitor and the only way to get a chanel into ChainMonitor is through the normal flow which expects to persist.

Here we add a simple method to load ChannelMonitors into the ChainMonitor without persisting them.

I'm kinda on the fence about backporting this. On the one hand its a pretty nontrivial speedup for users who switch to the new interface. On the other hand, it is a "feature" and its kinda weird to backport a feature.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Aug 7, 2025

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Prior to LDK 0.1, in rare cases we could replay payment claims to
`ChannelMonitor`s on startup, which we then expected to be
persisted prior to normal node operation. This required
re-persisting `ChannelMonitor`s after deserializing the
`ChannelManager`, delaying startup in some cases substantially.

In 0.1 we fixed this, moving claim replays to the background to run
after the `ChannelManager` starts operating (and only
updating/persisting changes to the `ChannelMonitor`s which need
it). However, we didn't actually enable this meaningfully in our
API - nearly all users use our `ChainMonitor` and the only way to
get a chanel into `ChainMonitor` is through the normal flow which
expects to persist.

Here we add a simple method to load `ChannelMonitor`s into the
`ChainMonitor` without persisting them.
@TheBlueMatt TheBlueMatt force-pushed the 2025-08-faster-0.1-load branch from f968fa1 to e45bfed Compare August 7, 2025 22:38
@tnull tnull requested review from tnull and removed request for joostjager August 8, 2025 06:48
@tnull
Copy link
Contributor

tnull commented Aug 8, 2025

On the other hand, it is a "feature" and its kinda weird to backport a feature.

Yeah, I'd also lean towards not backporting this, also as it changes API (even though just extending).

@ldk-reviews-bot
Copy link

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

Copy link

codecov bot commented Aug 8, 2025

Codecov Report

❌ Patch coverage is 86.27451% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.94%. Comparing base (ed1f304) to head (76e1d55).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/chain/chainmonitor.rs 81.81% 3 Missing and 1 partial ⚠️
lightning/src/util/test_utils.rs 85.71% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3996   +/-   ##
=======================================
  Coverage   88.93%   88.94%           
=======================================
  Files         174      174           
  Lines      124575   124629   +54     
  Branches   124575   124629   +54     
=======================================
+ Hits       110790   110850   +60     
+ Misses      11287    11280    -7     
- Partials     2498     2499    +1     
Flag Coverage Δ
fuzzing 22.17% <7.84%> (-0.02%) ⬇️
tests 88.77% <86.27%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@TheBlueMatt TheBlueMatt added this to the 0.2 milestone Aug 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants