Reconstruct `ChannelManager` forwarded HTLCs maps from `Channel`s

[valentinewallace]

We have an overarching goal of (mostly) getting rid of ChannelManager persistence and rebuilding the ChannelManager's state from existing ChannelMonitors, due to issues when the two structs are out-of-sync on restart. The main issue that can arise is channel force closure.

Here we start this process by rebuilding ChannelManager::decode_update_add_htlcs, forward_htlcs, and pending_intercepted_htlcs from the Channels, which will soon be included in the ChannelMonitors as part of #4218.

• [ ] test upgrading from a manager containing pending HTLC forwards that was serialized on <= LDK 0.2, i.e. where Channels will not contain committed update_add_htlcs
• [ ] currently, no tests fail when we force using the new rebuilt decode_update_add_htlcs map and ignoring the legacy maps. This may indicate missing test coverage, since in theory we need to re-forward these HTLCs sometimes so they go back in the forward_htlcs map for processing
• [ ] only use the old legacy maps if the manager and its channels were last serialized on <= 0.2. Currently this is not guaranteed

[ldk-reviews-bot]

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

[codecov[bot]]

Codecov Report

:x: Patch coverage is 88.70968% with 28 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 89.34%. Comparing base (de384ff) to head (0b83e80).

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	87.76%	14 Missing and 3 partials :warning:
lightning/src/ln/channel.rs	80.00%	9 Missing and 1 partial :warning:
lightning/src/ln/reload_tests.rs	98.30%	1 Missing :warning:

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #4227    +/-   ##
========================================
  Coverage   89.33%   89.34%            
========================================
  Files         180      180            
  Lines      139042   139234   +192     
  Branches   139042   139234   +192     
========================================
+ Hits       124219   124398   +179     
- Misses      12196    12210    +14     
+ Partials     2627     2626     -1     

Flag	Coverage Δ
fuzzing	35.98% <22.87%> (+0.01%)	:arrow_up:
tests	88.70% <88.70%> (-0.01%)	:arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[valentinewallace]

~@joostjager helped me realize this may be way overcomplicated, essentially all tests pass on main when we simply read-and-discard the pending forwards maps. It's a bit suspicious though that all tests pass so it seems like additional test coverage could be useful.~

Nvm, our test coverage for reload of these maps is just pretty incomplete.

[valentinewallace]

Updated with new testing and a few tweaks: diff

Will rebase next

[valentinewallace]

I still need to finish adding some comments and responding to feedback, but I believe the tests are done and this should be mostly there so pushed up the latest state.

[tnull]

I still need to finish adding some comments and responding to feedback, but I believe the tests are done and this should be mostly there so pushed up the latest state.

We had discussed whether to split out receive_htlcs as part of this work, since you're already reconstructing the forwards_htlcs map. Are you still considering doing this as part of this PR or rather not?

[valentinewallace]

I still need to finish adding some comments and responding to feedback, but I believe the tests are done and this should be mostly there so pushed up the latest state.

We had discussed whether to split out receive_htlcs as part of this work, since you're already reconstructing the forwards_htlcs map. Are you still considering doing this as part of this PR or rather not?

Probably not in this PR since it's already decently sized and has a clear scope, but I will look into it for follow-up!

[tnull]

I still need to finish adding some comments and responding to feedback, but I believe the tests are done and this should be mostly there so pushed up the latest state.

We had discussed whether to split out receive_htlcs as part of this work, since you're already reconstructing the forwards_htlcs map. Are you still considering doing this as part of this PR or rather not?

Probably not in this PR since it's already decently sized and has a clear scope, but I will look into it for follow-up!

SGTM!

[tnull]

Btw, feel free to ping me whenever you think this is ready for a secondary reviewer!