Adding gossip_queries messages and serializations #684

bmancini55 · 2020-09-07T21:28:45Z

WIP for #615. This currently includes the first of several commits for adding gossip_queries.

The first commit adds structs for gossip_queries. I was playing around with a more robust de/ser implementation for encoded_short_ids, but figured I would just implement a straightforward version as a first pass.

Currently have the following in-progress:

adding new types to the wire module and stubbing peer_handler for handling inbound messages of for the new types
adding gossip_queries to features

I'm thinking I will create a new GossipQueriesMessageHandler to be used by the PeerManager for managing the querying process. I'm still working out the details on this part though.

If there's anything glaringly off about this first commit please let me know! Thanks!

codecov · 2020-09-07T21:56:23Z

Codecov Report

Merging #684 into master will decrease coverage by 16.58%.
The diff coverage is 89.00%.

@@             Coverage Diff             @@
##           master     #684       +/-   ##
===========================================
- Coverage   91.38%   74.79%   -16.59%     
===========================================
  Files          35       55       +20     
  Lines       21724    24668     +2944     
===========================================
- Hits        19852    18451     -1401     
- Misses       1872     6217     +4345

Impacted Files	Coverage Δ
lightning/src/ln/msgs.rs	`86.96% <89.00%> (-3.36%)`	⬇️
lightning/src/util/errors.rs	`25.58% <0.00%> (-55.82%)`	⬇️
lightning/src/util/byte_utils.rs	`57.69% <0.00%> (-42.31%)`	⬇️
lightning/src/util/macro_logger.rs	`70.96% <0.00%> (-18.32%)`	⬇️
lightning/src/util/ser_macros.rs	`78.65% <0.00%> (-17.95%)`	⬇️
lightning/src/util/config.rs	`52.38% <0.00%> (-16.67%)`	⬇️
lightning/src/util/chacha20.rs	`79.51% <0.00%> (-11.88%)`	⬇️
lightning/src/ln/features.rs	`87.26% <0.00%> (-11.46%)`	⬇️
lightning/src/util/logger.rs	`48.33% <0.00%> (-11.35%)`	⬇️
lightning/src/routing/router.rs	`87.79% <0.00%> (-8.89%)`	⬇️
... and 45 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 3defcc8...10e818a. Read the comment docs.

ariard

Overall pretty clean, only minors. What's your approach do you prefer keeping building on this PR or land already messages/serializations ?

I'm thinking I will create a new GossipQueriesMessageHandler to be used by the PeerManager for managing the querying process. I'm still working out the details on this part though.

Right, as a new struct or as new trait implemented by default on the current NetGraphMsgHandler ? I would favor the second one as it's where NetworkGraph is currently living.

And welcome on the repo :)

ariard · 2020-09-08T21:59:19Z

lightning/src/ln/msgs.rs

+pub struct QueryChannelRange {
+	/// The genesis hash of the blockchain being queried
+	pub chain_hash: BlockHash,
+	/// The height of the first block to be queried


For gossip newcomers, precise we don't fetch a block but channel utxos included in those one.

Updated to clarify that first_blocknum is for the channel utxos being queried

ariard · 2020-09-08T22:02:50Z

lightning/src/ln/msgs.rs

+	pub first_blocknum: u32,
+	/// The number of blocks included in for the range of the reply
+	pub number_of_blocks: u32,
+	/// Indicates if the query was able to be served


I think we should document that QueryChannelRange receiver just have a best-effort requirement to send back network view. So we could document it's full local information, not perfect information of network history.

I the comment on ReplyChannelRange to indicate the recipient replies with a best effort based on their local network view. I also updated full_information with text from the spec on whether the recipient maintains up-to-date information the chain_hash`. Happy to change this around if you think it can be clarified better.

lightning/src/ln/msgs.rs

bmancini55 · 2020-09-09T22:55:43Z

Thanks for taking a look!

What's your approach do you prefer keeping building on this PR or land already messages/serializations ?

I'll clean this up and mark this part ready since its fairly isolated and I don't think there will be much iteration on it. I can open another WIP PR for the remainder of the work then.

Right, as a new struct or as new trait implemented by default on the current NetGraphMsgHandler ? I would favor the second one as it's where NetworkGraph is currently living.

I was thinking a new trait. NetGraphMsgHandler makes sense to me as well. I was thinking about using the MessageSendEvent pipeline to wire up handling replies (eg: ReplyQueryRange SCIDs get sent in m number of QueryShortChannelIds messages). I haven't had a chance to dig into how that all works yet other than reviewing process_events in PeerManager. So hopefully I'm not totally off base there.

And welcome on the repo :)

Thanks! Glad to be able to assist!

TheBlueMatt · 2020-09-10T15:59:47Z

This needs new fuzzers for the new messages - add them to fuzz/src/msg_targets/gen_target.sh and run it to auto-generate new fuzzers.

bmancini55 · 2020-09-11T00:10:16Z

I'm not sure why code coverage is not picking up the message structs. I would have thought the tests in ln::msgs::tests would have them covered. Any ideas?

ariard

Just left few more minors.

I was thinking a new trait. NetGraphMsgHandler makes sense to me as well. I was thinking about using the MessageSendEvent pipeline to wire up handling replies (eg: ReplyQueryRange SCIDs get sent in m number of QueryShortChannelIds messages). I haven't had a chance to dig into how that all works yet other than reviewing process_events in PeerManager. So hopefully I'm not totally off base there.

Sounds good to me, MessageSendEvent can be used to reply from local node to requesting peers. Feel free to open a draft PR as soon as you want more concrete feedback.

ariard · 2020-09-11T15:11:09Z

lightning/src/ln/msgs.rs

+		}
+
+		let encoding_type: u8 = Readable::read(r)?;
+		if encoding_type != EncodingType::Uncompressed as u8 {


Maybe comment we expect 1-byte length short_channel_id. Also that queries/replies must be uncompressed.

Do you mean 8-byte length short_channel_id?

Ah sorry, you're right !

No worries! Just wanted to make sure I wasn't misunderstanding how things worked haha

Added comments about encoding_len, encoding_type, and reading the short_channel_ids

ariard · 2020-09-11T15:12:03Z

lightning/src/ln/msgs.rs

+/// Multiple reply_channel_range messages can be sent in reply to a single
+/// query_channel_range message. The query recipient makes a best effort
+/// to respond based on their local network view which may not be
+/// a perfect view of the network.


Mark we don't support zlib serialization.

Added comment about zlib

lightning/src/ln/msgs.rs

ariard · 2020-09-11T15:14:33Z

lightning/src/ln/msgs.rs

+}
+
+/// Encoding type for data compression of collections in gossip queries
+enum EncodingType {


Maybe mark explicitly somewhere we don't support spec encoding-type of 1 for zlib serialization.

Added comment about not supporting zlib

ariard · 2020-09-11T15:16:52Z

lightning/src/ln/msgs.rs

+	pub number_of_blocks: u32,
+}
+
+/// A reply_channel_range message that can be sent or received from a peer.


Every p2p messages can be sent or received, that doesn't add that much information. I realized we do this for every other message, you can drop it IMO.

ariard · 2020-09-11T15:18:02Z

lightning/src/ln/msgs.rs

@@ -570,6 +570,77 @@ pub struct ChannelUpdate {
 	pub contents: UnsignedChannelUpdate,
 }

+/// A query_channel_range message that can be sent or received from a peer


You can explain shortly purpose of this message, what response you aim to provoke sending it to your peer to fulfill which need.

I updated comments for remove the old standard message and added general description of what the message does, what the reply should be, and any notes on serialization limitations. Please let me know if it's too verbose now or if there is anything that is unclear, thanks!

ariard · 2020-09-11T15:30:17Z

I'm not sure why code coverage is not picking up the message structs. I would have thought the tests in ln::msgs::tests would have them covered. Any ideas?

Doesn't mind for now, AFAICT codecov isn't perfect.

ariard · 2020-09-14T00:03:57Z

Code Review ACK 9db02ef

Thanks for the extended comments, they do provide value as the BOLT7 gossip specs is quite blurred.

TheBlueMatt

Looks great, thanks. Four trivial one-line changes to avoid needless allocation/nits, but happy to take after those.

TheBlueMatt · 2020-09-14T01:18:40Z

lightning/src/ln/msgs.rs

+
+		// Read short_channel_ids (8-bytes each), for the encoding_len
+		// less the 1-byte encoding_type
+		let mut short_channel_ids = vec![];


This needs a Vec::with_capacity(cmp::max(ln::peer_channel_encryptor::LN_MAX_MSG_LEN / 8, (encoding_len - 1)/8))

Ah good call on allocations!

Do you think we should move the upper-bound range check up to where we validate encoding_length? I'm thinking that if we get an encoding_length that causes an LN_MAX_MSG_LEN overflow we're receiving a malformed message and should reject it outright instead of limiting the allocation length.

That'd be totally fine too, yea.

encoding_len is a u16 so the range check or limiting of allocation against LN_MAX_MSG_LEN seems superfluous. You ok with just doing: Vec::with_capacity(((encoding_len - 1) / 8) as usize);?

Oh! Right indeed. Sure, that works too, maybe with a comment noting its a u16.

TheBlueMatt · 2020-09-14T01:18:58Z

lightning/src/ln/msgs.rs

+		// less the 1-byte encoding_type
+		let mut short_channel_ids = vec![];
+		let mut read_remaining = encoding_len - 1;
+		while read_remaining > 0 {


Why not for _ in 0..(encoding_len - 1)/8?

Yes! I'm still getting used to replacing c-style for loops with for in range haha

TheBlueMatt · 2020-09-14T01:21:11Z

lightning/src/ln/msgs.rs

+
+		// Read short_channel_ids (8-bytes each), for the encoding_len
+		// less the 1-byte encoding_type
+		let mut short_channel_ids = vec![];


Same two comments as above here and two lines down.

This adds the message structs and implements Readable and Writeable traits for the standard gossip_queries messages.

This commit adds ser/deser fuzzers for five new structs in ln::msgs used for gossip_queries.

Small tweaks post #633/#684

bmancini55 changed the title ~~Adding support for gossip_queries~~ WIP: Adding support for gossip_queries Sep 7, 2020

ariard reviewed Sep 8, 2020

View reviewed changes

bmancini55 force-pushed the gossip_queries branch from c4d771e to 9b88d5f Compare September 10, 2020 15:28

bmancini55 force-pushed the gossip_queries branch 2 times, most recently from d2ca2d0 to a3973d4 Compare September 10, 2020 23:23

bmancini55 changed the title ~~WIP: Adding support for gossip_queries~~ Adding gossip_queries messages and serializations Sep 11, 2020

bmancini55 marked this pull request as ready for review September 11, 2020 00:10

ariard reviewed Sep 11, 2020

View reviewed changes

bmancini55 force-pushed the gossip_queries branch from a3973d4 to 9db02ef Compare September 13, 2020 16:35

TheBlueMatt reviewed Sep 14, 2020

View reviewed changes

bmancini55 added 2 commits September 14, 2020 16:19

Adding gossip_queries message structs and serialization

5ba4560

This adds the message structs and implements Readable and Writeable traits for the standard gossip_queries messages.

Adding fuzzers for gossip_queries messages

10e818a

This commit adds ser/deser fuzzers for five new structs in ln::msgs used for gossip_queries.

bmancini55 force-pushed the gossip_queries branch from 9db02ef to 10e818a Compare September 14, 2020 20:20

TheBlueMatt approved these changes Sep 14, 2020

View reviewed changes

TheBlueMatt merged commit 343aacc into lightningdevkit:master Sep 14, 2020

bmancini55 deleted the gossip_queries branch September 14, 2020 20:54

TheBlueMatt added a commit that referenced this pull request Sep 16, 2020

Merge pull request #690 from TheBlueMatt/2020-09-633-bindings

d735085

Small tweaks post #633/#684

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding gossip_queries messages and serializations #684

Adding gossip_queries messages and serializations #684

bmancini55 commented Sep 7, 2020

codecov bot commented Sep 7, 2020 •

edited

Loading

ariard left a comment

ariard Sep 8, 2020

bmancini55 Sep 10, 2020

ariard Sep 8, 2020

bmancini55 Sep 10, 2020

bmancini55 commented Sep 9, 2020

TheBlueMatt commented Sep 10, 2020

bmancini55 commented Sep 11, 2020

ariard left a comment

ariard Sep 11, 2020

bmancini55 Sep 11, 2020

ariard Sep 12, 2020

bmancini55 Sep 12, 2020

bmancini55 Sep 13, 2020

ariard Sep 11, 2020

bmancini55 Sep 13, 2020

ariard Sep 11, 2020

bmancini55 Sep 13, 2020

ariard Sep 11, 2020

ariard Sep 11, 2020

bmancini55 Sep 13, 2020

ariard commented Sep 11, 2020

ariard commented Sep 14, 2020

TheBlueMatt left a comment

TheBlueMatt Sep 14, 2020

bmancini55 Sep 14, 2020

TheBlueMatt Sep 14, 2020

bmancini55 Sep 14, 2020

TheBlueMatt Sep 14, 2020

TheBlueMatt Sep 14, 2020

bmancini55 Sep 14, 2020

TheBlueMatt Sep 14, 2020

Adding gossip_queries messages and serializations #684

Adding gossip_queries messages and serializations #684

Conversation

bmancini55 commented Sep 7, 2020

codecov bot commented Sep 7, 2020 • edited Loading

Codecov Report

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmancini55 commented Sep 9, 2020

TheBlueMatt commented Sep 10, 2020

bmancini55 commented Sep 11, 2020

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented Sep 11, 2020

ariard commented Sep 14, 2020

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 7, 2020 •

edited

Loading