Trivial Serialization Tweaks #1956

TheBlueMatt · 2023-01-15T23:52:58Z

These are the serialization prereqs for #1897.

An enum implements de/serialization via `impl_writeable_tlv_based_enum_upgradable` currently cannot contain an object that only implements `MaybeReadable`. This solves that by implementing the required blocks for `ignorable`, opting to return `Ok(None)` in the top-level read in cases where the inner field read returns `Ok(None)`.

...to make it easier to add new implementations and implement it for all tuples which implement `Readabe` + `Writeable`. Note that we don't want to just convert to a blanket implementation as we'd really like to keep our optimized `Vec<u8>` wrapper or we'll end up spinning way too much when writing vecs of bytes.

codecov-commenter · 2023-01-16T00:21:04Z

Codecov Report

Base: 90.72% // Head: 90.75% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (06e87c9) compared to base (ce6bcf6).
Patch coverage: 78.33% of modified lines in pull request are covered.

❗ Current head 06e87c9 differs from pull request most recent head 6270a8f. Consider uploading reports for the commit 6270a8f to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1956      +/-   ##
==========================================
+ Coverage   90.72%   90.75%   +0.03%     
==========================================
  Files          97       97              
  Lines       50579    51356     +777     
  Branches    50579    51356     +777     
==========================================
+ Hits        45888    46610     +722     
- Misses       4691     4746      +55

Impacted Files	Coverage Δ
lightning/src/util/ser_macros.rs	`87.03% <ø> (ø)`
lightning/src/util/ser.rs	`91.71% <78.33%> (-0.14%)`	⬇️
lightning/src/chain/onchaintx.rs	`95.18% <0.00%> (-0.21%)`	⬇️
lightning/src/chain/channelmonitor.rs	`90.93% <0.00%> (-0.15%)`	⬇️
lightning/src/ln/channelmanager.rs	`87.11% <0.00%> (-0.02%)`	⬇️
lightning/src/ln/priv_short_conf_tests.rs	`96.53% <0.00%> (-0.01%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.09% <0.00%> (+0.06%)`	⬆️
lightning/src/ln/peer_handler.rs	`56.67% <0.00%> (+0.85%)`	⬆️
lightning/src/ln/reload_tests.rs	`96.48% <0.00%> (+0.89%)`	⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

lightning/src/util/ser.rs

valentinewallace

ACK after Arik's comment

valentinewallace · 2023-01-16T22:39:23Z

lightning/src/util/ser_macros.rs

@@ -581,6 +587,9 @@ macro_rules! _init_tlv_based_struct_field {
 	($field: ident, option) => {
 		$field
 	};
+	($field: ident, ignorable) => {
+		if $field.is_none() { return Ok(None); } else { $field.unwrap() }


No test coverage here, I assume it's covered in follow-up

Yea, I can drop it if you prefer but it is used in #1897

jkczyz · 2023-01-17T17:11:15Z

lightning/src/util/ser.rs

+/// more than 65K entries, we need to be able to store larger values. Thus, we define a variable
+/// length integer here which is backwards-compatible but treats 0xffff as "read eight more bytes".
+struct U16Or64(pub u64);
+impl Writeable for U16Or64 {


This kinda looks like U160r64. Would prefer to name this either CollectionLength or at least U16OrU64 which is a little easier to read.

Strongly agree with Jeff. I only ever read it as 160r64 and didn't realize there was an Or in there until Jeff's comment. I wasn't sure what either 160 or r meant, but didn't even think to ask. The O of Or remains rather unfortunate.

lol I had to Google it before realizing it was not a zero. Not sure it's worth putting the implementation details into the name, tbh.

Heh, alright, well y'all need to get better terminal fonts, but in the mean time I'll rename it :p

jkczyz · 2023-01-17T17:23:32Z

lightning/src/util/ser.rs

@@ -694,18 +723,23 @@ macro_rules! impl_for_vec {
 impl Writeable for Vec<u8> {
 	#[inline]
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
-		(self.len() as u16).write(w)?;
+		U16Or64(self.len() as u64).write(w)?;


Should we be concerned with writing an incompatible length for an LN message? Or are we leaving that to the caller to enforce / assume will never happen?

Yea, I mean we could add some assertion, but its currently up to the caller anyway (we as), so it remains up to the caller. If there's something with more than 2^16 elements it won't fit in the wire 2^16-byte messages anyway, though.

lightning/src/util/ser.rs

valentinewallace · 2023-01-17T18:43:40Z

lightning/src/util/ser.rs

+			0xffffu16.write(writer)?;
+			(self.0 - 0xffff).write(writer)


It's unlikely, but I don't really see how this is backwards compatible if we write more than 65k entries, seems like we could still fail to deserialize on previous versions. Maybe there's no specific map this is possible for, though.

valentinewallace · 2023-01-17T18:44:28Z

lightning/src/util/ser.rs

+	#[inline]
+	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+		let mut val: u64 = <u16 as Readable>::read(r)? as u64;
+		if val == 0xffff {


To be clear, if a previous version happened to write 0xffff exactly, we could fail to deserialize now?

TheBlueMatt · 2023-01-17T19:58:12Z

Regarding the 64k-entries stuff - yes, this is technically not backwards compatible for exactly 0xffff entries, and very not backwards compatible for anything more. So, in general, we should continue to not let things grow beyond 0xffff entries. The change here is more belt-and-suspenders - we shouldn't, but if we do, its better to not read corrupt garbage than to fail. In some edge cases I assume this is already reachable - 64k pending HTLCs is only a few hundred channels, assuming everything happens all at once.

lightning/src/util/ser.rs

valentinewallace · 2023-01-17T20:39:54Z

LGTM after squash

jkczyz · 2023-01-17T21:03:27Z

lightning/src/util/ser.rs

+/// The lightning protocol uses u16s for lengths in most cases. As our serialization framework
+/// primarily targets that, we must as well. However, because we may serialize objects that have
+/// more than 65K entries, we need to be able to store larger values. Thus, we define a variable
+/// length integer here which is backwards-compatible but treats 0xffff as "read eight more bytes".


s/which is backwards-compatible/that is backwards-compatible for < 0xffff

Note the last part that says it's backwards compatible only for < 0xffff.

I think if somebody is at exactly 0xffff, they're about to run into problems anyway.

TheBlueMatt · 2023-01-17T21:19:01Z

Squashed, diff since the initial PR:

$ git diff-tree -U1 abbd452fae0956400c0512ea87b15b4fc3e44ef1 d0c8b14c
diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs
index 8d4ab9f2f..ff5e7c923 100644
--- a/lightning/src/util/ser.rs
+++ b/lightning/src/util/ser.rs
@@ -388,5 +388,9 @@ impl Readable for BigSize {
 /// more than 65K entries, we need to be able to store larger values. Thus, we define a variable
-/// length integer here which is backwards-compatible but treats 0xffff as "read eight more bytes".
-struct U16Or64(pub u64);
-impl Writeable for U16Or64 {
+/// length integer here that is backwards-compatible but treats 0xffff as "read eight more bytes".
+///
+/// To ensure we only have one valid encoding per value, we add 0xffff to values written as eight
+/// bytes. Thus, 0xfffe is serialized as 0xfffe, whereas 0xffff is serialized as
+/// 0xffff0000000000000000 (i.e. read-eight-bytes then zero).
+struct U16OrU64(pub u64);
+impl Writeable for U16OrU64 {
 	#[inline]
@@ -397,3 +401,3 @@ impl Writeable for U16Or64 {
 			0xffffu16.write(writer)?;
-			(self.0 - 0xfffe).write(writer)
+			(self.0 - 0xffff).write(writer)
 		}
@@ -402,3 +406,3 @@ impl Writeable for U16Or64 {
 
-impl Readable for U16Or64 {
+impl Readable for U16OrU64 {
 	#[inline]
@@ -408,5 +412,5 @@ impl Readable for U16Or64 {
 			val = <u64 as Readable>::read(r)?
-				.checked_add(0xfffe).ok_or(DecodeError::InvalidValue)?;
+				.checked_add(0xffff).ok_or(DecodeError::InvalidValue)?;
 		}
-		Ok(U16Or64(val))
+		Ok(U16OrU64(val))
 	}
@@ -628,3 +632,3 @@ macro_rules! impl_for_map {
 			fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
-				U16Or64(self.len() as u64).write(w)?;
+				U16OrU64(self.len() as u64).write(w)?;
 				for (key, value) in self.iter() {
@@ -642,3 +646,3 @@ macro_rules! impl_for_map {
 			fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
-				let len: U16Or64 = Readable::read(r)?;
+				let len: U16OrU64 = Readable::read(r)?;
 				let mut ret = $constr(len.0 as usize);
@@ -668,3 +672,3 @@ where T: Writeable + Eq + Hash
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
-		U16Or64(self.len() as u64).write(w)?;
+		U16OrU64(self.len() as u64).write(w)?;
 		for item in self.iter() {
@@ -681,3 +685,3 @@ where T: Readable + Eq + Hash
 	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
-		let len: U16Or64 = Readable::read(r)?;
+		let len: U16OrU64 = Readable::read(r)?;
 		let mut ret = HashSet::with_capacity(cmp::min(len.0 as usize, MAX_BUF_SIZE / core::mem::size_of::<T>()));
@@ -698,3 +702,3 @@ macro_rules! impl_for_vec {
 			fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
-				U16Or64(self.len() as u64).write(w)?;
+				U16OrU64(self.len() as u64).write(w)?;
 				for elem in self.iter() {
@@ -709,3 +713,3 @@ macro_rules! impl_for_vec {
 			fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
-				let len: U16Or64 = Readable::read(r)?;
+				let len: U16OrU64 = Readable::read(r)?;
 				let mut ret = Vec::with_capacity(cmp::min(len.0 as usize, MAX_BUF_SIZE / core::mem::size_of::<$ty>()));
@@ -725,3 +729,3 @@ impl Writeable for Vec<u8> {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
-		U16Or64(self.len() as u64).write(w)?;
+		U16OrU64(self.len() as u64).write(w)?;
 		w.write_all(&self)
@@ -733,3 +737,3 @@ impl Readable for Vec<u8> {
 	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
-		let mut len: U16Or64 = Readable::read(r)?;
+		let mut len: U16OrU64 = Readable::read(r)?;
 		let mut ret = Vec::new();
@@ -1076,3 +1080,3 @@ impl Writeable for String {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
-		U16Or64(self.len() as u64).write(w)?;
+		U16OrU64(self.len() as u64).write(w)?;
 		w.write_all(self.as_bytes())

TheBlueMatt · 2023-01-17T21:22:36Z

Actually, pushed another rename to align with Jeff/Arik's push above.

arik-so

I blame Github's and my diff tool's fonts.

The lightning protocol uses u16s for lengths in most cases. As our serialization framework primarily targets that, we must as well. However, because we may serialize objects that have more than 65K entries, we want to be able to store larger values. Thus, we define a variable length integer here which is backwards-compatible but treats 0xffff as "read eight more bytes". This doesn't address any specific known issue, but feels like good practice just in case.

TheBlueMatt · 2023-01-17T21:48:30Z

Squashed with no further changes.

jkczyz · 2023-01-17T21:55:11Z

lightning/src/util/ser.rs

+			val = <u64 as Readable>::read(r)?
+				.checked_add(0xffff).ok_or(DecodeError::InvalidValue)?;


Oh, wait, can we forgo the addition here and the subtraction in the write by simply ignoring the u16 on read (when val == 0xffff) and writing the full u64 after 0xffff in the write (when val >= 0xffff)?

We can, yes, but then we have multiple ways to represent the same integers, which is both awkward, and makes the fuzzers angry, which would be a bit of work to fix and would reduce coverage on the gossip messages we need to round-trip exactly. I don't think the performance penalty here matters, we should really almost never be hitting the 8-byte cases.

TheBlueMatt added 3 commits January 15, 2023 20:28

Implement (de)serialization for BTreeMap same as HashMap

7fd9b33

TheBlueMatt added the Seeking Code Review label Jan 16, 2023

TheBlueMatt added this to the 0.0.114 milestone Jan 16, 2023

arik-so reviewed Jan 17, 2023

View reviewed changes

lightning/src/util/ser.rs Outdated Show resolved Hide resolved

valentinewallace previously approved these changes Jan 17, 2023

View reviewed changes

jkczyz reviewed Jan 17, 2023

View reviewed changes

TheBlueMatt dismissed valentinewallace’s stale review via 1d00df7 January 17, 2023 17:41

valentinewallace reviewed Jan 17, 2023

View reviewed changes

lightning/src/util/ser.rs Outdated Show resolved Hide resolved

valentinewallace reviewed Jan 17, 2023

View reviewed changes

arik-so reviewed Jan 17, 2023

View reviewed changes

lightning/src/util/ser.rs Show resolved Hide resolved

jkczyz reviewed Jan 17, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-01-ser-cleanups branch from b439740 to d0c8b14 Compare January 17, 2023 21:18

arik-so previously approved these changes Jan 17, 2023

View reviewed changes

TheBlueMatt dismissed arik-so’s stale review via 6270a8f January 17, 2023 21:28

jkczyz previously approved these changes Jan 17, 2023

View reviewed changes

TheBlueMatt dismissed jkczyz’s stale review via 3e9727b January 17, 2023 21:48

TheBlueMatt force-pushed the 2023-01-ser-cleanups branch from 6270a8f to 3e9727b Compare January 17, 2023 21:48

jkczyz reviewed Jan 17, 2023

View reviewed changes

TheBlueMatt removed the Seeking Code Review label Jan 17, 2023

jkczyz approved these changes Jan 17, 2023

View reviewed changes

valentinewallace approved these changes Jan 17, 2023

View reviewed changes

TheBlueMatt merged commit ad40573 into lightningdevkit:main Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trivial Serialization Tweaks #1956

Trivial Serialization Tweaks #1956

TheBlueMatt commented Jan 15, 2023

codecov-commenter commented Jan 16, 2023 •

edited

Loading

valentinewallace left a comment

valentinewallace Jan 16, 2023

TheBlueMatt Jan 17, 2023

jkczyz Jan 17, 2023

arik-so Jan 17, 2023

jkczyz Jan 17, 2023

TheBlueMatt Jan 17, 2023

jkczyz Jan 17, 2023 •

edited

Loading

TheBlueMatt Jan 17, 2023

valentinewallace Jan 17, 2023

valentinewallace Jan 17, 2023

TheBlueMatt commented Jan 17, 2023

valentinewallace commented Jan 17, 2023

jkczyz Jan 17, 2023

jkczyz Jan 17, 2023

arik-so Jan 17, 2023

TheBlueMatt commented Jan 17, 2023

TheBlueMatt commented Jan 17, 2023

arik-so left a comment

TheBlueMatt commented Jan 17, 2023

jkczyz Jan 17, 2023 •

edited

Loading

TheBlueMatt Jan 17, 2023 •

edited

Loading

		val = <u64 as Readable>::read(r)?
		.checked_add(0xffff).ok_or(DecodeError::InvalidValue)?;

Trivial Serialization Tweaks #1956

Trivial Serialization Tweaks #1956

Conversation

TheBlueMatt commented Jan 15, 2023

codecov-commenter commented Jan 16, 2023 • edited Loading

Codecov Report

valentinewallace 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

jkczyz Jan 17, 2023 • edited Loading

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

TheBlueMatt commented Jan 17, 2023

valentinewallace commented Jan 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jan 17, 2023

TheBlueMatt commented Jan 17, 2023

arik-so left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Jan 17, 2023

jkczyz Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Jan 16, 2023 •

edited

Loading

jkczyz Jan 17, 2023 •

edited

Loading

jkczyz Jan 17, 2023 •

edited

Loading

TheBlueMatt Jan 17, 2023 •

edited

Loading