Rename network-related types #1159

jkczyz · 2021-11-05T19:00:59Z

Rename some network-related types to be a bit more intuitive:

NetworkUpdate::ChannelClosed enum variant to NetworkUpdate::ChannelFailure
NetGraphMsgHandler struct to ~~NetworkGossip~~ P2PGossipSync
network_graph module to ~~network~~ gossip

codecov · 2021-11-05T19:14:59Z

Codecov Report

Merging #1159 (1a9ff7e) into main (c0bbd4d) will increase coverage by 0.02%.
The diff coverage is 99.01%.

@@            Coverage Diff             @@
##             main    #1159      +/-   ##
==========================================
+ Coverage   90.14%   90.17%   +0.02%     
==========================================
  Files          70       70              
  Lines       36448    36448              
==========================================
+ Hits        32856    32866      +10     
+ Misses       3592     3582      -10

Impacted Files	Coverage Δ
lightning-invoice/src/de.rs	`80.94% <ø> (+0.20%)`	⬆️
lightning-invoice/src/lib.rs	`88.51% <ø> (+0.12%)`	⬆️
lightning-invoice/src/payment.rs	`91.15% <ø> (ø)`
lightning-invoice/src/utils.rs	`83.48% <ø> (ø)`
lightning/src/ln/channelmanager.rs	`83.72% <ø> (ø)`
lightning/src/ln/mod.rs	`95.00% <ø> (+5.00%)`	⬆️
lightning/src/ln/monitor_tests.rs	`100.00% <ø> (ø)`
lightning/src/ln/peer_handler.rs	`49.77% <ø> (+0.11%)`	⬆️
lightning/src/ln/reorg_tests.rs	`100.00% <ø> (ø)`
lightning/src/ln/shutdown_tests.rs	`95.87% <ø> (ø)`
... and 20 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 c0bbd4d...1a9ff7e. Read the comment docs.

valentinewallace

Only one I'm eh on is ChannelFailure because I like the symmetry with Event::ChannelClosed and we already kinda tend to deviate from the spec on word choice (which I think is good)

I guess it wouldn't be crazy if NetworkGossip were just changed to Gossip?

Looks good as is though, nice improvement!

valentinewallace · 2021-11-05T20:25:51Z

lightning/src/routing/network_graph.rs

 	/// If permanent, removes a channel from the local storage.
 	/// May cause the removal of nodes too, if this was their last channel.
 	/// If not permanent, makes channels unavailable for routing.
-	pub fn close_channel_from_update(&self, short_channel_id: u64, is_permanent: bool) {
+	pub fn fail_channel(&self, short_channel_id: u64, is_permanent: bool) {


suggestion: prefer channel_failed because imo the gossip isn't the one failing the channel

Also renamed fail_node.

jkczyz · 2021-11-08T17:15:02Z

Only one I'm eh on is ChannelFailure because I like the symmetry with Event::ChannelClosed and we already kinda tend to deviate from the spec on word choice (which I think is good)

lol it was your idea to rename it... 😛

#1117 (comment)

Note that this is describing why a payment failed (BOLT 4). The channel itself might not be closed, which should be determined by monitoring the blockchain according to BOLT 7.

Although only tangentially related, looking through BOLT 4, I think the intention is that a channel failure either has a channel_update or it does not (i.e., there is never an error code with both NODE and UPDATE set). So we may want to model NetworkUpdate in that manner at some point, IIUC. Something like:

pub struct RoutingFailure {
    cause: RoutingFailureCause,
    is_permanent: bool,
}

pub enum RoutingFailureCause {
    Channel {
        short_channel_id: u64,
        update: Option<ChannelUpdate>,
    },
    Node {
        node_id: PublicKey,
    }
}

(Though it's kinda strange that anything with BADONION set doesn't also have NODE set.)

Then NetworkGossip would interpret this as it sees fit rather than have the enum be in terms of how to handle the failure, as it currently is formed. Probably deserves further thought before making this change, though.

I guess it wouldn't be crazy if NetworkGossip were just changed to Gossip?

Hmm... I sorta like the symmetry with the other types in network. 🙂 It's specifically handling gossip about the network.

TheBlueMatt · 2021-11-08T18:09:38Z

I don't really understand the reason behind renaming NetGraphMsgHandler to NetworkGossip, the second seems entirely less descriptive - the object is a message handler that handles graph (or gossip) messages, not a gossip...? Same goes for the module rename of network_graph - its all about the graph/gossip data, "network" could refer to a number of other things, I think?

valentinewallace · 2021-11-08T18:26:12Z

Only one I'm eh on is ChannelFailure because I like the symmetry with Event::ChannelClosed and we already kinda tend to deviate from the spec on word choice (which I think is good)

lol it was your idea to rename it... stuck_out_tongue

#1117 (comment)

Note that this is describing why a payment failed (BOLT 4). The channel itself might not be closed, which should be determined by monitoring the blockchain according to BOLT 7.

Lol!! Oh right... Disregard that 😛

jkczyz · 2021-11-08T21:24:14Z

I don't really understand the reason behind renaming NetGraphMsgHandler to NetworkGossip, the second seems entirely less descriptive - the object is a message handler that handles graph (or gossip) messages, not a gossip...?

NetworkGossip describes what it is responsible for. The NetworkGraph view is a byproduct of this, while message handling is how it achieves only part of this responsibility.

IMHO, NetGraphMsgHandler focuses a bit too much on implementation details and is repetitive -- its interface takes a NetworkGraph and it implements a message handling trait. Additionally, it's a bit of a mouthful and uses abbreviations to make the name shorter.

Same goes for the module rename of network_graph - its all about the graph/gossip data, "network" could refer to a number of other things, I think?

I was thinking about this name in the context of it being a submodule of routing. But opened to another name like gossip, perhaps?

TheBlueMatt · 2021-11-08T22:57:09Z

is repetitive -- its interface takes a NetworkGraph

Fair.

it implements a message handling trait

I dunno if I buy the "it implements a specific trait as its main raison d'être, so the name shouldn't reference that" argument - it exists to handle the messages, trait implementation is somewhat hidden in both rustdoc and other languages, so I feel like its good to communicate that in the object naming?

I don't think I have a specific alternative to propose, but maybe GraphGossipMessageHandler or GraphGossiper?

TheBlueMatt · 2021-11-17T19:00:49Z

Needs rebase.

TheBlueMatt · 2022-05-24T21:37:59Z

really needs rebase lol

jkczyz · 2022-05-28T00:47:58Z

I don't think I have a specific alternative to propose, but maybe GraphGossipMessageHandler or GraphGossiper?

How about GossipProtocol? That encapsulates the concept of message handling in a way that isn't overly verbose.

I don't feel very strongly about renaming the module, so fully expressed it wold be either network::GossipProtocol<&NetworkGraph, ...> or network_graph::GossipProtocol<&NetworkGraph, ...>.

TheBlueMatt · 2022-05-28T18:58:52Z

How about GossipProtocol? That encapsulates the concept of message handling in a way that isn't overly verbose.

I'm not sure that it does? Protocol doesn't scream "handles the message" or "implements the protocol" to me, or at least not nearly as much as overly-verbose type names (which I think I'm generally more okay with than you are).

I don't feel very strongly about renaming the module, so fully expressed it wold be either network::GossipProtocol<&NetworkGraph, ...> or network_graph::GossipProtocol<&NetworkGraph, ...>.

I don't feel super strongly about it either, but I like your above suggestion of gossip over network. routing::gossip makes sense as a path name to me.

jkczyz · 2022-05-31T19:24:28Z

How about GossipProtocol? That encapsulates the concept of message handling in a way that isn't overly verbose.

I'm not sure that it does? Protocol doesn't scream "handles the message" or "implements the protocol" to me, or at least not nearly as much as overly-verbose type names (which I think I'm generally more okay with than you are).

Discussed offline. Will name it P2PGossipSync, which goes nicely with the forthcoming RapidGossipSync struct in #1433.

I don't feel very strongly about renaming the module, so fully expressed it wold be either network::GossipProtocol<&NetworkGraph, ...> or network_graph::GossipProtocol<&NetworkGraph, ...>.

I don't feel super strongly about it either, but I like your above suggestion of gossip over network. routing::gossip makes sense as a path name to me.

SGTM, let's go with gossip.

codecov-commenter · 2022-06-02T20:43:11Z

Codecov Report

Merging #1159 (c2ed78a) into main (9c50083) will increase coverage by 0.37%.
The diff coverage is 99.07%.

❗ Current head c2ed78a differs from pull request most recent head 574870e. Consider uploading reports for the commit 574870e to get more accurate results

@@            Coverage Diff             @@
##             main    #1159      +/-   ##
==========================================
+ Coverage   90.94%   91.32%   +0.37%     
==========================================
  Files          80       80              
  Lines       43250    45740    +2490     
  Branches    43250    45740    +2490     
==========================================
+ Hits        39335    41772    +2437     
- Misses       3915     3968      +53

Impacted Files	Coverage Δ
lightning-invoice/src/de.rs	`82.38% <ø> (ø)`
lightning-invoice/src/lib.rs	`87.39% <ø> (ø)`
lightning-invoice/src/payment.rs	`92.91% <ø> (ø)`
lightning-invoice/src/utils.rs	`96.77% <ø> (ø)`
lightning-rapid-gossip-sync/src/lib.rs	`90.66% <ø> (ø)`
lightning-rapid-gossip-sync/src/processing.rs	`90.17% <ø> (ø)`
lightning/src/ln/mod.rs	`95.00% <ø> (ø)`
lightning/src/ln/peer_handler.rs	`57.06% <ø> (ø)`
lightning/src/routing/scoring.rs	`97.08% <ø> (ø)`
lightning/src/util/events.rs	`42.62% <ø> (+0.96%)`	⬆️
... and 17 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 9c50083...574870e. Read the comment docs.

jkczyz · 2022-06-02T21:00:03Z

Oops, looks like git mv plus edit didn't remove network_graph.rs.

Edit: Never mind, it did

TheBlueMatt

LGTM. Feel free to squash IMO.

valentinewallace · 2022-06-02T21:55:27Z

lightning/src/routing/network_graph.rs

-	/// An error indicating only that a channel has been closed, which should be applied via
-	/// [`NetworkGraph::close_channel_from_update`].
-	ChannelClosed {
+	/// An error indicating only that a channel has failed, which should be applied via


I think it'd be nice to elaborate on what "failed" means here

Done. Let me know if you'd prefer a different elaboration.

TheBlueMatt · 2022-06-02T22:09:08Z

LGTM (still) feel free to squash.

A NetworkUpdate indicating ChannelClosed actually corresponds to a channel failure as described in BOLT 4: 0x2000 (NODE): node failure (otherwise channel) Rename the enum variant to ChannelFailure and rename NetworkGraph methods close_channel_from_update and fail_node to channel_failed and node_failed, respectively.

NetGraphMsgHandler implements RoutingMessageHandler to handle gossip messages defined in BOLT 7 and maintains a view of the network by updating NetworkGraph. Rename it to P2PGossipSync, which better describes its purpose, and to contrast with RapidGossipSync.

The routing::network_graph module contains a few structs related to p2p gossip. So renaming the module to 'gossip' seems more appropriate.

valentinewallace previously approved these changes Nov 5, 2021

View reviewed changes

jkczyz force-pushed the 2021-11-network-gossip branch from f506f4d to 44f31d6 Compare November 8, 2021 17:15

jkczyz force-pushed the 2021-11-network-gossip branch from 44f31d6 to 1a9ff7e Compare November 8, 2021 21:24

TheBlueMatt dismissed valentinewallace’s stale review via 1a9ff7e December 15, 2021 23:32

jkczyz added this to the 0.0.107 milestone Jun 1, 2022

jkczyz added 2 commits June 2, 2022 13:15

Fix build warnings

8c5ca95

Update missed references to get_route in docs

dcffefa

jkczyz force-pushed the 2021-11-network-gossip branch from 1a9ff7e to c2ed78a Compare June 2, 2022 20:42

TheBlueMatt reviewed Jun 2, 2022

View reviewed changes

valentinewallace reviewed Jun 2, 2022

View reviewed changes

jkczyz force-pushed the 2021-11-network-gossip branch from c2ed78a to 7976512 Compare June 2, 2022 21:58

TheBlueMatt previously approved these changes Jun 2, 2022

View reviewed changes

jkczyz dismissed TheBlueMatt’s stale review via d7a1b2f June 2, 2022 22:06

jkczyz force-pushed the 2021-11-network-gossip branch from 7976512 to d7a1b2f Compare June 2, 2022 22:06

jkczyz added 2 commits June 2, 2022 15:15

Move network_graph.rs to gossip.rs

574870e

The routing::network_graph module contains a few structs related to p2p gossip. So renaming the module to 'gossip' seems more appropriate.

jkczyz force-pushed the 2021-11-network-gossip branch from d7a1b2f to 574870e Compare June 2, 2022 22:15

TheBlueMatt approved these changes Jun 2, 2022

View reviewed changes

valentinewallace approved these changes Jun 2, 2022

View reviewed changes

TheBlueMatt merged commit 0017bc8 into lightningdevkit:main Jun 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename network-related types #1159

Rename network-related types #1159

jkczyz commented Nov 5, 2021 •

edited

Loading

codecov bot commented Nov 5, 2021 •

edited

Loading

valentinewallace left a comment

valentinewallace Nov 5, 2021

jkczyz Nov 8, 2021

jkczyz commented Nov 8, 2021

TheBlueMatt commented Nov 8, 2021

valentinewallace commented Nov 8, 2021

jkczyz commented Nov 8, 2021

TheBlueMatt commented Nov 8, 2021

TheBlueMatt commented Nov 17, 2021

TheBlueMatt commented May 24, 2022

jkczyz commented May 28, 2022

TheBlueMatt commented May 28, 2022

jkczyz commented May 31, 2022

codecov-commenter commented Jun 2, 2022 •

edited

Loading

jkczyz commented Jun 2, 2022 •

edited

Loading

TheBlueMatt left a comment

valentinewallace Jun 2, 2022

jkczyz Jun 2, 2022

TheBlueMatt commented Jun 2, 2022

Rename network-related types #1159

Rename network-related types #1159

Conversation

jkczyz commented Nov 5, 2021 • edited Loading

codecov bot commented Nov 5, 2021 • edited Loading

Codecov Report

valentinewallace left a comment

Choose a reason for hiding this comment

valentinewallace Nov 5, 2021

Choose a reason for hiding this comment

jkczyz Nov 8, 2021

Choose a reason for hiding this comment

jkczyz commented Nov 8, 2021

TheBlueMatt commented Nov 8, 2021

valentinewallace commented Nov 8, 2021

jkczyz commented Nov 8, 2021

TheBlueMatt commented Nov 8, 2021

TheBlueMatt commented Nov 17, 2021

TheBlueMatt commented May 24, 2022

jkczyz commented May 28, 2022

TheBlueMatt commented May 28, 2022

jkczyz commented May 31, 2022

codecov-commenter commented Jun 2, 2022 • edited Loading

Codecov Report

jkczyz commented Jun 2, 2022 • edited Loading

TheBlueMatt left a comment

Choose a reason for hiding this comment

valentinewallace Jun 2, 2022

Choose a reason for hiding this comment

jkczyz Jun 2, 2022

Choose a reason for hiding this comment

TheBlueMatt commented Jun 2, 2022

jkczyz commented Nov 5, 2021 •

edited

Loading

codecov bot commented Nov 5, 2021 •

edited

Loading

codecov-commenter commented Jun 2, 2022 •

edited

Loading

jkczyz commented Jun 2, 2022 •

edited

Loading