Handle query_channel_range gossip queries

[bmancini55]

Finally back in the saddle on this. This PR implements the NetGraphMsgHandler method for processing inbound query_channel_range messages. I'm working on another PR to handle query_short_channel_ids as well.

Notes:

• This implementation enqueues each reply message as an event. Right now, at most this will only generate a few events per query due to the graph size. In the future it could generate a large number of events if the graph is large. I'm considering switching to a polling model like the init_sync code in do_attempt_write_data to be more aware of the write buffer. Wanted opinions before I go down that path though.
• Sequencing metadata in replies (first_blocknum, number_of_blocks) is still necessary to be spec compliant even though its ugly. I added notes on overflow safety. I'm happy to make that more explicit if that's preferable.

[codecov]

Codecov Report

Merging #828 (7b842b6) into main (4894d52) will increase coverage by 1.38%.
The diff coverage is 92.67%.

@@            Coverage Diff             @@
##             main     #828      +/-   ##
==========================================
+ Coverage   90.97%   92.35%   +1.38%     
==========================================
  Files          48       51       +3     
  Lines       26452    32827    +6375     
==========================================
+ Hits        24064    30318    +6254     
- Misses       2388     2509     +121     

Impacted Files	Coverage Δ
lightning/src/ln/peer_handler.rs	44.22% <0.00%> (-0.60%)	⬇️
lightning/src/util/events.rs	23.89% <0.00%> (+0.47%)	⬆️
lightning/src/ln/channel.rs	90.92% <75.00%> (+3.08%)	⬆️
lightning/src/routing/network_graph.rs	91.86% <98.22%> (+0.83%)	⬆️
lightning/src/ln/channelmanager.rs	88.62% <100.00%> (+3.33%)	⬆️
lightning/src/ln/msgs.rs	88.76% <100.00%> (+0.13%)	⬆️
lightning/src/util/scid_utils.rs	100.00% <100.00%> (ø)
lightning/src/ln/onchaintx.rs	86.98% <0.00%> (-7.34%)	⬇️
lightning/src/util/macro_logger.rs	87.09% <0.00%> (-1.24%)	⬇️
background-processor/src/lib.rs	100.00% <0.00%> (ø)
... and 15 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 4894d52...7b842b6. Read the comment docs.

[TheBlueMatt]

Some pretty trivial nits, but overall looks good. Will get a second reviewer this week.

This implementation enqueues each reply message as an event. Right now, at most this will only generate a few events per query due to the graph size. In the future it could generate a large number of events if the graph is large. I'm considering switching to a polling model like the init_sync code in do_attempt_write_data to be more aware of the write buffer. Wanted opinions before I go down that path though.

Hmm, right, ~5 messages for ~32KB is definitely not worth doing that for, but we will need it to respond to ReplyChannelRange messages, which could be a lot of additional messages and we can't block channel messages while sending them. For that we definitely need to pull the InitSyncTracker struct out into network_graph.rs and make it an associated type for the RoutingMessageHandler trait. I leave it up to you if you want to take that complexity here and also apply it to this code or not.

[bmancini55]

Thanks for the review, I updated with nit fixes!

but we will need it to respond to ReplyChannelRange messages, which could be a lot of additional messages and we can't block channel messages while sending them. For that we definitely need to pull the InitSyncTracker struct out into network_graph.rs and make it an associated type for the RoutingMessageHandler trait. I leave it up to you if you want to take that complexity here and also apply it to this code or not.

Do you mean reply to QueryShortChannelIds? If so, was thinking the same thing in regards to InitSyncTracker, thanks for confirming. I'd like to defer that complexity into the PR that addresses QueryShortChannelIds.

[ariard]

Overall sounds good, mostly minor comments. I did exercise the test coverage, can't find holes.

This implementation enqueues each reply message as an event. Right now, at most this will only generate a few events per query due to the graph size. In the future it could generate a large number of events if the graph is large. I'm considering switching to a polling model like the init_sync code in do_attempt_write_data to be more aware of the write buffer. Wanted opinions before I go down that path though.

What are you thinking of more exactly ? I would say the rational for the direction is justified, though can't identify the init_sync code you're mentioning.

[TheBlueMatt]

Do you mean reply to QueryShortChannelIds? If so, was thinking the same thing in regards to InitSyncTracker, thanks for confirming. I'd like to defer that complexity into the PR that addresses QueryShortChannelIds.

Yes, and, ok, sounds good. Can you add a comment noting the amount of memory that can be used in these messages and at what point we should change the logic to avoid it?

[bmancini55]

@ariard thanks for the review! Will clean up those points.

I would say the rational for the direction is justified, though can't identify the init_sync code you're mentioning.

I was refer to PeerHandler::do_attempt_write_data where it uses the InitSyncTracker enum to track state of an initial sync. I was planning to apply this (or similar) to process QueryShortChannelIds in another PR.

[ariard]

Code Review ACK 7b842b6

Thanks for simplifying sequencing of handle_query_channel_range. Reading the spec again, this new way to fulfill first_blocknum/number_of_blocks is compliant, all good.