Replace dual-sync-async persistence panic with Watch contract

[joostjager]

Fix false panic by relaxing the Watch contract from a global "pick one mode" restriction to a per-channel rule: don't return Completed while prior InProgress updates are still pending for that channel.

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt 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.

[joostjager]

I didn't add a dedicated test for the InProgress -> complete -> Completed progression since it's already well-covered by existing tests that exercise this flow without disabling the contract check: test_monitor_update_fail_cs, test_monitor_update_fail_no_rebroadcast, test_monitor_update_fail_reestablish, monitor_failed_no_reestablish_response, test_monitor_update_fail_claim, test_monitor_update_on_pending_forwards, monitor_update_claim_fail_no_response, do_during_funding_monitor_fail, test_path_paused_mpp, test_temporary_error_during_shutdown, double_temp_error, test_manager_persisted_post_outbound_edge_holding_cell.

The ChainMonitor override path (where update_monitor fails and the return is overridden to InProgress) is also exercised without disabling the check by: test_monitor_and_persister_update_fail, test_concurrent_monitor_claim, test_update_err_monitor_lockdown.

[codecov]

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 85.97%. Comparing base (24062c0) to head (c37be93).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4436      +/-   ##
==========================================
+ Coverage   85.94%   85.97%   +0.03%     
==========================================
  Files         159      159              
  Lines      104607   104618      +11     
  Branches   104607   104618      +11     
==========================================
+ Hits        89901    89943      +42     
+ Misses      12194    12168      -26     
+ Partials     2512     2507       -5     

Flag	Coverage Δ
tests	85.97% <90.90%> (+0.03%)	⬆️

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]

There's more context in the PR description than the commit message, please update the commit message with the context (PR descriptions should almost always just be the commit messages themselves!). Also, IIRC the chanmon_consistency target is conservative to avoid violating the old API, we should update it to allow returning Completed more often to match the API changes here.

[ldk-reviews-bot]

👋 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.

[joostjager]

Updated fuzzer

[TheBlueMatt]

We can probably drop mon_style again, no? Given we added it in the being-basically-reverted commit?

[joostjager]

It's currently only used to remember what style to use after a node reload. But I could read from update_ret too and set that again on the new persister.

We could also reduce the number of command bytes by defining a command to toggle the mode.

[joostjager]

Pushed two commits that remove mon_style and also save command bytes by toggling the style.

The only functional change is that nodes now always start in sync mode, but the fuzzer can immediately toggle it to async. On reload, the style is carried over.

[TheBlueMatt]

DId you run the fuzzer for a while? I'm not sure that we shouldn't be able to hit a failure like the below.

[TheBlueMatt]

Hmm, thinking about this a bit more there's a weird race. Async updates are "completed" only after release_pending_monitor_events returns the completed update, but also only after it has been processed, which may be some arbitrary and indeterminate amount of time later.

For the ChainMonitor return case, this is fine (though we need to update ChainMonitor - we should probably be returning InProgress for any new update that comes after one for which we have to return a spurious InProgress. Do you already intend to do that in a followup? should we not do it here?). But for the general API its pretty weird - you can in theory return a Completed after returning InProgress as long as you want some indeterminate and arbitrary amount of time, so in practice you can't...

We could fix this by adding some mutual exclusion in channelmanager.rs where we wait before processing Completed monitor updates until after any pending MonitorEvents are processed - this should be basically zero cost as ~no one is going to be using mixed-async-sync persistence in practice so we'll rarely if ever have any contention.

On the flip side, we could say that implementations are allowed to flip from Completed to InProgress for a channel, but never back (without a restart). Its more complicated to document, but it captures what we need to allow the ChainMonitor behavior.

[joostjager]

implementations are allowed to flip from Completed to InProgress for a channel, but never back

This sounds like the easier way. There is no need to support mixing sync and async for Persist implementations. I do wonder now whether my initial version #4435 wasn't just sufficient. Moving the mixed mode check to the chainmonitor level and keeping everything else the same.

Fuzzer run was planned for tonight.