Give peers which are sending us messages/receiving messages from us longer to respond to ping #1137

TheBlueMatt · 2021-10-22T21:08:48Z

This should largely resolve disconnection issues during initial sync, without the larger changes in #1023. It also resolves one None unwrap (which may or may not even be present on latest git).

codecov · 2021-10-22T21:31:56Z

Codecov Report

Merging #1137 (28b7430) into main (2bf39a6) will increase coverage by 1.10%.
The diff coverage is 82.03%.

❗ Current head 28b7430 differs from pull request most recent head 0caa8bb. Consider uploading reports for the commit 0caa8bb to get more accurate results

@@            Coverage Diff             @@
##             main    #1137      +/-   ##
==========================================
+ Coverage   90.57%   91.67%   +1.10%     
==========================================
  Files          67       69       +2     
  Lines       34567    43809    +9242     
==========================================
+ Hits        31309    40163    +8854     
- Misses       3258     3646     +388

Impacted Files	Coverage Δ
lightning-background-processor/src/lib.rs	`94.68% <0.00%> (+0.45%)`	⬆️
lightning/src/ln/peer_handler.rs	`57.18% <82.40%> (+11.30%)`	⬆️
lightning/src/util/test_utils.rs	`86.30% <100.00%> (+4.53%)`	⬆️
lightning/src/chain/mod.rs	`50.00% <0.00%> (-11.12%)`	⬇️
lightning/src/util/macro_logger.rs	`86.59% <0.00%> (-1.29%)`	⬇️
lightning/src/util/chacha20poly1305rfc.rs	`96.77% <0.00%> (-1.19%)`	⬇️
lightning/src/chain/chainmonitor.rs	`95.35% <0.00%> (-1.01%)`	⬇️
lightning/src/util/ser.rs	`88.43% <0.00%> (-0.71%)`	⬇️
lightning/src/util/byte_utils.rs	`100.00% <0.00%> (ø)`
lightning-invoice/src/payment.rs	`92.83% <0.00%> (ø)`
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bf39a6...0caa8bb. Read the comment docs.

lightning/src/ln/peer_handler.rs

arik-so · 2021-10-25T17:21:15Z

lightning/src/ln/peer_handler.rs

@@ -1148,14 +1182,15 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref, CMH: Deref> P
 							!peer.should_forward_channel_announcement(msg.contents.short_channel_id)  {
 						continue
 					}
-					if peer.pending_outbound_buffer.len() > OUTBOUND_BUFFER_LIMIT_DROP_GOSSIP {
+					if peer.pending_outbound_buffer.len() > OUTBOUND_BUFFER_LIMIT_DROP_GOSSIP || peer.msgs_sent_since_pong > BUFFER_DRAIN_MSGS_PER_TICK * 2 {


this check seems to be done a lot. Would it be possible to create a method such as can_queue_outbound_message(&peer) and skip if it returns false?

Hmm, its just done in this specific method - we dont limit non-gossip messages in the buffer (as we can't just drop HTLC messages or whatever).

arik-so · 2021-10-25T17:22:57Z

lightning/src/ln/peer_handler.rs

+				if !peer.channel_encryptor.is_ready_for_encryption() || peer.their_node_id.is_none() {
+					// The peer needs to complete its handshake before we can exchange messages. We
+					// give peers one timer tick to complet handshake.
+					if peer.awaiting_pong_tick_intervals != 0 {


how can a peer that is not ready for encryption be already be owing us pongs?

Its more a deadline for when the peer completes the initial handshake - we reset awaiting_pong_tick_intervals to 0 after the handshake completes, I'll document better.

arik-so · 2021-10-25T17:25:15Z

lightning/src/ln/peer_handler.rs

+			peers[1].read_event(&mut fd_b, &fd_a.outbound_data.lock().unwrap().split_off(0)).unwrap();
+			peers[1].process_events();
+			peers[0].read_event(&mut fd_a, &fd_b.outbound_data.lock().unwrap().split_off(0)).unwrap();
+		}



shouldn't we add tests that check precisely the limits of the buffer constants and verify there are no off-by-one errors?

We can't check the message count exactly cause we just have bytes, but we can count how many iterations it took and check that.

The `cmp::min` appeared to confused `end` for a count.

In the coming commits simply calling `timer_tick_occurred` will no longer disconnect all peers, so its helpful to have a utility method.

valentinewallace · 2021-10-27T16:47:07Z

Just successfully opened a channel to Matt with this PR!

TheBlueMatt · 2021-10-27T20:07:06Z

Added one more commit that is incredibly useful when grep'ing log entries - ensure we print who tan error is from.

lightning/src/ln/peer_handler.rs

jkczyz · 2021-10-28T14:05:53Z

lightning/src/ln/peer_handler.rs

@@ -285,14 +285,18 @@ enum InitSyncTracker{
 	NodesSyncing(PublicKey),
 }

+/// The ratio between buffer sizes at which we stop sending initial sync messages vs when we stop
+/// forwarding messages to peers altogether.
+const FORWARD_INIT_SYNC_BUFFER_SIZE_LIMIT: usize = 2;


The naming here is a bit strange. Is it really a limit? Also, why say "INIT_SYNC" here but "GOSSIP" below?

Hmmmm, I think the confusion comes from OUTBOUND_BUFFER_LIMIT_READ_PAUSE being used for two things - when the buffer is "full" implying we stop reading new messages and when the buffer is "full" implying we stop putting more initial-gossip-sync messages in the buffer. The DROP_GOSSIP limit is for post-initial-sync gossip with a ratio of init-to-normal-forwards. Do you have a concrete suggestion for changing these names?

Not sure. Was thinking something with "factor" in the name given it is multiplied. I think my main complaint is that SIZE_LIMIT seems to imply units of "number of messages" but this constant is really unit-less since it is a ratio of two size limits. So an expression like BUFFER_DRAIN_MSGS_PER_TICK * FACTOR should have units "number of messages per tick" not "number of messages squared per tick" as the SIZE_LIMIT naming would imply.

Ah! Ok, I'd somewhat misunderstood your complaint here, yea, definitely this needs RATIO or FACTOR in it.

How do the new fixup names sit for you?

lightning/src/ln/peer_handler.rs

jkczyz · 2021-10-28T15:15:09Z

lightning/src/ln/peer_handler.rs

+struct OptionalFromDebugger<'a>(&'a Option<PublicKey>);
+impl core::fmt::Display for OptionalFromDebugger<'_> {
+	fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> Result<(), core::fmt::Error> {
+		if let Some(node_id) = self.0 { write!(f, " from {}", log_pubkey!(node_id)) } else { Ok(()) }


Perhaps keep the "from" in each message and use "peer" in the else case.

That feels wrong? from peer isn't exactly a useful log string. Note that the need to introduce this wrapper is because of the optional public key itself, not the "from".

jkczyz · 2021-10-28T15:15:51Z

lightning/src/ln/peer_handler.rs

@@ -979,7 +986,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref, CMH: Deref> P
 					return Err(PeerHandleError{ no_connection_possible: false }.into());
 				}

-				log_info!(self.logger, "Received peer Init message: {}", msg.features);
+				log_info!(self.logger, "Received peer Init message from {}: {}", log_pubkey!(peer.their_node_id.unwrap()), msg.features);


Use the wrapper here?

I mean the wrapper only exists to solve a very specific lifetime issue thanks for format!() being kinda braindead (creating references to every argument as-is instead of accepting references as-is).

jkczyz · 2021-10-28T15:18:19Z

lightning/src/ln/peer_handler.rs

@@ -493,6 +493,13 @@ impl<Descriptor: SocketDescriptor, RM: Deref, L: Deref> PeerManager<Descriptor,
 	}
 }

+struct OptionalFromDebugger<'a>(&'a Option<PublicKey>);


Maybe name this something like MessageSender wrapping a Peer reference?

I added some docs explaining why this was added at all. Given its not really particularly specific to Peer I dunno if its worth renaming.

arik-so

LGTM

ConorOkus · 2021-10-28T19:43:04Z

Successfully opened a channel with Matt & Val as a result of this PR. Exciting!

tACK

This marginally simplifies coming commits.

See comment for rationale.

This ensures we don't let a hung connection stick around forever if the peer never completes the initial handshake. This also resolves a race where, on receiving a second connection from a peer, we may reset their_node_id to None to prevent sending messages even though the `channel_encryptor` `is_ready_for_encryption()`. Sending pings only checks the `channel_encryptor` status, not `their_node_id` resulting in an `unwrap` on `None` in `enqueue_message`.

TheBlueMatt · 2021-10-28T20:07:38Z

Squashed without changes, only diff since Arik's ACK is to fix compilation. Will land after CI:

$ git diff-tree -U1 b4dcb56 0caa8bb5d
diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
index 9a1b04853..0d8bc1c98 100644
--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -1160,3 +1160,3 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref, CMH: Deref> P
 					if peer.pending_outbound_buffer.len() > OUTBOUND_BUFFER_LIMIT_DROP_GOSSIP
-						|| peer.msgs_sent_since_pong > BUFFER_DRAIN_MSGS_PER_TICK * FORWARD_INIT_SYNC_BUFFER_SIZE_LIMIT
+						|| peer.msgs_sent_since_pong > BUFFER_DRAIN_MSGS_PER_TICK * FORWARD_INIT_SYNC_BUFFER_LIMIT_RATIO
 					{
@@ -1185,3 +1185,3 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref, CMH: Deref> P
 					if peer.pending_outbound_buffer.len() > OUTBOUND_BUFFER_LIMIT_DROP_GOSSIP
-						|| peer.msgs_sent_since_pong > BUFFER_DRAIN_MSGS_PER_TICK * FORWARD_INIT_SYNC_BUFFER_SIZE_LIMIT
+						|| peer.msgs_sent_since_pong > BUFFER_DRAIN_MSGS_PER_TICK * FORWARD_INIT_SYNC_BUFFER_LIMIT_RATIO
 					{
@@ -1209,3 +1209,3 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref, CMH: Deref> P
 					if peer.pending_outbound_buffer.len() > OUTBOUND_BUFFER_LIMIT_DROP_GOSSIP
-						|| peer.msgs_sent_since_pong > BUFFER_DRAIN_MSGS_PER_TICK * FORWARD_INIT_SYNC_BUFFER_SIZE_LIMIT
+						|| peer.msgs_sent_since_pong > BUFFER_DRAIN_MSGS_PER_TICK * FORWARD_INIT_SYNC_BUFFER_LIMIT_RATIO
 					{
$

TheBlueMatt added this to the 0.0.103 milestone Oct 22, 2021

TheBlueMatt force-pushed the 2021-10-ping-fixes branch from ef39f70 to 007cce0 Compare October 23, 2021 02:12

jkczyz self-requested a review October 25, 2021 16:18

arik-so reviewed Oct 25, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-10-ping-fixes branch 4 times, most recently from 82ab64d to fa04659 Compare October 25, 2021 20:54

Correct send-bounding logic in TestRoutingMessageHandler

10580f5

The `cmp::min` appeared to confused `end` for a count.

TheBlueMatt force-pushed the 2021-10-ping-fixes branch 3 times, most recently from 698ef00 to 8616cf1 Compare October 26, 2021 01:47

Add PeerManager::disconnect_all_peers to avoid complexity in BP

4a58e9a

In the coming commits simply calling `timer_tick_occurred` will no longer disconnect all peers, so its helpful to have a utility method.

TheBlueMatt force-pushed the 2021-10-ping-fixes branch 2 times, most recently from cff18d1 to f714dca Compare October 26, 2021 17:30

arik-so reviewed Oct 27, 2021

View reviewed changes

lightning/src/ln/peer_handler.rs Show resolved Hide resolved

jkczyz reviewed Oct 28, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-10-ping-fixes branch 2 times, most recently from 7189cc3 to b4dcb56 Compare October 28, 2021 18:26

arik-so approved these changes Oct 28, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-10-ping-fixes branch from b4dcb56 to 28b7430 Compare October 28, 2021 18:54

jkczyz approved these changes Oct 28, 2021

View reviewed changes

TheBlueMatt added 4 commits October 28, 2021 20:06

Constify the ratio in buf limits between forward and init sync msgs

e496c9b

Util-ify enqueueing an encoded message in peer_handler

3f9a7de

This marginally simplifies coming commits.

Give peers which are sending us messages longer to respond to ping

ed4a39f

See comment for rationale.

Log peer public key more thoroughly when logging in peer_handler

0caa8bb

TheBlueMatt force-pushed the 2021-10-ping-fixes branch from 28b7430 to 0caa8bb Compare October 28, 2021 20:06

TheBlueMatt merged commit 070e22b into lightningdevkit:main Oct 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Give peers which are sending us messages/receiving messages from us longer to respond to ping #1137

Give peers which are sending us messages/receiving messages from us longer to respond to ping #1137

TheBlueMatt commented Oct 22, 2021

codecov bot commented Oct 22, 2021 •

edited

Loading

arik-so Oct 25, 2021

TheBlueMatt Oct 25, 2021

arik-so Oct 25, 2021

TheBlueMatt Oct 25, 2021

arik-so Oct 25, 2021

TheBlueMatt Oct 25, 2021

valentinewallace commented Oct 27, 2021

TheBlueMatt commented Oct 27, 2021

jkczyz Oct 28, 2021

TheBlueMatt Oct 28, 2021

jkczyz Oct 28, 2021

TheBlueMatt Oct 28, 2021

TheBlueMatt Oct 28, 2021

jkczyz Oct 28, 2021

jkczyz Oct 28, 2021

TheBlueMatt Oct 28, 2021

jkczyz Oct 28, 2021

TheBlueMatt Oct 28, 2021

jkczyz Oct 28, 2021

TheBlueMatt Oct 28, 2021

arik-so left a comment

ConorOkus commented Oct 28, 2021

TheBlueMatt commented Oct 28, 2021

Give peers which are sending us messages/receiving messages from us longer to respond to ping #1137

Give peers which are sending us messages/receiving messages from us longer to respond to ping #1137

Conversation

TheBlueMatt commented Oct 22, 2021

codecov bot commented Oct 22, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Oct 27, 2021

TheBlueMatt commented Oct 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arik-so left a comment

Choose a reason for hiding this comment

ConorOkus commented Oct 28, 2021

TheBlueMatt commented Oct 28, 2021

codecov bot commented Oct 22, 2021 •

edited

Loading