Provide Dummy routing and channel message handlers for users #814

TheBlueMatt · 2021-03-01T18:36:33Z

We currently "support" not having a router or channel in memory by
forcing users to implement the handler trait and ignore every
message. Instead, we can just do that ourselves to simplify the API.

lightning/src/ln/peer_handler.rs

TheBlueMatt · 2021-03-01T20:15:44Z

Overhauled to just provide dummy implementations instead.

codecov · 2021-03-01T20:30:46Z

Codecov Report

Merging #814 (f156fe2) into main (9fba7c9) will decrease coverage by 0.33%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #814      +/-   ##
==========================================
- Coverage   91.06%   90.73%   -0.34%     
==========================================
  Files          48       48              
  Lines       25495    25575      +80     
==========================================
- Hits        23218    23205      -13     
- Misses       2277     2370      +93

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`93.15% <ø> (ø)`
lightning/src/ln/peer_handler.rs	`44.82% <0.00%> (-4.90%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.92% <0.00%> (-0.23%)`	⬇️

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 9fba7c9...d95f145. Read the comment docs.

lightning/src/ln/peer_handler.rs

valentinewallace · 2021-03-01T22:18:15Z

lightning/src/ln/peer_handler.rs

 /// Provides references to trait impls which handle different types of messages.
 pub struct MessageHandler<CM: Deref, RM: Deref> where
 		CM::Target: ChannelMessageHandler,
 		RM::Target: RoutingMessageHandler {
 	/// A message handler which handles messages specific to channels. Usually this is just a
-	/// ChannelManager object.
+	/// ChannelManager object or a DummyChannelHandler.


This comment applies to PeerManager::new(..) function:
could we make message_handler an optional parameter to that function, and insert these two dummies if it's None?

You probably never actually want to create a MessageHandler with both set to dummy values...you won't ever do anything. Maybe I misunderstood your comment?

Oh oops true. Revised suggestion: could we make PeerManager::new(..) be parameterized by a CM and an RM separately (and not parameterized by a MessageHandler struct), and have the RM be optional? I feel like that would cover the majority of use cases (which does not include building a routing-only node..)

I actually think both are likely, its totally reasonable to run a thing to fetch the routing graph. Added the constructors.

arik-so

LGTM!

lightning/src/ln/peer_handler.rs

arik-so

ACK changes

valentinewallace

Looks good after my last nits :)

lightning/src/ln/peer_handler.rs

jkczyz · 2021-03-02T01:12:59Z

lightning/src/ln/peer_handler.rs

+	// Any messages which are related to a specific channel generate an error message to let the
+	// peer know we don't care about channels.
+	fn handle_open_channel(&self, their_node_id: &PublicKey, _their_features: InitFeatures, msg: &msgs::OpenChannel) {
+		ErroringMessageHandler::push_error(self, their_node_id, msg.temporary_channel_id);


nit: could write as self.push_error?

No :/. Because of the (somewhat anti-pattern of) impl Deref for X { type Target = X; } which we do on the handlers, trying to access self directly gives you a recursion limit error in rustc.

We currently "support" not having a router or channel in memory by forcing users to implement the same, but its trivial to provide our own dummy implementations.

TheBlueMatt · 2021-03-02T01:24:03Z

Squashed with no diff.

This is largely useful for bindings, and the off-github discussion around lightningdevkit#814 concluded these should be pub, but the PR was not updated to capture this. Now that the bindings support generation for the structs, expose them.

jkczyz · 2021-03-06T03:02:30Z

lightning/src/ln/peer_handler.rs

+impl Deref for IgnoringMessageHandler {
+	type Target = IgnoringMessageHandler;
+	fn deref(&self) -> &Self { self }
+}


Was wondering why Deref was needed but forgot to ask. It's clear to me now, but I'm wondering if MessageHandler really needs to be parameterized by types implementing Derefs as not doing so would avoid this anti-pattern.

Hmm, the Derefs are there because you almost certainly need multiple references to a ChannelManager (or NetworkGraph), eg via an Arc. We originally parametrized things by Derefs with targets to the implementations, but looking back it may have made more sense to implement the traits for Derefs to implementations. We could also implement the traits on Deref for only our specific types.

I see. One thought that just came to mind -- but I haven't spent much time mulling over -- is to have a standard implementation of the trait that calls out to a ChannelManager rather than having ChannelManager implement the trait directly. This wouldn't be much different than the currently implementation which delegates to internal methods, but it could be freely moved.

The advantage being that users could define their own handler if the standard one didn't suffice while still leveraging ChannelManager's interface. Or even reuse the standard implementation but tack on functionality kinda like Read wrappers do. For example, a wrapper could log network messages before delegating to the internal standard implementation.

Then the standard implementation itself could take a Borrow<ChannelManager> to offer flexibility similar to Deref in what sort of pointer it accepts. Or something like this. Not pressing or anything, but having a more composable interface could be valuable.

Hmm, I wonder if it makes sense to swap a bunch of our Deref use for Borrow? Indirection may work, but it'd be nice to try other things and see if they're sufficient before we jump to more indirection IMO.

This is largely useful for bindings, and the off-github discussion around lightningdevkit#814 concluded these should be pub, but the PR was not updated to capture this. Now that the bindings support generation for the structs, expose them.

TheBlueMatt force-pushed the 2021-03-optional-handlers branch from a2691e3 to bfef079 Compare March 1, 2021 18:36

jkczyz reviewed Mar 1, 2021

View reviewed changes

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

TheBlueMatt force-pushed the 2021-03-optional-handlers branch from bfef079 to c70e7c1 Compare March 1, 2021 20:15

TheBlueMatt changed the title ~~Make route_handler and chan_handler Option<>al in MessageHandler~~ Provide Dummy routing and channel message handlers for users Mar 1, 2021

valentinewallace reviewed Mar 1, 2021

View reviewed changes

arik-so approved these changes Mar 1, 2021

View reviewed changes

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

TheBlueMatt force-pushed the 2021-03-optional-handlers branch from 925c7fb to 751cd7e Compare March 1, 2021 23:06

arik-so approved these changes Mar 1, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-03-optional-handlers branch from 751cd7e to f3575d4 Compare March 1, 2021 23:18

valentinewallace reviewed Mar 1, 2021

View reviewed changes

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

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

jkczyz reviewed Mar 2, 2021

View reviewed changes

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

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

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

TheBlueMatt force-pushed the 2021-03-optional-handlers branch from 10f671a to d5b9a08 Compare March 2, 2021 00:41

valentinewallace approved these changes Mar 2, 2021

View reviewed changes

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

jkczyz approved these changes Mar 2, 2021

View reviewed changes

TheBlueMatt added 2 commits March 1, 2021 20:23

Provide Dummy routing and channel message handlers for users

825a238

We currently "support" not having a router or channel in memory by forcing users to implement the same, but its trivial to provide our own dummy implementations.

Add utility constructors to PeerHandler to use a dummy handler

d95f145

TheBlueMatt force-pushed the 2021-03-optional-handlers branch from 43fde8f to d95f145 Compare March 2, 2021 01:24

TheBlueMatt merged commit 81c6bdc into lightningdevkit:main Mar 2, 2021

jkczyz reviewed Mar 6, 2021

View reviewed changes

jkczyz mentioned this pull request Mar 17, 2021

Expose counterparty forwarding info in ChannelDetails #841

Merged

Provide Dummy routing and channel message handlers for users #814

Provide Dummy routing and channel message handlers for users #814

Uh oh!

Conversation

TheBlueMatt commented Mar 1, 2021

Uh oh!

Uh oh!

TheBlueMatt commented Mar 1, 2021

Uh oh!

codecov bot commented Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arik-so left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arik-so left a comment

Choose a reason for hiding this comment

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Mar 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Mar 1, 2021 •

edited

Loading

valentinewallace Mar 1, 2021 •

edited

Loading

TheBlueMatt Mar 1, 2021 •

edited

Loading