Handle initial_routing_sync requests from peers in their Init messages (issue 148) #202

SWvheerden · 2018-10-02T14:08:31Z

I have made an initial outline for #148.
Although the code currently sits in router, I am thinking of doing it the same way as router and channelmanger is linked to peer_handler.

There is two ways we can do this, one through a new independent trait, which means that peer_handler will have two ref to router.
Or we replace MessageHandler.route_handler with a trait that incorporates RoutingMessagerHandler and NetworkAPI.

ariard · 2018-10-02T23:49:59Z

Which are the reasons to split the 2 APIs in different traits ? Seems to me a Network store which can get you channel/node announcements but you can't update with new msgs from peers isn't gonna be really useful, is it ?

One of the reason could be easing the management of different networks (public/privates) ?

SWvheerden · 2018-10-03T05:41:53Z

This comes back the SOLID principle, as currently the traits are quite clearly only message handling.
I think the better question would be to find out what the reasoning behind making them traits and open to implementation. Do we want the user to be able to chose which way he wants to handle the routing, network calls, both or only a single one?

As to your question for our implementation using the router struct, we should ensure both pointers point to the same object which seems kinda stupid.

The only advantage of splitting the message handling and network api traits is if for some reason you as a user might want to add some kind of middleware between one or both. But I cant think of a reason why you would want to do this.

SWvheerden · 2018-10-03T10:09:18Z

I believe this is much cleaner code. I combined the two traits, and it now requires both from the same object. I have kept the naming in place for now, but I dont think they will be completely correct if we follow this layout.

SWvheerden · 2018-10-05T09:42:47Z

So I have played around with the idea of caching messages, as we need them to be signed. I dont think we can sign them as we need secret keys for that.

src/ln/msgs.rs

TheBlueMatt · 2018-10-15T22:03:20Z

Dunno why email response got dropped...

Hmm, I think you missed my point about the pagination - if, for each peer currently getting a channel dump, you keep only the short_channel_id of the highest channel that we have told it about, then you keep it's network buffer full with updates by querying the Router for the next N updates starting at I'd X (and you can ensure they don't miss anything by relaying normally updates below X).

In the current model you query, get a full list of updates and shove them into the Peet's network buffer all at once. Even if you re-query for the full set of channels each time more room is available in the network buffer (which would be expensive in its own) and send starting from some index you remember, you still end up with race conditions where you will either re-relay information about channels to them or miss relaying information to them because of updates while you were sending them the full set.

src/ln/msgs.rs

src/ln/peer_handler.rs

src/ln/router.rs

TheBlueMatt · 2018-10-25T19:29:21Z

src/ln/msgs.rs

@@ -591,6 +592,8 @@ pub trait RoutingMessageHandler : Send + Sync {
 	fn handle_channel_update(&self, msg: &ChannelUpdate) -> Result<bool, HandleError>;
 	/// Handle some updates to the route graph that we learned due to an outbound failed payment.
 	fn handle_htlc_fail_channel_update(&self, update: &HTLCFailChannelUpdate);
+	///This returns a vec of vec's, one for ChannelAnnouncements, one for ChannelUpdates and one for NodeAnnouncements. It also returns the id of the last channel passed through


It doesn't return a vec of vecs? Also this doc doesn't describe what the function is for.

TheBlueMatt · 2018-10-25T19:31:56Z

src/ln/peer_handler.rs

+															//we wait to see if the buffer clears before we retrieve more message to send
+															//we push up to 15 message out, 5 remaining means they have cleared 2/3rds
+															while peer.pending_outbound_buffer.len()>5{
+																thread::sleep(time::Duration::from_millis(50));


We shouldn't sleep here, we should instead mark where we are in the peer and add more messages to the outbound queue when we get a write_event() from the user.

SWvheerden · 2018-10-29T15:35:21Z

Okay, so I created another macro that firs checks the messages then passes it on towards the encode and send macro

src/ln/peer_handler.rs

src/ln/msgs.rs

SWvheerden · 2018-10-30T09:07:04Z

Okay, I think I added this to the correct location

TheBlueMatt

Yea, I think this is the right approach, nice. Doesn't look like it should work (looks panic-y to me), but I didn't test it yet. Ideally this would come with a test, but I dont think we currently have any tests for PeerHandler, so I won't suggest its blocking, but it would be really nice to have something that tests PeerHandler if its gonna grow any more complicated.

src/ln/msgs.rs

src/ln/peer_handler.rs

src/ln/msgs.rs

src/ln/peer_handler.rs

src/ln/router.rs

src/util/test_utils.rs

SWvheerden · 2018-10-31T11:07:01Z

I agree on the test thing, but we need to create tests to test the message interaction first (simulate the network with announcement and update messages)

Otherwise its going to be a very fake kinda useless test, as we need to test the routing with this.

src/ln/peer_handler.rs

src/ln/msgs.rs

src/ln/peer_handler.rs

TheBlueMatt · 2018-11-09T02:31:57Z

Will take as #247 the patch from this PR to that is TheBlueMatt@b46c3cc.

TheBlueMatt reviewed Oct 6, 2018

View reviewed changes

src/ln/msgs.rs Outdated Show resolved Hide resolved

src/ln/msgs.rs Outdated Show resolved Hide resolved

src/ln/msgs.rs Outdated Show resolved Hide resolved

SWvheerden force-pushed the Handle-initial_routing_sync-requests-from-peers-in-their-Init-messages branch 2 times, most recently from f0672a4 to cc88399 Compare October 17, 2018 11:43

TheBlueMatt reviewed Oct 19, 2018

View reviewed changes

SWvheerden force-pushed the Handle-initial_routing_sync-requests-from-peers-in-their-Init-messages branch 3 times, most recently from f6ef81d to 1c7e142 Compare October 25, 2018 05:23

SWvheerden changed the title ~~RFC for initial outline for issue 148~~ Handle initial_routing_sync requests from peers in their Init messages (issue 148) Oct 25, 2018

TheBlueMatt reviewed Oct 25, 2018

View reviewed changes

SWvheerden force-pushed the Handle-initial_routing_sync-requests-from-peers-in-their-Init-messages branch from 1c7e142 to adb39ae Compare October 29, 2018 15:32

TheBlueMatt reviewed Oct 29, 2018

View reviewed changes

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

src/ln/msgs.rs Outdated Show resolved Hide resolved

SWvheerden force-pushed the Handle-initial_routing_sync-requests-from-peers-in-their-Init-messages branch from f212c5e to 4577cae Compare October 30, 2018 09:06

TheBlueMatt reviewed Oct 31, 2018

View reviewed changes

SWvheerden force-pushed the Handle-initial_routing_sync-requests-from-peers-in-their-Init-messages branch 3 times, most recently from 244ad7e to 00f3c88 Compare November 1, 2018 13:05

SWvheerden mentioned this pull request Nov 1, 2018

Check Bolt 9 specs #236

Open

4 tasks

TheBlueMatt reviewed Nov 1, 2018

View reviewed changes

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

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

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

src/ln/msgs.rs Outdated Show resolved Hide resolved

src/ln/peer_handler.rs Show resolved Hide resolved

SWvheerden force-pushed the Handle-initial_routing_sync-requests-from-peers-in-their-Init-messages branch 3 times, most recently from 40e8774 to 0a2aed9 Compare November 5, 2018 12:44

TheBlueMatt reviewed Nov 5, 2018

View reviewed changes

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

SWvheerden force-pushed the Handle-initial_routing_sync-requests-from-peers-in-their-Init-messages branch from 82926a3 to 44e5203 Compare November 6, 2018 05:18

Handle-initial_routing_sync-requests-from-peers-in-their-Init-messages

fda1096

SWvheerden force-pushed the Handle-initial_routing_sync-requests-from-peers-in-their-Init-messages branch from 1716ddb to fda1096 Compare November 6, 2018 05:38

TheBlueMatt mentioned this pull request Nov 9, 2018

Handle-initial_routing_sync-requests-from-peers-in-their-Init-messages #247

Merged

TheBlueMatt closed this Nov 9, 2018

TheBlueMatt mentioned this pull request Nov 23, 2018

Handle initial_routing_sync requests from peers in their Init messages #148

Closed

Handle initial_routing_sync requests from peers in their Init messages (issue 148) #202

Handle initial_routing_sync requests from peers in their Init messages (issue 148) #202

Uh oh!

Conversation

SWvheerden commented Oct 2, 2018

Uh oh!

ariard commented Oct 2, 2018

Uh oh!

SWvheerden commented Oct 3, 2018

Uh oh!

SWvheerden commented Oct 3, 2018

Uh oh!

SWvheerden commented Oct 5, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Oct 15, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

SWvheerden commented Oct 29, 2018

Uh oh!

Uh oh!

Uh oh!

SWvheerden commented Oct 30, 2018

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SWvheerden commented Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Nov 9, 2018

Uh oh!

Uh oh!

SWvheerden commented Oct 31, 2018 •

edited

Loading