Clean up message forwarding and relay gossip messages by TheBlueMatt · Pull Request #948 · lightningdevkit/rust-lightning

TheBlueMatt · 2021-06-10T18:50:20Z

While we really should eventually clean up some of the big PeerHandler refactors and merge them, this fixes a bit of low-hanging fruit in the mean time, also adding forwarding of gossip messages which @devrandom wanted.

codecov · 2021-06-10T19:09:15Z

Codecov Report

Merging #948 (895d1a8) into main (94528f0) will increase coverage by 0.03%.
The diff coverage is 26.27%.

@@ Coverage Diff @@## main #948 +/- ##
==========================================
+ Coverage 90.58% 90.62% +0.03% 
==========================================
Files 60 60 Lines 30423 30409 -14 ==========================================
- Hits 27560 27559 -1 + Misses 2863 2850 -13

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`45.44% <26.27%> (+1.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.17% <0.00%> (-0.05%)`	⬇️
lightning/src/ln/channelmanager.rs	`83.85% <0.00%> (-0.05%)`	⬇️

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 94528f0...895d1a8. Read the comment docs.

devrandom · 2021-06-11T11:28:08Z

is there a quick explanation why do_attempt_write_data call was superfluous?

devrandom · 2021-06-11T11:29:22Z

utACK

TheBlueMatt · 2021-06-11T14:43:31Z

is there a quick explanation why do_attempt_write_data call was superfluous?

Not that its superfluous, but that it violates the stated API of read handling not generating reentrant callbacks, see eae0904.

devrandom · 2021-06-11T15:04:53Z

I guess I'm asking if something else will kick off any writes that need to be sent out?

TheBlueMatt · 2021-06-11T15:27:52Z

I guess I'm asking if something else will kick off any writes that need to be sent out?

Yes, the docs indicate you need to call process_events() to flush outbound message buffers.

ariard

Otherwise, LGTM

ariard · 2021-06-14T17:48:07Z

 }
 }

- self.do_attempt_write_data(peer_descriptor, peer);


If you remember about it, what the initial purpose of this "write-just-after-read-in-case-of-reply" ? Save a call to process_events to the API user?

Yea, just to be quicker about it - save an inertion into the peers_needing_send map and such. Its not all that critical unless the user only calls process_events in a loop and has some sleep we end up blocking on.

ariard · 2021-06-14T17:58:50Z

+ }
+ if except_node.is_some() && peer.their_node_id.as_ref() == except_node {
+ continue;
+ }


Should you include a non-replay-to-channel-update-broadcaster check as it's done for the 2 other gossips ?

There is no node_id field in a channel_update, its assumed the network graph can figure it out from the short_channel_id.

ariard · 2021-06-14T17:59:39Z

 Ok(())
 	}

+	fn forward_broadcast_msg(&self, peers: &mut PeerHolder<Descriptor>, msg: &wire::Message, except_node: Option<&PublicKey>) {


nit: forward_gossip_msg ?

Presumably it could be used for any broadcast message? I like mentioning the word "broadcast" in it.

ariard · 2021-06-14T18:21:11Z


+/// When the outbound buffer has this many messages, we'll stop reading bytes from the peer until
+/// we manage to send messages until we reach this limit.
+const OUTBOUND_BUFFER_LIMIT_READ_PAUSE: usize = 10;


Shouldn't this constant call OUTBOUND_BUFFER_LIMIT_SYNC_PAUSE ? Otherwise, it's a bit confusing given it's called in do_attempt_write_data

Hmm, its used for both, not sure which is better to focus on. I could just call it like OUTBOUND_BUFFER_LOW_WATER_MARK which is pretty normal for these types of constants, and document in the doccomment what its used for?

devrandom · 2021-06-17T12:46:23Z

tested ACK

We can never assume that messages were reliably delivered whether we placed them in the socket or not, so there isn't a lot of use in explicitly handling the case that a peer was not connected when we went to send it a message. Two TODOs are left for the generation of a `FundingAbandoned` (or similar) event, though it ultimately belongs in `ChannelManager`.

`Julian Knutsen <julianknutsen@users.noreply.github.com>` pointed out in a previous discussion that `read_event` can reenter user code despite the documentation stating explicitly that it will not. This was addressed in lightningdevkit#456 by simply codifying the reentrancy, but its somewhat simpler to just drop the `do_attempt_write_data` call. Ideally we could land most of Julian's work, but its still in need of substantial git history cleanup to get it in a reviewable state and this solves the immediate issue.

This will allow us to broadcast messages received in the next commit.

TheBlueMatt · 2021-06-21T16:02:50Z

Rebased to fix a trivial conflict and squashed fixup commits.

devrandom

ACK other than nits

TheBlueMatt · 2021-06-21T19:04:28Z

Addressed comments (with fixup commits and one new commit at the head).

We do a lot of work to track which peers have buffers which need to be flushed, when we could instead just try to flush ever peer's buffer.

This avoids a now-unnecessary SocketDescriptor clone() call in addition to cleaning up the callsite code somewhat.

TheBlueMatt · 2021-06-21T19:45:37Z

Squashed with no changes. Changes since @devrandom's last ack are also trivial. Will merge after CI.

TheBlueMatt · 2021-06-10T18:50:20Z

While we really should eventually clean up some of the big PeerHandler refactors and merge them, this fixes a bit of low-hanging fruit in the mean time, also adding forwarding of gossip messages which @devrandom wanted.

codecov · 2021-06-10T19:09:15Z

Codecov Report

Merging #948 (895d1a8) into main (94528f0) will increase coverage by 0.03%.
The diff coverage is 26.27%.

@@ Coverage Diff @@## main #948 +/- ##
==========================================
+ Coverage 90.58% 90.62% +0.03% 
==========================================
Files 60 60 Lines 30423 30409 -14 ==========================================
- Hits 27560 27559 -1 + Misses 2863 2850 -13

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`45.44% <26.27%> (+1.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.17% <0.00%> (-0.05%)`	⬇️
lightning/src/ln/channelmanager.rs	`83.85% <0.00%> (-0.05%)`	⬇️

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 94528f0...895d1a8. Read the comment docs.

devrandom · 2021-06-11T11:28:08Z

is there a quick explanation why do_attempt_write_data call was superfluous?

devrandom · 2021-06-11T11:29:22Z

utACK

TheBlueMatt · 2021-06-11T14:43:31Z

is there a quick explanation why do_attempt_write_data call was superfluous?

Not that its superfluous, but that it violates the stated API of read handling not generating reentrant callbacks, see eae0904.

devrandom · 2021-06-11T15:04:53Z

I guess I'm asking if something else will kick off any writes that need to be sent out?

TheBlueMatt · 2021-06-11T15:27:52Z

I guess I'm asking if something else will kick off any writes that need to be sent out?

Yes, the docs indicate you need to call process_events() to flush outbound message buffers.

ariard

Otherwise, LGTM

ariard · 2021-06-14T17:48:07Z

 }
 }

- self.do_attempt_write_data(peer_descriptor, peer);


If you remember about it, what the initial purpose of this "write-just-after-read-in-case-of-reply" ? Save a call to process_events to the API user?

Yea, just to be quicker about it - save an inertion into the peers_needing_send map and such. Its not all that critical unless the user only calls process_events in a loop and has some sleep we end up blocking on.

ariard · 2021-06-14T17:58:50Z

+ }
+ if except_node.is_some() && peer.their_node_id.as_ref() == except_node {
+ continue;
+ }


Should you include a non-replay-to-channel-update-broadcaster check as it's done for the 2 other gossips ?

There is no node_id field in a channel_update, its assumed the network graph can figure it out from the short_channel_id.

ariard · 2021-06-14T17:59:39Z

 Ok(())
 	}

+	fn forward_broadcast_msg(&self, peers: &mut PeerHolder<Descriptor>, msg: &wire::Message, except_node: Option<&PublicKey>) {


nit: forward_gossip_msg ?

Presumably it could be used for any broadcast message? I like mentioning the word "broadcast" in it.

ariard · 2021-06-14T18:21:11Z


+/// When the outbound buffer has this many messages, we'll stop reading bytes from the peer until
+/// we manage to send messages until we reach this limit.
+const OUTBOUND_BUFFER_LIMIT_READ_PAUSE: usize = 10;


Shouldn't this constant call OUTBOUND_BUFFER_LIMIT_SYNC_PAUSE ? Otherwise, it's a bit confusing given it's called in do_attempt_write_data

Hmm, its used for both, not sure which is better to focus on. I could just call it like OUTBOUND_BUFFER_LOW_WATER_MARK which is pretty normal for these types of constants, and document in the doccomment what its used for?

devrandom · 2021-06-17T12:46:23Z

tested ACK

We can never assume that messages were reliably delivered whether we placed them in the socket or not, so there isn't a lot of use in explicitly handling the case that a peer was not connected when we went to send it a message. Two TODOs are left for the generation of a `FundingAbandoned` (or similar) event, though it ultimately belongs in `ChannelManager`.

`Julian Knutsen <julianknutsen@users.noreply.github.com>` pointed out in a previous discussion that `read_event` can reenter user code despite the documentation stating explicitly that it will not. This was addressed in lightningdevkit#456 by simply codifying the reentrancy, but its somewhat simpler to just drop the `do_attempt_write_data` call. Ideally we could land most of Julian's work, but its still in need of substantial git history cleanup to get it in a reviewable state and this solves the immediate issue.

This will allow us to broadcast messages received in the next commit.

TheBlueMatt · 2021-06-21T16:02:50Z

Rebased to fix a trivial conflict and squashed fixup commits.

devrandom

ACK other than nits

TheBlueMatt · 2021-06-21T19:04:28Z

Addressed comments (with fixup commits and one new commit at the head).

We do a lot of work to track which peers have buffers which need to be flushed, when we could instead just try to flush ever peer's buffer.

This avoids a now-unnecessary SocketDescriptor clone() call in addition to cleaning up the callsite code somewhat.

TheBlueMatt · 2021-06-21T19:45:37Z

Squashed with no changes. Changes since @devrandom's last ack are also trivial. Will merge after CI.

TheBlueMatt · 2021-06-10T18:50:20Z

While we really should eventually clean up some of the big PeerHandler refactors and merge them, this fixes a bit of low-hanging fruit in the mean time, also adding forwarding of gossip messages which @devrandom wanted.

codecov · 2021-06-10T19:09:15Z

Codecov Report

Merging #948 (895d1a8) into main (94528f0) will increase coverage by 0.03%.
The diff coverage is 26.27%.

@@ Coverage Diff @@## main #948 +/- ##
==========================================
+ Coverage 90.58% 90.62% +0.03% 
==========================================
Files 60 60 Lines 30423 30409 -14 ==========================================
- Hits 27560 27559 -1 + Misses 2863 2850 -13

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`45.44% <26.27%> (+1.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.17% <0.00%> (-0.05%)`	⬇️
lightning/src/ln/channelmanager.rs	`83.85% <0.00%> (-0.05%)`	⬇️

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 94528f0...895d1a8. Read the comment docs.

devrandom · 2021-06-11T11:28:08Z

is there a quick explanation why do_attempt_write_data call was superfluous?

devrandom · 2021-06-11T11:29:22Z

utACK

TheBlueMatt · 2021-06-11T14:43:31Z

is there a quick explanation why do_attempt_write_data call was superfluous?

Not that its superfluous, but that it violates the stated API of read handling not generating reentrant callbacks, see eae0904.

devrandom · 2021-06-11T15:04:53Z

I guess I'm asking if something else will kick off any writes that need to be sent out?

TheBlueMatt · 2021-06-11T15:27:52Z

I guess I'm asking if something else will kick off any writes that need to be sent out?

Yes, the docs indicate you need to call process_events() to flush outbound message buffers.

ariard

Otherwise, LGTM

ariard · 2021-06-14T17:48:07Z

 }
 }

- self.do_attempt_write_data(peer_descriptor, peer);


If you remember about it, what the initial purpose of this "write-just-after-read-in-case-of-reply" ? Save a call to process_events to the API user?

Yea, just to be quicker about it - save an inertion into the peers_needing_send map and such. Its not all that critical unless the user only calls process_events in a loop and has some sleep we end up blocking on.

ariard · 2021-06-14T17:58:50Z

+ }
+ if except_node.is_some() && peer.their_node_id.as_ref() == except_node {
+ continue;
+ }


Should you include a non-replay-to-channel-update-broadcaster check as it's done for the 2 other gossips ?

There is no node_id field in a channel_update, its assumed the network graph can figure it out from the short_channel_id.

ariard · 2021-06-14T17:59:39Z

 Ok(())
 	}

+	fn forward_broadcast_msg(&self, peers: &mut PeerHolder<Descriptor>, msg: &wire::Message, except_node: Option<&PublicKey>) {


nit: forward_gossip_msg ?

Presumably it could be used for any broadcast message? I like mentioning the word "broadcast" in it.

ariard · 2021-06-14T18:21:11Z


+/// When the outbound buffer has this many messages, we'll stop reading bytes from the peer until
+/// we manage to send messages until we reach this limit.
+const OUTBOUND_BUFFER_LIMIT_READ_PAUSE: usize = 10;


Shouldn't this constant call OUTBOUND_BUFFER_LIMIT_SYNC_PAUSE ? Otherwise, it's a bit confusing given it's called in do_attempt_write_data

Hmm, its used for both, not sure which is better to focus on. I could just call it like OUTBOUND_BUFFER_LOW_WATER_MARK which is pretty normal for these types of constants, and document in the doccomment what its used for?

devrandom · 2021-06-17T12:46:23Z

tested ACK

We can never assume that messages were reliably delivered whether we placed them in the socket or not, so there isn't a lot of use in explicitly handling the case that a peer was not connected when we went to send it a message. Two TODOs are left for the generation of a `FundingAbandoned` (or similar) event, though it ultimately belongs in `ChannelManager`.

`Julian Knutsen <julianknutsen@users.noreply.github.com>` pointed out in a previous discussion that `read_event` can reenter user code despite the documentation stating explicitly that it will not. This was addressed in lightningdevkit#456 by simply codifying the reentrancy, but its somewhat simpler to just drop the `do_attempt_write_data` call. Ideally we could land most of Julian's work, but its still in need of substantial git history cleanup to get it in a reviewable state and this solves the immediate issue.

This will allow us to broadcast messages received in the next commit.

TheBlueMatt · 2021-06-21T16:02:50Z

Rebased to fix a trivial conflict and squashed fixup commits.

devrandom

ACK other than nits

TheBlueMatt · 2021-06-21T19:04:28Z

Addressed comments (with fixup commits and one new commit at the head).

We do a lot of work to track which peers have buffers which need to be flushed, when we could instead just try to flush ever peer's buffer.

This avoids a now-unnecessary SocketDescriptor clone() call in addition to cleaning up the callsite code somewhat.

TheBlueMatt · 2021-06-21T19:45:37Z

Squashed with no changes. Changes since @devrandom's last ack are also trivial. Will merge after CI.

TheBlueMatt · 2021-06-10T18:50:20Z

While we really should eventually clean up some of the big PeerHandler refactors and merge them, this fixes a bit of low-hanging fruit in the mean time, also adding forwarding of gossip messages which @devrandom wanted.

codecov · 2021-06-10T19:09:15Z

Codecov Report

Merging #948 (895d1a8) into main (94528f0) will increase coverage by 0.03%.
The diff coverage is 26.27%.

@@ Coverage Diff @@## main #948 +/- ##
==========================================
+ Coverage 90.58% 90.62% +0.03% 
==========================================
Files 60 60 Lines 30423 30409 -14 ==========================================
- Hits 27560 27559 -1 + Misses 2863 2850 -13

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`45.44% <26.27%> (+1.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.17% <0.00%> (-0.05%)`	⬇️
lightning/src/ln/channelmanager.rs	`83.85% <0.00%> (-0.05%)`	⬇️

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 94528f0...895d1a8. Read the comment docs.

devrandom · 2021-06-11T11:28:08Z

is there a quick explanation why do_attempt_write_data call was superfluous?

devrandom · 2021-06-11T11:29:22Z

utACK

TheBlueMatt · 2021-06-11T14:43:31Z

is there a quick explanation why do_attempt_write_data call was superfluous?

Not that its superfluous, but that it violates the stated API of read handling not generating reentrant callbacks, see eae0904.

devrandom · 2021-06-11T15:04:53Z

I guess I'm asking if something else will kick off any writes that need to be sent out?

TheBlueMatt · 2021-06-11T15:27:52Z

I guess I'm asking if something else will kick off any writes that need to be sent out?

Yes, the docs indicate you need to call process_events() to flush outbound message buffers.

ariard

Otherwise, LGTM

ariard · 2021-06-14T17:48:07Z

 }
 }

- self.do_attempt_write_data(peer_descriptor, peer);


If you remember about it, what the initial purpose of this "write-just-after-read-in-case-of-reply" ? Save a call to process_events to the API user?

Yea, just to be quicker about it - save an inertion into the peers_needing_send map and such. Its not all that critical unless the user only calls process_events in a loop and has some sleep we end up blocking on.

ariard · 2021-06-14T17:58:50Z

+ }
+ if except_node.is_some() && peer.their_node_id.as_ref() == except_node {
+ continue;
+ }


Should you include a non-replay-to-channel-update-broadcaster check as it's done for the 2 other gossips ?

There is no node_id field in a channel_update, its assumed the network graph can figure it out from the short_channel_id.

ariard · 2021-06-14T17:59:39Z

 Ok(())
 	}

+	fn forward_broadcast_msg(&self, peers: &mut PeerHolder<Descriptor>, msg: &wire::Message, except_node: Option<&PublicKey>) {


nit: forward_gossip_msg ?

Presumably it could be used for any broadcast message? I like mentioning the word "broadcast" in it.

ariard · 2021-06-14T18:21:11Z


+/// When the outbound buffer has this many messages, we'll stop reading bytes from the peer until
+/// we manage to send messages until we reach this limit.
+const OUTBOUND_BUFFER_LIMIT_READ_PAUSE: usize = 10;


Shouldn't this constant call OUTBOUND_BUFFER_LIMIT_SYNC_PAUSE ? Otherwise, it's a bit confusing given it's called in do_attempt_write_data

Hmm, its used for both, not sure which is better to focus on. I could just call it like OUTBOUND_BUFFER_LOW_WATER_MARK which is pretty normal for these types of constants, and document in the doccomment what its used for?

devrandom · 2021-06-17T12:46:23Z

tested ACK

We can never assume that messages were reliably delivered whether we placed them in the socket or not, so there isn't a lot of use in explicitly handling the case that a peer was not connected when we went to send it a message. Two TODOs are left for the generation of a `FundingAbandoned` (or similar) event, though it ultimately belongs in `ChannelManager`.

`Julian Knutsen <julianknutsen@users.noreply.github.com>` pointed out in a previous discussion that `read_event` can reenter user code despite the documentation stating explicitly that it will not. This was addressed in lightningdevkit#456 by simply codifying the reentrancy, but its somewhat simpler to just drop the `do_attempt_write_data` call. Ideally we could land most of Julian's work, but its still in need of substantial git history cleanup to get it in a reviewable state and this solves the immediate issue.

This will allow us to broadcast messages received in the next commit.

TheBlueMatt · 2021-06-21T16:02:50Z

Rebased to fix a trivial conflict and squashed fixup commits.

devrandom

ACK other than nits

TheBlueMatt · 2021-06-21T19:04:28Z

Addressed comments (with fixup commits and one new commit at the head).

We do a lot of work to track which peers have buffers which need to be flushed, when we could instead just try to flush ever peer's buffer.

This avoids a now-unnecessary SocketDescriptor clone() call in addition to cleaning up the callsite code somewhat.

TheBlueMatt · 2021-06-21T19:45:37Z

Squashed with no changes. Changes since @devrandom's last ack are also trivial. Will merge after CI.

TheBlueMatt · 2021-06-10T18:50:20Z

While we really should eventually clean up some of the big PeerHandler refactors and merge them, this fixes a bit of low-hanging fruit in the mean time, also adding forwarding of gossip messages which @devrandom wanted.

codecov · 2021-06-10T19:09:15Z

Codecov Report

Merging #948 (895d1a8) into main (94528f0) will increase coverage by 0.03%.
The diff coverage is 26.27%.

@@ Coverage Diff @@## main #948 +/- ##
==========================================
+ Coverage 90.58% 90.62% +0.03% 
==========================================
Files 60 60 Lines 30423 30409 -14 ==========================================
- Hits 27560 27559 -1 + Misses 2863 2850 -13

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`45.44% <26.27%> (+1.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.17% <0.00%> (-0.05%)`	⬇️
lightning/src/ln/channelmanager.rs	`83.85% <0.00%> (-0.05%)`	⬇️

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 94528f0...895d1a8. Read the comment docs.

devrandom · 2021-06-11T11:28:08Z

is there a quick explanation why do_attempt_write_data call was superfluous?

devrandom · 2021-06-11T11:29:22Z

utACK

TheBlueMatt · 2021-06-11T14:43:31Z

is there a quick explanation why do_attempt_write_data call was superfluous?

Not that its superfluous, but that it violates the stated API of read handling not generating reentrant callbacks, see eae0904.

devrandom · 2021-06-11T15:04:53Z

I guess I'm asking if something else will kick off any writes that need to be sent out?

TheBlueMatt · 2021-06-11T15:27:52Z

I guess I'm asking if something else will kick off any writes that need to be sent out?

Yes, the docs indicate you need to call process_events() to flush outbound message buffers.

ariard

Otherwise, LGTM

ariard · 2021-06-14T17:48:07Z

 }
 }

- self.do_attempt_write_data(peer_descriptor, peer);


If you remember about it, what the initial purpose of this "write-just-after-read-in-case-of-reply" ? Save a call to process_events to the API user?

Yea, just to be quicker about it - save an inertion into the peers_needing_send map and such. Its not all that critical unless the user only calls process_events in a loop and has some sleep we end up blocking on.

ariard · 2021-06-14T17:58:50Z

+ }
+ if except_node.is_some() && peer.their_node_id.as_ref() == except_node {
+ continue;
+ }


Should you include a non-replay-to-channel-update-broadcaster check as it's done for the 2 other gossips ?

There is no node_id field in a channel_update, its assumed the network graph can figure it out from the short_channel_id.

ariard · 2021-06-14T17:59:39Z

 Ok(())
 	}

+	fn forward_broadcast_msg(&self, peers: &mut PeerHolder<Descriptor>, msg: &wire::Message, except_node: Option<&PublicKey>) {


nit: forward_gossip_msg ?

Presumably it could be used for any broadcast message? I like mentioning the word "broadcast" in it.

ariard · 2021-06-14T18:21:11Z


+/// When the outbound buffer has this many messages, we'll stop reading bytes from the peer until
+/// we manage to send messages until we reach this limit.
+const OUTBOUND_BUFFER_LIMIT_READ_PAUSE: usize = 10;


Shouldn't this constant call OUTBOUND_BUFFER_LIMIT_SYNC_PAUSE ? Otherwise, it's a bit confusing given it's called in do_attempt_write_data

Hmm, its used for both, not sure which is better to focus on. I could just call it like OUTBOUND_BUFFER_LOW_WATER_MARK which is pretty normal for these types of constants, and document in the doccomment what its used for?

devrandom · 2021-06-17T12:46:23Z

tested ACK

We can never assume that messages were reliably delivered whether we placed them in the socket or not, so there isn't a lot of use in explicitly handling the case that a peer was not connected when we went to send it a message. Two TODOs are left for the generation of a `FundingAbandoned` (or similar) event, though it ultimately belongs in `ChannelManager`.

`Julian Knutsen <julianknutsen@users.noreply.github.com>` pointed out in a previous discussion that `read_event` can reenter user code despite the documentation stating explicitly that it will not. This was addressed in lightningdevkit#456 by simply codifying the reentrancy, but its somewhat simpler to just drop the `do_attempt_write_data` call. Ideally we could land most of Julian's work, but its still in need of substantial git history cleanup to get it in a reviewable state and this solves the immediate issue.

This will allow us to broadcast messages received in the next commit.

TheBlueMatt · 2021-06-21T16:02:50Z

Rebased to fix a trivial conflict and squashed fixup commits.

devrandom

ACK other than nits

TheBlueMatt · 2021-06-21T19:04:28Z

Addressed comments (with fixup commits and one new commit at the head).

We do a lot of work to track which peers have buffers which need to be flushed, when we could instead just try to flush ever peer's buffer.

This avoids a now-unnecessary SocketDescriptor clone() call in addition to cleaning up the callsite code somewhat.

TheBlueMatt · 2021-06-21T19:45:37Z

Squashed with no changes. Changes since @devrandom's last ack are also trivial. Will merge after CI.

TheBlueMatt · 2021-06-10T18:50:20Z

While we really should eventually clean up some of the big PeerHandler refactors and merge them, this fixes a bit of low-hanging fruit in the mean time, also adding forwarding of gossip messages which @devrandom wanted.

codecov · 2021-06-10T19:09:15Z

Codecov Report

Merging #948 (895d1a8) into main (94528f0) will increase coverage by 0.03%.
The diff coverage is 26.27%.

@@ Coverage Diff @@## main #948 +/- ##
==========================================
+ Coverage 90.58% 90.62% +0.03% 
==========================================
Files 60 60 Lines 30423 30409 -14 ==========================================
- Hits 27560 27559 -1 + Misses 2863 2850 -13

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`45.44% <26.27%> (+1.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.17% <0.00%> (-0.05%)`	⬇️
lightning/src/ln/channelmanager.rs	`83.85% <0.00%> (-0.05%)`	⬇️

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 94528f0...895d1a8. Read the comment docs.

devrandom · 2021-06-11T11:28:08Z

is there a quick explanation why do_attempt_write_data call was superfluous?

devrandom · 2021-06-11T11:29:22Z

utACK

TheBlueMatt · 2021-06-11T14:43:31Z

is there a quick explanation why do_attempt_write_data call was superfluous?

Not that its superfluous, but that it violates the stated API of read handling not generating reentrant callbacks, see eae0904.

devrandom · 2021-06-11T15:04:53Z

I guess I'm asking if something else will kick off any writes that need to be sent out?

TheBlueMatt · 2021-06-11T15:27:52Z

I guess I'm asking if something else will kick off any writes that need to be sent out?

Yes, the docs indicate you need to call process_events() to flush outbound message buffers.

ariard

Otherwise, LGTM

ariard · 2021-06-14T17:48:07Z

 }
 }

- self.do_attempt_write_data(peer_descriptor, peer);


If you remember about it, what the initial purpose of this "write-just-after-read-in-case-of-reply" ? Save a call to process_events to the API user?

Yea, just to be quicker about it - save an inertion into the peers_needing_send map and such. Its not all that critical unless the user only calls process_events in a loop and has some sleep we end up blocking on.

ariard · 2021-06-14T17:58:50Z

+ }
+ if except_node.is_some() && peer.their_node_id.as_ref() == except_node {
+ continue;
+ }


Should you include a non-replay-to-channel-update-broadcaster check as it's done for the 2 other gossips ?

There is no node_id field in a channel_update, its assumed the network graph can figure it out from the short_channel_id.

ariard · 2021-06-14T17:59:39Z

 Ok(())
 	}

+	fn forward_broadcast_msg(&self, peers: &mut PeerHolder<Descriptor>, msg: &wire::Message, except_node: Option<&PublicKey>) {


nit: forward_gossip_msg ?

Presumably it could be used for any broadcast message? I like mentioning the word "broadcast" in it.

ariard · 2021-06-14T18:21:11Z


+/// When the outbound buffer has this many messages, we'll stop reading bytes from the peer until
+/// we manage to send messages until we reach this limit.
+const OUTBOUND_BUFFER_LIMIT_READ_PAUSE: usize = 10;


Shouldn't this constant call OUTBOUND_BUFFER_LIMIT_SYNC_PAUSE ? Otherwise, it's a bit confusing given it's called in do_attempt_write_data

Hmm, its used for both, not sure which is better to focus on. I could just call it like OUTBOUND_BUFFER_LOW_WATER_MARK which is pretty normal for these types of constants, and document in the doccomment what its used for?

devrandom · 2021-06-17T12:46:23Z

tested ACK

We can never assume that messages were reliably delivered whether we placed them in the socket or not, so there isn't a lot of use in explicitly handling the case that a peer was not connected when we went to send it a message. Two TODOs are left for the generation of a `FundingAbandoned` (or similar) event, though it ultimately belongs in `ChannelManager`.

`Julian Knutsen <julianknutsen@users.noreply.github.com>` pointed out in a previous discussion that `read_event` can reenter user code despite the documentation stating explicitly that it will not. This was addressed in lightningdevkit#456 by simply codifying the reentrancy, but its somewhat simpler to just drop the `do_attempt_write_data` call. Ideally we could land most of Julian's work, but its still in need of substantial git history cleanup to get it in a reviewable state and this solves the immediate issue.

This will allow us to broadcast messages received in the next commit.

TheBlueMatt · 2021-06-21T16:02:50Z

Rebased to fix a trivial conflict and squashed fixup commits.

devrandom

ACK other than nits

TheBlueMatt · 2021-06-21T19:04:28Z

Addressed comments (with fixup commits and one new commit at the head).

We do a lot of work to track which peers have buffers which need to be flushed, when we could instead just try to flush ever peer's buffer.

This avoids a now-unnecessary SocketDescriptor clone() call in addition to cleaning up the callsite code somewhat.

TheBlueMatt · 2021-06-21T19:45:37Z

Squashed with no changes. Changes since @devrandom's last ack are also trivial. Will merge after CI.

TheBlueMatt · 2021-06-10T18:50:20Z

While we really should eventually clean up some of the big PeerHandler refactors and merge them, this fixes a bit of low-hanging fruit in the mean time, also adding forwarding of gossip messages which @devrandom wanted.

codecov · 2021-06-10T19:09:15Z

Codecov Report

Merging #948 (895d1a8) into main (94528f0) will increase coverage by 0.03%.
The diff coverage is 26.27%.

@@ Coverage Diff @@## main #948 +/- ##
==========================================
+ Coverage 90.58% 90.62% +0.03% 
==========================================
Files 60 60 Lines 30423 30409 -14 ==========================================
- Hits 27560 27559 -1 + Misses 2863 2850 -13

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`45.44% <26.27%> (+1.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.17% <0.00%> (-0.05%)`	⬇️
lightning/src/ln/channelmanager.rs	`83.85% <0.00%> (-0.05%)`	⬇️

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 94528f0...895d1a8. Read the comment docs.

devrandom · 2021-06-11T11:28:08Z

is there a quick explanation why do_attempt_write_data call was superfluous?

devrandom · 2021-06-11T11:29:22Z

utACK

TheBlueMatt · 2021-06-11T14:43:31Z

is there a quick explanation why do_attempt_write_data call was superfluous?

Not that its superfluous, but that it violates the stated API of read handling not generating reentrant callbacks, see eae0904.

devrandom · 2021-06-11T15:04:53Z

I guess I'm asking if something else will kick off any writes that need to be sent out?

TheBlueMatt · 2021-06-11T15:27:52Z

I guess I'm asking if something else will kick off any writes that need to be sent out?

Yes, the docs indicate you need to call process_events() to flush outbound message buffers.

ariard

Otherwise, LGTM

ariard · 2021-06-14T17:48:07Z

 }
 }

- self.do_attempt_write_data(peer_descriptor, peer);


If you remember about it, what the initial purpose of this "write-just-after-read-in-case-of-reply" ? Save a call to process_events to the API user?

Yea, just to be quicker about it - save an inertion into the peers_needing_send map and such. Its not all that critical unless the user only calls process_events in a loop and has some sleep we end up blocking on.

ariard · 2021-06-14T17:58:50Z

+ }
+ if except_node.is_some() && peer.their_node_id.as_ref() == except_node {
+ continue;
+ }


Should you include a non-replay-to-channel-update-broadcaster check as it's done for the 2 other gossips ?

There is no node_id field in a channel_update, its assumed the network graph can figure it out from the short_channel_id.

ariard · 2021-06-14T17:59:39Z

 Ok(())
 	}

+	fn forward_broadcast_msg(&self, peers: &mut PeerHolder<Descriptor>, msg: &wire::Message, except_node: Option<&PublicKey>) {


nit: forward_gossip_msg ?

Presumably it could be used for any broadcast message? I like mentioning the word "broadcast" in it.

ariard · 2021-06-14T18:21:11Z


+/// When the outbound buffer has this many messages, we'll stop reading bytes from the peer until
+/// we manage to send messages until we reach this limit.
+const OUTBOUND_BUFFER_LIMIT_READ_PAUSE: usize = 10;


Shouldn't this constant call OUTBOUND_BUFFER_LIMIT_SYNC_PAUSE ? Otherwise, it's a bit confusing given it's called in do_attempt_write_data

Hmm, its used for both, not sure which is better to focus on. I could just call it like OUTBOUND_BUFFER_LOW_WATER_MARK which is pretty normal for these types of constants, and document in the doccomment what its used for?

devrandom · 2021-06-17T12:46:23Z

tested ACK

We can never assume that messages were reliably delivered whether we placed them in the socket or not, so there isn't a lot of use in explicitly handling the case that a peer was not connected when we went to send it a message. Two TODOs are left for the generation of a `FundingAbandoned` (or similar) event, though it ultimately belongs in `ChannelManager`.

`Julian Knutsen <julianknutsen@users.noreply.github.com>` pointed out in a previous discussion that `read_event` can reenter user code despite the documentation stating explicitly that it will not. This was addressed in lightningdevkit#456 by simply codifying the reentrancy, but its somewhat simpler to just drop the `do_attempt_write_data` call. Ideally we could land most of Julian's work, but its still in need of substantial git history cleanup to get it in a reviewable state and this solves the immediate issue.

This will allow us to broadcast messages received in the next commit.

TheBlueMatt · 2021-06-21T16:02:50Z

Rebased to fix a trivial conflict and squashed fixup commits.

devrandom

ACK other than nits

TheBlueMatt · 2021-06-21T19:04:28Z

Addressed comments (with fixup commits and one new commit at the head).

We do a lot of work to track which peers have buffers which need to be flushed, when we could instead just try to flush ever peer's buffer.

This avoids a now-unnecessary SocketDescriptor clone() call in addition to cleaning up the callsite code somewhat.

TheBlueMatt · 2021-06-21T19:45:37Z

Squashed with no changes. Changes since @devrandom's last ack are also trivial. Will merge after CI.

TheBlueMatt · 2021-06-10T18:50:20Z

While we really should eventually clean up some of the big PeerHandler refactors and merge them, this fixes a bit of low-hanging fruit in the mean time, also adding forwarding of gossip messages which @devrandom wanted.

codecov · 2021-06-10T19:09:15Z

Codecov Report

Merging #948 (895d1a8) into main (94528f0) will increase coverage by 0.03%.
The diff coverage is 26.27%.

@@ Coverage Diff @@## main #948 +/- ##
==========================================
+ Coverage 90.58% 90.62% +0.03% 
==========================================
Files 60 60 Lines 30423 30409 -14 ==========================================
- Hits 27560 27559 -1 + Misses 2863 2850 -13

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	`45.44% <26.27%> (+1.13%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.17% <0.00%> (-0.05%)`	⬇️
lightning/src/ln/channelmanager.rs	`83.85% <0.00%> (-0.05%)`	⬇️

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 94528f0...895d1a8. Read the comment docs.

devrandom · 2021-06-11T11:28:08Z

is there a quick explanation why do_attempt_write_data call was superfluous?

devrandom · 2021-06-11T11:29:22Z

utACK

TheBlueMatt · 2021-06-11T14:43:31Z

is there a quick explanation why do_attempt_write_data call was superfluous?

Not that its superfluous, but that it violates the stated API of read handling not generating reentrant callbacks, see eae0904.

devrandom · 2021-06-11T15:04:53Z

I guess I'm asking if something else will kick off any writes that need to be sent out?

TheBlueMatt · 2021-06-11T15:27:52Z

I guess I'm asking if something else will kick off any writes that need to be sent out?

Yes, the docs indicate you need to call process_events() to flush outbound message buffers.

ariard

Otherwise, LGTM

ariard · 2021-06-14T17:48:07Z

 }
 }

- self.do_attempt_write_data(peer_descriptor, peer);


If you remember about it, what the initial purpose of this "write-just-after-read-in-case-of-reply" ? Save a call to process_events to the API user?

Yea, just to be quicker about it - save an inertion into the peers_needing_send map and such. Its not all that critical unless the user only calls process_events in a loop and has some sleep we end up blocking on.

ariard · 2021-06-14T17:58:50Z

+ }
+ if except_node.is_some() && peer.their_node_id.as_ref() == except_node {
+ continue;
+ }


Should you include a non-replay-to-channel-update-broadcaster check as it's done for the 2 other gossips ?

There is no node_id field in a channel_update, its assumed the network graph can figure it out from the short_channel_id.

ariard · 2021-06-14T17:59:39Z

 Ok(())
 	}

+	fn forward_broadcast_msg(&self, peers: &mut PeerHolder<Descriptor>, msg: &wire::Message, except_node: Option<&PublicKey>) {


nit: forward_gossip_msg ?

Presumably it could be used for any broadcast message? I like mentioning the word "broadcast" in it.

ariard · 2021-06-14T18:21:11Z


+/// When the outbound buffer has this many messages, we'll stop reading bytes from the peer until
+/// we manage to send messages until we reach this limit.
+const OUTBOUND_BUFFER_LIMIT_READ_PAUSE: usize = 10;


Shouldn't this constant call OUTBOUND_BUFFER_LIMIT_SYNC_PAUSE ? Otherwise, it's a bit confusing given it's called in do_attempt_write_data

Hmm, its used for both, not sure which is better to focus on. I could just call it like OUTBOUND_BUFFER_LOW_WATER_MARK which is pretty normal for these types of constants, and document in the doccomment what its used for?

devrandom · 2021-06-17T12:46:23Z

tested ACK

We can never assume that messages were reliably delivered whether we placed them in the socket or not, so there isn't a lot of use in explicitly handling the case that a peer was not connected when we went to send it a message. Two TODOs are left for the generation of a `FundingAbandoned` (or similar) event, though it ultimately belongs in `ChannelManager`.

`Julian Knutsen <julianknutsen@users.noreply.github.com>` pointed out in a previous discussion that `read_event` can reenter user code despite the documentation stating explicitly that it will not. This was addressed in lightningdevkit#456 by simply codifying the reentrancy, but its somewhat simpler to just drop the `do_attempt_write_data` call. Ideally we could land most of Julian's work, but its still in need of substantial git history cleanup to get it in a reviewable state and this solves the immediate issue.

This will allow us to broadcast messages received in the next commit.

TheBlueMatt · 2021-06-21T16:02:50Z

Rebased to fix a trivial conflict and squashed fixup commits.

devrandom

ACK other than nits

TheBlueMatt · 2021-06-21T19:04:28Z

Addressed comments (with fixup commits and one new commit at the head).

We do a lot of work to track which peers have buffers which need to be flushed, when we could instead just try to flush ever peer's buffer.

This avoids a now-unnecessary SocketDescriptor clone() call in addition to cleaning up the callsite code somewhat.

TheBlueMatt · 2021-06-21T19:45:37Z

Squashed with no changes. Changes since @devrandom's last ack are also trivial. Will merge after CI.

TheBlueMatt mentioned this pull request Jun 12, 2021
Send channel_update messages to direct peers on private channels #949
Merged

ariard reviewed Jun 14, 2021
View reviewed changes

TheBlueMatt force-pushed the 2021-06-p2p-fixes branch from 41bb42a to 72a668bCompare June 16, 2021 19:17

devrandom reviewed Jun 17, 2021
View reviewed changes

Comment threadlightning/src/ln/peer_handler.rs Outdated

TheBlueMatt mentioned this pull request Jun 17, 2021
Fix P2P Deadlocks #957
Merged

TheBlueMatt added 4 commits June 21, 2021 16:02

Refactor message broadcasting out into a utility method
9de22ae
This will allow us to broadcast messages received in the next commit.

Forward gossip msgs
c56cd1e

TheBlueMatt force-pushed the 2021-06-p2p-fixes branch from c505f07 to 0d374cfCompare June 21, 2021 16:02

devrandom reviewed Jun 21, 2021
View reviewed changes

Comment threadlightning/src/ln/peer_handler.rs Outdated
Comment threadlightning/src/ln/peer_handler.rs Outdated
Comment threadlightning/src/ln/peer_handler.rs Outdated
Comment threadlightning/src/ln/peer_handler.rs Outdated

devrandom approved these changes Jun 21, 2021
View reviewed changes

valentinewallace self-requested a review June 21, 2021 17:32

valentinewallace reviewed Jun 21, 2021
View reviewed changes

Comment threadlightning/src/ln/peer_handler.rs
Comment threadlightning/src/ln/peer_handler.rs Outdated
Comment threadlightning/src/ln/peer_handler.rs Outdated

TheBlueMatt force-pushed the 2021-06-p2p-fixes branch from 0d374cf to 0d4fa0eCompare June 21, 2021 19:04

valentinewallace approved these changes Jun 21, 2021
View reviewed changes

TheBlueMatt added 4 commits June 21, 2021 19:45

Skip forwarding gossip messages to peers if their buffer is over-full
0426d6e

Drop peers_needing_send tracking and try to write for each peer
ae39b2c
We do a lot of work to track which peers have buffers which need to be flushed, when we could instead just try to flush ever peer's buffer.

Somewhat simplify message handling error mapping in peer_handler
f76e14c

[peer_handler] Drop unused return from get_peer_for_forwarding!()
895d1a8
This avoids a now-unnecessary SocketDescriptor clone() call in addition to cleaning up the callsite code somewhat.

TheBlueMatt force-pushed the 2021-06-p2p-fixes branch from 0d4fa0e to 895d1a8Compare June 21, 2021 19:45

TheBlueMatt merged commit 2f6205b into lightningdevkit:mainJun 21, 2021

TheBlueMatt mentioned this pull request Jun 23, 2021
Remove peers_needing_send set from peer_handling #456
Closed