Make skimmed_fee_msat optional in LSPS2's payment_forwarded API

[tnull]

Recent changes made skimmed_fee_msat a required field of LSPS2ServiceHandler's payment_forwarded API, which seemed reasonable given that the field is available since LDK 0.0.122.

However, the field is of course only set post-0.0.122 when a fee was actually withheld, which makes forcing the user to unwrap_or(0) potentially confusing, especially since our internal logic was written so that it can handle non-intercept-SCID-forwarding cases. Here, we restore the previous idea of "just pass PaymentForwarded fields on, we'll handle the appropriately internally anyways.

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.77%. Comparing base (b6607a7) to head (0964c7c).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4162      +/-   ##
==========================================
- Coverage   88.77%   88.77%   -0.01%     
==========================================
  Files         180      180              
  Lines      136622   136622              
  Branches   136622   136622              
==========================================
- Hits       121293   121289       -4     
- Misses      12517    12525       +8     
+ Partials     2812     2808       -4     

Flag	Coverage Δ
fuzzing	20.99% <0.00%> (ø)
tests	88.62% <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 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]

I really disagree that we can just make this an Option. I raised it at #3838 (comment) originally, and generally don't think its okay to have an unsafe API here - if someone passes None we really shouldn't be blindly assuming enough was paid (or that there's never enough paid depending on how the comparison works).

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

[tnull]

I really disagree that we can just make this an Option. I raised it at #3838 (comment) originally, and generally don't think its okay to have an unsafe API here - if someone passes None we really shouldn't be blindly assuming enough was paid (or that there's never enough paid depending on how the comparison works).

Huh, but that's exactly the point: it's not unsafe? We only update the state to PaymentForwarded if if skimmed_fee_msat >= Some(*opening_fee_msat). Otherwise we do nothing. So the idea always has been that you hand-in any forwarded payment data and our logic only acts if there is something to do.

[TheBlueMatt]

I would still call that "unsafe" - Option in an API implies that its optional, and I'd argue it definitely isn't - if a dev passes None here we'll silently never do anything and never actually open the channel, which is very much incorrect behavior. The LSP might not lose money, but the client is. Making devs actually look at what they're doing and decide, actively, to unwrap_or(0) or just unwrap makes clearer in the calling code what's actually going on vs trying to hide it in LDK.

[martinsaposnic]

FWIW skimmed_fee_msat is what triggers the transition to PaymentForwarded in both lsp_trusts_client and client_trusts_lsp now, so having it as an Option reads like “skip if you want, whatever” which is misleading. making it required and forcing an explicit 0 makes its role clear and avoids that confusion IMO

[tnull]

Alright, no too strong opinion here. It does change how we sofar thought about the API, but maybe for the better.