Skip to content

Add upgrade test for legacy post-close monitor update persistence#4755

Open
GideonBature wants to merge 1 commit into
lightningdevkit:mainfrom
GideonBature:upgrade-test-for-monitor-updating-persister
Open

Add upgrade test for legacy post-close monitor update persistence#4755
GideonBature wants to merge 1 commit into
lightningdevkit:mainfrom
GideonBature:upgrade-test-for-monitor-updating-persister

Conversation

@GideonBature

Copy link
Copy Markdown

LDK prior to 0.1 used u64::MAX (LEGACY_CLOSED_CHANNEL_UPDATE_ID) as the update_id for any ChannelMonitorUpdate generated after a channel had been closed. Since the change to stop using a constant monitor update_id after closure, we no longer generate these updates. However, both ChannelMonitor::update_monitor and MonitorUpdatingPersister still need to handle them so that nodes upgrading from pre-0.1 versions continue to work correctly.

This adds legacy_closed_channel_update, which synthesizes a pre-0.1 on-disk artifact: a ChannelForceClosed update with update_id == u64::MAX. The test exercises both directions of the legacy handling:

  • Read path: The update is written directly to the KV store as a standalone update file named by the sentinel ID, matching what a pre-0.1 persister may have left behind. When the monitor is read back, it must discover and replay the update, leaving the monitor at update_id == u64::MAX.

  • Write path: Passing the persister a sentinel-ID update must result in a full monitor write instead of a standalone update file. It must also purge stale update files through the legacy cleanup branch.

Closes #4104

@ldk-reviews-bot

ldk-reviews-bot commented Jun 27, 2026

Copy link
Copy Markdown

I've assigned @wpaulino 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.

@ldk-claude-review-bot

Copy link
Copy Markdown
Collaborator

I reviewed the entire diff and cross-checked the test against the implementation it exercises.

No issues found.

The test correctly exercises both legacy-handling paths:

  • Read path: the standalone u64::MAX update file is discovered and replayed by maybe_read_channel_monitor_with_updates and applied via update_monitor, which sets latest_update_id = u64::MAX. Asserts match this behavior.
  • Write path: passing a u64::MAX update to update_persisted_channel triggers a full monitor write (not a standalone update file) and the legacy cleanup branch cleanup_stale_updates_for_monitor_to(u64::MAX), which purges all update files. The list-empty assertion holds because TestStore::remove_internal ignores the lazy flag and removes synchronously.

Verified: field visibility (ChannelMonitorUpdate fields are pub/pub(crate), accessible in-crate), ChannelForceClosed { should_broadcast } variant shape, UpdateName round-trips u64::MAX correctly, and TestStore::new(false) (non-read-only) permits the writes/removes the test performs.

@ldk-reviews-bot ldk-reviews-bot requested a review from wpaulino June 27, 2026 14:50
@codecov

codecov Bot commented Jun 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.97%. Comparing base (43ea836) to head (2963bd6).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4755   +/-   ##
=======================================
  Coverage   86.96%   86.97%           
=======================================
  Files         161      161           
  Lines      111648   111715   +67     
  Branches   111648   111715   +67     
=======================================
+ Hits        97099    97160   +61     
- Misses      12045    12050    +5     
- Partials     2504     2505    +1     
Flag Coverage Δ
fuzzing-fake-hashes 8.43% <ø> (ø)
fuzzing-real-hashes 32.41% <ø> (ø)
tests 86.30% <100.00%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Harness.
📢 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.

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.

Add Upgrade test for MonitorUpdatingPersister LEGACY_CLOSED_CHANNEL_UPDATE_ID writes

3 participants