lightning: don't require Unpin futures for OnionMessenger process events

[phlip9]

Hit this today while impl'ing onion message forwarding to offline peers in our LSP.

Both ChannelManager and ChainMonitor only require the handler passed to process_pending_events_async to return a Future. However, OnionMessenger requires a Future + Unpin, which means I have to individually box each handler future :(

This PR enables OnionMessenger to accept any Future by enabling MultiResultFuturePoller to accept any Future.

Sadly Pin ergonomics are awful in stable Rust, so this requires a few unsafe invocations to do standard pin-projection. The impl is effectively the same as the one in the standard futures crate (see: futures::future::join_all) which doesn't appear to have had any safety issues over the last few years.

[ldk-reviews-bot]

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

Codecov Report

Attention: Patch coverage is 2.22222% with 44 lines in your changes missing coverage. Please review.

Project coverage is 89.07%. Comparing base (7b45811) to head (18ca922).
Report is 16 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/util/async_poll.rs	0.00%	44 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3722      +/-   ##
==========================================
- Coverage   89.10%   89.07%   -0.03%     
==========================================
  Files         156      156              
  Lines      123431   123451      +20     
  Branches   123431   123451      +20     
==========================================
- Hits       109985   109970      -15     
- Misses      10760    10788      +28     
- Partials     2686     2693       +7     

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

[tnull]

I'm really not convinced we want to go down this path. Could you expand on why boxing the event handler futures is that painful to you that it warrants these rewrites using unsafe code that introduces additional complexity and assumptions?

[phlip9]

1. It would fix an irritating API inconsistency, where some process_events_async require Unpin and some don't.
2. The unsafe code is relatively low risk and self-contained. I've been using the futures crate (which this impl is taken from) for years and never had an issue. It has 13M+ dl's on crates.io, etc.

[tnull]

1. It would fix an irritating API inconsistency, where some process_events_async require Unpin and some don't.

Hmm, okay, so this is just about an API inconsistency? What is the impact it has on your code in practice?

2. The unsafe code is relatively low risk and self-contained. I've been using the futures crate (which this impl is taken from) for years and never had an issue. It has 13M+ dl's on crates.io, etc.

Well, there are good reasons why avoid/forbade unsafe code. IIRC, you historically used to complain about the existence about some unsafe code in lightning-net-tokio, so what changed your opinion on the topic?

[phlip9]

Well, there are good reasons why avoid/forbade unsafe code. IIRC, you historically used to complain about the existence about some unsafe code in lightning-net-tokio, so what changed your opinion on the topic?

100% and definitely a fair callout 😅. I guess I just trust the futures-rs impl that's run for however many billion CPU hours w/o issue to be functionally safe at this point.

Hmm, okay, so this is just about an API inconsistency? What is the impact it has on your code in practice?

Well it would have required me to box some of the handler futures but not others, which was mildly annoying and probably a very minor perf hit.

Anyway, I found a much simpler approach that doesn't require handling the OnionMessenger events async, so I don't actually need this anymore. I'll close the PR -- and thanks again for the review!