Making message size limit an exportable constant #623

dr-orlovsky · 2020-05-17T14:04:10Z

It's always better to have all magic numbers defined in specs in form of global constants rather than in-code literals

moneyball · 2020-05-17T14:18:33Z

concept ACK :)

i don't know the code base well enough to give actual ACK although it looks right to me

codecov · 2020-05-17T14:49:47Z

Codecov Report

Merging #623 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master     #623    +/-   ##
========================================
  Coverage   91.26%   91.26%            
========================================
  Files          35       35            
  Lines       21229    20787   -442     
========================================
- Hits        19375    18972   -403     
+ Misses       1854     1815    -39

Impacted Files	Coverage Δ
lightning/src/ln/wire.rs	`66.15% <ø> (ø)`
lightning/src/ln/peer_channel_encryptor.rs	`94.40% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`77.84% <0.00%> (-7.37%)`	⬇️
lightning/src/ln/channelmonitor.rs	`95.52% <0.00%> (-0.19%)`	⬇️
lightning/src/ln/channel.rs	`86.61% <0.00%> (-0.17%)`	⬇️
lightning/src/ln/msgs.rs	`90.03% <0.00%> (ø)`
lightning/src/util/ser.rs	`87.45% <0.00%> (ø)`
lightning/src/util/logger.rs	`59.67% <0.00%> (ø)`
lightning/src/ln/chan_utils.rs	`97.17% <0.00%> (ø)`
lightning/src/chain/transaction.rs	`100.00% <0.00%> (ø)`
... and 12 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 d6d01ba...f186ae8. Read the comment docs.

naumenkogs · 2020-05-17T16:02:57Z

lightning/src/ln/wire.rs

@@ -16,6 +16,13 @@
 use ln::msgs;
 use util::ser::{Readable, Writeable, Writer};

+/// Maximum Lightning message data length according to
+/// [BOLT-8](https://github.com/lightningnetwork/lightning-rfc/blob/v1.0/08-transport.md#lightning-message-specification):
+/// "The maximum size of any Lightning message MUST NOT exceed 65535 bytes.


Not sure the quote adds much value, but it takes readers' attention.
Keeping a number is also not useful, because it's already in the field.

You mean quotation marks or the quoted text?

I mean the text. I think it's enough to say:
" /// Maximum Lightning message data length according to BOLT-8"

I've used spec referencing model from c-lightning, where they directly cite a part of the spec right in the code — and it works really good for code reading

Never read c-lightning, but big files generally want me to close the tab all the time. And this seems like a path to get there. Maybe you are right.

Anyway, I don't have the energy to argue about this, feel free to do whatever :)

The counter argument is that the spec changes, so quoting it can lead to the code comment getting out of sync with the spec as it evolves. It is also mentioned in BOLT 1:

The size of the message is required by the transport layer to fit into a 2-byte unsigned int; therefore, the maximum possible size is 65535 bytes.

My preference would be to reference the spec but leave out the quote.

In general, we tend to never quote the spec - the spec can be confusing, and docs should be self-contained - users should be able to read them and figure out what's going on without having to flip to another reference.

naumenkogs · 2020-05-17T16:05:08Z

lightning/src/ln/peer_channel_encryptor.rs

@@ -373,7 +374,7 @@ impl PeerChannelEncryptor {
 	/// Encrypts the given message, returning the encrypted version
 	/// panics if msg.len() > 65535 or Noise handshake has not finished.


You deliberately leave "65535" in the comments or you just missed it? I actually don't know what is better. Probably you're right and the number works fine for a doc reader.

Deliberately. Constant is unreadable in text, you have to look up for the value; while integer literal allow to check what value the constant must have and understand on which conditions the code will fail.

TheBlueMatt · 2020-05-17T20:34:55Z

lightning/src/ln/wire.rs

@@ -16,6 +16,13 @@
 use ln::msgs;
 use util::ser::{Readable, Writeable, Writer};

+/// Maximum Lightning message data length according to
+/// [BOLT-8](https://github.com/lightningnetwork/lightning-rfc/blob/v1.0/08-transport.md#lightning-message-specification):


I mean, its more because the length is encoded with two bytes than because the spec says so :p.

or the spec uses two byte length encoding to enforce the message size limitation :)

jkczyz · 2020-05-18T17:32:14Z

lightning/src/ln/wire.rs

@@ -16,6 +16,13 @@
 use ln::msgs;
 use util::ser::{Readable, Writeable, Writer};

+/// Maximum Lightning message data length according to
+/// [BOLT-8](https://github.com/lightningnetwork/lightning-rfc/blob/v1.0/08-transport.md#lightning-message-specification):
+/// "The maximum size of any Lightning message MUST NOT exceed 65535 bytes.


The counter argument is that the spec changes, so quoting it can lead to the code comment getting out of sync with the spec as it evolves. It is also mentioned in BOLT 1:

The size of the message is required by the transport layer to fit into a 2-byte unsigned int; therefore, the maximum possible size is 65535 bytes.

My preference would be to reference the spec but leave out the quote.

jkczyz · 2020-05-18T17:56:07Z

lightning/src/ln/wire.rs

+/// "The maximum size of any Lightning message MUST NOT exceed 65535 bytes.
+/// A maximum size of 65535 simplifies testing, makes memory management easier,
+/// and helps mitigate memory-exhaustion attacks."
+pub const LN_MAX_MSG_LEN: usize = 65535;


Given the spec references the transport layer, would it be more appropriate to define this constant there (i.e. where it is used)? Especially given #494 refactors that code but seems to have missed this restriction. It would be easily caught if defined there, IMHO. It may have not been caught in the review since the old code has not (yet) been removed in that PR.

jkczyz · 2020-05-18T17:58:46Z

lightning/src/ln/wire.rs

+/// "The maximum size of any Lightning message MUST NOT exceed 65535 bytes.
+/// A maximum size of 65535 simplifies testing, makes memory management easier,
+/// and helps mitigate memory-exhaustion attacks."
+pub const LN_MAX_MSG_LEN: usize = 65535;


Could we simply use std::u16::MAX given the intention given in BOLT 1?

I agree, it makes more sense to define the constant here. 65535 is more convenient to the reader though, so perhaps just std::u16::MAX; // 65535 for optimal legibility

dr-orlovsky · 2020-07-21T12:15:51Z

With new force-push I have matched all questions, rebased to the current master, added test cases + fixed some of the issues with the existing panic messages and test cases, related to message encryption/decryption. Each of these changes is done as a separate commit to make review easier.

TheBlueMatt · 2020-07-22T03:03:33Z

Thanks!

naumenkogs reviewed May 17, 2020

View reviewed changes

TheBlueMatt reviewed May 17, 2020

View reviewed changes

ariard added the Easy to Review label May 18, 2020

jkczyz self-requested a review May 18, 2020 17:57

jkczyz reviewed May 18, 2020

View reviewed changes

dr-orlovsky force-pushed the feat-msgsizelimit branch from 72d1b37 to f186ae8 Compare July 21, 2020 12:14

dr-orlovsky force-pushed the feat-msgsizelimit branch 2 times, most recently from f10e46a to a4c92dc Compare July 21, 2020 16:10

dr-orlovsky added 5 commits July 21, 2020 18:10

Making message size limit an exportable constant

eb8bcaa

Fixing panic messages for decryption procedures

bcb50a6

Test cases for message encryption/decryption size limits

caed31a

Removing duplicated code in message decryption test cases

eae748a

Removing spec quotation from LN_MAX_MSG_LEN definition

0e5dfad

dr-orlovsky force-pushed the feat-msgsizelimit branch 2 times, most recently from 90d2c3a to fc74130 Compare July 21, 2020 16:32

Moving LN_MAX_MSG_LEN const to the actual use place

4bb5955

dr-orlovsky force-pushed the feat-msgsizelimit branch from fc74130 to 4bb5955 Compare July 21, 2020 16:54

TheBlueMatt merged commit 5a1c0cc into lightningdevkit:master Jul 22, 2020

dr-orlovsky mentioned this pull request Sep 12, 2020

Make upstream PRs accepted & publish v0.1.0 release on crates.io LNP-BP/rust-lnpbp#77

Closed

22 tasks

		@@ -373,7 +374,7 @@ impl PeerChannelEncryptor {
		/// Encrypts the given message, returning the encrypted version
		/// panics if msg.len() > 65535 or Noise handshake has not finished.

Making message size limit an exportable constant #623

Making message size limit an exportable constant #623

Uh oh!

Conversation

dr-orlovsky commented May 17, 2020

Uh oh!

moneyball commented May 17, 2020

Uh oh!

codecov bot commented May 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

dr-orlovsky May 18, 2020 • 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!

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!

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!

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!

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!

dr-orlovsky commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Jul 22, 2020

Uh oh!

Uh oh!

codecov bot commented May 17, 2020 •

edited

Loading

dr-orlovsky May 18, 2020 •

edited

Loading

dr-orlovsky commented Jul 21, 2020 •

edited

Loading