Implement serialize/deserialize for Router #271

ariard · 2018-12-17T03:07:55Z

Will add a test and maybe more docs

Part of #57

TheBlueMatt · 2018-12-18T03:59:36Z

src/ln/msgs.rs

@@ -382,7 +382,81 @@ impl NetAddress {
 	}
 }

-#[derive(Clone)]
+impl Writeable for NetAddress {


Can you adapt the UnsignedNodeAnnouncement write/read to use these as well?

src/util/test_utils.rs

ariard · 2018-12-19T01:52:37Z

Damn seems cc dependencies break on 1.22 and 1.29

TheBlueMatt · 2018-12-20T02:39:08Z

Looks like this accidentally changed the serialization of some messages resulting in a router_target failure:

---- tests::duplicate_crash stdout ----
thread 'tests::duplicate_crash' panicked at 'We picked the length...', fuzz_targets/router_target.rs:175:46
note: Some details are omitted, run with RUST_BACKTRACE=full for a verbose backtrace.
stack backtrace:
0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
1: std::sys_common::backtrace::print
2: std::panicking::default_hook::{{closure}}
3: std::panicking::default_hook
4: std::panicking::rust_panic_with_hook
5: std::panicking::begin_panic
at libstd/panicking.rs:411
6: router_target::do_test
at fuzz_targets/router_target.rs:175
7: router_target::tests::duplicate_crash
at fuzz_targets/router_target.rs:263
8: router_target::tests::duplicate_crash::{{closure}}
at fuzz_targets/router_target.rs:262
9: core::ops::function::FnOnce::call_once
at libcore/ops/function.rs:238
10: <F as alloc::boxed::FnBox>::call_box
11: __rust_maybe_catch_panic

ariard · 2018-12-21T03:34:33Z

Sorry, stupid question, from where do you get this specific debug output ? I mean I only get crash in hfuzz_workspace/router_target/ with HONGGFUZZ.REPORT.TXT ?

TheBlueMatt · 2018-12-21T03:57:56Z

If you put the failing test's hex in the duplicate_crash test case in fuzz_targets/router_target.rs and run cargo test from the fuzz/ directory you should get a failing test just like any other cargo test.

ariard · 2018-12-22T03:48:27Z

Thanks got the same debug!

ariard · 2019-01-06T15:46:20Z

src/ln/msgs.rs

+					excess_byte = byte;
+					break;
+				},
+				Err(DecodeError::ShortRead) => return Err(DecodeError::BadLengthDescriptor),


There, as serialization is implemented for NetAddress now, a ShortRead bring back by deserialization field failure should be translated as a BadLengthDescriptor as address descriptor isn't accurate seems to me (or we can change msg_node_announcement_target constraints)

src/ln/router.rs

src/ln/msgs.rs

TheBlueMatt · 2019-01-08T18:47:39Z

src/ln/msgs.rs

+					port,
+				})
+			}
+			_ => return Err(DecodeError::UnknownVersion { byte }),


Doesn't InvalidValue make more sense here?

To me InvalidValue is data that is syntaxically incorrect at the protocol level, there address descriptor is may be right we just don't support it.

We use InvalidValue mostly to track non-0/1 for bool values, which seems kinda equivalent here - the type byte isnt a "version" so much as something analogous to a 2 for a bool

src/ln/msgs.rs

TheBlueMatt · 2019-01-08T18:57:38Z

src/ln/router.rs

+		let fee_proportional_millionths = Readable::read(reader)?;
+		let last_update_message = match <u8 as Readable<R>>::read(reader)? {
+			0 => None,
+			1 => Some(msgs::ChannelUpdate::read(reader)?),


Maybe we should just implement Readable/Writeable for Option<T : Readable/Writeable>? There's a bunch of places that would simplify things.

Hey right working on it

ariard · 2019-01-14T03:22:46Z

Still WIP, need to dissociate between None field and optional field (e,g option_shutdown_upfront_script), should do it with an enum

ariard · 2019-01-17T03:41:15Z

Rebased, and fix 1.22 build should be good now

TheBlueMatt · 2019-01-21T17:45:16Z

src/ln/msgs.rs

+					port,
+				})
+			}
+			_ => return Err(DecodeError::InvalidValue { byte: Some(byte) }),


Hmm, now that I think about it more I'm rather unconvinced of this approach. If, eg, at some point we check the ed25519_pubkey is valid in OnionV3 or the ports are non-0 we may want to return an InvalidValue but not have extra bytes. Maybe it makes sense to return a Result<enum { Ok(NetAddress), UnknownType(u8) }, DecodeError>? This would also make this patch much smaller, which is nice (especially when the "byte" field in InvalidValue only sometimes makes sense, hence it being an Option).

Added a NetAddess::Unknown { descriptor }, cleaner than an enum, Agree, cleaner and better do not touch InvalidValue so let it free to be return for an invalid ed25519_pubkey, zero ports or constrained fields in future addresses.

TheBlueMatt · 2019-01-21T17:47:24Z

src/ln/msgs.rs

+#[derive(Clone)]
+pub enum OptionalField<T> {
+	/// Optional field is included in message
+	Present {


Why not just Present(T)?

Thanks, still learning to be a good rustacean

TheBlueMatt · 2019-01-22T18:43:11Z

fuzz/fuzz_targets/router_target.rs

@@ -125,9 +125,9 @@ pub fn do_test(data: &[u8]) {
 			match <($MsgType)>::read(&mut reader) {
 				Ok(msg) => msg,
 				Err(e) => match e {
-					msgs::DecodeError::UnknownVersion => return,
+					msgs::DecodeError::UnknownVersion { .. } => return,


nit: stale.

TheBlueMatt · 2019-01-22T18:46:29Z

src/ln/msgs.rs

@@ -191,7 +190,7 @@ pub struct OpenChannel {
 	pub(crate) htlc_basepoint: PublicKey,
 	pub(crate) first_per_commitment_point: PublicKey,
 	pub(crate) channel_flags: u8,
-	pub(crate) shutdown_scriptpubkey: Option<Script>,
+	pub(crate) shutdown_scriptpubkey: OptionalField<Script>,


I believe you need a similar hunk in ChannelReestablish.

TheBlueMatt · 2019-01-22T18:48:07Z

src/ln/msgs.rs

@@ -372,6 +371,12 @@ pub enum NetAddress {
 		/// The port on which the node is listenting
 		port: u16,
 	},
+	/// An unsupported address.
+	/// At deserialization, we retrieve first byte of it if needed by caller.
+	Unknown {


Hmmm, the panics introduced in get_id/len are kinda annoying. A user may deserialize a NetAddress and then try to reserialize it and get a panic. It may be easier to get a higher-level enum that selects between an unknown byte and a NetAddress for the deserialization.

But an enum doesn't fit into Readable trait signature so seems to me here we are constrained between extending NetAddress enum type with UnknownAddress (but gross because of panic) or adding an UnknownAddressDescriptor with byte field in DecodeError ?

Hmm? We can implement Readable for NetAddressReadEnum instead of for NetAddress and just map it properly in the message thats calling it.

src/ln/msgs.rs

src/ln/peer_handler.rs

TheBlueMatt · 2019-01-23T02:09:08Z

src/ln/router.rs

+		self.htlc_minimum_msat.write(writer)?;
+		self.fee_base_msat.write(writer)?;
+		self.fee_proportional_millionths.write(writer)?;
+		match &self.last_update_message {


Once you add the writer for Option, cant this switch to the impl_writeable!?

There updated with impl_writeable_len_match

TheBlueMatt · 2019-01-23T02:09:20Z

src/ln/router.rs

+		self.features.write(writer)?;
+		self.one_to_two.write(writer)?;
+		self.two_to_one.write(writer)?;
+		match &self.announcement_message {


Same here - can we use impl_writeable?

And there not, I mean due to DirectionalChannelInfo having itself optional field and announcement_msg being optional we have a good chunk of pattern to cover, so if macro goal is to be hygienic I'm not sure it's reached there.

Err, would actually prefer imple_writeable!(.., 0, {}) for both as otherwise we give a wrong length descriptor.

Extend route_test to check if serialize/deserialize of NetworkMap works. Add PartialEq traits on some Router's structs. Modify also UnsignedNodeAnnouncement serialization

Add OptionalField in OpenChannel, AcceptChannel ChannelReestablish to avoid serialization implementation conflicts

TheBlueMatt

Will take as #298 with the following few comments fixed and the commit order swapped as the first commit did not compile on its own.

TheBlueMatt · 2019-01-23T15:35:57Z

src/ln/msgs.rs

+					break;
+				},
+				Err(DecodeError::ShortRead) => return Err(DecodeError::BadLengthDescriptor),
+				_ => unreachable!(),


We could hit at least DecodeError::Io if the user is streaming the message in to be deserialized live.

TheBlueMatt · 2019-01-23T15:44:24Z

src/ln/router.rs

+		self.features.write(writer)?;
+		self.one_to_two.write(writer)?;
+		self.two_to_one.write(writer)?;
+		match &self.announcement_message {


Err, would actually prefer imple_writeable!(.., 0, {}) for both as otherwise we give a wrong length descriptor.

TheBlueMatt · 2019-01-23T15:53:24Z

src/ln/router.rs

+impl<R: ::std::io::Read> Readable<R> for NodeInfo {
+	fn read(reader: &mut R) -> Result<NodeInfo, DecodeError> {
+		let channels_count: u64 = Readable::read(reader)?;
+		let mut channels = Vec::with_capacity(channels_count as usize);


Careful with_capacity()ing a u64 - this will result in corruption causing a crash instead of returning an Err.

TheBlueMatt · 2019-01-23T16:12:45Z

src/ln/msgs.rs

@@ -518,6 +592,16 @@ pub enum HTLCFailChannelUpdate {
 	}
 }

+/// Messages could have optional fields to use with extended features
+/// Due to serialization issues, we encapsulate it in enum


Would be nice to speciy what those "serialization issues" are.

ariard force-pushed the 2018-12-persist-route-db-on-disk branch from 47358b8 to ad45d92 Compare December 18, 2018 02:44

TheBlueMatt reviewed Dec 18, 2018

View reviewed changes

src/util/test_utils.rs Outdated Show resolved Hide resolved

ariard force-pushed the 2018-12-persist-route-db-on-disk branch from ad45d92 to 216edbb Compare December 19, 2018 01:01

ariard force-pushed the 2018-12-persist-route-db-on-disk branch from 216edbb to 0026aec Compare January 6, 2019 15:41

ariard commented Jan 6, 2019

View reviewed changes

TheBlueMatt reviewed Jan 8, 2019

View reviewed changes

ariard force-pushed the 2018-12-persist-route-db-on-disk branch 2 times, most recently from 0f97f11 to 605b5d2 Compare January 14, 2019 03:20

ariard force-pushed the 2018-12-persist-route-db-on-disk branch 4 times, most recently from 270e387 to e3f46f3 Compare January 17, 2019 03:40

TheBlueMatt reviewed Jan 21, 2019

View reviewed changes

ariard force-pushed the 2018-12-persist-route-db-on-disk branch from e3f46f3 to 2fdff3a Compare January 22, 2019 00:55

TheBlueMatt reviewed Jan 22, 2019

View reviewed changes

ariard force-pushed the 2018-12-persist-route-db-on-disk branch from 2fdff3a to 58d3b14 Compare January 23, 2019 01:54