Fix race in PeerManager read pausing.

[TheBlueMatt]

We recently ran into a race condition on macOS where `read_event`
would return `Ok(true)` (implying reads should be paused) but calls
to `send_data` which flushed the buffer completed before the
`read_event` caller was able to set the read-pause flag.

This should be fairly rare, but not unheard of - the `pause_read`
flag in `read_event` is calculated before handling the last
message, so there's some time between when its calculated and when
its returned. However, that has to race with multiple calls to
`send_data` to send all the pending messages, which all have to
complete before the `read_event` return happens. We've (as far as I
can tell) never hit this on Linux, but a benchmark HTLC-flood test
managed to hit it somewhat reliably within a few minutes on macOS.

Ultimately we can't fix this with the current API (though we could
make it more rare). Thus, here, we stick to a single "stream" of
pause-read events from `PeerManager` to user code via `send_data`
calls, dropping the read-pause flag return from `read_event`
entirely.

Technically this adds risk that someone can flood us with enough
messages fast enough to bloat our outbound buffer for a peer before
`PeerManager::process_events` gets called and can flush the pause
flag via `read_event` calls to all descriptors. This isn't ideal
but it should still be relatively hard to do as `process_events`
calls are pretty quick and should be triggered immediately after
each `read_event` call completes.

[ldk-reviews-bot]

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

❌ Patch coverage is 89.36170% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.80%. Comparing base (05f2848) to head (467d311).
⚠️ Report is 50 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/peer_handler.rs	92.50%	3 Missing ⚠️
lightning-background-processor/src/lib.rs	0.00%	1 Missing ⚠️
lightning-net-tokio/src/lib.rs	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4168      +/-   ##
==========================================
+ Coverage   88.78%   88.80%   +0.01%     
==========================================
  Files         180      180              
  Lines      137004   137271     +267     
  Branches   137004   137271     +267     
==========================================
+ Hits       121642   121901     +259     
- Misses      12538    12559      +21     
+ Partials     2824     2811      -13     

Flag	Coverage Δ
fuzzing	21.49% <65.00%> (+0.53%)	⬆️
tests	88.64% <89.36%> (+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.

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

Ultimately we can't fix this with the current API (though we could
make it more rare).

What API change would be required to fix it completely? And is a reason not to do it to avoid breaking external usage of this code?

[TheBlueMatt]

The API change in this PR should fix it completely. My comment was about backporting to 0.1, where we aren't allowed to remove the returned-bool from read_event, and probably shouldn't silently change the semantics of the resume_read/continue_read bool (which now implies we should stop reading if its false).

[TheBlueMatt]

Dropped the first commit as it makes it more annoying to remove the spurious Box::pins now that our MSRV is higher.

[ldk-reviews-bot]

✅ Added second reviewer: @wpaulino

[ldk-reviews-bot]

✅ Added second reviewer: @jkczyz

[ldk-reviews-bot]

✅ Added second reviewer: @tankyleo

[ldk-reviews-bot]

✅ Added second reviewer: @valentinewallace

[valentinewallace]

The API change in this PR should fix it completely. My comment was about backporting to 0.1, where we aren't allowed to remove the returned-bool from read_event, and probably shouldn't silently change the semantics of the resume_read/continue_read bool (which now implies we should stop reading if its false).

Can you clarify this in the commit message? It isn't clear atm

[TheBlueMatt]

Okay, I rewrote that paragraph

[valentinewallace]

We recently ran into a race condition on macOS where read_event
would return Ok(true) (implying reads should be paused) but calls
to send_data which flushed the buffer completed before the
read_event caller was able to set the read-pause flag.

^ from the commit message

I found this unclear at first, I think adding a sentence saying "This race condition caused us to unnecessarily halt reads from a peer, even after we got out some writes that should have unpaused the reads. Halting reads in this way triggered a hang in testing." would go a long way (assuming what I wrote is correct)

[TheBlueMatt]

I found this unclear at first, I think adding a sentence saying "This race condition caused us to unnecessarily halt reads from a peer, even after we got out some writes that should have unpaused the reads. Halting reads in this way triggered a hang in testing." would go a long way (assuming what I wrote is correct)

The proposed wording I found confusing as it seems to imply that the write events happen after the read event, which is only half-true. I tried again with the following:

We recently ran into a race condition on macOS where `read_event`
would return `Ok(true)` (implying reads should be paused) due to
many queued outbound messages but before the caller was able to
set the read-pause flag, the `send_data` calls to flush the
buffered messages completed. Thus, when the `read_event` caller got
scheduled again, the buffer was empty and we should be reading, but
it is finally processing the read-pause flag and we end up hanging,
unwilling to read messages and unable to learn that we should start
reading again as there are no messages to `send_data` for.

[valentinewallace]

Much clearer, thanks. I'm good with a squash (and CI fix)

[TheBlueMatt]

Squashed with an additional doc fix:

$ git diff-tree -U1 06ad9485c 6dd0836c8
diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
index 9ac39a6fbc..3c7f4a845b 100644
--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -783,3 +783,3 @@ struct Peer {
 	awaiting_write_event: bool,
-	/// Set to true if the last call to [`Descriptor::send_data`] for this peer had the
+	/// Set to true if the last call to [`SocketDescriptor::send_data`] for this peer had the
 	/// `should_read` flag unset, indicating we've told the driver to stop reading from this peer.

[valentinewallace]

fuzz is still sad

[TheBlueMatt]

Ha, heat me to it,

$ git diff-tree -U1 06ad9485c 5e93838f3
diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
index 186dce0933..97a74871ea 100644
--- a/fuzz/src/full_stack.rs
+++ b/fuzz/src/full_stack.rs
@@ -697,3 +697,3 @@ pub fn do_test(mut data: &[u8], logger: &Arc<dyn Logger>) {
 				match loss_detector.handler.read_event(&mut peer, get_slice!(get_slice!(1)[0])) {
-					Ok(res) => assert!(!res),
+					Ok(()) => {},
 					Err(_) => {
diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
index 9ac39a6fbc..3c7f4a845b 100644
--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -783,3 +783,3 @@ struct Peer {
 	awaiting_write_event: bool,
-	/// Set to true if the last call to [`Descriptor::send_data`] for this peer had the
+	/// Set to true if the last call to [`SocketDescriptor::send_data`] for this peer had the
 	/// `should_read` flag unset, indicating we've told the driver to stop reading from this peer.

[valentinewallace]

One suggestion but LGTM

[joostjager]

I'd indeed say this fix is also a simplification. Win-win.

Still don't have full understanding of this part of the code, so should consider my approval secondary.

[TheBlueMatt]

Oops, yea, duh, can be simplified:

$ git diff-tree -U2 5e93838f3 467d311c03
diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
index 3c7f4a845b..74f081b03a 100644
--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -1543,13 +1543,9 @@ where
 		&self, descriptor: &mut Descriptor, peer: &mut Peer, mut force_one_write: bool,
 	) {
-		if !self.should_read_from(peer) {
-			if !peer.sent_pause_read {
-				force_one_write = true;
-			}
-		} else {
-			if peer.sent_pause_read {
-				force_one_write = true;
-			}
-		}
+		// If we detect that we should be reading from the peer but reads are currently paused, or
+		// vice versa, then we need to tell the socket driver to update their internal flag
+		// indicating whether or not reads are paused. Do this by forcing a write with the desired
+		// `continue_read` flag set, even if no outbound messages are currently queued.
+		force_one_write |= self.should_read_from(peer) == peer.sent_pause_read;
 		while force_one_write || !peer.awaiting_write_event {
 			if peer.should_buffer_onion_message() {

[TheBlueMatt]

Backported to 0.2 in #4185.