Use the query start block for ReplyChannelRange response messages #961

TheBlueMatt · 2021-06-19T16:04:43Z

C-Lightning versions prior to 0.10 (incorrectly) enforce that the
reply_channel_range first_blocknum field is set to at least the
value they sent in their query_channel_range message. Sending a 0
results in them responding with an Error message, closing open
channels spuriously.

This code is extracted from a previous version of the original
query_channel_range PR in commit
44ba52c. The original commit is by
bmancini55 <[email protected]>.

codecov · 2021-06-21T16:20:45Z

Codecov Report

Merging #961 (59527b4) into main (4d1c1a3) will increase coverage by 1.48%.
The diff coverage is 100.00%.

❗ Current head 59527b4 differs from pull request most recent head 42f6a8f. Consider uploading reports for the commit 42f6a8f to get more accurate results

@@            Coverage Diff             @@
##             main     #961      +/-   ##
==========================================
+ Coverage   90.56%   92.04%   +1.48%     
==========================================
  Files          60       60              
  Lines       30423    38961    +8538     
==========================================
+ Hits        27553    35863    +8310     
- Misses       2870     3098     +228

Impacted Files	Coverage Δ
lightning/src/routing/network_graph.rs	`93.97% <100.00%> (+1.99%)`	⬆️
lightning/src/ln/wire.rs	`60.52% <0.00%> (-3.58%)`	⬇️
lightning/src/util/macro_logger.rs	`87.87% <0.00%> (-1.19%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.73% <0.00%> (-0.05%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`94.79% <0.00%> (-0.03%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (+0.37%)`	⬆️
lightning/src/chain/chainmonitor.rs	`96.21% <0.00%> (+0.41%)`	⬆️
lightning/src/util/ser.rs	`91.70% <0.00%> (+0.43%)`	⬆️
lightning/src/ln/msgs.rs	`88.82% <0.00%> (+0.48%)`	⬆️
lightning/src/util/ser_macros.rs	`97.34% <0.00%> (+0.59%)`	⬆️
... and 16 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 4d1c1a3...42f6a8f. Read the comment docs.

lightning/src/routing/network_graph.rs

TheBlueMatt · 2021-06-22T17:26:56Z

Added a commit to tweak the comments to hopefully address all the feedback.

lightning/src/routing/network_graph.rs

TheBlueMatt · 2021-06-22T21:28:41Z

Sadly it appears this is insufficient for c-lightning - it even checks that each message builds on the previous message (why, I don't fully understand, but...it does).

TheBlueMatt · 2021-06-22T22:40:42Z

Ended up rewriting this against the c-lightning 0.9.3 source - not only do they add requirements on the start height, but they enforce a strictly-increasing start height, which means if you get a query on an empty block boundary c-lightning will barf, even if your messages are otherwise great...

valentinewallace

Just nits and a question, LGTM

lightning/src/routing/network_graph.rs

valentinewallace · 2021-06-30T17:27:03Z

lightning/src/routing/network_graph.rs

+
+					// Enforce exactly the sequencing requirements present on c-lightning v0.9.3
+					assert!(msg.first_blocknum == c_lightning_0_9_prev_end_blocknum || msg.first_blocknum == c_lightning_0_9_prev_end_blocknum.saturating_add(1));
+					assert!(msg.first_blocknum >= max_firstblocknum);


Since we're checking for >=, wouldn't it be min_firstblocknum?

Is it fair to say that max_firstblocknum is checking that the msg's first_blocknum is at least the first_blocknum of the previous msg?

Its the maximum-seen-firstblocknum, which each new message must be greater than.

Is it fair to say that max_firstblocknum is checking that the msg's first_blocknum is at least the first_blocknum of the previous msg?
Yes

jkczyz · 2021-07-01T04:47:19Z

Is it worth adding a workaround for a misbehaving peer? From what I gathered from ElementsProject/lightning@b2ee44b, c-lightning prior to 0.10 misinterpreted older versions of the spec, while c-lightning 0.10 correctly interprets the most recent version of the spec.

TheBlueMatt · 2021-07-01T12:56:27Z

Is it worth adding a workaround for a misbehaving peer?

Yea, I might argue no if the change were on-its-face-bad, but I don't really think it is - we're updating to send info that more accurately matches maybe the intent of the spec, I think, so I wouldn't say this patch is "bad" on its face, if a bit useless. I agree its kinda awkward to work around a bug in old versions of c-lightning, but I did encounter it connecting to the "Blockstream Store" node, which is one of the largest liquidity nodes on the network today.

jkczyz · 2021-07-08T05:00:48Z

lightning/src/routing/network_graph.rs

+			// Per spec, the last end_block needs to be >= the query's `end_blocknum`.
+			// Thus, for the last reply, we calculate the difference between the query's
+			// `end_blocknum` and the start of the reply.


Earlier you use first_blocknum + number_of_blocks for end_blocknum. Given end_blocknum is not used in the spec, should we be consistent and expand it out here? Prior to the change, we were consistently using end_blocknum throughout.

I went for defining the "end block" and then using it as a non-` term.

- // Per spec, the last end_block needs to be >= the query's `end_blocknum`. - // Thus, for the last reply, we calculate the difference between the query's - // `end_blocknum` and the start of the reply. + // Per spec, the last end block (ie `first_blocknum + number_of_blocks`) needs to be + // >= the query's end block. Thus, for the last reply, we calculate the difference + // between the query's end block and the start of the reply. //

lightning/src/routing/network_graph.rs

C-Lightning versions prior to 0.10 (incorrectly) enforce that the reply_channel_range first_blocknum field is set to at least the value they sent in their query_channel_range message. Sending a 0 results in them responding with an Error message, closing open channels spuriously. Further, C-Lightning versions prior to 0.10 require that the reply_channel_range first_blocknum is either the same block implied as the last block of the previous reply_channel_range or one greater. This is not only a creative interpretation of the spec, but a perfectly reasonable implementation might still receive an Error message in the case of replies split by an empty block. This code is extracted and modified from a previous version of the original query_channel_range PR in commit 44ba52c. The original commit is by `bmancini55 <[email protected]>`.

TheBlueMatt · 2021-07-08T14:06:31Z

Will take after CI unless someone complains. Total diff from Val's ACK:

$ git diff-tree -U1 310ef96 42f6a8f1
diff --git a/lightning/src/routing/network_graph.rs b/lightning/src/routing/network_graph.rs
index d513154a..3cbcd912 100644
--- a/lightning/src/routing/network_graph.rs
+++ b/lightning/src/routing/network_graph.rs
@@ -380,3 +380,3 @@ impl<C: Deref , L: Deref > RoutingMessageHandler for NetGraphMsgHandler<C, L> wh
 			//
-			// Additionally, c-lightning versions < 0.10 require that the `first_blocknum of each
+			// Additionally, c-lightning versions < 0.10 require that the `first_blocknum` of each
 			// reply is >= the previous reply's `first_blocknum` and either exactly the previous
@@ -384,4 +384,4 @@ impl<C: Deref , L: Deref > RoutingMessageHandler for NetGraphMsgHandler<C, L> wh
 			// significant diversion from the requirements set by the spec, and, in case of blocks
-			// with no channel opens (eg empty blocks), requires that we use the previous value and
-			// *not* derive the first_blocknum from the actual first block of the reply.
+			// with no channel opens (e.g. empty blocks), requires that we use the previous value
+			// and *not* derive the first_blocknum from the actual first block of the reply.
 			let first_blocknum = prev_batch_endblock;
@@ -392,5 +392,5 @@ impl<C: Deref , L: Deref > RoutingMessageHandler for NetGraphMsgHandler<C, L> wh
 			//
-			// Per spec, the last end_block needs to be >= the query's `end_blocknum`.
-			// Thus, for the last reply, we calculate the difference between the query's
-			// `end_blocknum` and the start of the reply.
+			// Per spec, the last end block (ie `first_blocknum + number_of_blocks`) needs to be
+			// >= the query's end block. Thus, for the last reply, we calculate the difference
+			// between the query's end block and the start of the reply.
 			//
@@ -398,4 +398,4 @@ impl<C: Deref , L: Deref > RoutingMessageHandler for NetGraphMsgHandler<C, L> wh
 			// first_blocknum will be either msg.first_blocknum or a higher block height.
-			let number_of_blocks = if batch_index == batch_count-1 {
-				msg.end_blocknum() - first_blocknum
+			let (sync_complete, number_of_blocks) = if batch_index == batch_count-1 {
+				(true, msg.end_blocknum() - first_blocknum)
 			}
@@ -404,3 +404,3 @@ impl<C: Deref , L: Deref > RoutingMessageHandler for NetGraphMsgHandler<C, L> wh
 			else {
-				block_from_scid(batch.last().unwrap()) - first_blocknum
+				(false, block_from_scid(batch.last().unwrap()) - first_blocknum)
 			};
@@ -409,5 +409,2 @@ impl<C: Deref , L: Deref > RoutingMessageHandler for NetGraphMsgHandler<C, L> wh
 
-			// Only true for the last message in a sequence
-			let sync_complete = batch_index == batch_count - 1;
-
 			pending_events.push(MessageSendEvent::SendReplyChannelRange {
$

TheBlueMatt added this to the 0.0.99 milestone Jun 20, 2021

TheBlueMatt force-pushed the 2021-06-workaround-broken-cln branch from 302322c to 0e590cb Compare June 21, 2021 15:58

TheBlueMatt marked this pull request as ready for review June 21, 2021 15:59

valentinewallace reviewed Jun 21, 2021

View reviewed changes

lightning/src/routing/network_graph.rs Outdated Show resolved Hide resolved

lightning/src/routing/network_graph.rs Outdated Show resolved Hide resolved

lightning/src/routing/network_graph.rs Outdated Show resolved Hide resolved

valentinewallace reviewed Jun 22, 2021

View reviewed changes

lightning/src/routing/network_graph.rs Show resolved Hide resolved

TheBlueMatt force-pushed the 2021-06-workaround-broken-cln branch from ba42f76 to 310ef96 Compare June 22, 2021 22:40

valentinewallace approved these changes Jun 30, 2021

View reviewed changes

jkczyz approved these changes Jul 8, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-06-workaround-broken-cln branch from 59527b4 to 42f6a8f Compare July 8, 2021 14:05

TheBlueMatt merged commit 96a738a into lightningdevkit:main Jul 8, 2021

Use the query start block for ReplyChannelRange response messages #961

Use the query start block for ReplyChannelRange response messages #961

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jun 22, 2021

Uh oh!

Uh oh!

TheBlueMatt commented Jun 22, 2021

Uh oh!

TheBlueMatt commented Jun 22, 2021

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

valentinewallace Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

jkczyz commented Jul 1, 2021

Uh oh!

TheBlueMatt commented Jul 1, 2021

Uh oh!

jkczyz Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jun 19, 2021 •

edited

Loading

codecov bot commented Jun 21, 2021 •

edited

Loading

TheBlueMatt commented Jul 8, 2021 •

edited

Loading